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

[Eric Blake <eblake@redhat.com>]

On 7/27/20 4:58 PM, 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

in a call

> call always fails for the nbd driver since it does not implement
> bdrv_co_truncate().
> 
> For block devices, which have the same limits, the call succeeds since
> file driver implements bdrv_co_truncate(). If the caller asked to

since the file

> truncate to the same or smaller size with exact=false, the truncate
> succeeds. Implement the same logic for nbd.
> 
> Example failing without this change:
> 
> In one shell starts qemu-nbd:

start

> 
> $ truncate -s 1g test.tar
> $ qemu-nbd --socket=/tmp/nbd.sock --persistent --format=raw --offset 1536 test.tar
> 
> In another shell convert an image to qcow2 compressed via NBD:
> 
> $ echo "disk data" > disk.raw
> $ truncate -s 1g disk.raw
> $ qemu-img convert -f raw -O qcow2 -c disk1.raw nbd+unix:///?socket=/tmp/nbd.sock; echo $?
> 1
> 
> qemu-img failed, but the conversion was successful:
> 
> $ qemu-img info nbd+unix:///?socket=/tmp/nbd.sock
> image: nbd+unix://?socket=/tmp/nbd.sock
> file format: qcow2
> virtual size: 1 GiB (1073741824 bytes)
> ...
> 
> $ qemu-img check nbd+unix:///?socket=/tmp/nbd.sock
> No errors were found on the image.
> 1/16384 = 0.01% allocated, 100.00% fragmented, 100.00% compressed clusters
> Image end offset: 393216
> 
> $ qemu-img compare disk.raw nbd+unix:///?socket=/tmp/nbd.sock
> Images are identical.
> 
> Fixes: https://bugzilla.redhat.com/1860627
> Signed-off-by: Nir Soffer <nsoffer@redhat.com>
> ---
>   block/nbd.c | 30 ++++++++++++++++++++++++++++++
>   1 file changed, 30 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 65a4f56924..dcb0b03641 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -1966,6 +1966,33 @@ static void nbd_close(BlockDriverState *bs)
>       nbd_clear_bdrvstate(s);
>   }
>   
> +/*
> + * NBD cannot truncate, but if the caller asks to truncate to the same size, or
> + * to a smaller size with exact=false, there is no reason to fail the
> + * operation.
> + *
> + * Preallocation mode is ignored since it does not seems useful to fail when
> + * when never change anything.

s/when when/when we/

I tested with a quick hack to qemu-io-cmds.c:

diff --git i/qemu-io-cmds.c w/qemu-io-cmds.c
index 851f07e8f8b9..baeae86d8c85 100644
--- i/qemu-io-cmds.c
+++ w/qemu-io-cmds.c
@@ -1715,7 +1715,7 @@ static int truncate_f(BlockBackend *blk, int argc, 
char **argv)
       * exact=true.  It is better to err on the "emit more errors" side
       * than to be overly permissive.
       */
-    ret = blk_truncate(blk, offset, true, PREALLOC_MODE_OFF, 0, 
&local_err);
+    ret = blk_truncate(blk, offset, false, PREALLOC_MODE_OFF, 0, 
&local_err);
      if (ret < 0) {
          error_report_err(local_err);
          return ret;

[We should _really_ improve that command to take options, so you can 
control whether to be exact and what prealloc mode on the fly rather 
than hard-coded, but that's a different patch for a later day]

and with that hack in place, I observed:
$ truncate --size=3m file
$ ./qemu-nbd -f raw file
$ ./qemu-io -f raw nbd://localhost:10809
qemu-io> length
3 MiB
qemu-io> truncate 2m
qemu-io> length
3 MiB
qemu-io> truncate 4m
qemu-io: Cannot grow NBD nodes
qemu-io> length
3 MiB

so my initial worry that qemu would see the shrunken size, and therefore 
a later resize back up to the actual NBD limit would have to pay 
attention to preallocation mode for that portion of the file, appears to 
not matter.  But if we can find a case where it does matter, I guess 
it's still appropriate for a followup for -rc3.  Meanwhile, I'm queuing 
this for -rc2.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org