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

[Eric Blake <eblake@redhat.com>]

On 5/18/20 8:07 AM, Vladimir Sementsov-Ogievskiy wrote:
> 13.05.2020 04:16, 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 QMP field, present when
>> measuring an existing image and output format that both support
>> 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.
>>

>> @@ -616,6 +616,7 @@ Command description:
>>
>>       required size: 524288
>>       fully allocated size: 1074069504
>> +    bitmaps: 0
>>
>>     The ``required size`` is the file size of the new image.  It may 
>> be smaller
>>     than the virtual disk size if the image format supports compact 
>> representation.
>> @@ -625,6 +626,13 @@ Command description:
>>     occupy with the exception of internal snapshots, dirty bitmaps, 
>> vmstate data,
>>     and other advanced image format features.
>>
>> +  The ``bitmaps size`` is the additional size required if the
> 
> you called it "bitmaps" in example output above. Should it be 
> consistent? Either "``bitmaps``" here, or "bitmaps size: 0" above?

"bitmaps size: 0" is better. Will fix the description above.

>> +++ 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.
> 
> "copying just guest-visible" sounds like something less than "all 
> fully-allocated sectors"..
> But I don't have better suggestion.. Just, "not including bitmaps" 
> sounds weird too.

If we ever add support for copying internal snapshots, that would not be 
included either.  Maybe "copying just allocated guest-visible contents" 
for @required, and no change to the wording for @fully-allocated.


>> @@ -4796,13 +4797,38 @@ static BlockMeasureInfo 
>> *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,

>>
>> +        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),
>> +                                                   cluster_size);
>> +
>> +                /* Assume the entire bitmap is allocated */
>> +                bitmaps_size += bmclusters * cluster_size;
>> +                /* Also reserve space for the bitmap table entries */
>> +                bitmaps_size += ROUND_UP(bmclusters * sizeof(uint64_t),
>> +                                         cluster_size);
>> +                /* And space for contribution to bitmap directory 
>> size */
>> +                bitmap_dir_size += ROUND_UP(strlen(name) + 24,
>> +                                            sizeof(uint64_t));
> 
> Could we instead reuse code from qcow2_co_can_store_new_dirty_bitmap(), 
> which calls calc_dir_entry_size() for this thing?
> Possibly, make a function qcow2_measure_bitmaps in block/qcow2-bitmaps.c 
> with this FOR_EACH? All details about qcow2 bitmap structures sounds 
> better in block/qcow2-bitmaps.c

Could do.  Sounds like I'm better off submitting a v5 for this patch, 
although I'll go ahead and stage 1-6 for pull request today to minimize 
future rebase churn.


>> +    info->has_bitmaps = version >= 3 && in_bs &&
>> +        bdrv_supports_persistent_dirty_bitmap(in_bs);
>> +    info->bitmaps = bitmaps_size;
> 
> AFAIK, in QAPI, if has_<something> field is false, than <something> must 
> be zero. Maybe, it's only about nested structured fields, not about 
> simple numbers, but I think it's better keep bitmaps 0 in case when 
> has_bitmaps is false.

During creation (including when parsing QMP from the user over the 
monitor), everything is indeed guaranteed to be zero-initialized.  But 
we don't have any requirement that things remain zero-initialized even 
when has_FOO is false; at the same time, it's easy enough to make this 
code conditional.

> 
> Also, it seems a bit better to check version earlier, and don't do all 
> the calculations, if we are not going to use them.. But it's a rare 
> backward-compatibility case, I don't care.

I'll see how easy or hard it is for my v5 patch.


>> @@ -5275,9 +5285,24 @@ static int img_measure(int argc, char **argv)
>>           goto out;
>>       }
>>
>> +    if (bitmaps) {
>> +        if (!info->has_bitmaps) {
>> +            error_report("no bitmaps measured, either source or 
>> destination "
>> +                         "format lacks bitmap support");
>> +            goto out;
>> +        } else {
>> +            info->required += info->bitmaps;
>> +            info->fully_allocated += info->bitmaps;
>> +            info->has_bitmaps = false;
> 
> And here, I think better to zero info->bitmaps as well.

Here, the object is going to be subsequently freed; I'm less worried 
about wasting time doing local cleanups than I am about the earlier case 
about letting an object escape immediate scope in a different state than 
the usual preconditions.


>> +$QEMU_IMG bitmap --add --granularity 512 -f qcow2 "$TEST_IMG" b1
>> +$QEMU_IMG bitmap --add -g 2M -f qcow2 "$TEST_IMG" b2
>> +
>> +# No bitmap without a source
>> +$QEMU_IMG measure --bitmaps -O qcow2 --size 10M
> 
> should this be ored to  'echo "unexpected success"' as following failures?
> 

Can't hurt.


>> +# Compute expected output:
>> +echo
>> +val2T=$((2*1024*1024*1024*1024))
>> +cluster=$((64*1024))
>> +b1clusters=$(( (val2T/512/8 + cluster - 1) / cluster ))
>> +b2clusters=$(( (val2T/2/1024/1024/8 + cluster - 1) / cluster ))
> 
> comment on the following calculations won't hurt, at least something like
>   "bitmap clusters + bitmap tables + bitmaps directory"

Sure.

> 
>> +echo expected bitmap $((b1clusters * cluster +
>> +            (b1clusters * 8 + cluster - 1) / cluster * cluster +
>> +            b2clusters * cluster +
>> +            (b2clusters * 8 + cluster - 1) / cluster * cluster +
>> +            cluster))
>> +$QEMU_IMG measure -O qcow2 -o cluster_size=64k -f qcow2 "$TEST_IMG"
>> +$QEMU_IMG measure --bitmaps -O qcow2 -o cluster_size=64k -f qcow2 
>> "$TEST_IMG"
>> +

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