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 <nsoffer@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
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.
+#
+# @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)
#
# Since: 2.10
##
{ 'struct': 'BlockMeasureInfo',
- 'data': {'required': 'int', 'fully-allocated': 'int'} }
+ 'data': {'required': 'int', 'fully-allocated': 'int', '*bitmaps': 'int'} }
##
# @query-block:
diff --git a/block/crypto.c b/block/crypto.c
index 6b21d6bf6c01..eadbcb248563 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -552,7 +552,7 @@ static BlockMeasureInfo *block_crypto_measure(QemuOpts *opts,
* Unallocated blocks are still encrypted so allocation status makes no
* difference to the file size.
*/
- info = g_new(BlockMeasureInfo, 1);
+ info = g_new0(BlockMeasureInfo, 1);
info->fully_allocated = luks_payload_size + size;
info->required = luks_payload_size + size;
return info;
diff --git a/block/qcow2.c b/block/qcow2.c
index 838d810ca5ec..f836a6047879 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4721,6 +4721,7 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
PreallocMode prealloc;
bool has_backing_file;
bool has_luks;
+ uint64_t bitmaps_size = 0; /* size occupied by bitmaps in in_bs */
/* Parse image creation options */
cluster_size = qcow2_opt_get_cluster_size_del(opts, &local_err);
@@ -4796,13 +4797,38 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
/* Account for input image */
if (in_bs) {
+ BdrvDirtyBitmap *bm;
+ size_t bitmap_dir_size = 0;
int64_t ssize = bdrv_getlength(in_bs);
+
if (ssize < 0) {
error_setg_errno(&local_err, -ssize,
"Unable to get image virtual_size");
goto err;
}
+ 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));
+ }
+ }
+ bitmaps_size += ROUND_UP(bitmap_dir_size, cluster_size);
+
virtual_size = ROUND_UP(ssize, cluster_size);
if (has_backing_file) {
@@ -4849,16 +4875,21 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
required = virtual_size;
}
- info = g_new(BlockMeasureInfo, 1);
+ info = g_new0(BlockMeasureInfo, 1);
info->fully_allocated =
qcow2_calc_prealloc_size(virtual_size, cluster_size,
ctz32(refcount_bits)) + luks_payload_size;
- /* Remove data clusters that are not required. This overestimates the
+ /*
+ * Remove data clusters that are not required. This overestimates the
* required size because metadata needed for the fully allocated file is
- * still counted.
+ * still counted. Show bitmaps only if both source and destination
+ * would support them.
*/
info->required = info->fully_allocated - virtual_size + required;
+ info->has_bitmaps = version >= 3 && in_bs &&
+ bdrv_dirty_bitmap_supported(in_bs);
+ info->bitmaps = bitmaps_size;
return info;
err:
diff --git a/block/raw-format.c b/block/raw-format.c
index 9108e4369628..a134b1954ca2 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -346,7 +346,7 @@ static BlockMeasureInfo *raw_measure(QemuOpts *opts, BlockDriverState *in_bs,
BDRV_SECTOR_SIZE);
}
- info = g_new(BlockMeasureInfo, 1);
+ info = g_new0(BlockMeasureInfo, 1);
info->required = required;
/* Unallocated sectors count towards the file size in raw images */
diff --git a/qemu-img.c b/qemu-img.c
index 7ad86f7b8072..0e747247e0c5 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -5278,6 +5278,9 @@ static int img_measure(int argc, char **argv)
if (output_format == OFORMAT_HUMAN) {
printf("required size: %" PRIu64 "\n", info->required);
printf("fully allocated size: %" PRIu64 "\n", info->fully_allocated);
+ if (info->has_bitmaps) {
+ printf("bitmaps size: %" PRIu64 "\n", info->bitmaps);
+ }
} else {
dump_json_block_measure_info(info);
}
diff --git a/tests/qemu-iotests/178.out.qcow2 b/tests/qemu-iotests/178.out.qcow2
index f59bf4b2fbc4..0c6ca5e05713 100644
--- a/tests/qemu-iotests/178.out.qcow2
+++ b/tests/qemu-iotests/178.out.qcow2
@@ -37,6 +37,7 @@ qemu-img: The image size is too large (try using a larger cluster size)
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=0
required size: 196608
fully allocated size: 196608
+bitmaps size: 0
converted image file size in bytes: 196608
@@ -45,6 +46,7 @@ converted image file size in bytes: 196608
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
required size: 393216
fully allocated size: 1074135040
+bitmaps size: 0
wrote 512/512 bytes at offset 512
512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
wrote 65536/65536 bytes at offset 65536
@@ -53,6 +55,7 @@ wrote 64512/64512 bytes at offset 134217728
63 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
required size: 589824
fully allocated size: 1074135040
+bitmaps size: 0
converted image file size in bytes: 524288
@@ -60,6 +63,7 @@ converted image file size in bytes: 524288
required size: 524288
fully allocated size: 1074135040
+bitmaps size: 0
converted image file size in bytes: 458752
@@ -67,16 +71,19 @@ converted image file size in bytes: 458752
required size: 1074135040
fully allocated size: 1074135040
+bitmaps size: 0
== qcow2 input image and LUKS encryption ==
required size: 2686976
fully allocated size: 1076232192
+bitmaps size: 0
== qcow2 input image and preallocation (human) ==
required size: 1074135040
fully allocated size: 1074135040
+bitmaps size: 0
converted image file size in bytes: 1074135040
@@ -87,6 +94,7 @@ wrote 8388608/8388608 bytes at offset 0
8 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
required size: 8716288
fully allocated size: 8716288
+bitmaps size: 0
converted image file size in bytes: 8716288
@@ -173,6 +181,7 @@ qemu-img: The image size is too large (try using a larger cluster size)
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=0
{
+ "bitmaps": 0,
"required": 196608,
"fully-allocated": 196608
}
@@ -183,6 +192,7 @@ converted image file size in bytes: 196608
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
{
+ "bitmaps": 0,
"required": 393216,
"fully-allocated": 1074135040
}
@@ -193,6 +203,7 @@ wrote 65536/65536 bytes at offset 65536
wrote 64512/64512 bytes at offset 134217728
63 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
{
+ "bitmaps": 0,
"required": 589824,
"fully-allocated": 1074135040
}
@@ -202,6 +213,7 @@ converted image file size in bytes: 524288
== qcow2 input image with internal snapshot (json) ==
{
+ "bitmaps": 0,
"required": 524288,
"fully-allocated": 1074135040
}
@@ -211,6 +223,7 @@ converted image file size in bytes: 458752
== qcow2 input image and a backing file (json) ==
{
+ "bitmaps": 0,
"required": 1074135040,
"fully-allocated": 1074135040
}
@@ -218,6 +231,7 @@ converted image file size in bytes: 458752
== qcow2 input image and LUKS encryption ==
{
+ "bitmaps": 0,
"required": 2686976,
"fully-allocated": 1076232192
}
@@ -225,6 +239,7 @@ converted image file size in bytes: 458752
== qcow2 input image and preallocation (json) ==
{
+ "bitmaps": 0,
"required": 1074135040,
"fully-allocated": 1074135040
}
@@ -237,6 +252,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=8388608
wrote 8388608/8388608 bytes at offset 0
8 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
{
+ "bitmaps": 0,
"required": 8716288,
"fully-allocated": 8716288
}
diff --git a/tests/qemu-iotests/190 b/tests/qemu-iotests/190
index 6d41650438e1..1b5fff45bfcd 100755
--- a/tests/qemu-iotests/190
+++ b/tests/qemu-iotests/190
@@ -2,7 +2,7 @@
#
# qemu-img measure sub-command tests on huge qcow2 files
#
-# Copyright (C) 2017 Red Hat, Inc.
+# Copyright (C) 2017-2020 Red Hat, Inc.
#
# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
@@ -42,7 +42,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
_supported_fmt qcow2
_supported_proto file
-echo "== Huge file =="
+echo "== Huge file without bitmaps =="
echo
_make_test_img -o 'cluster_size=2M' 2T
@@ -51,6 +51,45 @@ $QEMU_IMG measure -O raw -f qcow2 "$TEST_IMG"
$QEMU_IMG measure -O qcow2 -o cluster_size=64k -f qcow2 "$TEST_IMG"
$QEMU_IMG measure -O qcow2 -o cluster_size=2M -f qcow2 "$TEST_IMG"
+echo
+echo "== Huge file with bitmaps =="
+echo
+
+$QEMU_IMG bitmap --add --granularity 512 -f qcow2 "$TEST_IMG" b1
+$QEMU_IMG bitmap --add -g 2M -f qcow2 "$TEST_IMG" b2
+
+# No bitmap output, since raw does not support it
+$QEMU_IMG measure -O raw -f qcow2 "$TEST_IMG"
+# No bitmap output, since no bitmaps on raw source
+$QEMU_IMG measure -O qcow2 -f raw "$TEST_IMG"
+# No bitmap output, since v2 does not support it
+$QEMU_IMG measure -O qcow2 -o compat=0.10 -f qcow2 "$TEST_IMG"
+
+# 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 ))
+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"
+
+# Compute expected output:
+echo
+cluster=$((2*1024*1024))
+b1clusters=$(( (val2T/512/8 + cluster - 1) / cluster ))
+b2clusters=$(( (val2T/2/1024/1024/8 + cluster - 1) / cluster ))
+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=2M -f qcow2 "$TEST_IMG"
+
# success, all done
echo "*** done"
rm -f $seq.full
diff --git a/tests/qemu-iotests/190.out b/tests/qemu-iotests/190.out
index d001942002db..6d5a25e9de2f 100644
--- a/tests/qemu-iotests/190.out
+++ b/tests/qemu-iotests/190.out
@@ -1,11 +1,32 @@
QA output created by 190
-== Huge file ==
+== Huge file without bitmaps ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2199023255552
required size: 2199023255552
fully allocated size: 2199023255552
required size: 335806464
fully allocated size: 2199359062016
+bitmaps size: 0
required size: 18874368
fully allocated size: 2199042129920
+bitmaps size: 0
+
+== Huge file with bitmaps ==
+
+required size: 2199023255552
+fully allocated size: 2199023255552
+required size: 7012352
+fully allocated size: 17170432
+required size: 335806464
+fully allocated size: 2199359062016
+
+expected bitmap 537198592
+required size: 335806464
+fully allocated size: 2199359062016
+bitmaps size: 537198592
+
+expected bitmap 545259520
+required size: 18874368
+fully allocated size: 2199042129920
+bitmaps size: 545259520
*** done
--
2.26.2
On 08.05.20 20:03, 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 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 <nsoffer@redhat.com> > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > 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. > +# > +# @bitmaps: Additional size required for bitmap metadata in a source image, s/in/from/? Otherwise it sounds like this would be about allocation in the source, which it clear can’t be, but, well. > +# if that bitmap metadata can be copied in addition to guest > +# contents. (since 5.1) [...] > diff --git a/block/qcow2.c b/block/qcow2.c > index 838d810ca5ec..f836a6047879 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c [...] > @@ -4849,16 +4875,21 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs, > required = virtual_size; > } > > - info = g_new(BlockMeasureInfo, 1); > + info = g_new0(BlockMeasureInfo, 1); > info->fully_allocated = > qcow2_calc_prealloc_size(virtual_size, cluster_size, > ctz32(refcount_bits)) + luks_payload_size; > > - /* Remove data clusters that are not required. This overestimates the > + /* > + * Remove data clusters that are not required. This overestimates the > * required size because metadata needed for the fully allocated file is > - * still counted. > + * still counted. Show bitmaps only if both source and destination > + * would support them. > */ > info->required = info->fully_allocated - virtual_size + required; > + info->has_bitmaps = version >= 3 && in_bs && > + bdrv_dirty_bitmap_supported(in_bs); Why is it important whether the source format supports persistent dirty bitmaps? I’m asking because I’d like there to be some concise reason when and why the @bitmaps field appears. “Whenever the target supports bitmaps” is more concise than “When both source and target support bitmaps”. Also, the latter is not really different from “When any bitmap data can be copied”, but in the latter case we should not show it when there are no bitmaps in the source (even though the format supports them). Or from the other perspective: As a user, I would never be annoyed by the @bitmaps field being present. I don’t mind a “0”. OTOH, what information can it convey to me that it it’s optional and sometimes not present? I can see these cases: - That the source format doesn’t support bitmaps? I want to convert it to something else anyway, so I don’t really care about what the source format can or can’t do. - That the destination doesn’t support bitmaps? Ah, yes, the fact that the bitmap field is missing might be a warning sign for this. - That qemu is too old to copy bitmaps? Same here. - That there are no bitmaps in the source? OK, but then I disregard the @bitmaps field anyway, present or not. So from that standpoint, the best use seems to me to take “The @bitmaps field isn’t present” as kind of a warning that something in the convert process won’t support copying bitmaps. If it’s present, all is well. So basically there’d be an iff relationship between “measure reports @bitmaps” and “convert --bitmap can work”. But the distinction between “the source format doesn’t support bitmaps” and “the source image doesn’t have bitmaps” doesn’t seem that important to me to make it visible in the interface. [...] > diff --git a/tests/qemu-iotests/190 b/tests/qemu-iotests/190 > index 6d41650438e1..1b5fff45bfcd 100755 > --- a/tests/qemu-iotests/190 > +++ b/tests/qemu-iotests/190 [...] > @@ -51,6 +51,45 @@ $QEMU_IMG measure -O raw -f qcow2 "$TEST_IMG" > $QEMU_IMG measure -O qcow2 -o cluster_size=64k -f qcow2 "$TEST_IMG" > $QEMU_IMG measure -O qcow2 -o cluster_size=2M -f qcow2 "$TEST_IMG" > > +echo > +echo "== Huge file with bitmaps ==" > +echo > + > +$QEMU_IMG bitmap --add --granularity 512 -f qcow2 "$TEST_IMG" b1 > +$QEMU_IMG bitmap --add -g 2M -f qcow2 "$TEST_IMG" b2 > + > +# No bitmap output, since raw does not support it > +$QEMU_IMG measure -O raw -f qcow2 "$TEST_IMG" > +# No bitmap output, since no bitmaps on raw source > +$QEMU_IMG measure -O qcow2 -f raw "$TEST_IMG" > +# No bitmap output, since v2 does not support it > +$QEMU_IMG measure -O qcow2 -o compat=0.10 -f qcow2 "$TEST_IMG" > + > +# 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 )) > +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" > + > +# Compute expected output: > +echo > +cluster=$((2*1024*1024)) > +b1clusters=$(( (val2T/512/8 + cluster - 1) / cluster )) > +b2clusters=$(( (val2T/2/1024/1024/8 + cluster - 1) / cluster )) > +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=2M -f qcow2 "$TEST_IMG" Thanks! Max
On 5/11/20 6:50 AM, Max Reitz wrote: > On 08.05.20 20:03, 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 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 <nsoffer@redhat.com> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> +# >> +# @bitmaps: Additional size required for bitmap metadata in a source image, > > s/in/from/? Otherwise it sounds like this would be about allocation in > the source, which it clear can’t be, but, well. > Yes, 'from' sounds nicer, especially since the size requirements being measured depend on the destination's cluster size (which may be different from the source's cluster size). >> +# if that bitmap metadata can be copied in addition to guest >> +# contents. (since 5.1) > > [...] > >> + /* >> + * Remove data clusters that are not required. This overestimates the >> * required size because metadata needed for the fully allocated file is >> - * still counted. >> + * still counted. Show bitmaps only if both source and destination >> + * would support them. >> */ >> info->required = info->fully_allocated - virtual_size + required; >> + info->has_bitmaps = version >= 3 && in_bs && >> + bdrv_dirty_bitmap_supported(in_bs); > > Why is it important whether the source format supports persistent dirty > bitmaps? If the source format does not support bitmaps, there is nothing to copy over. Reporting '0' would work, but adds verbosity. It also becomes a question as to whether 'qemu-img convert --bitmaps' should silently ignore such sources, or loudly error out that the option is unsupported because the source lacks bitmaps. I could lean either way. > > I’m asking because I’d like there to be some concise reason when and why > the @bitmaps field appears. “Whenever the target supports bitmaps” is > more concise than “When both source and target support bitmaps”. Also, > the latter is not really different from “When any bitmap data can be > copied”, but in the latter case we should not show it when there are no > bitmaps in the source (even though the format supports them). > > Or from the other perspective: As a user, I would never be annoyed by > the @bitmaps field being present. I don’t mind a “0”. > OTOH, what information can it convey to me that it it’s optional and > sometimes not present? The impact to the iotests .out files is larger if I do not require that the source supports bitmaps (more lines of 'bitmaps: 0' added). I'm fine doing that, if we decide we're okay with the simpler definition of '"bitmaps" is present if the destination supports them' (rather than this version's implementation of '"bitmaps" is present if both the source and destination support them'). > I can see these cases: > > - That the source format doesn’t support bitmaps? I want to convert it > to something else anyway, so I don’t really care about what the source > format can or can’t do. > > - That the destination doesn’t support bitmaps? Ah, yes, the fact that > the bitmap field is missing might be a warning sign for this. > > - That qemu is too old to copy bitmaps? Same here. In fact, that argument is a GOOD reason to output 'bitmaps: 0' in as many cases as possible, because it then becomes a side-effect witness of whether 'qemu-img convert --bitmaps' is even understood. > > - That there are no bitmaps in the source? OK, but then I disregard the > @bitmaps field anyway, present or not. > > So from that standpoint, the best use seems to me to take “The @bitmaps > field isn’t present” as kind of a warning that something in the convert > process won’t support copying bitmaps. If it’s present, all is well. > So basically there’d be an iff relationship between “measure reports > @bitmaps” and “convert --bitmap can work”. Yes, I can make that tweak for v4. > > But the distinction between “the source format doesn’t support bitmaps” > and “the source image doesn’t have bitmaps” doesn’t seem that important > to me to make it visible in the interface. > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Fri, May 8, 2020 at 9:03 PM Eric Blake <eblake@redhat.com> 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 <nsoffer@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 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. If we add a new
extension all users will have to
change the calculation again.
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.
If bitmaps are reported in both commands we will use the value when creating
block devices.
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.
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.
Nir
> #
> # Since: 2.10
> ##
> { 'struct': 'BlockMeasureInfo',
> - 'data': {'required': 'int', 'fully-allocated': 'int'} }
> + 'data': {'required': 'int', 'fully-allocated': 'int', '*bitmaps': 'int'} }
>
> ##
> # @query-block:
> diff --git a/block/crypto.c b/block/crypto.c
> index 6b21d6bf6c01..eadbcb248563 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -552,7 +552,7 @@ static BlockMeasureInfo *block_crypto_measure(QemuOpts *opts,
> * Unallocated blocks are still encrypted so allocation status makes no
> * difference to the file size.
> */
> - info = g_new(BlockMeasureInfo, 1);
> + info = g_new0(BlockMeasureInfo, 1);
> info->fully_allocated = luks_payload_size + size;
> info->required = luks_payload_size + size;
> return info;
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 838d810ca5ec..f836a6047879 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -4721,6 +4721,7 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
> PreallocMode prealloc;
> bool has_backing_file;
> bool has_luks;
> + uint64_t bitmaps_size = 0; /* size occupied by bitmaps in in_bs */
>
> /* Parse image creation options */
> cluster_size = qcow2_opt_get_cluster_size_del(opts, &local_err);
> @@ -4796,13 +4797,38 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
>
> /* Account for input image */
> if (in_bs) {
> + BdrvDirtyBitmap *bm;
> + size_t bitmap_dir_size = 0;
> int64_t ssize = bdrv_getlength(in_bs);
> +
> if (ssize < 0) {
> error_setg_errno(&local_err, -ssize,
> "Unable to get image virtual_size");
> goto err;
> }
>
> + 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));
> + }
> + }
> + bitmaps_size += ROUND_UP(bitmap_dir_size, cluster_size);
> +
> virtual_size = ROUND_UP(ssize, cluster_size);
>
> if (has_backing_file) {
> @@ -4849,16 +4875,21 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
> required = virtual_size;
> }
>
> - info = g_new(BlockMeasureInfo, 1);
> + info = g_new0(BlockMeasureInfo, 1);
> info->fully_allocated =
> qcow2_calc_prealloc_size(virtual_size, cluster_size,
> ctz32(refcount_bits)) + luks_payload_size;
>
> - /* Remove data clusters that are not required. This overestimates the
> + /*
> + * Remove data clusters that are not required. This overestimates the
> * required size because metadata needed for the fully allocated file is
> - * still counted.
> + * still counted. Show bitmaps only if both source and destination
> + * would support them.
> */
> info->required = info->fully_allocated - virtual_size + required;
> + info->has_bitmaps = version >= 3 && in_bs &&
> + bdrv_dirty_bitmap_supported(in_bs);
> + info->bitmaps = bitmaps_size;
> return info;
>
> err:
> diff --git a/block/raw-format.c b/block/raw-format.c
> index 9108e4369628..a134b1954ca2 100644
> --- a/block/raw-format.c
> +++ b/block/raw-format.c
> @@ -346,7 +346,7 @@ static BlockMeasureInfo *raw_measure(QemuOpts *opts, BlockDriverState *in_bs,
> BDRV_SECTOR_SIZE);
> }
>
> - info = g_new(BlockMeasureInfo, 1);
> + info = g_new0(BlockMeasureInfo, 1);
> info->required = required;
>
> /* Unallocated sectors count towards the file size in raw images */
> diff --git a/qemu-img.c b/qemu-img.c
> index 7ad86f7b8072..0e747247e0c5 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -5278,6 +5278,9 @@ static int img_measure(int argc, char **argv)
> if (output_format == OFORMAT_HUMAN) {
> printf("required size: %" PRIu64 "\n", info->required);
> printf("fully allocated size: %" PRIu64 "\n", info->fully_allocated);
> + if (info->has_bitmaps) {
> + printf("bitmaps size: %" PRIu64 "\n", info->bitmaps);
> + }
> } else {
> dump_json_block_measure_info(info);
> }
> diff --git a/tests/qemu-iotests/178.out.qcow2 b/tests/qemu-iotests/178.out.qcow2
> index f59bf4b2fbc4..0c6ca5e05713 100644
> --- a/tests/qemu-iotests/178.out.qcow2
> +++ b/tests/qemu-iotests/178.out.qcow2
> @@ -37,6 +37,7 @@ qemu-img: The image size is too large (try using a larger cluster size)
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=0
> required size: 196608
> fully allocated size: 196608
> +bitmaps size: 0
>
> converted image file size in bytes: 196608
>
> @@ -45,6 +46,7 @@ converted image file size in bytes: 196608
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
> required size: 393216
> fully allocated size: 1074135040
> +bitmaps size: 0
> wrote 512/512 bytes at offset 512
> 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> wrote 65536/65536 bytes at offset 65536
> @@ -53,6 +55,7 @@ wrote 64512/64512 bytes at offset 134217728
> 63 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> required size: 589824
> fully allocated size: 1074135040
> +bitmaps size: 0
>
> converted image file size in bytes: 524288
>
> @@ -60,6 +63,7 @@ converted image file size in bytes: 524288
>
> required size: 524288
> fully allocated size: 1074135040
> +bitmaps size: 0
>
> converted image file size in bytes: 458752
>
> @@ -67,16 +71,19 @@ converted image file size in bytes: 458752
>
> required size: 1074135040
> fully allocated size: 1074135040
> +bitmaps size: 0
>
> == qcow2 input image and LUKS encryption ==
>
> required size: 2686976
> fully allocated size: 1076232192
> +bitmaps size: 0
>
> == qcow2 input image and preallocation (human) ==
>
> required size: 1074135040
> fully allocated size: 1074135040
> +bitmaps size: 0
>
> converted image file size in bytes: 1074135040
>
> @@ -87,6 +94,7 @@ wrote 8388608/8388608 bytes at offset 0
> 8 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> required size: 8716288
> fully allocated size: 8716288
> +bitmaps size: 0
>
> converted image file size in bytes: 8716288
>
> @@ -173,6 +181,7 @@ qemu-img: The image size is too large (try using a larger cluster size)
>
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=0
> {
> + "bitmaps": 0,
> "required": 196608,
> "fully-allocated": 196608
> }
> @@ -183,6 +192,7 @@ converted image file size in bytes: 196608
>
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
> {
> + "bitmaps": 0,
> "required": 393216,
> "fully-allocated": 1074135040
> }
> @@ -193,6 +203,7 @@ wrote 65536/65536 bytes at offset 65536
> wrote 64512/64512 bytes at offset 134217728
> 63 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> {
> + "bitmaps": 0,
> "required": 589824,
> "fully-allocated": 1074135040
> }
> @@ -202,6 +213,7 @@ converted image file size in bytes: 524288
> == qcow2 input image with internal snapshot (json) ==
>
> {
> + "bitmaps": 0,
> "required": 524288,
> "fully-allocated": 1074135040
> }
> @@ -211,6 +223,7 @@ converted image file size in bytes: 458752
> == qcow2 input image and a backing file (json) ==
>
> {
> + "bitmaps": 0,
> "required": 1074135040,
> "fully-allocated": 1074135040
> }
> @@ -218,6 +231,7 @@ converted image file size in bytes: 458752
> == qcow2 input image and LUKS encryption ==
>
> {
> + "bitmaps": 0,
> "required": 2686976,
> "fully-allocated": 1076232192
> }
> @@ -225,6 +239,7 @@ converted image file size in bytes: 458752
> == qcow2 input image and preallocation (json) ==
>
> {
> + "bitmaps": 0,
> "required": 1074135040,
> "fully-allocated": 1074135040
> }
> @@ -237,6 +252,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=8388608
> wrote 8388608/8388608 bytes at offset 0
> 8 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> {
> + "bitmaps": 0,
> "required": 8716288,
> "fully-allocated": 8716288
> }
> diff --git a/tests/qemu-iotests/190 b/tests/qemu-iotests/190
> index 6d41650438e1..1b5fff45bfcd 100755
> --- a/tests/qemu-iotests/190
> +++ b/tests/qemu-iotests/190
> @@ -2,7 +2,7 @@
> #
> # qemu-img measure sub-command tests on huge qcow2 files
> #
> -# Copyright (C) 2017 Red Hat, Inc.
> +# Copyright (C) 2017-2020 Red Hat, Inc.
> #
> # This program is free software; you can redistribute it and/or modify
> # it under the terms of the GNU General Public License as published by
> @@ -42,7 +42,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
> _supported_fmt qcow2
> _supported_proto file
>
> -echo "== Huge file =="
> +echo "== Huge file without bitmaps =="
> echo
>
> _make_test_img -o 'cluster_size=2M' 2T
> @@ -51,6 +51,45 @@ $QEMU_IMG measure -O raw -f qcow2 "$TEST_IMG"
> $QEMU_IMG measure -O qcow2 -o cluster_size=64k -f qcow2 "$TEST_IMG"
> $QEMU_IMG measure -O qcow2 -o cluster_size=2M -f qcow2 "$TEST_IMG"
>
> +echo
> +echo "== Huge file with bitmaps =="
> +echo
> +
> +$QEMU_IMG bitmap --add --granularity 512 -f qcow2 "$TEST_IMG" b1
> +$QEMU_IMG bitmap --add -g 2M -f qcow2 "$TEST_IMG" b2
> +
> +# No bitmap output, since raw does not support it
> +$QEMU_IMG measure -O raw -f qcow2 "$TEST_IMG"
> +# No bitmap output, since no bitmaps on raw source
> +$QEMU_IMG measure -O qcow2 -f raw "$TEST_IMG"
> +# No bitmap output, since v2 does not support it
> +$QEMU_IMG measure -O qcow2 -o compat=0.10 -f qcow2 "$TEST_IMG"
> +
> +# 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 ))
> +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"
> +
> +# Compute expected output:
> +echo
> +cluster=$((2*1024*1024))
> +b1clusters=$(( (val2T/512/8 + cluster - 1) / cluster ))
> +b2clusters=$(( (val2T/2/1024/1024/8 + cluster - 1) / cluster ))
> +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=2M -f qcow2 "$TEST_IMG"
> +
> # success, all done
> echo "*** done"
> rm -f $seq.full
> diff --git a/tests/qemu-iotests/190.out b/tests/qemu-iotests/190.out
> index d001942002db..6d5a25e9de2f 100644
> --- a/tests/qemu-iotests/190.out
> +++ b/tests/qemu-iotests/190.out
> @@ -1,11 +1,32 @@
> QA output created by 190
> -== Huge file ==
> +== Huge file without bitmaps ==
>
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2199023255552
> required size: 2199023255552
> fully allocated size: 2199023255552
> required size: 335806464
> fully allocated size: 2199359062016
> +bitmaps size: 0
> required size: 18874368
> fully allocated size: 2199042129920
> +bitmaps size: 0
> +
> +== Huge file with bitmaps ==
> +
> +required size: 2199023255552
> +fully allocated size: 2199023255552
> +required size: 7012352
> +fully allocated size: 17170432
> +required size: 335806464
> +fully allocated size: 2199359062016
> +
> +expected bitmap 537198592
> +required size: 335806464
> +fully allocated size: 2199359062016
> +bitmaps size: 537198592
> +
> +expected bitmap 545259520
> +required size: 18874368
> +fully allocated size: 2199042129920
> +bitmaps size: 545259520
> *** done
> --
> 2.26.2
>
On 12.05.20 12:17, Nir Soffer wrote: > On Fri, May 8, 2020 at 9:03 PM Eric Blake <eblake@redhat.com> 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 <nsoffer@redhat.com> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> 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
On 5/12/20 6:10 AM, Max Reitz wrote: >> 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. First, a usage question: would you rather that 'qemu-img convert --bitmaps' silently succeeds even when the image being converted has no bitmaps, or would you rather that it fails loudly when there are no bitmaps to be copied over? As implemented in this patch series, patch 8 currently silently succeeds. But in order to make patch 7 and 8 consistent with one another, I need to know which behavior is easier to use: failing convert if the source lacks bitmaps (and thus measure would suppress the bitmaps:0 output), or treating lack of bitmaps as nothing additional to copy and thereby succeeding (and thus measure should output bitmaps:0 to show that no additional space is needed because nothing else will be copied, successfully). >> 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. More concretely, suppose down the road that we add the ability to copy internal snapshots (not that you want to mix internal and external snapshots, but that such information already exists and therefore can be used as an example). Which is easier: $ qemu-img measure -f qcow2 image.qcow2 required size: 8716288 fully allocated size: 8716288 bitmaps size: 1024 internal snapshot size: 2048 where you now have to add three numbers prior to creating dest.qcow2 and calling: $ qemu-img convert image.qcow2 -f dest.qcow2 --bitmaps --snapshots or using: $ qemu-img measure --bitmaps --snapshots -f qcow2 image.qcow2 required size: 8719360 fully allocated size: 8719360 where you then call: $ qemu-img convert image.qcow2 -f dest.qcow2 --bitmaps --snapshots with a single size that matches the same arguments you pass to qemu-img convert? What about failure cases? What happens when qemu-img doesn't understand --snapshots but does understand --bitmaps? Do you have to try a series of up to three calls to find how much information is supported: $ qemu-img measure -f qcow2 image.qcow2 --bitmaps --snapshots error: unknown argument $ qemu-img measure -f qcow2 image.qcow2 --bitmaps error: unknown argument $ qemu-img measure -f qcow2 image.qcow2 data given, now you know that neither --bitmaps nor --snapshots will work or is it nicer to issue just one measure without options, getting separate output lines, and seeing which output lines exist to know which convert options are supported, at the minor expense of having to add values yourself? And then back to my question: should 'measure --bitmaps' fail if there are no bitmaps to be measured, or silently succeed and not change the output size? >> 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. Why do you have to check 'qemu-img info' first? If measure reports bitmaps, then you know bitmaps can be copied; if it doesn't, then you can check info as a fallback path to compute size yourself - but computing the size yourself doesn't help unless you also have fallbacks to copy the bitmaps via QMP commands, because that qemu-img will also lack 'qemu-img convert --bitmaps' or 'qemu-img bitmaps' to do it via qemu-img. >> >> 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). Or, that could be a CLI-only feature: the QMP interface _always_ reports bitmaps separately, but if 'qemu-img measure --bitmaps' is used, the CLI then adds that value in on your behalf after the QMP command but before printing to the end user. > > (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.)) If we only touch it up in the CLI, then we would have two forms of CLI output: $ qemu-img measure -f qcow2 image.qcow2 required size: 8716288 fully allocated size: 8716288 bitmaps size: 1024 $ qemu-img measure -f qcow2 image.qcow2 --bitmaps required size: 8717312 fully allocated size: 8717312 > > 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. I'm leaning towards making v4 try a CLI-only 'measure --bitmaps', to see if I can speed the discussion along with concrete patches for comparison. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Tue, May 12, 2020 at 10:39 PM Eric Blake <eblake@redhat.com> wrote: > > On 5/12/20 6:10 AM, Max Reitz wrote: > > > >> 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. > > First, a usage question: would you rather that 'qemu-img convert > --bitmaps' silently succeeds even when the image being converted has no > bitmaps, or would you rather that it fails loudly when there are no > bitmaps to be copied over? I think the meaning of --bitmaps should be "copy also bitmaps". This request makes sense only for qcow2 images, since other images do not have bitmaps, so failing loudly when the source format does not support bitmaps seems right. Same for the target format does not support bitmaps, this is invalid request and it should fail loudly. If the source and target are qcow2, and there are no bitmaps in source, I don't see any reason to fail. We don't want to check if an image has bitmaps before we copy the image, it does not help us. > As implemented in this patch series, patch 8 > currently silently succeeds. Sounds good for qcow2 format. > But in order to make patch 7 and 8 > consistent with one another, I need to know which behavior is easier to > use: failing convert if the source lacks bitmaps (and thus measure would > suppress the bitmaps:0 output), or treating lack of bitmaps as nothing > additional to copy and thereby succeeding (and thus measure should > output bitmaps:0 to show that no additional space is needed because > nothing else will be copied, successfully). I don't think showing "bitmaps: 0" in measure is related to how --bitmaps behave in convert. If we will have --bitmaps in measure, we don't need to show "bitmaps" at all since "required" will include it. If we want to show bitmaps in measure, I think using the same logic is fine: - if format does not support bitmaps - fail - if format suppots bitmaps, show what we have - zero is valid result when image does not have any bitmap. > >> 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. > > More concretely, suppose down the road that we add the ability to copy > internal snapshots (not that you want to mix internal and external > snapshots, but that such information already exists and therefore can be > used as an example). Which is easier: > > $ qemu-img measure -f qcow2 image.qcow2 > required size: 8716288 > fully allocated size: 8716288 > bitmaps size: 1024 > internal snapshot size: 2048 > > where you now have to add three numbers prior to creating dest.qcow2 and > calling: > > $ qemu-img convert image.qcow2 -f dest.qcow2 --bitmaps --snapshots > > or using: > > $ qemu-img measure --bitmaps --snapshots -f qcow2 image.qcow2 > required size: 8719360 > fully allocated size: 8719360 > > where you then call: > > $ qemu-img convert image.qcow2 -f dest.qcow2 --bitmaps --snapshots > > with a single size that matches the same arguments you pass to qemu-img > convert? Yes, the second form is a good example of using --bitmaps consistently. > What about failure cases? What happens when qemu-img doesn't > understand --snapshots but does understand --bitmaps? Do you have to > try a series of up to three calls to find how much information is supported: > > $ qemu-img measure -f qcow2 image.qcow2 --bitmaps --snapshots > error: unknown argument > $ qemu-img measure -f qcow2 image.qcow2 --bitmaps > error: unknown argument > $ qemu-img measure -f qcow2 image.qcow2 > data given, now you know that neither --bitmaps nor --snapshots will work Assuming that I cannot require a version that support both features, which is usually the case when we have to support different platforms with different versions of qemu-img, I will check the capabilities using qemu-img --help and cache the result. For vdsm case we control the host so qemu-img should not be upgraded behind vdsm back. Then I will use the supported features. If both are missing, this convert will drop the the bitmaps and the snaphosts, and the next incremetnal backup will fail or fallback to full backup. > or is it nicer to issue just one measure without options, getting > separate output lines, and seeing which output lines exist to know which > convert options are supported, at the minor expense of having to add > values yourself? It is a little nicer since we don't need to check the capabilities, but we need to check them anyway for qemu-img convert, so this does not help much. But it introduces other issues: - we need to calculate the required size using required + bitmap or required + bitmaps + snapshots - what if measuring a snapshot is expensive - let's say take 20 seconds. This is still fast enough if the entire copy takes several minutes, so having a way to measure is useful. But users that do not care about the snapshot have to pay for this one every call. So we would end with a --snapshot flag to avoid this, and inconsistent API. > And then back to my question: should 'measure --bitmaps' fail if there > are no bitmaps to be measured, or silently succeed and not change the > output size? For raw file yes (invalid request), for qcow2 file no, it should just add 0 since this is the actual size required for bitmaps in this image. > >> 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. > > Why do you have to check 'qemu-img info' first? If measure reports > bitmaps, then you know bitmaps can be copied; This works only if qemu-img measure will report "bitmaps": 0 when there are no bitmaps. Otherwise I don't know if this version does not report bitmaps because it does not understand them, or because there are no bitmaps. Using qemu-img info I can tell the difference if measure does not report 0. > if it doesn't, then you > can check info as a fallback path to compute size yourself - but > computing the size yourself doesn't help unless you also have fallbacks > to copy the bitmaps via QMP commands, because that qemu-img will also > lack 'qemu-img convert --bitmaps' or 'qemu-img bitmaps' to do it via > qemu-img. When we started to work on this it was not clear that we will have a way to measure bitmaps. If we are going to have support both in convert and measure, we can check capability only in convert or only in measure. > >> 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). > > Or, that could be a CLI-only feature: the QMP interface _always_ reports > bitmaps separately, but if 'qemu-img measure --bitmaps' is used, the CLI > then adds that value in on your behalf after the QMP command but before > printing to the end user. > > > > > (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.)) > > If we only touch it up in the CLI, then we would have two forms of CLI > output: > > $ qemu-img measure -f qcow2 image.qcow2 > required size: 8716288 > fully allocated size: 8716288 > bitmaps size: 1024 > $ qemu-img measure -f qcow2 image.qcow2 --bitmaps > required size: 8717312 > fully allocated size: 8717312 I hope we will not have 2 forms. qemu-img is complicated enough ;-) > > 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. > > I'm leaning towards making v4 try a CLI-only 'measure --bitmaps', to see > if I can speed the discussion along with concrete patches for comparison. Thanks, that would be useful. Nir
On 5/12/20 3:35 PM, Nir Soffer wrote: >> First, a usage question: would you rather that 'qemu-img convert >> --bitmaps' silently succeeds even when the image being converted has no >> bitmaps, or would you rather that it fails loudly when there are no >> bitmaps to be copied over? > > I think the meaning of --bitmaps should be "copy also bitmaps". > > This request makes sense only for qcow2 images, since other images do > not have bitmaps, so failing loudly when the source format does not support > bitmaps seems right. > > Same for the target format does not support bitmaps, this is invalid request > and it should fail loudly. > > If the source and target are qcow2, and there are no bitmaps in source, I don't > see any reason to fail. We don't want to check if an image has bitmaps before > we copy the image, it does not help us. > >> As implemented in this patch series, patch 8 >> currently silently succeeds. > > Sounds good for qcow2 format. So, to make sure I'm clear, you'll be happy with something like: - no source $ qemu-img measure --bitmaps -O qcow2 1G error: --bitmaps requires source file - src cannot support bitmaps $ qemu-img measure --bitmaps -f raw -O qcow2 file.raw error: raw source does not support bitmaps $ qemu-img measure -f raw -O qcow2 file.raw success, totals are unchanged (since there are no bitmaps) $ qemu-img convert --bitmaps -f raw -O qcow2 file.raw file.qcow2 error: raw source does not support bitmaps - dest cannot support bitmaps $ qemu-img measure --bitmaps -f qcow2 -O raw file.qcow2 error: raw destination does not support bitmaps $ qemu-img measure -f qcow2 -O raw file.qcow2 success, totals are unchanged (since there are no bitmaps) $ qemu-img convert --bitmaps -f qcow2 -O raw file.qcow2 file.raw error: raw destination does not support bitmaps - another way dest cannot support bitmaps $ qemu-img convert --bitmaps -f qcow2 -O qcow2 -o compat=0.10 \ src.qcow2 dst.qcow2 error: v2 qcow2 destination does not support bitmaps - src and dest support bitmaps, but there are none $ qemu-img measure --bitmaps -f qcow2 -O qcow2 file.qcow2 success, no modification to totals since no bitmaps to copy $ qemu-img measure -f qcow2 -O qcow2 file.qcow2 success, separate line item for 'bitmap size: 0' $ qemu-img convert --bitmaps -f qcow2 -O qcow2 src.qcow2 dst.qcow2 success, even though no bitmaps to copy - src and dest support bitmaps, and there are some $ qemu-img measure --bitmaps -f qcow2 -O qcow2 file.qcow2 success, totals modified in place to include bitmaps without separate line $ qemu-img measure -f qcow2 -O qcow2 file.qcow2 success, separate line item added for 'bitmap size: NNN' $ qemu-img convert --bitmaps -f qcow2 -O qcow2 src.qcow2 dst.qcow2 success, and bitmaps were copied > >> But in order to make patch 7 and 8 >> consistent with one another, I need to know which behavior is easier to >> use: failing convert if the source lacks bitmaps (and thus measure would >> suppress the bitmaps:0 output), or treating lack of bitmaps as nothing >> additional to copy and thereby succeeding (and thus measure should >> output bitmaps:0 to show that no additional space is needed because >> nothing else will be copied, successfully). > > I don't think showing "bitmaps: 0" in measure is related to how --bitmaps > behave in convert. If we will have --bitmaps in measure, we don't need to > show "bitmaps" at all since "required" will include it. Showing 'bitmaps: 0' in the QMP is how the CLI will know whether it was an error to request measuring with bitmaps. > > If we want to show bitmaps in measure, I think using the same logic is fine: > - if format does not support bitmaps - fail > - if format suppots bitmaps, show what we have - zero is valid result when > image does not have any bitmap. More precisely, fail in QMP if either the source being measured cannot support bitmaps at all, or if the destination that determines the measurements cannot support bitmaps. But if both support bitmaps, then the QMP will output 0 if there happen to be no bitmaps to copy, and the CLI can add 'measure --bitmaps' to silently fold in a present bitmaps number (be it 0 or non-zero) into the other fields, or error if QMP did not include a bitmaps field. >> And then back to my question: should 'measure --bitmaps' fail if there >> are no bitmaps to be measured, or silently succeed and not change the >> output size? > > For raw file yes (invalid request), for qcow2 file no, it should just > add 0 since this is the actual > size required for bitmaps in this image. Okay, that is slightly different than what Max was describing (which was to output 'bitmaps: 0' in QMP even if the source is raw), but easy enough to be consistent on, and as this is your feature request, I can make v4 along those lines. > >>>> 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. >> >> Why do you have to check 'qemu-img info' first? If measure reports >> bitmaps, then you know bitmaps can be copied; > > This works only if qemu-img measure will report "bitmaps": 0 when there > are no bitmaps. Otherwise I don't know if this version does not report bitmaps > because it does not understand them, or because there are no bitmaps. > > Using qemu-img info I can tell the difference if measure does not report 0. But similarly, with the above rules, 'qemu-img measure --bitmaps' will either fail because qemu is too old, or fail because the source format cannot support bitmaps to be copied; and succeed if 'qemu-img convert --bitmaps' will also succeed. All without having to check 'qemu-img info' to see if there were bitmaps. > >> if it doesn't, then you >> can check info as a fallback path to compute size yourself - but >> computing the size yourself doesn't help unless you also have fallbacks >> to copy the bitmaps via QMP commands, because that qemu-img will also >> lack 'qemu-img convert --bitmaps' or 'qemu-img bitmaps' to do it via >> qemu-img. > > When we started to work on this it was not clear that we will have a > way to measure > bitmaps. If we are going to have support both in convert and measure, > we can check > capability only in convert or only in measure. Yes, the point of this series is that it is an all-or-none improvement to qemu-img: all of the bitmap manipulations you are wanting will be included at the same time in qemu 5.1, and likely backported at the same time by whatever downstream distros that want backports included. Existing qemu-img 5.0 cannot manipulate bitmaps in any reasonable manner, at most it has 'qemu-img info' that tells you that bitmaps exist, but you're still stuck with a non-qemu-img fallback if there is no qemu-img support for manipulating bitmaps at all (including the fallback of just declaring incremental backups unsupported, and using full backups instead). >>> (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.)) >> >> If we only touch it up in the CLI, then we would have two forms of CLI >> output: >> >> $ qemu-img measure -f qcow2 image.qcow2 >> required size: 8716288 >> fully allocated size: 8716288 >> bitmaps size: 1024 >> $ qemu-img measure -f qcow2 image.qcow2 --bitmaps >> required size: 8717312 >> fully allocated size: 8717312 > > I hope we will not have 2 forms. qemu-img is complicated enough ;-) 2 forms is the easiest way to add it. And it is not that hard to explain. Either you pass --bitmaps up front (no extra lines, but success only if measure saw bitmap support, even if the bitmaps occupy 0 spze), or you don't pass --bitmaps up front (an extra bitmaps: line if bitmaps are supported, even if the size is 0, and no change to existing lines). Just because 2 forms exist does not mean you are required to use both forms. >> I'm leaning towards making v4 try a CLI-only 'measure --bitmaps', to see >> if I can speed the discussion along with concrete patches for comparison. > > Thanks, that would be useful. Okay, that's the 2-form approach. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 12.05.20 21:39, Eric Blake wrote: > On 5/12/20 6:10 AM, Max Reitz wrote: > > >>> 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. > > First, a usage question: would you rather that 'qemu-img convert > --bitmaps' silently succeeds even when the image being converted has no > bitmaps, or would you rather that it fails loudly when there are no > bitmaps to be copied over? As implemented in this patch series, patch 8 > currently silently succeeds. But in order to make patch 7 and 8 > consistent with one another, I need to know which behavior is easier to > use: failing convert if the source lacks bitmaps (and thus measure would > suppress the bitmaps:0 output), or treating lack of bitmaps as nothing > additional to copy and thereby succeeding (and thus measure should > output bitmaps:0 to show that no additional space is needed because > nothing else will be copied, successfully). “Easier to use” in my opinion would be if it never fails, but the question is whether that’s also the most useful approach. I think it should match what I thought when @bitmaps should be reported, i.e. it should be accepted whenever the target format (and qemu-img convert, obviously) supports bitmaps. I don’t think there’s value in aborting just because the source doesn’t have bitmaps. What’s the user going to do in response? “Oops, thanks, qemu-img, I accidentally tried to convert the wrong image”? (Whereas if we reject --bitmaps because the target format doesn’t support it, there’s actually value, because the user is reminded that they should choose a different target format.) >>> 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. > > More concretely, suppose down the road that we add the ability to copy > internal snapshots (not that you want to mix internal and external > snapshots, but that such information already exists and therefore can be > used as an example). Which is easier: > > $ qemu-img measure -f qcow2 image.qcow2 > required size: 8716288 > fully allocated size: 8716288 > bitmaps size: 1024 > internal snapshot size: 2048 > > where you now have to add three numbers prior to creating dest.qcow2and > calling: > > $ qemu-img convert image.qcow2 -f dest.qcow2 --bitmaps --snapshots > > or using: > > $ qemu-img measure --bitmaps --snapshots -f qcow2 image.qcow2 > required size: 8719360 > fully allocated size: 8719360 > > where you then call: > > $ qemu-img convert image.qcow2 -f dest.qcow2 --bitmaps --snapshots “Which is easier” is misleading, because both are actually trivial. Adding three numbers is what a computer does best. It’s a question of style, not of “which is easier”. > with a single size that matches the same arguments you pass to qemu-img > convert? What about failure cases? What happens when qemu-img doesn't > understand --snapshots but does understand --bitmaps? Do you have to > try a series of up to three calls to find how much information is > supported: > > $ qemu-img measure -f qcow2 image.qcow2 --bitmaps --snapshots > error: unknown argument > $ qemu-img measure -f qcow2 image.qcow2 --bitmaps > error: unknown argument > $ qemu-img measure -f qcow2 image.qcow2 > data given, now you know that neither --bitmaps nor --snapshots will work > > or is it nicer to issue just one measure without options, getting > separate output lines, and seeing which output lines exist to know which > convert options are supported, at the minor expense of having to add > values yourself? Sounds good to me, but then again, calling a command to see whether it fails isn’t exactly hard either. So it remains a question of style to me. From my perspective, measure is a query function similar to info. It should return all information that’s trivial to obtain. Switches to select what information to return how only make sense when that optional information is hard to obtain (think qemu-img info --backing-chain). Switches make sense on data-modifying operations like convert. Not so much for querying operations, because why not just return all information you have and let the caller sort it out? [...] >>> 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). > > Or, that could be a CLI-only feature: the QMP interface _always_ reports > bitmaps separately, but if 'qemu-img measure --bitmaps' is used, the CLI > then adds that value in on your behalf after the QMP command but before > printing to the end user. > >> >> (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.)) > > If we only touch it up in the CLI, then we would have two forms of CLI > output: > > $ qemu-img measure -f qcow2 image.qcow2 > required size: 8716288 > fully allocated size: 8716288 > bitmaps size: 1024 > $ qemu-img measure -f qcow2 image.qcow2 --bitmaps > required size: 8717312 > fully allocated size: 8717312 I would hope that programs consuming qemu-img measure’s output use --output=json, and then you definitely can’t just ignore the QAPI object definition. Furthermore, I don’t like that 2-form at all, for two reasons. First, again, I can’t see a reasonable QAPI definition to match it; and second, I don’t like it as a consistent interface. We should decide on one thing and do that. Either you need to pass --bitmaps to make them included in required, or they aren’t in the output at all. The way you propose it with this 2-form approach makes --bitmap make qemu-img be a calculator. But why? It’s absolutely trivial for consumers to: irb(main):002:1` result = JSON.parse(`./qemu-img measure --output=json \ irb(main):003:0> -O qcow2 src.qcow2`) irb(main):004:0* result['required'] + irb(main):005:0> (result['bitmaps'] ? result['bitmaps'] : 0) => 524288 (Example in Ruby, but I don’t think that matters much.) There is no value in qemu-img optionally performing that operation for the caller. Adding two integers is the simplest thing there is. Max
© 2016 - 2026 Red Hat, Inc.