Re: [Qemu-stable] [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 <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).

-- 
Best regards,
Vladimir