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"
+