[Qemu-devel] [PATCH v12 0/2] backup: copy_range fixes

[Qemu-devel] [PATCH v12 0/2] backup: copy_range fixes

[Vladimir Sementsov-Ogievskiy]

Hi all!

Here are two small fixes. They fixes old commit, so qemu-stable is in
CC, but actually, I don't think they are critical.

01: is new
02: is just copied from
    [PATCH v11 01/14] block/backup: fix backup_cow_with_offload for last cluster
    (I only add Fixes: to commit-message)
    and this is why I called this v12, to not interfere with previous
    emails

I'd prefer this to go through Max's block branch, as Max is reviewing my
backup-top series, which will refer to these patches and seems simpler
to queue them all together.

Vladimir Sementsov-Ogievskiy (2):
  block/backup: fix max_transfer handling for copy_range
  block/backup: fix backup_cow_with_offload for last cluster

 block/backup.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH v12 1/2] block/backup: fix max_transfer handling for copy_range

[Vladimir Sementsov-Ogievskiy]

Of course, QEMU_ALIGN_UP is a typo, it should be QEMU_ALIGN_DOWN, as we
are trying to find aligned size which satisfy both source and target.
Also, don't ignore too small max_transfer. In this case seems safer to
disable copy_range.

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

diff --git a/block/backup.c b/block/backup.c
index 763f0d7ff6..d8fdbfadfe 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -741,12 +741,16 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
     job->cluster_size = cluster_size;
     job->copy_bitmap = copy_bitmap;
     copy_bitmap = NULL;
-    job->use_copy_range = !compress; /* compression isn't supported for it */
     job->copy_range_size = MIN_NON_ZERO(blk_get_max_transfer(job->common.blk),
                                         blk_get_max_transfer(job->target));
-    job->copy_range_size = MAX(job->cluster_size,
-                               QEMU_ALIGN_UP(job->copy_range_size,
-                                             job->cluster_size));
+    job->copy_range_size = QEMU_ALIGN_DOWN(job->copy_range_size,
+                                           job->cluster_size);
+    /*
+     * Compression is not supported for copy_range. Also, we don't want to
+     * handle small max_transfer for copy_range (which currently don't
+     * handle max_transfer at all).
+     */
+    job->use_copy_range = !compress && job->copy_range_size > 0;
 
     /* Required permissions are already taken with target's blk_new() */
     block_job_add_bdrv(&job->common, "target", target, 0, BLK_PERM_ALL,
-- 
2.21.0

Re: [Qemu-devel] [PATCH v12 1/2] block/backup: fix max_transfer handling for copy_range

[John Snow]

On 9/17/19 12:07 PM, Vladimir Sementsov-Ogievskiy wrote:
> Of course, QEMU_ALIGN_UP is a typo, it should be QEMU_ALIGN_DOWN, as we
> are trying to find aligned size which satisfy both source and target.
> Also, don't ignore too small max_transfer. In this case seems safer to
> disable copy_range.
> 
> Fixes: 9ded4a0114968e
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block/backup.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index 763f0d7ff6..d8fdbfadfe 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -741,12 +741,16 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
>       job->cluster_size = cluster_size;
>       job->copy_bitmap = copy_bitmap;
>       copy_bitmap = NULL;
> -    job->use_copy_range = !compress; /* compression isn't supported for it */
>       job->copy_range_size = MIN_NON_ZERO(blk_get_max_transfer(job->common.blk),
>                                           blk_get_max_transfer(job->target));
> -    job->copy_range_size = MAX(job->cluster_size,
> -                               QEMU_ALIGN_UP(job->copy_range_size,
> -                                             job->cluster_size));
> +    job->copy_range_size = QEMU_ALIGN_DOWN(job->copy_range_size,
> +                                           job->cluster_size);
> +    /*
> +     * Compression is not supported for copy_range. Also, we don't want to
> +     * handle small max_transfer for copy_range (which currently don't
> +     * handle max_transfer at all).
> +     */
> +    job->use_copy_range = !compress && job->copy_range_size > 0;
>   
>       /* Required permissions are already taken with target's blk_new() */
>       block_job_add_bdrv(&job->common, "target", target, 0, BLK_PERM_ALL,
> 

I'm clear on the alignment fix, I'm not clear on the comment about 
max_transfer and how it relates to copy_range_size being non-zero.

"small max transfer" -- what happens when it's zero? we're apparently OK 
with a single cluster, but when it's zero, what happens?

Re: [Qemu-devel] [PATCH v12 1/2] block/backup: fix max_transfer handling for copy_range

[Vladimir Sementsov-Ogievskiy]

18.09.2019 22:57, John Snow wrote:
> 
> 
> On 9/17/19 12:07 PM, Vladimir Sementsov-Ogievskiy wrote:
>> Of course, QEMU_ALIGN_UP is a typo, it should be QEMU_ALIGN_DOWN, as we
>> are trying to find aligned size which satisfy both source and target.
>> Also, don't ignore too small max_transfer. In this case seems safer to
>> disable copy_range.
>>
>> Fixes: 9ded4a0114968e
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/backup.c | 12 ++++++++----
>>   1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/block/backup.c b/block/backup.c
>> index 763f0d7ff6..d8fdbfadfe 100644
>> --- a/block/backup.c
>> +++ b/block/backup.c
>> @@ -741,12 +741,16 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
>>       job->cluster_size = cluster_size;
>>       job->copy_bitmap = copy_bitmap;
>>       copy_bitmap = NULL;
>> -    job->use_copy_range = !compress; /* compression isn't supported for it */
>>       job->copy_range_size = MIN_NON_ZERO(blk_get_max_transfer(job->common.blk),
>>                                           blk_get_max_transfer(job->target));
>> -    job->copy_range_size = MAX(job->cluster_size,
>> -                               QEMU_ALIGN_UP(job->copy_range_size,
>> -                                             job->cluster_size));
>> +    job->copy_range_size = QEMU_ALIGN_DOWN(job->copy_range_size,
>> +                                           job->cluster_size);
>> +    /*
>> +     * Compression is not supported for copy_range. Also, we don't want to
>> +     * handle small max_transfer for copy_range (which currently don't
>> +     * handle max_transfer at all).
>> +     */
>> +    job->use_copy_range = !compress && job->copy_range_size > 0;
>>       /* Required permissions are already taken with target's blk_new() */
>>       block_job_add_bdrv(&job->common, "target", target, 0, BLK_PERM_ALL,
>>
> 
> I'm clear on the alignment fix, I'm not clear on the comment about max_transfer and how it relates to copy_range_size being non-zero.
> 
> "small max transfer" -- what happens when it's zero? we're apparently OK with a single cluster, but when it's zero, what happens?

if it zero it means that source or target requires max_transfer less than cluster_size. It seems not valid to call copy_range in this case.
Still it's OK to use normal read/write, as they handle max_transfer internally in a loop (copy_range doesn't do it).

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v12 1/2] block/backup: fix max_transfer handling for copy_range

[John Snow]

On 9/19/19 2:50 AM, Vladimir Sementsov-Ogievskiy wrote:
> 18.09.2019 22:57, John Snow wrote:
>>
>>
>> On 9/17/19 12:07 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> Of course, QEMU_ALIGN_UP is a typo, it should be QEMU_ALIGN_DOWN, as we
>>> are trying to find aligned size which satisfy both source and target.
>>> Also, don't ignore too small max_transfer. In this case seems safer to
>>> disable copy_range.
>>>
>>> Fixes: 9ded4a0114968e
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block/backup.c | 12 ++++++++----
>>>   1 file changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/block/backup.c b/block/backup.c
>>> index 763f0d7ff6..d8fdbfadfe 100644
>>> --- a/block/backup.c
>>> +++ b/block/backup.c
>>> @@ -741,12 +741,16 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
>>>       job->cluster_size = cluster_size;
>>>       job->copy_bitmap = copy_bitmap;
>>>       copy_bitmap = NULL;
>>> -    job->use_copy_range = !compress; /* compression isn't supported for it */
>>>       job->copy_range_size = MIN_NON_ZERO(blk_get_max_transfer(job->common.blk),
>>>                                           blk_get_max_transfer(job->target));
>>> -    job->copy_range_size = MAX(job->cluster_size,
>>> -                               QEMU_ALIGN_UP(job->copy_range_size,
>>> -                                             job->cluster_size));
>>> +    job->copy_range_size = QEMU_ALIGN_DOWN(job->copy_range_size,
>>> +                                           job->cluster_size);
>>> +    /*
>>> +     * Compression is not supported for copy_range. Also, we don't want to
>>> +     * handle small max_transfer for copy_range (which currently don't
>>> +     * handle max_transfer at all).
>>> +     */
>>> +    job->use_copy_range = !compress && job->copy_range_size > 0;
>>>       /* Required permissions are already taken with target's blk_new() */
>>>       block_job_add_bdrv(&job->common, "target", target, 0, BLK_PERM_ALL,
>>>
>>
>> I'm clear on the alignment fix, I'm not clear on the comment about max_transfer and how it relates to copy_range_size being non-zero.
>>
>> "small max transfer" -- what happens when it's zero? we're apparently OK with a single cluster, but when it's zero, what happens?
> 
> if it zero it means that source or target requires max_transfer less than cluster_size. It seems not valid to call copy_range in this case.
> Still it's OK to use normal read/write, as they handle max_transfer internally in a loop (copy_range doesn't do it).
> 

oh, I'm ... sorry, I just didn't quite understand the comment.

You're just making sure copy_range after all of our checks is non-zero,
plain and simple. If max_transfer was *smaller than a job cluster*, we
might end up with a copy_range size that's zero, which is obviously...
not useful.

So, I might phrase "Also, we don't want to..." as:

"copy_range does not respect max_transfer, so we factor that in here. If
it's smaller than the job->cluster_size, we are unable to use copy_range."

Just a suggestion, though, so:

Reviewed-by: John Snow <jsnow@redhat.com>


(SHOULD copy_range respect max_transfer? I guess it would be quite
different -- it would only count things it had to fall back and actually
*transfer*, right? I suppose that because it can have that fallback we
need to accommodate it here in backup.c, hence this workaround clamp.)

Re: [Qemu-devel] [PATCH v12 1/2] block/backup: fix max_transfer handling for copy_range

[Vladimir Sementsov-Ogievskiy]

20.09.2019 4:13, John Snow wrote:
> 
> 
> On 9/19/19 2:50 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 18.09.2019 22:57, John Snow wrote:
>>>
>>>
>>> On 9/17/19 12:07 PM, Vladimir Sementsov-Ogievskiy wrote:
>>>> Of course, QEMU_ALIGN_UP is a typo, it should be QEMU_ALIGN_DOWN, as we
>>>> are trying to find aligned size which satisfy both source and target.
>>>> Also, don't ignore too small max_transfer. In this case seems safer to
>>>> disable copy_range.
>>>>
>>>> Fixes: 9ded4a0114968e
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>    block/backup.c | 12 ++++++++----
>>>>    1 file changed, 8 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/block/backup.c b/block/backup.c
>>>> index 763f0d7ff6..d8fdbfadfe 100644
>>>> --- a/block/backup.c
>>>> +++ b/block/backup.c
>>>> @@ -741,12 +741,16 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
>>>>        job->cluster_size = cluster_size;
>>>>        job->copy_bitmap = copy_bitmap;
>>>>        copy_bitmap = NULL;
>>>> -    job->use_copy_range = !compress; /* compression isn't supported for it */
>>>>        job->copy_range_size = MIN_NON_ZERO(blk_get_max_transfer(job->common.blk),
>>>>                                            blk_get_max_transfer(job->target));
>>>> -    job->copy_range_size = MAX(job->cluster_size,
>>>> -                               QEMU_ALIGN_UP(job->copy_range_size,
>>>> -                                             job->cluster_size));
>>>> +    job->copy_range_size = QEMU_ALIGN_DOWN(job->copy_range_size,
>>>> +                                           job->cluster_size);
>>>> +    /*
>>>> +     * Compression is not supported for copy_range. Also, we don't want to
>>>> +     * handle small max_transfer for copy_range (which currently don't
>>>> +     * handle max_transfer at all).
>>>> +     */
>>>> +    job->use_copy_range = !compress && job->copy_range_size > 0;
>>>>        /* Required permissions are already taken with target's blk_new() */
>>>>        block_job_add_bdrv(&job->common, "target", target, 0, BLK_PERM_ALL,
>>>>
>>>
>>> I'm clear on the alignment fix, I'm not clear on the comment about max_transfer and how it relates to copy_range_size being non-zero.
>>>
>>> "small max transfer" -- what happens when it's zero? we're apparently OK with a single cluster, but when it's zero, what happens?
>>
>> if it zero it means that source or target requires max_transfer less than cluster_size. It seems not valid to call copy_range in this case.
>> Still it's OK to use normal read/write, as they handle max_transfer internally in a loop (copy_range doesn't do it).
>>
> 
> oh, I'm ... sorry, I just didn't quite understand the comment.
> 
> You're just making sure copy_range after all of our checks is non-zero,
> plain and simple. If max_transfer was *smaller than a job cluster*, we
> might end up with a copy_range size that's zero, which is obviously...
> not useful.
> 
> So, I might phrase "Also, we don't want to..." as:
> 
> "copy_range does not respect max_transfer, so we factor that in here. If
> it's smaller than the job->cluster_size, we are unable to use copy_range."


We actually able to: just using a loop and calling copy_range several times. May be just:

copy_range does not respect max_transfer, so we factor that in here. If
it's smaller than the job->cluster_size, we do not use copy_range.

> 
> Just a suggestion, though, so:
> 
> Reviewed-by: John Snow <jsnow@redhat.com>
> 
> 
> (SHOULD copy_range respect max_transfer? I guess it would be quite
> different -- it would only count things it had to fall back and actually
> *transfer*, right? I suppose that because it can have that fallback we
> need to accommodate it here in backup.c, hence this workaround clamp.)
> 


-- 
Best regards,
Vladimir

[Qemu-devel] [PATCH v12 2/2] block/backup: fix backup_cow_with_offload for last cluster

[Vladimir Sementsov-Ogievskiy]

We shouldn't try to copy bytes beyond EOF. Fix it.

Fixes: 9ded4a0114968e
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/backup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/backup.c b/block/backup.c
index d8fdbfadfe..89f7f89200 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -161,7 +161,7 @@ static int coroutine_fn backup_cow_with_offload(BackupBlockJob *job,
 
     assert(QEMU_IS_ALIGNED(job->copy_range_size, job->cluster_size));
     assert(QEMU_IS_ALIGNED(start, job->cluster_size));
-    nbytes = MIN(job->copy_range_size, end - start);
+    nbytes = MIN(job->copy_range_size, MIN(end, job->len) - start);
     nr_clusters = DIV_ROUND_UP(nbytes, job->cluster_size);
     bdrv_reset_dirty_bitmap(job->copy_bitmap, start,
                             job->cluster_size * nr_clusters);
-- 
2.21.0

Re: [Qemu-devel] [PATCH v12 2/2] block/backup: fix backup_cow_with_offload for last cluster

[John Snow]

On 9/17/19 12:07 PM, Vladimir Sementsov-Ogievskiy wrote:
> We shouldn't try to copy bytes beyond EOF. Fix it.
> 
> Fixes: 9ded4a0114968e
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>   block/backup.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index d8fdbfadfe..89f7f89200 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -161,7 +161,7 @@ static int coroutine_fn backup_cow_with_offload(BackupBlockJob *job,
>   
>       assert(QEMU_IS_ALIGNED(job->copy_range_size, job->cluster_size));
>       assert(QEMU_IS_ALIGNED(start, job->cluster_size));
> -    nbytes = MIN(job->copy_range_size, end - start);
> +    nbytes = MIN(job->copy_range_size, MIN(end, job->len) - start);

I'm a little confused. I think the patch as written is correct, but I 
don't know what problem it solves.

If we're going to allow the caller to pass in an end that's beyond EOF, 
does that mean we are *requiring* the caller to pass in a value that's 
aligned?

We should probably assert what kind of a value we're accepted here, 
right? We do for start, but should we for 'end' as well?

Then ...

>       nr_clusters = DIV_ROUND_UP(nbytes, job->cluster_size);

Don't we just round this right back up immediately anyway? Does that 
mean we have callers that are passing in an 'end' that's more than 1 
job-cluster beyond EOF? That seems like something that should be fixed 
in the caller, surely?

>       bdrv_reset_dirty_bitmap(job->copy_bitmap, start,
>                               job->cluster_size * nr_clusters);
> 

Re: [Qemu-devel] [PATCH v12 2/2] block/backup: fix backup_cow_with_offload for last cluster

[Vladimir Sementsov-Ogievskiy]

18.09.2019 23:14, John Snow wrote:
> 
> 
> On 9/17/19 12:07 PM, Vladimir Sementsov-Ogievskiy wrote:
>> We shouldn't try to copy bytes beyond EOF. Fix it.
>>
>> Fixes: 9ded4a0114968e
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/backup.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/block/backup.c b/block/backup.c
>> index d8fdbfadfe..89f7f89200 100644
>> --- a/block/backup.c
>> +++ b/block/backup.c
>> @@ -161,7 +161,7 @@ static int coroutine_fn backup_cow_with_offload(BackupBlockJob *job,
>>       assert(QEMU_IS_ALIGNED(job->copy_range_size, job->cluster_size));
>>       assert(QEMU_IS_ALIGNED(start, job->cluster_size));
>> -    nbytes = MIN(job->copy_range_size, end - start);
>> +    nbytes = MIN(job->copy_range_size, MIN(end, job->len) - start);
> 
> I'm a little confused. I think the patch as written is correct, but I don't know what problem it solves.

last cluster may exceed EOF. And backup_do_cow (who calls  backup_cow_with_offload) rounds all to clusters.
It's not bad, but we need to crop nbytes before calling actual io functions. backup_cow_with_bounce_buffer does the same thing.

> 
> If we're going to allow the caller to pass in an end that's beyond EOF, does that mean we are *requiring* the caller to pass in a value that's aligned?

Actually yes, as we are resetting dirty bitmap.

> 
> We should probably assert what kind of a value we're accepted here, right? We do for start, but should we for 'end' as well?

Yes assertion may be added.

> 
> Then ...
> 
>>       nr_clusters = DIV_ROUND_UP(nbytes, job->cluster_size);
> 
> Don't we just round this right back up immediately anyway? Does that mean we have callers that are passing in an 'end' that's more than 1 job-cluster beyond EOF? That seems like something that should be fixed in the caller, surely?

nr_clusters are used to set/reset dirty bitmap. It's OK. Still, for last cluster we can drop it and use nbytes directly. No there should not be such callers.
nbytes is used to call blk_co_copy_range, and must be cropped to not exceed EOF.

Also, backup_cow_with_bounce_buffer behave in similar way: it crops nbytes.

Of course, there is a place for good refactoring, but I think not in this patch, it's a small bug fix.

> 
>>       bdrv_reset_dirty_bitmap(job->copy_bitmap, start,
>>                               job->cluster_size * nr_clusters);
>>
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v12 2/2] block/backup: fix backup_cow_with_offload for last cluster

[John Snow]

On 9/19/19 3:02 AM, Vladimir Sementsov-Ogievskiy wrote:
> 18.09.2019 23:14, John Snow wrote:
>>
>>
>> On 9/17/19 12:07 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> We shouldn't try to copy bytes beyond EOF. Fix it.
>>>
>>> Fixes: 9ded4a0114968e
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>>   block/backup.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/block/backup.c b/block/backup.c
>>> index d8fdbfadfe..89f7f89200 100644
>>> --- a/block/backup.c
>>> +++ b/block/backup.c
>>> @@ -161,7 +161,7 @@ static int coroutine_fn backup_cow_with_offload(BackupBlockJob *job,
>>>       assert(QEMU_IS_ALIGNED(job->copy_range_size, job->cluster_size));
>>>       assert(QEMU_IS_ALIGNED(start, job->cluster_size));
>>> -    nbytes = MIN(job->copy_range_size, end - start);
>>> +    nbytes = MIN(job->copy_range_size, MIN(end, job->len) - start);
>>
>> I'm a little confused. I think the patch as written is correct, but I don't know what problem it solves.
> 
> last cluster may exceed EOF. And backup_do_cow (who calls  backup_cow_with_offload) rounds all to clusters.
> It's not bad, but we need to crop nbytes before calling actual io functions. backup_cow_with_bounce_buffer does the same thing.
> 
>>
>> If we're going to allow the caller to pass in an end that's beyond EOF, does that mean we are *requiring* the caller to pass in a value that's aligned?
> 
> Actually yes, as we are resetting dirty bitmap.
> 
>>
>> We should probably assert what kind of a value we're accepted here, right? We do for start, but should we for 'end' as well?
> 
> Yes assertion may be added.
> 
>>
>> Then ...
>>
>>>       nr_clusters = DIV_ROUND_UP(nbytes, job->cluster_size);
>>
>> Don't we just round this right back up immediately anyway? Does that mean we have callers that are passing in an 'end' that's more than 1 job-cluster beyond EOF? That seems like something that should be fixed in the caller, surely?
> 
> nr_clusters are used to set/reset dirty bitmap. It's OK. Still, for last cluster we can drop it and use nbytes directly. No there should not be such callers.
> nbytes is used to call blk_co_copy_range, and must be cropped to not exceed EOF.
> 

Ah, right, right ... I *was* confused. We don't use nr_clusters for the
IO itself, just the bitmap. So we effectively re-calculate aligned and
unaligned values for use in different places.

> Also, backup_cow_with_bounce_buffer behave in similar way: it crops nbytes.
> 
> Of course, there is a place for good refactoring, but I think not in this patch, it's a small bug fix.
> 
>>
>>>       bdrv_reset_dirty_bitmap(job->copy_bitmap, start,
>>>                               job->cluster_size * nr_clusters);
>>>
>>
> 
> 

We should make the interface here a little more clear I think, but what
you wrote is correct.

Reviewed-by: John Snow <jsnow@redhat.com>