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

[Max Reitz]

On 12.05.20 21:39, Eric Blake wrote:
> 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).

“Easier to use” in my opinion would be if it never fails, but the
question is whether that’s also the most useful approach.

I think it should match what I thought when @bitmaps should be reported,
i.e. it should be accepted whenever the target format (and qemu-img
convert, obviously) supports bitmaps.  I don’t think there’s value in
aborting just because the source doesn’t have bitmaps.  What’s the user
going to do in response?  “Oops, thanks, qemu-img, I accidentally tried
to convert the wrong image”?

(Whereas if we reject --bitmaps because the target format doesn’t
support it, there’s actually value, because the user is reminded that
they should choose a different target format.)

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

“Which is easier” is misleading, because both are actually trivial.
Adding three numbers is what a computer does best.

It’s a question of style, not of “which is easier”.

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

Sounds good to me, but then again, calling a command to see whether it
fails isn’t exactly hard either.

So it remains a question of style to me.

>From my perspective, measure is a query function similar to info.  It
should return all information that’s trivial to obtain.  Switches to
select what information to return how only make sense when that optional
information is hard to obtain (think qemu-img info --backing-chain).

Switches make sense on data-modifying operations like convert.  Not so
much for querying operations, because why not just return all
information you have and let the caller sort it out?

[...]

>>> 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
I would hope that programs consuming qemu-img measure’s output use
--output=json, and then you definitely can’t just ignore the QAPI object
definition.

Furthermore, I don’t like that 2-form at all, for two reasons.  First,
again, I can’t see a reasonable QAPI definition to match it; and second,
I don’t like it as a consistent interface.  We should decide on one
thing and do that.  Either you need to pass --bitmaps to make them
included in required, or they aren’t in the output at all.

The way you propose it with this 2-form approach makes --bitmap make
qemu-img be a calculator.   But why?  It’s absolutely trivial for
consumers to:

irb(main):002:1` result = JSON.parse(`./qemu-img measure --output=json \
irb(main):003:0>                                 -O qcow2 src.qcow2`)
irb(main):004:0* result['required'] +
irb(main):005:0>   (result['bitmaps'] ? result['bitmaps'] : 0)
=> 524288

(Example in Ruby, but I don’t think that matters much.)

There is no value in qemu-img optionally performing that operation for
the caller.  Adding two integers is the simplest thing there is.

Max

signature.asc
Description: OpenPGP digital signature