Re: [PATCH v3 7/9] qcow2: Expose bitmaps' size during measure

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 7712 bytes --]

On 08.05.20 20:03, Eric Blake wrote:
> It's useful to know how much space can be occupied by qcow2 persistent
> bitmaps, even though such metadata is unrelated to the guest-visible
> data.  Report this value as an additional field, present when
> measuring an existing image and the output format supports bitmaps.
> Update iotest 178 and 190 to updated output, as well as new coverage
> in 190 demonstrating non-zero values made possible with the
> recently-added qemu-img bitmap command.
> 
> The addition of a new field demonstrates why we should always
> zero-initialize qapi C structs; while the qcow2 driver still fully
> populates all fields, the raw and crypto drivers had to be tweaked to
> avoid uninitialized data.
> 
> See also: https://bugzilla.redhat.com/1779904
> 
> Reported-by: Nir Soffer <nsoffer@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  qapi/block-core.json             | 15 +++++++----
>  block/crypto.c                   |  2 +-
>  block/qcow2.c                    | 37 ++++++++++++++++++++++++---
>  block/raw-format.c               |  2 +-
>  qemu-img.c                       |  3 +++
>  tests/qemu-iotests/178.out.qcow2 | 16 ++++++++++++
>  tests/qemu-iotests/190           | 43 ++++++++++++++++++++++++++++++--
>  tests/qemu-iotests/190.out       | 23 ++++++++++++++++-
>  8 files changed, 128 insertions(+), 13 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 943df1926a91..9a7a388c7ad3 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -633,18 +633,23 @@
>  # efficiently so file size may be smaller than virtual disk size.
>  #
>  # The values are upper bounds that are guaranteed to fit the new image file.
> -# Subsequent modification, such as internal snapshot or bitmap creation, may
> -# require additional space and is not covered here.
> +# Subsequent modification, such as internal snapshot or further bitmap
> +# creation, may require additional space and is not covered here.
>  #
> -# @required: Size required for a new image file, in bytes.
> +# @required: Size required for a new image file, in bytes, when copying just
> +#            guest-visible contents.
>  #
>  # @fully-allocated: Image file size, in bytes, once data has been written
> -#                   to all sectors.
> +#                   to all sectors, when copying just guest-visible contents.
> +#
> +# @bitmaps: Additional size required for bitmap metadata in a source image,

s/in/from/?  Otherwise it sounds like this would be about allocation in
the source, which it clear can’t be, but, well.

> +#           if that bitmap metadata can be copied in addition to guest
> +#           contents. (since 5.1)

[...]

> diff --git a/block/qcow2.c b/block/qcow2.c
> index 838d810ca5ec..f836a6047879 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c

[...]

> @@ -4849,16 +4875,21 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
>          required = virtual_size;
>      }
> 
> -    info = g_new(BlockMeasureInfo, 1);
> +    info = g_new0(BlockMeasureInfo, 1);
>      info->fully_allocated =
>          qcow2_calc_prealloc_size(virtual_size, cluster_size,
>                                   ctz32(refcount_bits)) + luks_payload_size;
> 
> -    /* Remove data clusters that are not required.  This overestimates the
> +    /*
> +     * Remove data clusters that are not required.  This overestimates the
>       * required size because metadata needed for the fully allocated file is
> -     * still counted.
> +     * still counted.  Show bitmaps only if both source and destination
> +     * would support them.
>       */
>      info->required = info->fully_allocated - virtual_size + required;
> +    info->has_bitmaps = version >= 3 && in_bs &&
> +        bdrv_dirty_bitmap_supported(in_bs);

Why is it important whether the source format supports persistent dirty
bitmaps?

I’m asking because I’d like there to be some concise reason when and why
the @bitmaps field appears.  “Whenever the target supports bitmaps” is
more concise than “When both source and target support bitmaps”.  Also,
the latter is not really different from “When any bitmap data can be
copied”, but in the latter case we should not show it when there are no
bitmaps in the source (even though the format supports them).

Or from the other perspective: As a user, I would never be annoyed by
the @bitmaps field being present.  I don’t mind a “0”.
OTOH, what information can it convey to me that it it’s optional and
sometimes not present?
I can see these cases:

- That the source format doesn’t support bitmaps?  I want to convert it
to something else anyway, so I don’t really care about what the source
format can or can’t do.

- That the destination doesn’t support bitmaps?  Ah, yes, the fact that
the bitmap field is missing might be a warning sign for this.

- That qemu is too old to copy bitmaps?  Same here.

- That there are no bitmaps in the source?  OK, but then I disregard the
@bitmaps field anyway, present or not.

So from that standpoint, the best use seems to me to take “The @bitmaps
field isn’t present” as kind of a warning that something in the convert
process won’t support copying bitmaps.  If it’s present, all is well.
So basically there’d be an iff relationship between “measure reports
@bitmaps” and “convert --bitmap can work”.

But the distinction between “the source format doesn’t support bitmaps”
and “the source image doesn’t have bitmaps” doesn’t seem that important
to me to make it visible in the interface.

[...]

> diff --git a/tests/qemu-iotests/190 b/tests/qemu-iotests/190
> index 6d41650438e1..1b5fff45bfcd 100755
> --- a/tests/qemu-iotests/190
> +++ b/tests/qemu-iotests/190

[...]

> @@ -51,6 +51,45 @@ $QEMU_IMG measure -O raw -f qcow2 "$TEST_IMG"
>  $QEMU_IMG measure -O qcow2 -o cluster_size=64k -f qcow2 "$TEST_IMG"
>  $QEMU_IMG measure -O qcow2 -o cluster_size=2M -f qcow2 "$TEST_IMG"
> 
> +echo
> +echo "== Huge file with bitmaps =="
> +echo
> +
> +$QEMU_IMG bitmap --add --granularity 512 -f qcow2 "$TEST_IMG" b1
> +$QEMU_IMG bitmap --add -g 2M -f qcow2 "$TEST_IMG" b2
> +
> +# No bitmap output, since raw does not support it
> +$QEMU_IMG measure -O raw -f qcow2 "$TEST_IMG"
> +# No bitmap output, since no bitmaps on raw source
> +$QEMU_IMG measure -O qcow2 -f raw "$TEST_IMG"
> +# No bitmap output, since v2 does not support it
> +$QEMU_IMG measure -O qcow2 -o compat=0.10 -f qcow2 "$TEST_IMG"
> +
> +# Compute expected output:
> +echo
> +val2T=$((2*1024*1024*1024*1024))
> +cluster=$((64*1024))
> +b1clusters=$(( (val2T/512/8 + cluster - 1) / cluster ))
> +b2clusters=$(( (val2T/2/1024/1024/8 + cluster - 1) / cluster ))
> +echo expected bitmap $((b1clusters * cluster +
> +			(b1clusters * 8 + cluster - 1) / cluster * cluster +
> +			b2clusters * cluster +
> +			(b2clusters * 8 + cluster - 1) / cluster * cluster +
> +			cluster))
> +$QEMU_IMG measure -O qcow2 -o cluster_size=64k -f qcow2 "$TEST_IMG"
> +
> +# Compute expected output:
> +echo
> +cluster=$((2*1024*1024))
> +b1clusters=$(( (val2T/512/8 + cluster - 1) / cluster ))
> +b2clusters=$(( (val2T/2/1024/1024/8 + cluster - 1) / cluster ))
> +echo expected bitmap $((b1clusters * cluster +
> +			(b1clusters * 8 + cluster - 1) / cluster * cluster +
> +			b2clusters * cluster +
> +			(b2clusters * 8 + cluster - 1) / cluster * cluster +
> +			cluster))
> +$QEMU_IMG measure -O qcow2 -o cluster_size=2M -f qcow2 "$TEST_IMG"

Thanks!

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]