Re: [PATCH v3 2/2] qemu-img: Make unallocated part of backing chain obvious in map

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

01.07.2021 22:06, Eric Blake wrote:
> The recently-added NBD context qemu:allocation-depth is able to
> distinguish between locally-present data (even when that data is
> sparse) [shown as depth 1 over NBD], and data that could not be found
> anywhere in the backing chain [shown as depth 0]; and the libnbd
> project was recently patched to give the human-readable name "absent"
> to an allocation-depth of 0.  But qemu-img map --output=json predates
> that addition, and has the unfortunate behavior that all portions of
> the backing chain that resolve without finding a hit in any backing
> layer report the same depth as the final backing layer.  This makes it
> harder to reconstruct a qcow2 backing chain using just 'qemu-img map'
> output, especially when using "backing":null to artificially limit a
> backing chain, because it is impossible to distinguish between a
> QCOW2_CLUSTER_UNALLOCATED (which defers to a [missing] backing file)
> and a QCOW2_CLUSTER_ZERO_PLAIN cluster (which would override any
> backing file), since both types of clusters otherwise show as
> "data":false,"zero":true" (but note that we can distinguish a
> QCOW2_CLUSTER_ZERO_ALLOCATED, which would also have an "offset":
> listing).
> 
> The task of reconstructing a qcow2 chain was made harder in commit
> 0da9856851 (nbd: server: Report holes for raw images), because prior
> to that point, it was possible to abuse NBD's block status command to
> see which portions of a qcow2 file resulted in BDRV_BLOCK_ALLOCATED
> (showing up as NBD_STATE_ZERO in isolation) vs. missing from the chain
> (showing up as NBD_STATE_ZERO|NBD_STATE_HOLE); but now qemu reports
> more accurate sparseness information over NBD.
> 
> An obvious solution is to make 'qemu-img map --output=json' add an
> additional "present":false designation to any cluster lacking an
> allocation anywhere in the chain, without any change to the "depth"
> parameter to avoid breaking existing clients.  The iotests have
> several examples where this distinction demonstrates the additional
> accuracy.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   docs/tools/qemu-img.rst                       |   3 +
>   qapi/block-core.json                          |   7 +-
>   qemu-img.c                                    |   7 +-
>   tests/qemu-iotests/122.out                    |  84 ++++----
>   tests/qemu-iotests/154.out                    | 190 +++++++++---------
>   tests/qemu-iotests/179.out                    | 133 ++++++++----
>   tests/qemu-iotests/223.out                    |  56 +++---
>   tests/qemu-iotests/244.out                    |  23 ++-
>   tests/qemu-iotests/252.out                    |  10 +-
>   tests/qemu-iotests/274.out                    |  48 ++---
>   tests/qemu-iotests/291.out                    |  24 +--
>   .../tests/nbd-qemu-allocation.out             |  16 +-
>   12 files changed, 330 insertions(+), 271 deletions(-)
> 
> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
> index cfe11478791f..d6300f7ee03d 100644
> --- a/docs/tools/qemu-img.rst
> +++ b/docs/tools/qemu-img.rst
> @@ -597,6 +597,9 @@ Command description:
>       if false, the sectors are either unallocated or stored as optimized
>       all-zero clusters);
>     - whether the data is known to read as zero (boolean field ``zero``);
> +  - whether the data is actually present (boolean field ``present``);
> +    if false, rebasing the backing chain onto a deeper file would pick
> +    up data from the deeper file;

Preexisting, but rather strange style of documentation, when described option doesn't go first in the paragraph..

>     - in order to make the output shorter, the target file is expressed as
>       a ``depth``; for example, a depth of 2 refers to the backing file
>       of the backing file of *FILENAME*.
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index a54f37dbef06..912a7d9265e5 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -261,6 +261,9 @@
>   #         images in the chain)) before reaching one for which the
>   #         range is allocated
>   #
> +# @present: true if this layer provides the data, false if adding a backing
> +#           layer could impact this region (since 6.1)
> +#
>   # @offset: if present, the image file stores the data for this range
>   #          in raw format at the given (host) offset
>   #
> @@ -271,8 +274,8 @@
>   ##
>   { 'struct': 'MapEntry',
>     'data': {'start': 'int', 'length': 'int', 'data': 'bool',
> -           'zero': 'bool', 'depth': 'int', '*offset': 'int',
> -           '*filename': 'str' } }
> +           'zero': 'bool', 'depth': 'int', 'present': 'bool',
> +           '*offset': 'int', '*filename': 'str' } }
> 
>   ##
>   # @BlockdevCacheInfo:
> diff --git a/qemu-img.c b/qemu-img.c
> index 7956a8996512..8090facc5087 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -2980,8 +2980,9 @@ static int dump_map_entry(OutputFormat output_format, MapEntry *e,
>           break;
>       case OFORMAT_JSON:
>           printf("{ \"start\": %"PRId64", \"length\": %"PRId64","
> -               " \"depth\": %"PRId64", \"zero\": %s, \"data\": %s",
> -               e->start, e->length, e->depth,
> +               " \"depth\": %"PRId64", \"present\": %s, \"zero\": %s,"
> +               "\"data\": %s", e->start, e->length, e->depth,
> +               e->present ? "true" : "false",

Didn't you want to put present at the end? Still, this shouldn't be significant. And it make sense to keep present, zero and data together.

You missied a whitespace after '"zero": %s,', which is obvious from further test diff hunks.

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

>                  e->zero ? "true" : "false",
>                  e->data ? "true" : "false");
>           if (e->has_offset) {
> @@ -3047,6 +3048,7 @@ static int get_block_status(BlockDriverState *bs, int64_t offset,
>           .offset = map,
>           .has_offset = has_offset,
>           .depth = depth,
> +        .present = !!(ret & BDRV_BLOCK_ALLOCATED),
>           .has_filename = filename,
>           .filename = filename,
>       };
> @@ -3062,6 +3064,7 @@ static inline bool entry_mergeable(const MapEntry *curr, const MapEntry *next)
>       if (curr->zero != next->zero ||
>           curr->data != next->data ||
>           curr->depth != next->depth ||
> +        curr->present != next->present ||
>           curr->has_filename != next->has_filename ||
>           curr->has_offset != next->has_offset) {
>           return false;
> diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out
> index 3a3e121d579d..83dfcf75281f 100644
> --- a/tests/qemu-iotests/122.out
> +++ b/tests/qemu-iotests/122.out
> @@ -67,12 +67,12 @@ read 65536/65536 bytes at offset 4194304

[..]

-- 
Best regards,
Vladimir