Re: [PATCH 5/5] block/io: auto-no-fallback for write-zeroes

[Vladimir Sementsov-Ogievskiy]

14.03.2020 0:56, Eric Blake wrote:
> On 3/2/20 4:05 AM, Vladimir Sementsov-Ogievskiy wrote:
> > NBD driver may has max_pwrite_zeroes but doesn't has
> > max_pwrite_zeroes_no_fallback limit. This means, that (when
> > BDRV_REQ_NO_FALLBACK is supported) it is beneficial to try send request
> > with BDRV_REQ_NO_FALLBACK instead of splitting the request accordingly
> > to max_pwrite_zeroes.
> >
> > If failed, fallback to old behavior.
>
> Grammar:
>
> When BDRV_REQ_NO_FALLBACK is supported, the NBD driver supports a larger 
> request size.  Add code to try large zero requests with a NO_FALLBACK request 
> prior to having to split a request into chunks according to max_pwrite_zeroes.
>
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > ---
> >   block/io.c | 30 ++++++++++++++++++++++++++----
> >   1 file changed, 26 insertions(+), 4 deletions(-)
> >
> > diff --git a/block/io.c b/block/io.c
> > index c64566b4cf..48d71b0883 100644
> > --- a/block/io.c
> > +++ b/block/io.c
> > @@ -1752,17 +1752,28 @@ static int coroutine_fn 
> > bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
> >       int head = 0;
> >       int tail = 0;
> > -    int max_write_zeroes = MIN_NON_ZERO((flags & BDRV_REQ_NO_FALLBACK) ?
> > -                                        bs->bl.max_pwrite_zeroes_no_fallback :
> > -                                        bs->bl.max_pwrite_zeroes, INT_MAX);
> > +    int max_write_zeroes;
> >       int alignment = MAX(bs->bl.pwrite_zeroes_alignment,
> >                           bs->bl.request_alignment);
> >       int max_transfer = MIN_NON_ZERO(bs->bl.max_transfer, MAX_BOUNCE_BUFFER);
> > +    bool auto_no_fallback;
> >       if (!drv) {
> >           return -ENOMEDIUM;
> >       }
> > +    if (!(flags & BDRV_REQ_NO_FALLBACK) &&
> > +        (bs->supported_zero_flags & BDRV_REQ_NO_FALLBACK) &&
> > +        bs->bl.max_pwrite_zeroes &&
> > +        bs->bl.max_pwrite_zeroes < bytes &&
> > +        (bs->bl.max_pwrite_zeroes < bs->bl.max_pwrite_zeroes_no_fallback ||
> > +         bs->bl.max_pwrite_zeroes_no_fallback == 0))
>
> Why are we letting max_pwrite_zeroes_no_fallback ever be 0?  It might be more 
> convenient if it is always guaranteed to be >= max_pwrite_zeroes by the block 
> layer.


All other limits may be 0 too, which means some default.. So, if we want to set 
the default for max_pwrite_zeroes_no_fallback explicitly, I think we should do 
it for all other limits too. And it should be separate thing..

> > +    {
> > +        assert(drv->bdrv_co_pwrite_zeroes);
> > +        flags |= BDRV_REQ_NO_FALLBACK;
> > +        auto_no_fallback = true;
> > +    }
> > +
> >       if ((flags & ~bs->supported_zero_flags) & BDRV_REQ_NO_FALLBACK) {
> >           return -ENOTSUP;
> >       }
> > @@ -1770,7 +1781,11 @@ static int coroutine_fn 
> > bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
> >       assert(alignment % bs->bl.request_alignment == 0);
> >       head = offset % alignment;
> >       tail = (offset + bytes) % alignment;
> > -    max_write_zeroes = QEMU_ALIGN_DOWN(max_write_zeroes, alignment);
> > +    max_write_zeroes =
> > +        QEMU_ALIGN_DOWN(MIN_NON_ZERO((flags & BDRV_REQ_NO_FALLBACK) ?
> > +                                     bs->bl.max_pwrite_zeroes_no_fallback :
> > +                                     bs->bl.max_pwrite_zeroes, INT_MAX),
> > +                        alignment);
> >       assert(max_write_zeroes >= bs->bl.request_alignment);
> >       while (bytes > 0 && !ret) {
> > @@ -1801,6 +1816,13 @@ static int coroutine_fn 
> > bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
> >           if (drv->bdrv_co_pwrite_zeroes) {
> >               ret = drv->bdrv_co_pwrite_zeroes(bs, offset, num,
> >                                                flags & 
> > bs->supported_zero_flags);
> > +            if (ret == -ENOTSUP && auto_no_fallback) {
> > +                flags &= ~BDRV_REQ_NO_FALLBACK;
> > +                max_write_zeroes =
> > +                    QEMU_ALIGN_DOWN(MIN_NON_ZERO(bs->bl.max_pwrite_zeroes,
> > +                                                 INT_MAX), alignment);
> > +                continue;
> > +            }
> >               if (ret != -ENOTSUP && (flags & BDRV_REQ_FUA) &&
> >                   !(bs->supported_zero_flags & BDRV_REQ_FUA)) {
> >                   need_flush = true;
>
> Otherwise makes sense.


--
Best regards,
Vladimir