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

[Max Reitz]

On 12.05.20 12:17, Nir Soffer wrote:
> On Fri, May 8, 2020 at 9:03 PM Eric Blake <address@hidden> 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>
>> ---
>>  qapi/block-core.json             | 15 +++++++----
>>  block/crypto.c                   |  2 +-
>>  block/qcow2.c                    | 37 ++++++++++++++++++++++++---
>>  block/raw-format.c               |  2 +-
>>  qemu-img.c                       |  3 +++
>>  tests/qemu-iotests/178.out.qcow2 | 16 ++++++++++++
>>  tests/qemu-iotests/190           | 43 ++++++++++++++++++++++++++++++--
>>  tests/qemu-iotests/190.out       | 23 ++++++++++++++++-
>>  8 files changed, 128 insertions(+), 13 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 943df1926a91..9a7a388c7ad3 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.
> 
> 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.

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

> I think keeping the semantics that "required" and "fully-allocated"
> are the size you need based
> on the parameters of the command is easier to use and more consistent.
> Current the measure
> command contract is that invoking it with similar parameters as used
> in convert will give
> the right measurement needed for the convert command.
> 
>> +#
>> +# @bitmaps: Additional size required for bitmap metadata in a source image,
>> +#           if that bitmap metadata can be copied in addition to guest
>> +#           contents. (since 5.1)
> 
> Reporting bitmaps without specifying that bitmaps are needed is less 
> consistent
> with "qemu-img convert", but has the advantage that we don't need to
> check if measure
> supports bitmaps. But this will work only if new versions always
> report "bitmaps" even when
> the value is zero.
> 
> 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.
> 
> 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).

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

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.

Max

signature.asc
Description: OpenPGP digital signature