Re: [PATCH] nbd: server: Report holes for raw images

[Vladimir Sementsov-Ogievskiy]

19.02.2021 19:07, Nir Soffer wrote:
> When querying image extents for raw image, qemu-nbd reports holes as
> zero:
>
> $ qemu-nbd -t -r -f raw empty-6g.raw
>
> $ qemu-img map --output json nbd://localhost
> [{ "start": 0, "length": 6442450944, "depth": 0, "zero": true, "data": true, 
> "offset": 0}]
>
> $ qemu-img map --output json empty-6g.raw
> [{ "start": 0, "length": 6442450944, "depth": 0, "zero": true, "data": false, 
> "offset": 0}]
>
> Turns out that qemu-img map reports a hole based on BDRV_BLOCK_DATA, but
> nbd server reports a hole based on BDRV_BLOCK_ALLOCATED.
>
> The NBD protocol says:
>
>      NBD_STATE_HOLE (bit 0): if set, the block represents a hole (and
>      future writes to that area may cause fragmentation or encounter an
>      NBD_ENOSPC error); if clear, the block is allocated or the server
>      could not otherwise determine its status.

So, it makes sense to repot HOLE for lseek holes on raw files (as write may 
increase disk usage) and for qcow2 ZERO clusters: again, writing will lead to 
allocation of NORMAL cluster.

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> qemu-img manual says:
>
>      whether the sectors contain actual data or not (boolean field data;
>      if false, the sectors are either unallocated or stored as
>      optimized all-zero clusters);
>
> To me, data=false looks compatible with NBD_STATE_HOLE. From user point
> of view, getting same results from qemu-nbd and qemu-img is more
> important than being more correct about allocation status.
>
> Changing nbd server to report holes using BDRV_BLOCK_DATA makes qemu-nbd
> results compatible with qemu-img map:
>
> $ qemu-img map --output json nbd://localhost
> [{ "start": 0, "length": 6442450944, "depth": 0, "zero": true, "data": false, 
> "offset": 0}]
>
> Signed-off-by: Nir Soffer <nsoffer@redhat.com>
> ---
>   nbd/server.c               | 4 ++--
>   tests/qemu-iotests/241.out | 4 ++--
>   2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/nbd/server.c b/nbd/server.c
> index 7229f487d2..86a44a9b41 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -2087,8 +2087,8 @@ static int blockstatus_to_extents(BlockDriverState *bs, 
> uint64_t offset,
>               return ret;
>           }
>   
> -        flags = (ret & BDRV_BLOCK_ALLOCATED ? 0 : NBD_STATE_HOLE) |
> -                (ret & BDRV_BLOCK_ZERO      ? NBD_STATE_ZERO : 0);
> +        flags = (ret & BDRV_BLOCK_DATA ? 0 : NBD_STATE_HOLE) |
> +                (ret & BDRV_BLOCK_ZERO ? NBD_STATE_ZERO : 0);
>   
>           if (nbd_extent_array_add(ea, num, flags) < 0) {
>               return 0;
> diff --git a/tests/qemu-iotests/241.out b/tests/qemu-iotests/241.out
> index 75f9f465e5..3f8c173cc8 100644
> --- a/tests/qemu-iotests/241.out
> +++ b/tests/qemu-iotests/241.out
> @@ -5,7 +5,7 @@ QA output created by 241
>     size:  1024
>     min block: 1
>   [{ "start": 0, "length": 1000, "depth": 0, "zero": false, "data": true, 
> "offset": OFFSET},
> -{ "start": 1000, "length": 24, "depth": 0, "zero": true, "data": true, 
> "offset": OFFSET}]
> +{ "start": 1000, "length": 24, "depth": 0, "zero": true, "data": false, 
> "offset": OFFSET}]
>   1 KiB (0x400) bytes     allocated at offset 0 bytes (0x0)
>   
>   === Exporting unaligned raw image, forced server sector alignment ===
> @@ -23,6 +23,6 @@ WARNING: Image format was not specified for 'TEST_DIR/t.raw' 
> and probing guessed
>     size:  1024
>     min block: 1
>   [{ "start": 0, "length": 1000, "depth": 0, "zero": false, "data": true, 
> "offset": OFFSET},
> -{ "start": 1000, "length": 24, "depth": 0, "zero": true, "data": true, 
> "offset": OFFSET}]
> +{ "start": 1000, "length": 24, "depth": 0, "zero": true, "data": false, 
> "offset": OFFSET}]
>   1 KiB (0x400) bytes     allocated at offset 0 bytes (0x0)
>   *** done


--
Best regards,
Vladimir