On Mon, Jul 27, 2020 at 5:04 PM Eric Blake <ebl...@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.
I started with ignoring ENOTSUP in qcow2, but felt less safe about this approach since the same issue may happen in other flows, and making nbd driver behave like a block device looks like a safer change. > > 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 <nsof...@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). > > 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. I'll post v2 with the test fixes later today. > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3226 > Virtualization: qemu.org | libvirt.org >