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

[Eric Blake]

On 5/12/20 6:10 AM, Max Reitz wrote:


> > This does not break old code since previously we always reported only
> > guest visible content
> > here, but it changes the semantics, and now you cannot allocate
> > "required" size, you need
> > to allocate "required" size with "bitmaps" size.
>
> Only if you copy the bitmaps, though, which requires using the --bitmaps
> switch for convert.

First, a usage question: would you rather that 'qemu-img convert 
--bitmaps' silently succeeds even when the image being converted has no 
bitmaps, or would you rather that it fails loudly when there are no 
bitmaps to be copied over?  As implemented in this patch series, patch 8 
currently silently succeeds.  But in order to make patch 7 and 8 
consistent with one another, I need to know which behavior is easier to 
use: failing convert if the source lacks bitmaps (and thus measure would 
suppress the bitmaps:0 output), or treating lack of bitmaps as nothing 
additional to copy and thereby succeeding (and thus measure should 
output bitmaps:0 to show that no additional space is needed because 
nothing else will be copied, successfully).

> > If we add a new
> > extension all users will have to
> > change the calculation again.
>
> It was my impression that existing users won’t have to do that, because
> they don’t use --bitmaps yet.
>
> In contrast, if we included the bitmap size in @required or
> @fully-allocated, then previous users that didn’t copy bitmaps to the
> destination (which they couldn’t) would allocate too much space.
>
> ...revisiting this after reading more of your mail: With a --bitmaps
> switch, existing users wouldn’t suffer from such compatibility problems.
>   However, then users (that now want to copy bitmaps) will have to pass
> the new --bitmaps flag in the command line, and I don’t see how that’s
> less complicated than just adding @bitmaps to @required.

More concretely, suppose down the road that we add the ability to copy 
internal snapshots (not that you want to mix internal and external 
snapshots, but that such information already exists and therefore can be 
used as an example).  Which is easier:

$ qemu-img measure -f qcow2 image.qcow2
required size: 8716288
fully allocated size: 8716288
bitmaps size: 1024
internal snapshot size: 2048

where you now have to add three numbers prior to creating dest.qcow2 and 
calling:

$ qemu-img convert image.qcow2 -f dest.qcow2 --bitmaps --snapshots

or using:

$ qemu-img measure --bitmaps --snapshots -f qcow2 image.qcow2
required size: 8719360
fully allocated size: 8719360

where you then call:

$ qemu-img convert image.qcow2 -f dest.qcow2 --bitmaps --snapshots

with a single size that matches the same arguments you pass to qemu-img 
convert?  What about failure cases?  What happens when qemu-img doesn't 
understand --snapshots but does understand --bitmaps?  Do you have to 
try a series of up to three calls to find how much information is supported:

$ qemu-img measure -f qcow2 image.qcow2 --bitmaps --snapshots
error: unknown argument
$ qemu-img measure -f qcow2 image.qcow2 --bitmaps
error: unknown argument
$ qemu-img measure -f qcow2 image.qcow2
data given, now you know that neither --bitmaps nor --snapshots will work

or is it nicer to issue just one measure without options, getting 
separate output lines, and seeing which output lines exist to know which 
convert options are supported, at the minor expense of having to add 
values yourself?

And then back to my question: should 'measure --bitmaps' fail if there 
are no bitmaps to be measured, or silently succeed and not change the 
output size?


> > With the current way, to measure an image we need to do:
> >
> > qemu-img info --output json ...
> > check if image contains bitmaps
> > qemu-img measure --output json ...
> > check if bitmaps are reported.

Why do you have to check 'qemu-img info' first?  If measure reports 
bitmaps, then you know bitmaps can be copied; if it doesn't, then you 
can check info as a fallback path to compute size yourself - but 
computing the size yourself doesn't help unless you also have fallbacks 
to copy the bitmaps via QMP commands, because that qemu-img will also 
lack 'qemu-img convert --bitmaps' or 'qemu-img bitmaps' to do it via 
qemu-img.

> > If image has bitmaps and bitmaps are not reported, we know that we have an old
> > version of qemu-img that does not know how to measure bitmaps.
>
> Well, but you’ll also see that convert --bitmaps will fail.  But I can
> see that you probably want to know about that before you create the
> target image.
>
> > If bitmaps are reported in both commands we will use the value when creating
> > block devices.
>
> And otherwise?  Do you error out, or do you just omit --bitmaps from
> convert?
>
> > If we always report bitmaps even when they are zero, we don't need to
> > run "qemu-img info".
> >
> > But  my preferred interface is:
> >
> >     qemu-img measure --bitmaps ...
> >
> > And report bitmaps only if the user asked to get the value. In this
> > case the required and
> > fully-allocated values will include bitmaps.
>
> Well, it would be consistent with the convert interface.  If you specify
> it for one, you specify it for the other.
>
> OTOH, this would mean having to pass around the @bitmaps bool in the
> block layer, which is a bit more difficult than just adding a new field
> in BlockMeasureInfo.  It would also mean to add a new bool every time we
> add a new extension (which you hinted at above that it might happen).

Or, that could be a CLI-only feature: the QMP interface _always_ reports 
bitmaps separately, but if 'qemu-img measure --bitmaps' is used, the CLI 
then adds that value in on your behalf after the QMP command but before 
printing to the end user.

> (We could also let img_measure() in qemu-img add @bitmaps to @required
> if --bitmaps was given, so we wouldn’t have to pass the bool around; but
> I think letting qemu-img fix up a QAPI object filled by the block driver
> sounds wrong.  (Because that means the block driver didn’t fill it
> correctly.))

If we only touch it up in the CLI, then we would have two forms of CLI 
output:

$ qemu-img measure -f qcow2 image.qcow2
required size: 8716288
fully allocated size: 8716288
bitmaps size: 1024
$ qemu-img measure -f qcow2 image.qcow2 --bitmaps
required size: 8717312
fully allocated size: 8717312

> And I don’t see how the interface proposed here by Eric (or rather, what
> I think we had agreed on for the next version) poses any problems for
> users.  If you want to copy bitmaps, you just use @required + @bitmaps.
>   (If @bitmaps isn’t present, you can’t copy bitmaps, so that should be
> an error.)  If you don’t want to copy bitmaps, you just use @required.
>
> (And if you want to copy bitmaps if present, you use @required +
> @bitmaps, and treat @bitmaps as 0 if not present.)
>
> > To learn if qemu-img measure understand bitmaps we can check --help
> > output, like we did
> > in the past until we can require the version on all platforms.
> >
> > An advantage is not having to change old tests.
> I personally don’t really consider that a significant advantage...  (On
> the contrary, seeing the field in all old tests means the code path is
> exercised more often, even though it’s supposed to always just report 0.)
>
> So all in all the main benefit I see in your proposal, i.e. having
> @bitmaps be included in @required with --bitmaps given, is that it would
> give a symmetrical interface between measure and convert: For simple
> cases, you can just replace the “convert” in your command line by
> “measure”, retrieve @required/@fully-allocated, create the target image
> based on that, and then re-run the same command line, but with “convert”
> this time.
>
> But I’m not sure whether that’s really an advantage in practice or more
> of a gimmick.  With Eric’s proposal, if you want to convert with
> bitmaps, just add @bitmaps to the target size.  If you don’t, don’t.  If
> you’d prefer to but don’t really care, add “@bitmaps ?: 0”.
>
> The benefit of Eric’s proposal (not including @bitmaps in @required or
> @fully-allocated) is that it doesn’t require passing an additional
> parameter to the block driver.  It also makes the definition of
> BlockMeasureInfo simpler.  With your proposal, it would need to be
> parameterized.  (As in, @required sometimes includes the bitmaps,
> sometimes it doesn’t, depending on the parameter used to retrieve
> BlockMeasureInfo.)  I’m not sure whether that even makes sense in the
> QAPI definition.

I'm leaning towards making v4 try a CLI-only 'measure --bitmaps', to see 
if I can speed the discussion along with concrete patches for comparison.

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