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

[Eric Blake]

On 5/4/20 6:36 AM, Max Reitz wrote:
> 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 <address@hidden>
> > Signed-off-by: Eric Blake <address@hidden>
> > ---

> >   # @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.

Hmm. I was trying to make it obvious that bitmap size is separate from 
fully-allocated (and not double-counted), but you may have a point that 
just using "Additional size required for bitmap metadata, when that 
metadata can be copied in addition..." would work.

> >                                   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.

The patch will require a tweak to report an explicit 0 (when there is 
nothing to be copied), but doing that makes sense (explicit 0 for v3 
qcow2 images, and maybe even for v2, but omitted for other formats that 
have no bitmap support).


> > @@ -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.

POSIX requires CHAR_BIT to be 8.  (C99 allows some odd machines where 
CHAR_BIT is 9, 16, or even 32, but we don't ever try to port to such 
machines).

> > +== 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...)

Feels like duplicate work, which would require tweaking in two spots if 
we ever change our implementation or the qcow2 format is further 
enhanced, but it would also make it obvious that we are aware of the 
impact of such future changes. Okay, I'll see what I can add.

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