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