Simplify backup_incremental_init_copy_bitmap using the function
bdrv_dirty_bitmap_next_dirty_area.

Note: move to job->len instead of bitmap size: it should not matter but
less code.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/backup.c | 40 ++++++++++++----------------------------
 1 file changed, 12 insertions(+), 28 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 435414e964..fbe7ce19e1 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -406,43 +406,27 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
 /* init copy_bitmap from sync_bitmap */
 static void backup_incremental_init_copy_bitmap(BackupBlockJob *job)
 {
-    BdrvDirtyBitmapIter *dbi;
-    int64_t offset;
-    int64_t end = DIV_ROUND_UP(bdrv_dirty_bitmap_size(job->sync_bitmap),
-                               job->cluster_size);
-
-    dbi = bdrv_dirty_iter_new(job->sync_bitmap);
-    while ((offset = bdrv_dirty_iter_next(dbi)) != -1) {
-        int64_t cluster = offset / job->cluster_size;
-        int64_t next_cluster;
-
-        offset += bdrv_dirty_bitmap_granularity(job->sync_bitmap);
-        if (offset >= bdrv_dirty_bitmap_size(job->sync_bitmap)) {
-            hbitmap_set(job->copy_bitmap, cluster, end - cluster);
-            break;
-        }
+    uint64_t offset = 0;
+    uint64_t bytes = job->len;
 
-        offset = bdrv_dirty_bitmap_next_zero(job->sync_bitmap, offset,
-                                             UINT64_MAX);
-        if (offset == -1) {
-            hbitmap_set(job->copy_bitmap, cluster, end - cluster);
-            break;
-        }
+    while (bdrv_dirty_bitmap_next_dirty_area(job->sync_bitmap,
+                                             &offset, &bytes))
+    {
+        uint64_t cluster = offset / job->cluster_size;
+        uint64_t last_cluster = (offset + bytes) / job->cluster_size;
 
-        next_cluster = DIV_ROUND_UP(offset, job->cluster_size);
-        hbitmap_set(job->copy_bitmap, cluster, next_cluster - cluster);
-        if (next_cluster >= end) {
+        hbitmap_set(job->copy_bitmap, cluster, last_cluster - cluster + 1);
+
+        offset = (last_cluster + 1) * job->cluster_size;
+        if (offset >= job->len) {
             break;
         }
-
-        bdrv_set_dirty_iter(dbi, next_cluster * job->cluster_size);
+        bytes = job->len - offset;
     }
 
     /* TODO job_progress_set_remaining() would make more sense */
     job_progress_update(&job->common.job,
         job->len - hbitmap_count(job->copy_bitmap) * job->cluster_size);
-
-    bdrv_dirty_iter_free(dbi);
 }
 
 static int coroutine_fn backup_run(Job *job, Error **errp)
-- 
2.18.0

On 29.12.18 13:20, Vladimir Sementsov-Ogievskiy wrote:
> Simplify backup_incremental_init_copy_bitmap using the function
> bdrv_dirty_bitmap_next_dirty_area.
> 
> Note: move to job->len instead of bitmap size: it should not matter but
> less code.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/backup.c | 40 ++++++++++++----------------------------
>  1 file changed, 12 insertions(+), 28 deletions(-)

Overall: What is this function even supposed to do?  To me, it looks
like it marks all areas in job->copy_bitmap dirty that are dirty in
job->sync_bitmap.

If so, wouldn't just replacing this by hbitmap_merge() simplify things
further?

> diff --git a/block/backup.c b/block/backup.c
> index 435414e964..fbe7ce19e1 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -406,43 +406,27 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)

[...]

> +    while (bdrv_dirty_bitmap_next_dirty_area(job->sync_bitmap,
> +                                             &offset, &bytes))
> +    {
> +        uint64_t cluster = offset / job->cluster_size;
> +        uint64_t last_cluster = (offset + bytes) / job->cluster_size;
>  
> -        next_cluster = DIV_ROUND_UP(offset, job->cluster_size);
> -        hbitmap_set(job->copy_bitmap, cluster, next_cluster - cluster);
> -        if (next_cluster >= end) {
> +        hbitmap_set(job->copy_bitmap, cluster, last_cluster - cluster + 1);

Why the +1?  Shouldn't the division for last_cluster round up instead?

> +
> +        offset = (last_cluster + 1) * job->cluster_size;

Same here.

Max

On 14.01.19 14:10, Max Reitz wrote:
> On 29.12.18 13:20, Vladimir Sementsov-Ogievskiy wrote:
>> Simplify backup_incremental_init_copy_bitmap using the function
>> bdrv_dirty_bitmap_next_dirty_area.
>>
>> Note: move to job->len instead of bitmap size: it should not matter but
>> less code.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>  block/backup.c | 40 ++++++++++++----------------------------
>>  1 file changed, 12 insertions(+), 28 deletions(-)
> 
> Overall: What is this function even supposed to do?  To me, it looks
> like it marks all areas in job->copy_bitmap dirty that are dirty in
> job->sync_bitmap.
> 
> If so, wouldn't just replacing this by hbitmap_merge() simplify things
> further?

Ah, no, because they would need to have the same granularity...

Max

14.01.2019 16:10, Max Reitz wrote:
> On 29.12.18 13:20, Vladimir Sementsov-Ogievskiy wrote:
>> Simplify backup_incremental_init_copy_bitmap using the function
>> bdrv_dirty_bitmap_next_dirty_area.
>>
>> Note: move to job->len instead of bitmap size: it should not matter but
>> less code.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/backup.c | 40 ++++++++++++----------------------------
>>   1 file changed, 12 insertions(+), 28 deletions(-)
> 
> Overall: What is this function even supposed to do?  To me, it looks
> like it marks all areas in job->copy_bitmap dirty that are dirty in
> job->sync_bitmap.
> 
> If so, wouldn't just replacing this by hbitmap_merge() simplify things
> further?
> 
>> diff --git a/block/backup.c b/block/backup.c
>> index 435414e964..fbe7ce19e1 100644
>> --- a/block/backup.c
>> +++ b/block/backup.c
>> @@ -406,43 +406,27 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
> 
> [...]
> 
>> +    while (bdrv_dirty_bitmap_next_dirty_area(job->sync_bitmap,
>> +                                             &offset, &bytes))
>> +    {
>> +        uint64_t cluster = offset / job->cluster_size;
>> +        uint64_t last_cluster = (offset + bytes) / job->cluster_size;
>>   
>> -        next_cluster = DIV_ROUND_UP(offset, job->cluster_size);
>> -        hbitmap_set(job->copy_bitmap, cluster, next_cluster - cluster);
>> -        if (next_cluster >= end) {
>> +        hbitmap_set(job->copy_bitmap, cluster, last_cluster - cluster + 1);
> 
> Why the +1?  Shouldn't the division for last_cluster round up instead?
> 
>> +
>> +        offset = (last_cluster + 1) * job->cluster_size;
> 
> Same here.

last cluster is not "end", but it's last dirty cluster. so number of dirty clusters is last_cluster - cluster + 1, and next offset is calculated through +1 too.

If I round up division result, I'll get last for most cases, but "end" (next after the last), for the case when offset % job->cluster_size == 0, so, how to use it?


-- 
Best regards,
Vladimir

On 14.01.19 15:01, Vladimir Sementsov-Ogievskiy wrote:
> 14.01.2019 16:10, Max Reitz wrote:
>> On 29.12.18 13:20, Vladimir Sementsov-Ogievskiy wrote:
>>> Simplify backup_incremental_init_copy_bitmap using the function
>>> bdrv_dirty_bitmap_next_dirty_area.
>>>
>>> Note: move to job->len instead of bitmap size: it should not matter but
>>> less code.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block/backup.c | 40 ++++++++++++----------------------------
>>>   1 file changed, 12 insertions(+), 28 deletions(-)
>>
>> Overall: What is this function even supposed to do?  To me, it looks
>> like it marks all areas in job->copy_bitmap dirty that are dirty in
>> job->sync_bitmap.
>>
>> If so, wouldn't just replacing this by hbitmap_merge() simplify things
>> further?
>>
>>> diff --git a/block/backup.c b/block/backup.c
>>> index 435414e964..fbe7ce19e1 100644
>>> --- a/block/backup.c
>>> +++ b/block/backup.c
>>> @@ -406,43 +406,27 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
>>
>> [...]
>>
>>> +    while (bdrv_dirty_bitmap_next_dirty_area(job->sync_bitmap,
>>> +                                             &offset, &bytes))
>>> +    {
>>> +        uint64_t cluster = offset / job->cluster_size;
>>> +        uint64_t last_cluster = (offset + bytes) / job->cluster_size;
>>>   
>>> -        next_cluster = DIV_ROUND_UP(offset, job->cluster_size);
>>> -        hbitmap_set(job->copy_bitmap, cluster, next_cluster - cluster);
>>> -        if (next_cluster >= end) {
>>> +        hbitmap_set(job->copy_bitmap, cluster, last_cluster - cluster + 1);
>>
>> Why the +1?  Shouldn't the division for last_cluster round up instead?
>>
>>> +
>>> +        offset = (last_cluster + 1) * job->cluster_size;
>>
>> Same here.
> 
> last cluster is not "end", but it's last dirty cluster. so number of dirty clusters is last_cluster - cluster + 1, and next offset is calculated through +1 too.
> 
> If I round up division result, I'll get last for most cases, but "end" (next after the last), for the case when offset % job->cluster_size == 0, so, how to use it?

Doesn't bdrv_dirty_bitmap_next_dirty_area() return a range [offset,
offset + bytes), i.e. where "offset + bytes" is the first clean offset?

Max

14.01.2019 17:13, Max Reitz wrote:
> On 14.01.19 15:01, Vladimir Sementsov-Ogievskiy wrote:
>> 14.01.2019 16:10, Max Reitz wrote:
>>> On 29.12.18 13:20, Vladimir Sementsov-Ogievskiy wrote:
>>>> Simplify backup_incremental_init_copy_bitmap using the function
>>>> bdrv_dirty_bitmap_next_dirty_area.
>>>>
>>>> Note: move to job->len instead of bitmap size: it should not matter but
>>>> less code.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>    block/backup.c | 40 ++++++++++++----------------------------
>>>>    1 file changed, 12 insertions(+), 28 deletions(-)
>>>
>>> Overall: What is this function even supposed to do?  To me, it looks
>>> like it marks all areas in job->copy_bitmap dirty that are dirty in
>>> job->sync_bitmap.
>>>
>>> If so, wouldn't just replacing this by hbitmap_merge() simplify things
>>> further?
>>>
>>>> diff --git a/block/backup.c b/block/backup.c
>>>> index 435414e964..fbe7ce19e1 100644
>>>> --- a/block/backup.c
>>>> +++ b/block/backup.c
>>>> @@ -406,43 +406,27 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
>>>
>>> [...]
>>>
>>>> +    while (bdrv_dirty_bitmap_next_dirty_area(job->sync_bitmap,
>>>> +                                             &offset, &bytes))
>>>> +    {
>>>> +        uint64_t cluster = offset / job->cluster_size;
>>>> +        uint64_t last_cluster = (offset + bytes) / job->cluster_size;
>>>>    
>>>> -        next_cluster = DIV_ROUND_UP(offset, job->cluster_size);
>>>> -        hbitmap_set(job->copy_bitmap, cluster, next_cluster - cluster);
>>>> -        if (next_cluster >= end) {
>>>> +        hbitmap_set(job->copy_bitmap, cluster, last_cluster - cluster + 1);
>>>
>>> Why the +1?  Shouldn't the division for last_cluster round up instead?
>>>
>>>> +
>>>> +        offset = (last_cluster + 1) * job->cluster_size;
>>>
>>> Same here.
>>
>> last cluster is not "end", but it's last dirty cluster. so number of dirty clusters is last_cluster - cluster + 1, and next offset is calculated through +1 too.
>>
>> If I round up division result, I'll get last for most cases, but "end" (next after the last), for the case when offset % job->cluster_size == 0, so, how to use it?
> 
> Doesn't bdrv_dirty_bitmap_next_dirty_area() return a range [offset,
> offset + bytes), i.e. where "offset + bytes" is the first clean offset?

oops, you are right. then I need
uint64_t last_cluster = (offset + bytes - 1) / job->cluster_size;

> 
> Max
> 


-- 
Best regards,
Vladimir

On 14.01.19 15:48, Vladimir Sementsov-Ogievskiy wrote:
> 14.01.2019 17:13, Max Reitz wrote:
>> On 14.01.19 15:01, Vladimir Sementsov-Ogievskiy wrote:
>>> 14.01.2019 16:10, Max Reitz wrote:
>>>> On 29.12.18 13:20, Vladimir Sementsov-Ogievskiy wrote:
>>>>> Simplify backup_incremental_init_copy_bitmap using the function
>>>>> bdrv_dirty_bitmap_next_dirty_area.
>>>>>
>>>>> Note: move to job->len instead of bitmap size: it should not matter but
>>>>> less code.
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>> ---
>>>>>    block/backup.c | 40 ++++++++++++----------------------------
>>>>>    1 file changed, 12 insertions(+), 28 deletions(-)
>>>>
>>>> Overall: What is this function even supposed to do?  To me, it looks
>>>> like it marks all areas in job->copy_bitmap dirty that are dirty in
>>>> job->sync_bitmap.
>>>>
>>>> If so, wouldn't just replacing this by hbitmap_merge() simplify things
>>>> further?
>>>>
>>>>> diff --git a/block/backup.c b/block/backup.c
>>>>> index 435414e964..fbe7ce19e1 100644
>>>>> --- a/block/backup.c
>>>>> +++ b/block/backup.c
>>>>> @@ -406,43 +406,27 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
>>>>
>>>> [...]
>>>>
>>>>> +    while (bdrv_dirty_bitmap_next_dirty_area(job->sync_bitmap,
>>>>> +                                             &offset, &bytes))
>>>>> +    {
>>>>> +        uint64_t cluster = offset / job->cluster_size;
>>>>> +        uint64_t last_cluster = (offset + bytes) / job->cluster_size;
>>>>>    
>>>>> -        next_cluster = DIV_ROUND_UP(offset, job->cluster_size);
>>>>> -        hbitmap_set(job->copy_bitmap, cluster, next_cluster - cluster);
>>>>> -        if (next_cluster >= end) {
>>>>> +        hbitmap_set(job->copy_bitmap, cluster, last_cluster - cluster + 1);
>>>>
>>>> Why the +1?  Shouldn't the division for last_cluster round up instead?
>>>>
>>>>> +
>>>>> +        offset = (last_cluster + 1) * job->cluster_size;
>>>>
>>>> Same here.
>>>
>>> last cluster is not "end", but it's last dirty cluster. so number of dirty clusters is last_cluster - cluster + 1, and next offset is calculated through +1 too.
>>>
>>> If I round up division result, I'll get last for most cases, but "end" (next after the last), for the case when offset % job->cluster_size == 0, so, how to use it?
>>
>> Doesn't bdrv_dirty_bitmap_next_dirty_area() return a range [offset,
>> offset + bytes), i.e. where "offset + bytes" is the first clean offset?
> 
> oops, you are right. then I need
> uint64_t last_cluster = (offset + bytes - 1) / job->cluster_size;

That, or you just use a rounding up division and rename it from
last_cluster to end_cluster or first_clean_cluster or something (and
subsequently drop the +1s).

Max

16.01.2019 16:05, Max Reitz wrote:
> On 14.01.19 15:48, Vladimir Sementsov-Ogievskiy wrote:
>> 14.01.2019 17:13, Max Reitz wrote:
>>> On 14.01.19 15:01, Vladimir Sementsov-Ogievskiy wrote:
>>>> 14.01.2019 16:10, Max Reitz wrote:
>>>>> On 29.12.18 13:20, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> Simplify backup_incremental_init_copy_bitmap using the function
>>>>>> bdrv_dirty_bitmap_next_dirty_area.
>>>>>>
>>>>>> Note: move to job->len instead of bitmap size: it should not matter but
>>>>>> less code.
>>>>>>
>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>>> ---
>>>>>>     block/backup.c | 40 ++++++++++++----------------------------
>>>>>>     1 file changed, 12 insertions(+), 28 deletions(-)
>>>>>
>>>>> Overall: What is this function even supposed to do?  To me, it looks
>>>>> like it marks all areas in job->copy_bitmap dirty that are dirty in
>>>>> job->sync_bitmap.
>>>>>
>>>>> If so, wouldn't just replacing this by hbitmap_merge() simplify things
>>>>> further?
>>>>>
>>>>>> diff --git a/block/backup.c b/block/backup.c
>>>>>> index 435414e964..fbe7ce19e1 100644
>>>>>> --- a/block/backup.c
>>>>>> +++ b/block/backup.c
>>>>>> @@ -406,43 +406,27 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
>>>>>
>>>>> [...]
>>>>>
>>>>>> +    while (bdrv_dirty_bitmap_next_dirty_area(job->sync_bitmap,
>>>>>> +                                             &offset, &bytes))
>>>>>> +    {
>>>>>> +        uint64_t cluster = offset / job->cluster_size;
>>>>>> +        uint64_t last_cluster = (offset + bytes) / job->cluster_size;
>>>>>>     
>>>>>> -        next_cluster = DIV_ROUND_UP(offset, job->cluster_size);
>>>>>> -        hbitmap_set(job->copy_bitmap, cluster, next_cluster - cluster);
>>>>>> -        if (next_cluster >= end) {
>>>>>> +        hbitmap_set(job->copy_bitmap, cluster, last_cluster - cluster + 1);
>>>>>
>>>>> Why the +1?  Shouldn't the division for last_cluster round up instead?
>>>>>
>>>>>> +
>>>>>> +        offset = (last_cluster + 1) * job->cluster_size;
>>>>>
>>>>> Same here.
>>>>
>>>> last cluster is not "end", but it's last dirty cluster. so number of dirty clusters is last_cluster - cluster + 1, and next offset is calculated through +1 too.
>>>>
>>>> If I round up division result, I'll get last for most cases, but "end" (next after the last), for the case when offset % job->cluster_size == 0, so, how to use it?
>>>
>>> Doesn't bdrv_dirty_bitmap_next_dirty_area() return a range [offset,
>>> offset + bytes), i.e. where "offset + bytes" is the first clean offset?
>>
>> oops, you are right. then I need
>> uint64_t last_cluster = (offset + bytes - 1) / job->cluster_size;
> 
> That, or you just use a rounding up division and rename it from
> last_cluster to end_cluster or first_clean_cluster or something (and
> subsequently drop the +1s).

This will not work, as ((offset + bytes) / job->cluster_size) is not first clean cluster
or end cluster. It's a cluster, where is first clean bit located, but it may have dirty
bits too (or, may not).

So, to rewrite based on end_cluster, it should be calculated as

(offset + bytes - 1) / job->cluster_size + 1

and, I'm going to do so, one "+1" instead of two, and, may be, a bit more understandable.

-- 
Best regards,
Vladimir

On 23.01.19 09:20, Vladimir Sementsov-Ogievskiy wrote:
> 16.01.2019 16:05, Max Reitz wrote:
>> On 14.01.19 15:48, Vladimir Sementsov-Ogievskiy wrote:
>>> 14.01.2019 17:13, Max Reitz wrote:
>>>> On 14.01.19 15:01, Vladimir Sementsov-Ogievskiy wrote:
>>>>> 14.01.2019 16:10, Max Reitz wrote:
>>>>>> On 29.12.18 13:20, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>> Simplify backup_incremental_init_copy_bitmap using the function
>>>>>>> bdrv_dirty_bitmap_next_dirty_area.
>>>>>>>
>>>>>>> Note: move to job->len instead of bitmap size: it should not matter but
>>>>>>> less code.
>>>>>>>
>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>>>> ---
>>>>>>>     block/backup.c | 40 ++++++++++++----------------------------
>>>>>>>     1 file changed, 12 insertions(+), 28 deletions(-)
>>>>>>
>>>>>> Overall: What is this function even supposed to do?  To me, it looks
>>>>>> like it marks all areas in job->copy_bitmap dirty that are dirty in
>>>>>> job->sync_bitmap.
>>>>>>
>>>>>> If so, wouldn't just replacing this by hbitmap_merge() simplify things
>>>>>> further?
>>>>>>
>>>>>>> diff --git a/block/backup.c b/block/backup.c
>>>>>>> index 435414e964..fbe7ce19e1 100644
>>>>>>> --- a/block/backup.c
>>>>>>> +++ b/block/backup.c
>>>>>>> @@ -406,43 +406,27 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> +    while (bdrv_dirty_bitmap_next_dirty_area(job->sync_bitmap,
>>>>>>> +                                             &offset, &bytes))
>>>>>>> +    {
>>>>>>> +        uint64_t cluster = offset / job->cluster_size;
>>>>>>> +        uint64_t last_cluster = (offset + bytes) / job->cluster_size;
>>>>>>>     
>>>>>>> -        next_cluster = DIV_ROUND_UP(offset, job->cluster_size);
>>>>>>> -        hbitmap_set(job->copy_bitmap, cluster, next_cluster - cluster);
>>>>>>> -        if (next_cluster >= end) {
>>>>>>> +        hbitmap_set(job->copy_bitmap, cluster, last_cluster - cluster + 1);
>>>>>>
>>>>>> Why the +1?  Shouldn't the division for last_cluster round up instead?
>>>>>>
>>>>>>> +
>>>>>>> +        offset = (last_cluster + 1) * job->cluster_size;
>>>>>>
>>>>>> Same here.
>>>>>
>>>>> last cluster is not "end", but it's last dirty cluster. so number of dirty clusters is last_cluster - cluster + 1, and next offset is calculated through +1 too.
>>>>>
>>>>> If I round up division result, I'll get last for most cases, but "end" (next after the last), for the case when offset % job->cluster_size == 0, so, how to use it?
>>>>
>>>> Doesn't bdrv_dirty_bitmap_next_dirty_area() return a range [offset,
>>>> offset + bytes), i.e. where "offset + bytes" is the first clean offset?
>>>
>>> oops, you are right. then I need
>>> uint64_t last_cluster = (offset + bytes - 1) / job->cluster_size;
>>
>> That, or you just use a rounding up division and rename it from
>> last_cluster to end_cluster or first_clean_cluster or something (and
>> subsequently drop the +1s).
> 
> This will not work, as ((offset + bytes) / job->cluster_size) is not first clean cluster
> or end cluster. It's a cluster, where is first clean bit located, but it may have dirty
> bits too (or, may not).
> 
> So, to rewrite based on end_cluster, it should be calculated as
> 
> (offset + bytes - 1) / job->cluster_size + 1

That's why I asked "Shouldn't the division for last_cluster round up
instead?"

Max

> and, I'm going to do so, one "+1" instead of two, and, may be, a bit more understandable.
> 

On 1/23/19 2:20 AM, Vladimir Sementsov-Ogievskiy wrote:

>>>>>>> +        hbitmap_set(job->copy_bitmap, cluster, last_cluster - cluster + 1);
>>>>>>
>>>>>> Why the +1?  Shouldn't the division for last_cluster round up instead?
>>>>>>
>>>>>>> +
>>>>>>> +        offset = (last_cluster + 1) * job->cluster_size;
>>>>>>
>>>>>> Same here.
>>>>>
>>>>> last cluster is not "end", but it's last dirty cluster. so number of dirty clusters is last_cluster - cluster + 1, and next offset is calculated through +1 too.
>>>>>
>>>>> If I round up division result, I'll get last for most cases, but "end" (next after the last), for the case when offset % job->cluster_size == 0, so, how to use it?
>>>>
>>>> Doesn't bdrv_dirty_bitmap_next_dirty_area() return a range [offset,
>>>> offset + bytes), i.e. where "offset + bytes" is the first clean offset?
>>>
>>> oops, you are right. then I need
>>> uint64_t last_cluster = (offset + bytes - 1) / job->cluster_size;
>>
>> That, or you just use a rounding up division and rename it from
>> last_cluster to end_cluster or first_clean_cluster or something (and
>> subsequently drop the +1s).
> 
> This will not work, as ((offset + bytes) / job->cluster_size) is not first clean cluster
> or end cluster. It's a cluster, where is first clean bit located, but it may have dirty
> bits too (or, may not).
> 
> So, to rewrite based on end_cluster, it should be calculated as
> 
> (offset + bytes - 1) / job->cluster_size + 1
> 
> and, I'm going to do so, one "+1" instead of two, and, may be, a bit more understandable.

Better is to use the macros in osdep.h, such as QEMU_ALIGN_UP/DOWN, to
make the intent of the code easier to read than having to closely check
which operation is being performed to see if it makes sense in context.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

23.01.2019 17:36, Eric Blake wrote:
> On 1/23/19 2:20 AM, Vladimir Sementsov-Ogievskiy wrote:
> 
>>>>>>>> +        hbitmap_set(job->copy_bitmap, cluster, last_cluster - cluster + 1);
>>>>>>>
>>>>>>> Why the +1?  Shouldn't the division for last_cluster round up instead?
>>>>>>>
>>>>>>>> +
>>>>>>>> +        offset = (last_cluster + 1) * job->cluster_size;
>>>>>>>
>>>>>>> Same here.
>>>>>>
>>>>>> last cluster is not "end", but it's last dirty cluster. so number of dirty clusters is last_cluster - cluster + 1, and next offset is calculated through +1 too.
>>>>>>
>>>>>> If I round up division result, I'll get last for most cases, but "end" (next after the last), for the case when offset % job->cluster_size == 0, so, how to use it?
>>>>>
>>>>> Doesn't bdrv_dirty_bitmap_next_dirty_area() return a range [offset,
>>>>> offset + bytes), i.e. where "offset + bytes" is the first clean offset?
>>>>
>>>> oops, you are right. then I need
>>>> uint64_t last_cluster = (offset + bytes - 1) / job->cluster_size;
>>>
>>> That, or you just use a rounding up division and rename it from
>>> last_cluster to end_cluster or first_clean_cluster or something (and
>>> subsequently drop the +1s).
>>
>> This will not work, as ((offset + bytes) / job->cluster_size) is not first clean cluster
>> or end cluster. It's a cluster, where is first clean bit located, but it may have dirty
>> bits too (or, may not).
>>
>> So, to rewrite based on end_cluster, it should be calculated as
>>
>> (offset + bytes - 1) / job->cluster_size + 1
>>
>> and, I'm going to do so, one "+1" instead of two, and, may be, a bit more understandable.
> 
> Better is to use the macros in osdep.h, such as QEMU_ALIGN_UP/DOWN, to
> make the intent of the code easier to read than having to closely check
> which operation is being performed to see if it makes sense in context.
> 

Yes, thanks. Exactly:
uint64_t end_cluster = DIV_ROUND_UP(offset + bytes, job->cluster_size);


-- 
Best regards,
Vladimir