Re: [PATCH 1/2] block: nbd: Fix convert qcow2 compressed to nbd

[Nir Soffer]

On Mon, Jul 27, 2020 at 5:04 PM Eric Blake <eblake@redhat.com> wrote:
>
> On 7/26/20 10:25 AM, Nir Soffer wrote:
> > When converting to qcow2 compressed format, the last step is a special
> > zero length compressed write, ending in call to bdrv_co_truncate(). This
> > call always fail for the nbd driver since it does not implement
>
> fails
>
> > bdrv_co_truncate().
>
> Arguably, qemu-img should be taught to ignore the failure, since it is
> not unique to the nbd driver. But I can live with your approach here.
>
> >
> > For block devices, which have the same limits, the call succeeds since
> > file driver implements bdrv_co_truncate(). If the caller asked to
> > truncate to the same or smaller size with exact=false, the truncate
> > succeeds. Implement the same logic for nbd.
> >
> > Example failing without this change:
> >
>
> >
> > Fixes: https://bugzilla.redhat.com/1860627
> > Signed-off-by: Nir Soffer <nsoffer@redhat.com>
> > ---
> >   block/nbd.c | 27 +++++++++++++++++++++++++++
> >   1 file changed, 27 insertions(+)
> >
> > diff --git a/block/nbd.c b/block/nbd.c
> > index 65a4f56924..2154113af3 100644
> > --- a/block/nbd.c
> > +++ b/block/nbd.c
> > @@ -1966,6 +1966,30 @@ static void nbd_close(BlockDriverState *bs)
> >       nbd_clear_bdrvstate(s);
> >   }
> >
> > +/*
> > + * NBD cannot truncate, but if the caller ask to truncate to the same 
> > size, or
>
> asks
>
> > + * to a smaller size with extact=false, there is not reason to fail the
>
> exact, no
>
> > + * operation.
> > + */
> > +static int coroutine_fn nbd_co_truncate(BlockDriverState *bs, int64_t 
> > offset,
> > +                                        bool exact, PreallocMode prealloc,
> > +                                        BdrvRequestFlags flags, Error 
> > **errp)
> > +{
> > +    BDRVNBDState *s = bs->opaque;
> > +
> > +    if (offset != s->info.size && exact) {
> > +        error_setg(errp, "Cannot resize NBD nodes");
> > +        return -ENOTSUP;
> > +    }
> > +
> > +    if (offset > s->info.size) {
> > +        error_setg(errp, "Cannot grow NBD nodes");
> > +        return -EINVAL;
> > +    }
> > +
> > +    return 0;
>
> Looks reasonable.  As Max said, I wonder if we want to reject particular
> preallocation modes (looking at block/file-posix.c:raw_co_truncate), in
> the case where the image was resized down and then back up (since
> s->info.size is constant, but the BDS size is not if inexact resize
> succeeds).

Do we want to fail if someone specifies -o preallocation={falloc,full}?

I see we convert DRV_REQ_MAY_UNMAP to NBD_CMD_FLAG_NO_HOLE
so using -o preallocation=falloc,full should be correct. But the last
request zero
length write request does not do anything, so failing does not look useful.

> As you have a bugzilla entry, I think this is safe for -rc2; I'll be
> touching up the typos and queuing it through my NBD tree later today.
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
>