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 <address@hidden>
>>>> ---
>>>>    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 <address@hidden>
> 
> 
> (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