Re: [PATCH 6/6] block/block-copy: increase buffered copy request

[Vladimir Sementsov-Ogievskiy]

07.10.2019 18:48, Max Reitz wrote:
> On 03.10.19 19:15, Vladimir Sementsov-Ogievskiy wrote:
>> No reason to limit buffered copy to one cluster. Let's allow up to 1
>> MiB.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>> ---
>>   include/block/block-copy.h |  2 +-
>>   block/block-copy.c         | 44 +++++++++++++++++++++++++++-----------
>>   2 files changed, 32 insertions(+), 14 deletions(-)
> 
> 
> Er, oops, looks like I was a bit quick there...
> 
>> @@ -100,17 +101,28 @@ BlockCopyState *block_copy_state_new(BdrvChild 
>> *source, BdrvChild *target,
>>           .mem = qemu_co_shared_amount_new(BLOCK_COPY_MAX_MEM),
>>       };
>>   
>> -    s->copy_range_size = QEMU_ALIGN_DOWN(max_transfer, cluster_size),
>> -    /*
>> -     * Set use_copy_range, consider the following:
>> -     * 1. Compression is not supported for copy_range.
>> -     * 2. copy_range does not respect max_transfer (it's a TODO), so we 
>> factor
>> -     *    that in here. If max_transfer is smaller than the 
>> job->cluster_size,
>> -     *    we do not use copy_range (in that case it's zero after aligning 
>> down
>> -     *    above).
>> -     */
>> -    s->use_copy_range =
>> -        !(write_flags & BDRV_REQ_WRITE_COMPRESSED) && s->copy_range_size > 
>> 0;
>> +    if (max_transfer < cluster_size) {
>> +        /*
>> +         * copy_range does not respect max_transfer. We don't want to bother
>> +         * with requests smaller than block-copy cluster size, so fallback 
>> to
>> +         * buffered copying (read and write respect max_transfer on their
>> +         * behalf).
>> +         */
>> +        s->use_copy_range = false;
>> +        s->copy_size = cluster_size;
>> +    } else if (write_flags & BDRV_REQ_WRITE_COMPRESSED) {
>> +        /* Compression is not supported for copy_range */
>> +        s->use_copy_range = false;
>> +        s->copy_size = MAX(cluster_size, BLOCK_COPY_MAX_BUFFER);
>> +    } else {
>> +        /*
>> +         * copy_range does not respect max_transfer (it's a TODO), so we 
>> factor
>> +         * that in here.
>> +         */
>> +        s->use_copy_range = true;
>> +        s->copy_size = MIN(MAX(cluster_size, BLOCK_COPY_MAX_COPY_RANGE),
> 
> This is already part of max_transfer, isn’t it?
> 
> (That doesn’t make it wrong, but I think max_transfer will always be
> less than or equal to MAX(cluster_size, BLOCK_COPY_MAX_COPY_RANGE) anyway.)
> 
> Max

Hmm, oops, yes. It'd better be just QEMU_ALIGN_DOWN(max_transfer, cluster_size)

> 
>> +                           QEMU_ALIGN_DOWN(max_transfer, cluster_size));
> 


-- 
Best regards,
Vladimir