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

[Eric Blake]

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