Re: [PATCH v2 1/7] block/block-copy: specialcase first copy_range request

[Vladimir Sementsov-Ogievskiy]

07.02.2020 20:28, Max Reitz wrote:
> On 27.11.19 19:08, Vladimir Sementsov-Ogievskiy wrote:
> > In block_copy_do_copy we fallback to read+write if copy_range failed.
> > In this case copy_size is larger than defined for buffered IO, and
> > there is corresponding commit. Still, backup copies data cluster by
> > cluster, and most of requests are limited to one cluster anyway, so the
> > only source of this one bad-limited request is copy-before-write
> > operation.
> >
> > Further patch will move backup to use block_copy directly, than for
> > cases where copy_range is not supported, first request will be
> > oversized in each backup. It's not good, let's change it now.
> >
> > Fix is simple: just limit first copy_range request like buffer-based
> > request. If it succeed, set larger copy_range limit.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > ---
> >   block/block-copy.c | 41 ++++++++++++++++++++++++++++++-----------
> >   1 file changed, 30 insertions(+), 11 deletions(-)
> >
> > diff --git a/block/block-copy.c b/block/block-copy.c
> > index 79798a1567..8602e2cae7 100644
> > --- a/block/block-copy.c
> > +++ b/block/block-copy.c
>
> [...]
>
> > @@ -168,7 +170,21 @@ static int coroutine_fn block_copy_do_copy(BlockCopyState 
> > *s,
> >               s->use_copy_range = false;
> >               s->copy_size = MAX(s->cluster_size, BLOCK_COPY_MAX_BUFFER);
> >               /* Fallback to read+write with allocated buffer */
> > -        } else {
> > +        } else if (s->use_copy_range) {
> > +            /*
> > +             * Successful copy-range. Now increase copy_size.
> > +             * copy_range does not respect max_transfer (it's a TODO), so we
> > +             * factor that in here.
> > +             *
> > +             * Note: we double-check s->use_copy_range for the case when
> > +             * parallel block-copy request unset it during previous
> > +             * bdrv_co_copy_range call.
>
> But we should still goto out anyway, shouldn’t we?

Oops yes, will fix.

> > +             */
> > +            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));
> >               goto out;
> >           }
> >       }
> > @@ -176,7 +192,10 @@ static int coroutine_fn block_copy_do_copy(BlockCopyState 
> > *s,
> >       /*
> >        * In case of failed copy_range request above, we may proceed with 
> > buffered
> >        * request larger than BLOCK_COPY_MAX_BUFFER. Still, further requests will
> > -     * be properly limited, so don't care too much.
> > +     * be properly limited, so don't care too much. Moreover the most possible
>
> s/possible/likely/
>
> Max
>
> > +     * case (copy_range is unsupported for the configuration, so the very first
> > +     * copy_range request fails) is handled by setting large copy_size only
> > +     * after first successful copy_range.
> >        */
> >   
> >       bounce_buffer = qemu_blockalign(s->source->bs, nbytes);


--
Best regards,
Vladimir