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

[Eric Blake]

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

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

Yes, 'from' sounds nicer, especially since the size requirements being 
measured depend on the destination's cluster size (which may be 
different from the source's cluster size).

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

> > +    /*
> > +     * 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?

If the source format does not support bitmaps, there is nothing to copy 
over.  Reporting '0' would work, but adds verbosity.  It also becomes a 
question as to whether 'qemu-img convert --bitmaps' should silently 
ignore such sources, or loudly error out that the option is unsupported 
because the source lacks bitmaps.  I could lean either way.

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

The impact to the iotests .out files is larger if I do not require that 
the source supports bitmaps (more lines of 'bitmaps: 0' added).  I'm 
fine doing that, if we decide we're okay with the simpler definition of 
'"bitmaps" is present if the destination supports them' (rather than 
this version's implementation of '"bitmaps" is present if both the 
source and destination support them').

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

In fact, that argument is a GOOD reason to output 'bitmaps: 0' in as 
many cases as possible, because it then becomes a side-effect witness of 
whether 'qemu-img convert --bitmaps' is even understood.

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

Yes, I can make that tweak for v4.

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

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