Re: [PATCH v2 4/6] qcow2: Expose bitmaps' size during measure

[Max Reitz <mreitz@redhat.com>]

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

On 21.04.20 23:20, 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.  Update iotest 190 to
> cover it and a portion of the just-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.
> 
> 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              | 29 ++++++++++++++++++++++++++++-
>  block/raw-format.c         |  2 +-
>  qemu-img.c                 |  3 +++
>  tests/qemu-iotests/190     | 15 +++++++++++++--
>  tests/qemu-iotests/190.out | 13 ++++++++++++-
>  7 files changed, 68 insertions(+), 11 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 943df1926a91..b47c6d69ba27 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 not directly used
> +#           for guest contents,

Not sure how I feel about the “not directly used for guest contents”,
because it feels a bit superfluous, and it sounds like there might be
bitmap data that is directly used for guest contents.

>                                  when that metadata can be copied in addition
> +#           to guest contents. (since 5.1)
>  #
>  # Since: 2.10
>  ##
>  { 'struct': 'BlockMeasureInfo',
> -  'data': {'required': 'int', 'fully-allocated': 'int'} }
> +  'data': {'required': 'int', 'fully-allocated': 'int', '*bitmaps': 'int'} }

Why is @bitmaps optional?  I.e., what does absence mean, besides “not
supported yet”?

Right now, absence means anything in “format doesn’t support bitmaps, so
nothing can be copied”, “no input image given, so there’s no data on
bitmaps”, to “0 bytes to copy”.

I think in the latter case we should emit it as 0, maybe even in the
former case, because I think the fact that there won’t be any bitmap
data to copy might be interesting.  (And it’s also definitely 0, not
just “don’t know”.)

I suppose absence does make sense in case the user didn’t specify an
input image, because then we just really don’t know.

[...]

> diff --git a/block/qcow2.c b/block/qcow2.c
> index b524b0c53f84..9fd650928016 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c

[...]

> @@ -4739,6 +4742,28 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
>              goto err;
>          }
> 
> +        FOR_EACH_DIRTY_BITMAP(in_bs, bm) {
> +            if (bdrv_dirty_bitmap_get_persistence(bm)) {
> +                const char *name = bdrv_dirty_bitmap_name(bm);
> +                uint32_t granularity = bdrv_dirty_bitmap_granularity(bm);
> +                uint64_t bmbits = DIV_ROUND_UP(bdrv_dirty_bitmap_size(bm),
> +                                               granularity);
> +                uint64_t bmclusters = DIV_ROUND_UP(DIV_ROUND_UP(bmbits,
> +                                                                CHAR_BIT),

I suppose if we allowed CHAR_BIT to be anything but 8, it would be wrong
to use it here.  So maybe just a plain 8 would be more correct; although
I suppose CHAR_BIT kind of describes what constant we want here.

> +                                                   cluster_size);
> +
> +                /* Assume the entire bitmap is allocated */
> +                bitmaps_size += bmclusters * cluster_size;
> +                /* Also reserve space for the bitmap table entries */
> +                bitmaps_size += ROUND_UP(bmclusters * sizeof(uint64_t),
> +                                         cluster_size);
> +                /* Guess at contribution to bitmap directory size */
> +                bitmap_overhead += ROUND_UP(strlen(name) + 24,
> +                                            sizeof(uint64_t));

Seems not just a guess to me, but actually correct.  Maybe
bitmap_overhead should be called bitmap_directory_size (or
bitmap_dir_size, or bmap_dir_size, or whatever (but probably not bds!
:)), because that’s what it is.

> +            }
> +        }
> +        bitmaps_size += ROUND_UP(bitmap_overhead, cluster_size);
> +
>          virtual_size = ROUND_UP(ssize, cluster_size);
> 
>          if (has_backing_file) {

[...]

> diff --git a/tests/qemu-iotests/190.out b/tests/qemu-iotests/190.out
> index d001942002db..11962f972429 100644
> --- a/tests/qemu-iotests/190.out
> +++ b/tests/qemu-iotests/190.out
> @@ -1,5 +1,5 @@
>  QA output created by 190
> -== Huge file ==
> +== Huge file without bitmaps ==
> 
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2199023255552
>  required size: 2199023255552
> @@ -8,4 +8,15 @@ required size: 335806464
>  fully allocated size: 2199359062016
>  required size: 18874368
>  fully allocated size: 2199042129920
> +
> +== Huge file with bitmaps ==
> +
> +required size: 2199023255552
> +fully allocated size: 2199023255552
> +required size: 335806464
> +fully allocated size: 2199359062016
> +bitmaps size: 537198592
> +required size: 18874368
> +fully allocated size: 2199042129920
> +bitmaps size: 545259520

Looks correct.

(It might be nicer to calculate the reference value in the script,
because this way I had to verify it by hand, but, well, now I did verify
it, so...)

Max


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