Re: [PATCH 5/6] block/nbd: Do not force-cap *pnum

[Max Reitz]

On 19.06.21 12:53, Vladimir Sementsov-Ogievskiy wrote:
> 17.06.2021 18:52, Max Reitz wrote:
> > bdrv_co_block_status() does it for us, we do not need to do it here.
> >
> > The advantage of not capping *pnum is that bdrv_co_block_status() can
> > cache larger data regions than requested by its caller.
> >
> > Signed-off-by: Max Reitz <mreitz@redhat.com>
> > ---
> >   block/nbd.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/block/nbd.c b/block/nbd.c
> > index 616f9ae6c4..930bd234de 100644
> > --- a/block/nbd.c
> > +++ b/block/nbd.c
> > @@ -1702,7 +1702,7 @@ static int coroutine_fn 
> > nbd_client_co_block_status(
> >           .type = NBD_CMD_BLOCK_STATUS,
> >           .from = offset,
> >           .len = MIN(QEMU_ALIGN_DOWN(INT_MAX, bs->bl.request_alignment),
> > -                   MIN(bytes, s->info.size - offset)),
> > +                   s->info.size - offset),
> >           .flags = NBD_CMD_FLAG_REQ_ONE,
> >       };
>
> Hmm..
>
> I don't that this change is correct. In contrast with file-posix you 
> don't get extra information for free, you just make a larger request. 
> This means that server will have to do more work.

Oh, oops.  Seems I was blind in my rage to replace this MIN() pattern.

You’re absolutely right.  So this patch should be dropped.

Max

> (look at blockstatus_to_extents, it calls bdrv_block_status_above in a 
> loop).
>
> For example, assume that nbd export is a qcow2 image with all clusters 
> allocated. With this change, nbd server will loop through the whole 
> qcow2 image, load all L2 tables to return big allocated extent.
>
> So, only server can decide, could it add some extra free information 
> to request or not. But unfortunately NBD_CMD_FLAG_REQ_ONE doesn't 
> allow it.