Re: [PATCH v2 1/7] block-copy: streamline choice of copy_range vs. read/write

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

21.05.2021 18:10, Paolo Bonzini wrote:
> On 20/05/21 16:42, Vladimir Sementsov-Ogievskiy wrote:
>> 18.05.2021 13:07, Emanuele Giuseppe Esposito wrote:
>>> From: Paolo Bonzini <pbonzini@redhat.com>
>>>
>>> Put the logic to determine the copy size in a separate function, so
>>> that there is a simple state machine for the possible methods of
>>> copying data from one BlockDriverState to the other.
>>
>> Honestly, for me 4-state state-maching + function to determine copy-size doesn't seem better than two simple variables copy_size and use_copy_range.
> 
> There were six states before (2 for s->use_copy_range, three for s->copy_size),
> of which two were unused.  The heuristics for going between copy and read/write
> were quite illegible.
> 
>> What's the benefit of it?
> 
> Less duplication here, for example:
> 
> +    if (s->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;
> +        s->method = COPY_READ_WRITE_CLUSTER;
>        } else if (write_flags & BDRV_REQ_WRITE_COMPRESSED) {
>            /* Compression supports only cluster-size writes and no copy-range. */
> -        s->use_copy_range = false;
> -        s->copy_size = cluster_size;
> +        s->method = COPY_READ_WRITE_CLUSTER;
> 
> and here:
> 
>                trace_block_copy_copy_range_fail(s, offset, ret);
> -            s->use_copy_range = false;
> -            s->copy_size = MAX(s->cluster_size, BLOCK_COPY_MAX_BUFFER);
> +            s->method = COPY_READ_WRITE;
> ...
>            /*
>             * We enable copy-range, but keep small copy_size, until first
>             * successful copy_range (look at block_copy_do_copy).
>             */
> -        s->use_copy_range = use_copy_range;
> -        s->copy_size = MAX(s->cluster_size, BLOCK_COPY_MAX_BUFFER);
> +        s->method = use_copy_range ? COPY_RANGE_SMALL : COPY_READ_WRITE;
> 
> where it's not obvious that the two assignments to copy_size should be
> the same (and they're suboptimal, too, since they don't obey max_transfer).
> 
> ... plus...
> 
>>> While at it, store the common computation of block_copy_max_transfer
>>> into a new field of BlockCopyState, and make sure that we always
>>> obey max_transfer; that's more efficient even for the
>>> COPY_RANGE_READ_WRITE case.
> 
> having a function makes it easier to spot slight differences that are
> just bugs, such as this one.
> 
>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>   block/block-copy.c | 59 ++++++++++++++++++++++++++++++----------------
>>
>> stats agree with me, that its' not a simplification.
> 
> Stats don't say everything.  Not having something like this:
> 
>                  s->copy_size =
>                          MIN(MAX(s->cluster_size, BLOCK_COPY_MAX_COPY_RANGE),
>                              QEMU_ALIGN_DOWN(block_copy_max_transfer(s->source,
>                                                                      s->target),
>                                              s->cluster_size));
> 
> in the inner loop is already worth the extra lines for the
> function declaration, for example.


After my "[PATCH v2 00/33] block: publish backup-top filter" copy_range path becomes unused. I keep it thinking about further moving qemu-img convert to block-copy. But I don't even have a plan when to start this work. So, if we want to do something around copy_range here to prepare for thread-safety, let's just drop it for now as unused on top of "[PATCH v2 00/33] block: publish backup-top filter" (you can take one patch that drop copy_range support in backup to your series to not make a dependency). It's not difficult to reimplement it later.


-- 
Best regards,
Vladimir