* [PATCH v5 0/5] qemu-img: Add convert --bitmaps
@ 2020-05-20 22:01 Eric Blake
2020-05-20 22:01 ` [PATCH v5 1/5] iotests: Fix test 178 Eric Blake
` (4 more replies)
0 siblings, 5 replies; 17+ messages in thread
From: Eric Blake @ 2020-05-20 22:01 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, nsoffer, vsementsov, qemu-block, mreitz
v4 was here:
https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg03182.html
original cover letter here:
https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg03464.html
Based-on: <20200519175707.815782-1-eblake@redhat.com>
[pull v3 bitmaps patches for 2020-05-18]
Since then:
- patch 1 is new (fixes regression from recent NBD pull)
- patch 2, 4: include fixes suggested by Vladimir; biggest is that
bitmaps computation is now in qcow2-bitmaps.c instead of qcow2.c
- patch 3: split out from patch 4 (was v4 8/9)
- patch 5: rebase to master
001/5:[down] 'iotests: Fix test 178'
002/5:[0106] [FC] 'qcow2: Expose bitmaps' size during measure'
003/5:[down] 'qemu-img: Factor out code for merging bitmaps'
004/5:[0012] [FC] 'qemu-img: Add convert --bitmaps option'
005/5:[0002] [FC] 'iotests: Add test 291 to for qemu-img bitmap coverage'
Series can also be downloaded at:
https://repo.or.cz/qemu/ericb.git/shortlog/refs/tags/qemu-img-bitmaps-v5
Eric Blake (5):
iotests: Fix test 178
qcow2: Expose bitmaps' size during measure
qemu-img: Factor out code for merging bitmaps
qemu-img: Add convert --bitmaps option
iotests: Add test 291 to for qemu-img bitmap coverage
docs/tools/qemu-img.rst | 18 ++++-
qapi/block-core.json | 15 ++--
block/qcow2.h | 2 +
block/crypto.c | 2 +-
block/qcow2-bitmap.c | 36 +++++++++
block/qcow2.c | 14 +++-
block/raw-format.c | 2 +-
qemu-img.c | 135 +++++++++++++++++++++++++++----
qemu-img-cmds.hx | 8 +-
tests/qemu-iotests/178.out.qcow2 | 18 ++++-
tests/qemu-iotests/178.out.raw | 2 +-
tests/qemu-iotests/190 | 58 ++++++++++++-
tests/qemu-iotests/190.out | 35 +++++++-
tests/qemu-iotests/291 | 112 +++++++++++++++++++++++++
tests/qemu-iotests/291.out | 80 ++++++++++++++++++
tests/qemu-iotests/group | 1 +
16 files changed, 501 insertions(+), 37 deletions(-)
create mode 100755 tests/qemu-iotests/291
create mode 100644 tests/qemu-iotests/291.out
--
2.26.2
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v5 1/5] iotests: Fix test 178
2020-05-20 22:01 [PATCH v5 0/5] qemu-img: Add convert --bitmaps Eric Blake
@ 2020-05-20 22:01 ` Eric Blake
2020-05-21 11:31 ` Vladimir Sementsov-Ogievskiy
2020-05-20 22:01 ` [PATCH v5 2/5] qcow2: Expose bitmaps' size during measure Eric Blake
` (3 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Eric Blake @ 2020-05-20 22:01 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, nsoffer, vsementsov, qemu-block, mreitz
A recent change to qemu-img changed expected error message output, but
178 takes long enough to execute that it does not get run by 'make
check' or './check -g quick'.
Fixes: 43d589b074
Signed-off-by: Eric Blake <eblake@redhat.com>
---
tests/qemu-iotests/178.out.qcow2 | 2 +-
tests/qemu-iotests/178.out.raw | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/178.out.qcow2 b/tests/qemu-iotests/178.out.qcow2
index f59bf4b2fbc4..4b69524c80ee 100644
--- a/tests/qemu-iotests/178.out.qcow2
+++ b/tests/qemu-iotests/178.out.qcow2
@@ -13,7 +13,7 @@ qemu-img: Invalid option list: ,
qemu-img: Invalid parameter 'snapshot.foo'
qemu-img: Failed in parsing snapshot param 'snapshot.foo'
qemu-img: --output must be used with human or json as argument.
-qemu-img: Image size must be less than 8 EiB!
+qemu-img: Invalid image size specified. Must be between 0 and 9223372036854775807.
qemu-img: Unknown file format 'foo'
== Size calculation for a new file (human) ==
diff --git a/tests/qemu-iotests/178.out.raw b/tests/qemu-iotests/178.out.raw
index 404ca908d8c2..20e17da115cb 100644
--- a/tests/qemu-iotests/178.out.raw
+++ b/tests/qemu-iotests/178.out.raw
@@ -13,7 +13,7 @@ qemu-img: Invalid option list: ,
qemu-img: Invalid parameter 'snapshot.foo'
qemu-img: Failed in parsing snapshot param 'snapshot.foo'
qemu-img: --output must be used with human or json as argument.
-qemu-img: Image size must be less than 8 EiB!
+qemu-img: Invalid image size specified. Must be between 0 and 9223372036854775807.
qemu-img: Unknown file format 'foo'
== Size calculation for a new file (human) ==
--
2.26.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v5 2/5] qcow2: Expose bitmaps' size during measure
2020-05-20 22:01 [PATCH v5 0/5] qemu-img: Add convert --bitmaps Eric Blake
2020-05-20 22:01 ` [PATCH v5 1/5] iotests: Fix test 178 Eric Blake
@ 2020-05-20 22:01 ` Eric Blake
2020-05-20 23:00 ` Nir Soffer
2020-05-20 22:01 ` [PATCH v5 3/5] qemu-img: Factor out code for merging bitmaps Eric Blake
` (2 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Eric Blake @ 2020-05-20 22:01 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, vsementsov, qemu-block, Markus Armbruster, mreitz,
nsoffer, John Snow
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.
On the command-line side, 'qemu-img measure' gains a new --bitmaps
flag. When present, the bitmap size is rolled into the two existing
measures (or errors if either the source image or destination format
lacks bitmaps); when absent, there is never an error (for
back-compat), but the output will instead include a new line item for
bitmaps (which you would have to manually add), with that line being
omitted in the same cases where passing --bitmaps would error.
The behavior chosen here is symmetrical with the upcoming 'qemu-img
convert --bitmaps' being added in the next patch: that is, either both
commands will succeed (your qemu-img was new enough to do bitmap
manipulations, AND you correctly measured and copied the bitmaps, even
if that measurement was 0 because there was nothing to copy) or both
fail (either your qemu-img is too old to understand --bitmaps, or it
understands it but your choice of images do not support seamless
transition of bitmaps because either source, destination, or both lack
bitmap support).
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>
---
docs/tools/qemu-img.rst | 12 ++++++-
qapi/block-core.json | 15 ++++++---
block/qcow2.h | 2 ++
block/crypto.c | 2 +-
block/qcow2-bitmap.c | 36 ++++++++++++++++++++
block/qcow2.c | 14 ++++++--
block/raw-format.c | 2 +-
qemu-img.c | 25 ++++++++++++++
qemu-img-cmds.hx | 4 +--
tests/qemu-iotests/178.out.qcow2 | 16 +++++++++
tests/qemu-iotests/190 | 58 ++++++++++++++++++++++++++++++--
tests/qemu-iotests/190.out | 35 ++++++++++++++++++-
12 files changed, 205 insertions(+), 16 deletions(-)
diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index 38d464ea3f23..9a8112fc9f58 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -593,7 +593,7 @@ Command description:
For more information, consult ``include/block/block.h`` in QEMU's
source code.
-.. option:: measure [--output=OFMT] [-O OUTPUT_FMT] [-o OPTIONS] [--size N | [--object OBJECTDEF] [--image-opts] [-f FMT] [-l SNAPSHOT_PARAM] FILENAME]
+.. option:: measure [--output=OFMT] [-O OUTPUT_FMT] [-o OPTIONS] [--size N | [--object OBJECTDEF] [--image-opts] [-f FMT] [--bitmaps] [-l SNAPSHOT_PARAM] FILENAME]
Calculate the file size required for a new image. This information
can be used to size logical volumes or SAN LUNs appropriately for
@@ -616,6 +616,7 @@ Command description:
required size: 524288
fully allocated size: 1074069504
+ bitmaps size: 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,15 @@ 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 in order to
+ copy bitmaps from a source image in addition to the guest-visible
+ data; the line is omitted if either source or destination lacks
+ bitmap support, or 0 if bitmaps are supported but there is nothing
+ to copy. If the ``--bitmaps`` option is in use, the bitmap size is
+ instead folded into the required and fully-allocated size for
+ convenience, rather than being a separate line item; using the
+ option will raise an error if bitmaps are not supported.
+
.. option:: snapshot [--object OBJECTDEF] [--image-opts] [-U] [-q] [-l | -a SNAPSHOT | -c SNAPSHOT | -d SNAPSHOT] FILENAME
List, apply, create or delete snapshots in image *FILENAME*.
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 6fbacddab2cc..d5049c309380 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -636,18 +636,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
+# allocated 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 if all the top-level bitmap metadata in
+# the source image were to be copied to the destination, present
+# when the destination supports persistent bitmaps. (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/qcow2.h b/block/qcow2.h
index 402e8acb1cb7..7ce2c23bdb7a 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -783,6 +783,8 @@ int qcow2_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
const char *name,
Error **errp);
bool qcow2_supports_persistent_dirty_bitmap(BlockDriverState *bs);
+uint64_t qcow2_get_persistent_dirty_bitmap_size(BlockDriverState *bs,
+ uint32_t cluster_size);
ssize_t coroutine_fn
qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
diff --git a/block/crypto.c b/block/crypto.c
index b216e12c3154..973b57b3eb74 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-bitmap.c b/block/qcow2-bitmap.c
index 1cf6d2ab77a3..7bf12502da8c 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1755,3 +1755,39 @@ bool qcow2_supports_persistent_dirty_bitmap(BlockDriverState *bs)
return s->qcow_version >= 3;
}
+
+/*
+ * Compute the space required for bitmaps in @bs.
+ *
+ * The computation is based as if copying to a new image with the
+ * given @cluster_size, which may differ from the cluster size in @bs.
+ */
+uint64_t qcow2_get_persistent_dirty_bitmap_size(BlockDriverState *bs,
+ uint32_t cluster_size)
+{
+ uint64_t bitmaps_size = 0;
+ BdrvDirtyBitmap *bm;
+ size_t bitmap_dir_size = 0;
+
+ FOR_EACH_DIRTY_BITMAP(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 bmbytes =
+ get_bitmap_bytes_needed(bdrv_dirty_bitmap_size(bm),
+ granularity);
+ uint64_t bmclusters = DIV_ROUND_UP(bmbytes, 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 += calc_dir_entry_size(strlen(name), 0);
+ }
+ }
+ bitmaps_size += ROUND_UP(bitmap_dir_size, cluster_size);
+
+ return bitmaps_size;
+}
diff --git a/block/qcow2.c b/block/qcow2.c
index dfab8d2f6cd8..0cd2e6757e8c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4953,16 +4953,24 @@ 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_supports_persistent_dirty_bitmap(in_bs);
+ if (info->has_bitmaps) {
+ info->bitmaps = qcow2_get_persistent_dirty_bitmap_size(in_bs,
+ cluster_size);
+ }
return info;
err:
diff --git a/block/raw-format.c b/block/raw-format.c
index 018441bddf27..233d019ca338 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -359,7 +359,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 2d30682f129f..d719b9d35468 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -78,6 +78,7 @@ enum {
OPTION_ENABLE = 272,
OPTION_DISABLE = 273,
OPTION_MERGE = 274,
+ OPTION_BITMAPS = 275,
};
typedef enum OutputFormat {
@@ -5128,6 +5129,7 @@ static int img_measure(int argc, char **argv)
{"output", required_argument, 0, OPTION_OUTPUT},
{"size", required_argument, 0, OPTION_SIZE},
{"force-share", no_argument, 0, 'U'},
+ {"bitmaps", no_argument, 0, OPTION_BITMAPS},
{0, 0, 0, 0}
};
OutputFormat output_format = OFORMAT_HUMAN;
@@ -5144,6 +5146,7 @@ static int img_measure(int argc, char **argv)
QemuOpts *sn_opts = NULL;
QemuOptsList *create_opts = NULL;
bool image_opts = false;
+ bool bitmaps = false;
uint64_t img_size = UINT64_MAX;
BlockMeasureInfo *info = NULL;
Error *local_err = NULL;
@@ -5216,6 +5219,9 @@ static int img_measure(int argc, char **argv)
img_size = (uint64_t)sval;
}
break;
+ case OPTION_BITMAPS:
+ bitmaps = true;
+ break;
}
}
@@ -5244,6 +5250,10 @@ static int img_measure(int argc, char **argv)
error_report("Either --size N or one filename must be specified.");
goto out;
}
+ if (!filename && bitmaps) {
+ error_report("--bitmaps is only supported with a filename.");
+ goto out;
+ }
if (filename) {
in_blk = img_open(image_opts, filename, fmt, 0,
@@ -5299,9 +5309,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;
+ }
+ }
+
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/qemu-img-cmds.hx b/qemu-img-cmds.hx
index a87d3cb264ce..235cc5fffadc 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -76,9 +76,9 @@ SRST
ERST
DEF("measure", img_measure,
-"measure [--output=ofmt] [-O output_fmt] [-o options] [--size N | [--object objectdef] [--image-opts] [-f fmt] [-l snapshot_param] filename]")
+"measure [--output=ofmt] [-O output_fmt] [-o options] [--size N | [--object objectdef] [--image-opts] [-f fmt] [--bitmaps] [-l snapshot_param] filename]")
SRST
-.. option:: measure [--output=OFMT] [-O OUTPUT_FMT] [-o OPTIONS] [--size N | [--object OBJECTDEF] [--image-opts] [-f FMT] [-l SNAPSHOT_PARAM] FILENAME]
+.. option:: measure [--output=OFMT] [-O OUTPUT_FMT] [-o OPTIONS] [--size N | [--object OBJECTDEF] [--image-opts] [-f FMT] [--bitmaps] [-l SNAPSHOT_PARAM] FILENAME]
ERST
DEF("snapshot", img_snapshot,
diff --git a/tests/qemu-iotests/178.out.qcow2 b/tests/qemu-iotests/178.out.qcow2
index 4b69524c80ee..c7997760fd6f 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..acb23ebae44b 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,60 @@ $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 without a source
+$QEMU_IMG measure --bitmaps -O qcow2 --size 10M &&
+ echo "unexpected success"
+# No bitmap output, since raw does not support it
+$QEMU_IMG measure -O raw -f qcow2 "$TEST_IMG" ||
+ echo "unexpected failure"
+$QEMU_IMG measure --bitmaps -O raw -f qcow2 "$TEST_IMG" &&
+ echo "unexpected success"
+# No bitmap output, since no bitmaps on raw source
+$QEMU_IMG measure -O qcow2 -f raw "$TEST_IMG" ||
+ echo "unexpected failure"
+$QEMU_IMG measure --bitmaps -O qcow2 -f raw "$TEST_IMG" &&
+ echo "unexpected success"
+# No bitmap output, since v2 does not support it
+$QEMU_IMG measure -O qcow2 -o compat=0.10 -f qcow2 "$TEST_IMG" ||
+ echo "unexpected failure"
+$QEMU_IMG measure --bitmaps -O qcow2 -o compat=0.10 -f qcow2 "$TEST_IMG" &&
+ echo "unexpected success"
+
+# Compute expected output: bitmap clusters + bitmap tables + bitmaps directory
+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"
+$QEMU_IMG measure --bitmaps -O qcow2 -o cluster_size=64k -f qcow2 "$TEST_IMG"
+
+# Compute expected output: bitmap clusters + bitmap tables + bitmaps directory
+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 --output=json -O qcow2 -o cluster_size=2M -f qcow2 "$TEST_IMG"
+$QEMU_IMG measure --output=json --bitmaps -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..5c35f9268068 100644
--- a/tests/qemu-iotests/190.out
+++ b/tests/qemu-iotests/190.out
@@ -1,11 +1,44 @@
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 ==
+
+qemu-img: --bitmaps is only supported with a filename.
+required size: 2199023255552
+fully allocated size: 2199023255552
+qemu-img: no bitmaps measured, either source or destination format lacks bitmap support
+required size: 7012352
+fully allocated size: 17170432
+qemu-img: no bitmaps measured, either source or destination format lacks bitmap support
+required size: 335806464
+fully allocated size: 2199359062016
+qemu-img: no bitmaps measured, either source or destination format lacks bitmap support
+
+expected bitmap 537198592
+required size: 335806464
+fully allocated size: 2199359062016
+bitmaps size: 537198592
+required size: 873005056
+fully allocated size: 2199896260608
+
+expected bitmap 545259520
+{
+ "bitmaps": 545259520,
+ "required": 18874368,
+ "fully-allocated": 2199042129920
+}
+{
+ "required": 564133888,
+ "fully-allocated": 2199587389440
+}
*** done
--
2.26.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v5 3/5] qemu-img: Factor out code for merging bitmaps
2020-05-20 22:01 [PATCH v5 0/5] qemu-img: Add convert --bitmaps Eric Blake
2020-05-20 22:01 ` [PATCH v5 1/5] iotests: Fix test 178 Eric Blake
2020-05-20 22:01 ` [PATCH v5 2/5] qcow2: Expose bitmaps' size during measure Eric Blake
@ 2020-05-20 22:01 ` Eric Blake
2020-05-20 22:01 ` [PATCH v5 4/5] qemu-img: Add convert --bitmaps option Eric Blake
2020-05-20 22:01 ` [PATCH v5 5/5] iotests: Add test 291 to for qemu-img bitmap coverage Eric Blake
4 siblings, 0 replies; 17+ messages in thread
From: Eric Blake @ 2020-05-20 22:01 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, nsoffer, vsementsov, qemu-block, mreitz
The next patch will add another client that wants to merge dirty
bitmaps; it will be easier to refactor the code to construct the QAPI
struct correctly into a helper function.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
qemu-img.c | 33 ++++++++++++++++++++-------------
1 file changed, 20 insertions(+), 13 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index d719b9d35468..c1bafb57023a 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1639,6 +1639,23 @@ out4:
return ret;
}
+static void do_dirty_bitmap_merge(const char *dst_node, const char *dst_name,
+ const char *src_node, const char *src_name,
+ Error **errp)
+{
+ BlockDirtyBitmapMergeSource *merge_src;
+ BlockDirtyBitmapMergeSourceList *list;
+
+ merge_src = g_new0(BlockDirtyBitmapMergeSource, 1);
+ merge_src->type = QTYPE_QDICT;
+ merge_src->u.external.node = g_strdup(src_node);
+ merge_src->u.external.name = g_strdup(src_name);
+ list = g_new0(BlockDirtyBitmapMergeSourceList, 1);
+ list->value = merge_src;
+ qmp_block_dirty_bitmap_merge(dst_node, dst_name, list, errp);
+ qapi_free_BlockDirtyBitmapMergeSourceList(list);
+}
+
enum ImgConvertBlockStatus {
BLK_DATA,
BLK_ZERO,
@@ -4715,21 +4732,11 @@ static int img_bitmap(int argc, char **argv)
qmp_block_dirty_bitmap_disable(bs->node_name, bitmap, &err);
op = "disable";
break;
- case BITMAP_MERGE: {
- BlockDirtyBitmapMergeSource *merge_src;
- BlockDirtyBitmapMergeSourceList *list;
-
- merge_src = g_new0(BlockDirtyBitmapMergeSource, 1);
- merge_src->type = QTYPE_QDICT;
- merge_src->u.external.node = g_strdup(src_bs->node_name);
- merge_src->u.external.name = g_strdup(act->src);
- list = g_new0(BlockDirtyBitmapMergeSourceList, 1);
- list->value = merge_src;
- qmp_block_dirty_bitmap_merge(bs->node_name, bitmap, list, &err);
- qapi_free_BlockDirtyBitmapMergeSourceList(list);
+ case BITMAP_MERGE:
+ do_dirty_bitmap_merge(bs->node_name, bitmap, src_bs->node_name,
+ act->src, &err);
op = "merge";
break;
- }
default:
g_assert_not_reached();
}
--
2.26.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v5 4/5] qemu-img: Add convert --bitmaps option
2020-05-20 22:01 [PATCH v5 0/5] qemu-img: Add convert --bitmaps Eric Blake
` (2 preceding siblings ...)
2020-05-20 22:01 ` [PATCH v5 3/5] qemu-img: Factor out code for merging bitmaps Eric Blake
@ 2020-05-20 22:01 ` Eric Blake
2020-05-21 15:11 ` Nir Soffer
2020-05-20 22:01 ` [PATCH v5 5/5] iotests: Add test 291 to for qemu-img bitmap coverage Eric Blake
4 siblings, 1 reply; 17+ messages in thread
From: Eric Blake @ 2020-05-20 22:01 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, nsoffer, vsementsov, qemu-block, mreitz
Make it easier to copy all the persistent bitmaps of (the top layer
of) a source image along with its guest-visible contents, by adding a
boolean flag for use with qemu-img convert. This is basically
shorthand, as the same effect could be accomplished with a series of
'qemu-img bitmap --add' and 'qemu-img bitmap --merge -b source'
commands, or by QMP commands.
Note that this command will fail in the same scenarios where 'qemu-img
measure --bitmaps' fails, when either the source or the destanation
lacks persistent bitmap support altogether.
See also https://bugzilla.redhat.com/show_bug.cgi?id=1779893
While touching this, clean up a couple coding issues spotted in the
same function: an extra blank line, and merging back-to-back 'if
(!skip_create)' blocks.
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20200513011648.166876-9-eblake@redhat.com>
---
docs/tools/qemu-img.rst | 6 +++-
qemu-img.c | 77 +++++++++++++++++++++++++++++++++++++++--
qemu-img-cmds.hx | 4 +--
3 files changed, 81 insertions(+), 6 deletions(-)
diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index 9a8112fc9f58..35050fc51070 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -162,6 +162,10 @@ Parameters to convert subcommand:
.. program:: qemu-img-convert
+.. option:: --bitmaps
+
+ Additionally copy all persistent bitmaps from the top layer of the source
+
.. option:: -n
Skip the creation of the target volume
@@ -397,7 +401,7 @@ Command description:
4
Error on reading data
-.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [--target-is-zero] [-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-m NUM_COROUTINES] [-W] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME
+.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [--target-is-zero] [--bitmaps] [-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-m NUM_COROUTINES] [-W] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME
Convert the disk image *FILENAME* or a snapshot *SNAPSHOT_PARAM*
to disk image *OUTPUT_FILENAME* using format *OUTPUT_FMT*. It can
diff --git a/qemu-img.c b/qemu-img.c
index c1bafb57023a..1494d8f5c409 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -192,6 +192,7 @@ static void QEMU_NORETURN help(void)
" hiding corruption that has already occurred.\n"
"\n"
"Parameters to convert subcommand:\n"
+ " '--bitmaps' copies all top-level persistent bitmaps to destination\n"
" '-m' specifies how many coroutines work in parallel during the convert\n"
" process (defaults to 8)\n"
" '-W' allow to write to the target out of order rather than sequential\n"
@@ -2139,6 +2140,39 @@ static int convert_do_copy(ImgConvertState *s)
return s->ret;
}
+static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst)
+{
+ BdrvDirtyBitmap *bm;
+ Error *err = NULL;
+
+ FOR_EACH_DIRTY_BITMAP(src, bm) {
+ const char *name;
+
+ if (!bdrv_dirty_bitmap_get_persistence(bm)) {
+ continue;
+ }
+ name = bdrv_dirty_bitmap_name(bm);
+ qmp_block_dirty_bitmap_add(dst->node_name, name,
+ true, bdrv_dirty_bitmap_granularity(bm),
+ true, true,
+ true, !bdrv_dirty_bitmap_enabled(bm),
+ &err);
+ if (err) {
+ error_reportf_err(err, "Failed to create bitmap %s: ", name);
+ return -1;
+ }
+
+ do_dirty_bitmap_merge(dst->node_name, name, src->node_name, name,
+ &err);
+ if (err) {
+ error_reportf_err(err, "Failed to populate bitmap %s: ", name);
+ return -1;
+ }
+ }
+
+ return 0;
+}
+
#define MAX_BUF_SECTORS 32768
static int img_convert(int argc, char **argv)
@@ -2160,6 +2194,8 @@ static int img_convert(int argc, char **argv)
int64_t ret = -EINVAL;
bool force_share = false;
bool explict_min_sparse = false;
+ bool bitmaps = false;
+ size_t nbitmaps = 0;
ImgConvertState s = (ImgConvertState) {
/* Need at least 4k of zeros for sparse detection */
@@ -2179,6 +2215,7 @@ static int img_convert(int argc, char **argv)
{"target-image-opts", no_argument, 0, OPTION_TARGET_IMAGE_OPTS},
{"salvage", no_argument, 0, OPTION_SALVAGE},
{"target-is-zero", no_argument, 0, OPTION_TARGET_IS_ZERO},
+ {"bitmaps", no_argument, 0, OPTION_BITMAPS},
{0, 0, 0, 0}
};
c = getopt_long(argc, argv, ":hf:O:B:Cco:l:S:pt:T:qnm:WU",
@@ -2304,6 +2341,9 @@ static int img_convert(int argc, char **argv)
*/
s.has_zero_init = true;
break;
+ case OPTION_BITMAPS:
+ bitmaps = true;
+ break;
}
}
@@ -2365,7 +2405,6 @@ static int img_convert(int argc, char **argv)
goto fail_getopt;
}
-
/* ret is still -EINVAL until here */
ret = bdrv_parse_cache_mode(src_cache, &src_flags, &src_writethrough);
if (ret < 0) {
@@ -2525,6 +2564,27 @@ static int img_convert(int argc, char **argv)
}
}
+ /* Determine how many bitmaps need copying */
+ if (bitmaps) {
+ BdrvDirtyBitmap *bm;
+
+ if (s.src_num > 1) {
+ error_report("Copying bitmaps only possible with single source");
+ ret = -1;
+ goto out;
+ }
+ if (!bdrv_supports_persistent_dirty_bitmap(blk_bs(s.src[0]))) {
+ error_report("Source lacks bitmap support");
+ ret = -1;
+ goto out;
+ }
+ FOR_EACH_DIRTY_BITMAP(blk_bs(s.src[0]), bm) {
+ if (bdrv_dirty_bitmap_get_persistence(bm)) {
+ nbitmaps++;
+ }
+ }
+ }
+
/*
* The later open call will need any decryption secrets, and
* bdrv_create() will purge "opts", so extract them now before
@@ -2533,9 +2593,7 @@ static int img_convert(int argc, char **argv)
if (!skip_create) {
open_opts = qdict_new();
qemu_opt_foreach(opts, img_add_key_secrets, open_opts, &error_abort);
- }
- if (!skip_create) {
/* Create the new image */
ret = bdrv_create(drv, out_filename, opts, &local_err);
if (ret < 0) {
@@ -2573,6 +2631,13 @@ static int img_convert(int argc, char **argv)
}
out_bs = blk_bs(s.target);
+ if (nbitmaps > 0 && !bdrv_supports_persistent_dirty_bitmap(out_bs)) {
+ error_report("Format driver '%s' does not support bitmaps",
+ out_fmt);
+ ret = -1;
+ goto out;
+ }
+
if (s.compressed && !block_driver_can_compress(out_bs->drv)) {
error_report("Compression not supported for this file format");
ret = -1;
@@ -2632,6 +2697,12 @@ static int img_convert(int argc, char **argv)
}
ret = convert_do_copy(&s);
+
+ /* Now copy the bitmaps */
+ if (nbitmaps > 0 && ret == 0) {
+ ret = convert_copy_bitmaps(blk_bs(s.src[0]), out_bs);
+ }
+
out:
if (!ret) {
qemu_progress_print(100, 0);
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 235cc5fffadc..e9beb15e614e 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -46,9 +46,9 @@ SRST
ERST
DEF("convert", img_convert,
- "convert [--object objectdef] [--image-opts] [--target-image-opts] [--target-is-zero] [-U] [-C] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-B backing_file] [-o options] [-l snapshot_param] [-S sparse_size] [-m num_coroutines] [-W] [--salvage] filename [filename2 [...]] output_filename")
+ "convert [--object objectdef] [--image-opts] [--target-image-opts] [--target-is-zero] [--bitmaps] [-U] [-C] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-B backing_file] [-o options] [-l snapshot_param] [-S sparse_size] [-m num_coroutines] [-W] [--salvage] filename [filename2 [...]] output_filename")
SRST
-.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [--target-is-zero] [-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-m NUM_COROUTINES] [-W] [--salvage] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME
+.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [--target-is-zero] [--bitmaps] [-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-m NUM_COROUTINES] [-W] [--salvage] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME
ERST
DEF("create", img_create,
--
2.26.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v5 5/5] iotests: Add test 291 to for qemu-img bitmap coverage
2020-05-20 22:01 [PATCH v5 0/5] qemu-img: Add convert --bitmaps Eric Blake
` (3 preceding siblings ...)
2020-05-20 22:01 ` [PATCH v5 4/5] qemu-img: Add convert --bitmaps option Eric Blake
@ 2020-05-20 22:01 ` Eric Blake
4 siblings, 0 replies; 17+ messages in thread
From: Eric Blake @ 2020-05-20 22:01 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, nsoffer, vsementsov, qemu-block, mreitz
Add a new test covering the 'qemu-img bitmap' subcommand, as well as
'qemu-img convert --bitmaps', both added in recent patches.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
tests/qemu-iotests/291 | 112 +++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/291.out | 80 ++++++++++++++++++++++++++
tests/qemu-iotests/group | 1 +
3 files changed, 193 insertions(+)
create mode 100755 tests/qemu-iotests/291
create mode 100644 tests/qemu-iotests/291.out
diff --git a/tests/qemu-iotests/291 b/tests/qemu-iotests/291
new file mode 100755
index 000000000000..3ca83b9cd1f7
--- /dev/null
+++ b/tests/qemu-iotests/291
@@ -0,0 +1,112 @@
+#!/usr/bin/env bash
+#
+# Test qemu-img bitmap handling
+#
+# Copyright (C) 2018-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
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+status=1 # failure is the default!
+
+_cleanup()
+{
+ _cleanup_test_img
+ nbd_server_stop
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.nbd
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+_require_command QEMU_NBD
+
+echo
+echo "=== Initial image setup ==="
+echo
+
+# Create backing image with one bitmap
+TEST_IMG="$TEST_IMG.base" _make_test_img 10M
+$QEMU_IMG bitmap --add -f $IMGFMT "$TEST_IMG.base" b0
+$QEMU_IO -c 'w 3M 1M' -f $IMGFMT "$TEST_IMG.base" | _filter_qemu_io
+
+# Create initial image and populate two bitmaps: one active, one inactive.
+ORIG_IMG=$TEST_IMG
+TEST_IMG=$TEST_IMG.orig
+_make_test_img -b "$ORIG_IMG.base" -F $IMGFMT 10M
+$QEMU_IO -c 'w 0 1M' -f $IMGFMT "$TEST_IMG" | _filter_qemu_io
+$QEMU_IMG bitmap --add -g 512k -f $IMGFMT "$TEST_IMG" b1
+$QEMU_IMG bitmap --add --disable -f $IMGFMT "$TEST_IMG" b2
+$QEMU_IO -c 'w 3M 1M' -f $IMGFMT "$TEST_IMG" | _filter_qemu_io
+$QEMU_IMG bitmap --clear -f $IMGFMT "$TEST_IMG" b1
+$QEMU_IO -c 'w 1M 1M' -f $IMGFMT "$TEST_IMG" | _filter_qemu_io
+$QEMU_IMG bitmap --disable -f $IMGFMT "$TEST_IMG" b1
+$QEMU_IMG bitmap --enable -f $IMGFMT "$TEST_IMG" b2
+$QEMU_IO -c 'w 2M 1M' -f $IMGFMT "$TEST_IMG" | _filter_qemu_io
+
+echo
+echo "=== Bitmap preservation not possible to non-qcow2 ==="
+echo
+
+TEST_IMG=$ORIG_IMG
+$QEMU_IMG convert --bitmaps -O raw "$TEST_IMG.orig" "$TEST_IMG" &&
+ echo "unexpected success"
+
+echo
+echo "=== Convert with bitmap preservation ==="
+echo
+
+# Only bitmaps from the active layer are copied
+$QEMU_IMG convert --bitmaps -O qcow2 "$TEST_IMG.orig" "$TEST_IMG"
+$QEMU_IMG info "$TEST_IMG" | _filter_img_info --format-specific
+# But we can also merge in bitmaps from other layers. This test is a bit
+# contrived to cover more code paths, in reality, you could merge directly
+# into b0 without going through tmp
+$QEMU_IMG bitmap --add --disable -f $IMGFMT "$TEST_IMG" b0
+$QEMU_IMG bitmap --add --merge b0 -b "$TEST_IMG.base" -F $IMGFMT \
+ -f $IMGFMT "$TEST_IMG" tmp
+$QEMU_IMG bitmap --merge tmp -f $IMGFMT "$TEST_IMG" b0
+$QEMU_IMG bitmap --remove --image-opts \
+ driver=$IMGFMT,file.driver=file,file.filename="$TEST_IMG" tmp
+$QEMU_IMG info "$TEST_IMG" | _filter_img_info --format-specific
+
+echo
+echo "=== Check bitmap contents ==="
+echo
+
+# x-dirty-bitmap is a hack for reading bitmaps; it abuses block status to
+# report "data":false for portions of the bitmap which are set
+IMG="driver=nbd,server.type=unix,server.path=$nbd_unix_socket"
+nbd_server_start_unix_socket -r -f qcow2 -B b0 "$TEST_IMG"
+$QEMU_IMG map --output=json --image-opts \
+ "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b0" | _filter_qemu_img_map
+nbd_server_start_unix_socket -r -f qcow2 -B b1 "$TEST_IMG"
+$QEMU_IMG map --output=json --image-opts \
+ "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b1" | _filter_qemu_img_map
+nbd_server_start_unix_socket -r -f qcow2 -B b2 "$TEST_IMG"
+$QEMU_IMG map --output=json --image-opts \
+ "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map
+
+# success, all done
+echo '*** done'
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/291.out b/tests/qemu-iotests/291.out
new file mode 100644
index 000000000000..8c62017567e9
--- /dev/null
+++ b/tests/qemu-iotests/291.out
@@ -0,0 +1,80 @@
+QA output created by 291
+
+=== Initial image setup ===
+
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=10485760
+wrote 1048576/1048576 bytes at offset 3145728
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=10485760 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1048576/1048576 bytes at offset 3145728
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1048576/1048576 bytes at offset 2097152
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Bitmap preservation not possible to non-qcow2 ===
+
+qemu-img: Format driver 'raw' does not support bitmaps
+
+=== Convert with bitmap preservation ===
+
+image: TEST_DIR/t.IMGFMT
+file format: IMGFMT
+virtual size: 10 MiB (10485760 bytes)
+disk size: 4.39 MiB
+Format specific information:
+ compat: 1.1
+ compression type: zlib
+ lazy refcounts: false
+ bitmaps:
+ [0]:
+ flags:
+ name: b1
+ granularity: 524288
+ [1]:
+ flags:
+ [0]: auto
+ name: b2
+ granularity: 65536
+ refcount bits: 16
+ corrupt: false
+image: TEST_DIR/t.IMGFMT
+file format: IMGFMT
+virtual size: 10 MiB (10485760 bytes)
+disk size: 4.48 MiB
+Format specific information:
+ compat: 1.1
+ compression type: zlib
+ lazy refcounts: false
+ bitmaps:
+ [0]:
+ flags:
+ name: b1
+ granularity: 524288
+ [1]:
+ flags:
+ [0]: auto
+ name: b2
+ granularity: 65536
+ [2]:
+ flags:
+ name: b0
+ granularity: 65536
+ refcount bits: 16
+ corrupt: false
+
+=== Check bitmap contents ===
+
+[{ "start": 0, "length": 3145728, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
+{ "start": 3145728, "length": 1048576, "depth": 0, "zero": false, "data": false},
+{ "start": 4194304, "length": 6291456, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
+[{ "start": 0, "length": 1048576, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
+{ "start": 1048576, "length": 1048576, "depth": 0, "zero": false, "data": false},
+{ "start": 2097152, "length": 8388608, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
+[{ "start": 0, "length": 2097152, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
+{ "start": 2097152, "length": 1048576, "depth": 0, "zero": false, "data": false},
+{ "start": 3145728, "length": 7340032, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 445c26f8d2e2..d886fa0cb355 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -299,5 +299,6 @@
288 quick
289 rw quick
290 rw auto quick
+291 rw quick
292 rw auto quick
297 meta
--
2.26.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v5 2/5] qcow2: Expose bitmaps' size during measure
2020-05-20 22:01 ` [PATCH v5 2/5] qcow2: Expose bitmaps' size during measure Eric Blake
@ 2020-05-20 23:00 ` Nir Soffer
2020-05-21 11:40 ` Vladimir Sementsov-Ogievskiy
2020-05-21 13:08 ` Eric Blake
0 siblings, 2 replies; 17+ messages in thread
From: Nir Soffer @ 2020-05-20 23:00 UTC (permalink / raw)
To: Eric Blake
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block,
Markus Armbruster, QEMU Developers, Max Reitz, John Snow
On Thu, May 21, 2020 at 1:01 AM 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 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.
>
> On the command-line side, 'qemu-img measure' gains a new --bitmaps
> flag. When present, the bitmap size is rolled into the two existing
> measures (or errors if either the source image or destination format
> lacks bitmaps); when absent, there is never an error (for
> back-compat), but the output will instead include a new line item for
> bitmaps (which you would have to manually add), with that line being
> omitted in the same cases where passing --bitmaps would error.
Supporting 2 ways to measure, one by specifying --bitmaps, and another
by adding bitmaps key is not a good idea. We really need one way.
Each one has advantages. adding --bitmaps flag is consistent with
"qemu-img convert"
and future extensions that may require new flag, and adding "bitmaps"
key is consistent
with "qmeu-img info", showing bitmaps when they exist.
Adding a "bitmaps" key has an advantage that we can use it to test if qemu-img
supports measuring and copying bitmaps (since both features are expected to
be delivered at the same time). So we can avoid checking --help learn about
the capabilities.
I'm ok with both options, can we have only one?
> The behavior chosen here is symmetrical with the upcoming 'qemu-img
> convert --bitmaps' being added in the next patch: that is, either both
> commands will succeed (your qemu-img was new enough to do bitmap
> manipulations, AND you correctly measured and copied the bitmaps, even
> if that measurement was 0 because there was nothing to copy) or both
> fail (either your qemu-img is too old to understand --bitmaps, or it
> understands it but your choice of images do not support seamless
> transition of bitmaps because either source, destination, or both lack
> bitmap support).
>
> 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>
> ---
> docs/tools/qemu-img.rst | 12 ++++++-
> qapi/block-core.json | 15 ++++++---
> block/qcow2.h | 2 ++
> block/crypto.c | 2 +-
> block/qcow2-bitmap.c | 36 ++++++++++++++++++++
> block/qcow2.c | 14 ++++++--
> block/raw-format.c | 2 +-
> qemu-img.c | 25 ++++++++++++++
> qemu-img-cmds.hx | 4 +--
> tests/qemu-iotests/178.out.qcow2 | 16 +++++++++
> tests/qemu-iotests/190 | 58 ++++++++++++++++++++++++++++++--
> tests/qemu-iotests/190.out | 35 ++++++++++++++++++-
> 12 files changed, 205 insertions(+), 16 deletions(-)
>
> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
> index 38d464ea3f23..9a8112fc9f58 100644
> --- a/docs/tools/qemu-img.rst
> +++ b/docs/tools/qemu-img.rst
> @@ -593,7 +593,7 @@ Command description:
> For more information, consult ``include/block/block.h`` in QEMU's
> source code.
>
> -.. option:: measure [--output=OFMT] [-O OUTPUT_FMT] [-o OPTIONS] [--size N | [--object OBJECTDEF] [--image-opts] [-f FMT] [-l SNAPSHOT_PARAM] FILENAME]
> +.. option:: measure [--output=OFMT] [-O OUTPUT_FMT] [-o OPTIONS] [--size N | [--object OBJECTDEF] [--image-opts] [-f FMT] [--bitmaps] [-l SNAPSHOT_PARAM] FILENAME]
>
> Calculate the file size required for a new image. This information
> can be used to size logical volumes or SAN LUNs appropriately for
> @@ -616,6 +616,7 @@ Command description:
>
> required size: 524288
> fully allocated size: 1074069504
> + bitmaps size: 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,15 @@ 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 in order to
> + copy bitmaps from a source image in addition to the guest-visible
> + data; the line is omitted if either source or destination lacks
> + bitmap support, or 0 if bitmaps are supported but there is nothing
> + to copy. If the ``--bitmaps`` option is in use, the bitmap size is
> + instead folded into the required and fully-allocated size for
> + convenience, rather than being a separate line item; using the
> + option will raise an error if bitmaps are not supported.
> +
> .. option:: snapshot [--object OBJECTDEF] [--image-opts] [-U] [-q] [-l | -a SNAPSHOT | -c SNAPSHOT | -d SNAPSHOT] FILENAME
>
> List, apply, create or delete snapshots in image *FILENAME*.
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 6fbacddab2cc..d5049c309380 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -636,18 +636,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
> +# allocated 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 if all the top-level bitmap metadata in
> +# the source image were to be copied to the destination, present
> +# when the destination supports persistent bitmaps. (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/qcow2.h b/block/qcow2.h
> index 402e8acb1cb7..7ce2c23bdb7a 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -783,6 +783,8 @@ int qcow2_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
> const char *name,
> Error **errp);
> bool qcow2_supports_persistent_dirty_bitmap(BlockDriverState *bs);
> +uint64_t qcow2_get_persistent_dirty_bitmap_size(BlockDriverState *bs,
> + uint32_t cluster_size);
>
> ssize_t coroutine_fn
> qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
> diff --git a/block/crypto.c b/block/crypto.c
> index b216e12c3154..973b57b3eb74 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-bitmap.c b/block/qcow2-bitmap.c
> index 1cf6d2ab77a3..7bf12502da8c 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1755,3 +1755,39 @@ bool qcow2_supports_persistent_dirty_bitmap(BlockDriverState *bs)
>
> return s->qcow_version >= 3;
> }
> +
> +/*
> + * Compute the space required for bitmaps in @bs.
> + *
> + * The computation is based as if copying to a new image with the
> + * given @cluster_size, which may differ from the cluster size in @bs.
> + */
> +uint64_t qcow2_get_persistent_dirty_bitmap_size(BlockDriverState *bs,
> + uint32_t cluster_size)
> +{
> + uint64_t bitmaps_size = 0;
> + BdrvDirtyBitmap *bm;
> + size_t bitmap_dir_size = 0;
> +
> + FOR_EACH_DIRTY_BITMAP(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 bmbytes =
> + get_bitmap_bytes_needed(bdrv_dirty_bitmap_size(bm),
> + granularity);
> + uint64_t bmclusters = DIV_ROUND_UP(bmbytes, 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 += calc_dir_entry_size(strlen(name), 0);
> + }
> + }
> + bitmaps_size += ROUND_UP(bitmap_dir_size, cluster_size);
> +
> + return bitmaps_size;
> +}
> diff --git a/block/qcow2.c b/block/qcow2.c
> index dfab8d2f6cd8..0cd2e6757e8c 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -4953,16 +4953,24 @@ 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_supports_persistent_dirty_bitmap(in_bs);
> + if (info->has_bitmaps) {
> + info->bitmaps = qcow2_get_persistent_dirty_bitmap_size(in_bs,
> + cluster_size);
> + }
> return info;
>
> err:
> diff --git a/block/raw-format.c b/block/raw-format.c
> index 018441bddf27..233d019ca338 100644
> --- a/block/raw-format.c
> +++ b/block/raw-format.c
> @@ -359,7 +359,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 2d30682f129f..d719b9d35468 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -78,6 +78,7 @@ enum {
> OPTION_ENABLE = 272,
> OPTION_DISABLE = 273,
> OPTION_MERGE = 274,
> + OPTION_BITMAPS = 275,
> };
>
> typedef enum OutputFormat {
> @@ -5128,6 +5129,7 @@ static int img_measure(int argc, char **argv)
> {"output", required_argument, 0, OPTION_OUTPUT},
> {"size", required_argument, 0, OPTION_SIZE},
> {"force-share", no_argument, 0, 'U'},
> + {"bitmaps", no_argument, 0, OPTION_BITMAPS},
> {0, 0, 0, 0}
> };
> OutputFormat output_format = OFORMAT_HUMAN;
> @@ -5144,6 +5146,7 @@ static int img_measure(int argc, char **argv)
> QemuOpts *sn_opts = NULL;
> QemuOptsList *create_opts = NULL;
> bool image_opts = false;
> + bool bitmaps = false;
> uint64_t img_size = UINT64_MAX;
> BlockMeasureInfo *info = NULL;
> Error *local_err = NULL;
> @@ -5216,6 +5219,9 @@ static int img_measure(int argc, char **argv)
> img_size = (uint64_t)sval;
> }
> break;
> + case OPTION_BITMAPS:
> + bitmaps = true;
> + break;
> }
> }
>
> @@ -5244,6 +5250,10 @@ static int img_measure(int argc, char **argv)
> error_report("Either --size N or one filename must be specified.");
> goto out;
> }
> + if (!filename && bitmaps) {
> + error_report("--bitmaps is only supported with a filename.");
> + goto out;
> + }
>
> if (filename) {
> in_blk = img_open(image_opts, filename, fmt, 0,
> @@ -5299,9 +5309,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;
> + }
> + }
> +
> 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/qemu-img-cmds.hx b/qemu-img-cmds.hx
> index a87d3cb264ce..235cc5fffadc 100644
> --- a/qemu-img-cmds.hx
> +++ b/qemu-img-cmds.hx
> @@ -76,9 +76,9 @@ SRST
> ERST
>
> DEF("measure", img_measure,
> -"measure [--output=ofmt] [-O output_fmt] [-o options] [--size N | [--object objectdef] [--image-opts] [-f fmt] [-l snapshot_param] filename]")
> +"measure [--output=ofmt] [-O output_fmt] [-o options] [--size N | [--object objectdef] [--image-opts] [-f fmt] [--bitmaps] [-l snapshot_param] filename]")
> SRST
> -.. option:: measure [--output=OFMT] [-O OUTPUT_FMT] [-o OPTIONS] [--size N | [--object OBJECTDEF] [--image-opts] [-f FMT] [-l SNAPSHOT_PARAM] FILENAME]
> +.. option:: measure [--output=OFMT] [-O OUTPUT_FMT] [-o OPTIONS] [--size N | [--object OBJECTDEF] [--image-opts] [-f FMT] [--bitmaps] [-l SNAPSHOT_PARAM] FILENAME]
> ERST
>
> DEF("snapshot", img_snapshot,
> diff --git a/tests/qemu-iotests/178.out.qcow2 b/tests/qemu-iotests/178.out.qcow2
> index 4b69524c80ee..c7997760fd6f 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..acb23ebae44b 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,60 @@ $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 without a source
> +$QEMU_IMG measure --bitmaps -O qcow2 --size 10M &&
> + echo "unexpected success"
> +# No bitmap output, since raw does not support it
> +$QEMU_IMG measure -O raw -f qcow2 "$TEST_IMG" ||
> + echo "unexpected failure"
> +$QEMU_IMG measure --bitmaps -O raw -f qcow2 "$TEST_IMG" &&
> + echo "unexpected success"
> +# No bitmap output, since no bitmaps on raw source
> +$QEMU_IMG measure -O qcow2 -f raw "$TEST_IMG" ||
> + echo "unexpected failure"
> +$QEMU_IMG measure --bitmaps -O qcow2 -f raw "$TEST_IMG" &&
> + echo "unexpected success"
> +# No bitmap output, since v2 does not support it
> +$QEMU_IMG measure -O qcow2 -o compat=0.10 -f qcow2 "$TEST_IMG" ||
> + echo "unexpected failure"
> +$QEMU_IMG measure --bitmaps -O qcow2 -o compat=0.10 -f qcow2 "$TEST_IMG" &&
> + echo "unexpected success"
> +
> +# Compute expected output: bitmap clusters + bitmap tables + bitmaps directory
> +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"
> +$QEMU_IMG measure --bitmaps -O qcow2 -o cluster_size=64k -f qcow2 "$TEST_IMG"
> +
> +# Compute expected output: bitmap clusters + bitmap tables + bitmaps directory
> +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 --output=json -O qcow2 -o cluster_size=2M -f qcow2 "$TEST_IMG"
> +$QEMU_IMG measure --output=json --bitmaps -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..5c35f9268068 100644
> --- a/tests/qemu-iotests/190.out
> +++ b/tests/qemu-iotests/190.out
> @@ -1,11 +1,44 @@
> 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 ==
> +
> +qemu-img: --bitmaps is only supported with a filename.
> +required size: 2199023255552
> +fully allocated size: 2199023255552
> +qemu-img: no bitmaps measured, either source or destination format lacks bitmap support
> +required size: 7012352
> +fully allocated size: 17170432
> +qemu-img: no bitmaps measured, either source or destination format lacks bitmap support
> +required size: 335806464
> +fully allocated size: 2199359062016
> +qemu-img: no bitmaps measured, either source or destination format lacks bitmap support
> +
> +expected bitmap 537198592
> +required size: 335806464
> +fully allocated size: 2199359062016
> +bitmaps size: 537198592
> +required size: 873005056
> +fully allocated size: 2199896260608
> +
> +expected bitmap 545259520
> +{
> + "bitmaps": 545259520,
> + "required": 18874368,
> + "fully-allocated": 2199042129920
> +}
> +{
> + "required": 564133888,
> + "fully-allocated": 2199587389440
> +}
> *** done
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 1/5] iotests: Fix test 178
2020-05-20 22:01 ` [PATCH v5 1/5] iotests: Fix test 178 Eric Blake
@ 2020-05-21 11:31 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-21 11:31 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: kwolf, nsoffer, qemu-block, mreitz
21.05.2020 01:01, Eric Blake wrote:
> A recent change to qemu-img changed expected error message output, but
> 178 takes long enough to execute that it does not get run by 'make
> check' or './check -g quick'.
>
> Fixes: 43d589b074
> Signed-off-by: Eric Blake<eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 2/5] qcow2: Expose bitmaps' size during measure
2020-05-20 23:00 ` Nir Soffer
@ 2020-05-21 11:40 ` Vladimir Sementsov-Ogievskiy
2020-05-21 13:29 ` Nir Soffer
2020-05-21 13:08 ` Eric Blake
1 sibling, 1 reply; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-21 11:40 UTC (permalink / raw)
To: Nir Soffer, Eric Blake
Cc: Kevin Wolf, qemu-block, Markus Armbruster, QEMU Developers,
Max Reitz, John Snow
21.05.2020 02:00, Nir Soffer wrote:
> On Thu, May 21, 2020 at 1:01 AM 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 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.
>>
>> On the command-line side, 'qemu-img measure' gains a new --bitmaps
>> flag. When present, the bitmap size is rolled into the two existing
>> measures (or errors if either the source image or destination format
>> lacks bitmaps); when absent, there is never an error (for
>> back-compat), but the output will instead include a new line item for
>> bitmaps (which you would have to manually add), with that line being
>> omitted in the same cases where passing --bitmaps would error.
>
> Supporting 2 ways to measure, one by specifying --bitmaps, and another
> by adding bitmaps key is not a good idea. We really need one way.
>
> Each one has advantages. adding --bitmaps flag is consistent with
> "qemu-img convert"
> and future extensions that may require new flag, and adding "bitmaps"
> key is consistent
> with "qmeu-img info", showing bitmaps when they exist.
>
> Adding a "bitmaps" key has an advantage that we can use it to test if qemu-img
> supports measuring and copying bitmaps (since both features are expected to
> be delivered at the same time). So we can avoid checking --help learn about
> the capabilities.
>
> I'm ok with both options, can we have only one?
Hi! What are your scenarios? Are you using qemu-img by hand, or it is used from some management tool? For management tool, I'd recommend to use qmp interface, which is a lot more strict, reliable and stable, and documented. You just need to run qemu binary in stopped mode for it.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 2/5] qcow2: Expose bitmaps' size during measure
2020-05-20 23:00 ` Nir Soffer
2020-05-21 11:40 ` Vladimir Sementsov-Ogievskiy
@ 2020-05-21 13:08 ` Eric Blake
2020-05-21 13:17 ` Nir Soffer
1 sibling, 1 reply; 17+ messages in thread
From: Eric Blake @ 2020-05-21 13:08 UTC (permalink / raw)
To: Nir Soffer
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block,
Markus Armbruster, QEMU Developers, Max Reitz, John Snow
On 5/20/20 6:00 PM, Nir Soffer wrote:
>>
>> On the command-line side, 'qemu-img measure' gains a new --bitmaps
>> flag. When present, the bitmap size is rolled into the two existing
>> measures (or errors if either the source image or destination format
>> lacks bitmaps); when absent, there is never an error (for
>> back-compat), but the output will instead include a new line item for
>> bitmaps (which you would have to manually add), with that line being
>> omitted in the same cases where passing --bitmaps would error.
>
> Supporting 2 ways to measure, one by specifying --bitmaps, and another
> by adding bitmaps key is not a good idea. We really need one way.
>
> Each one has advantages. adding --bitmaps flag is consistent with
> "qemu-img convert"
> and future extensions that may require new flag, and adding "bitmaps"
> key is consistent
> with "qmeu-img info", showing bitmaps when they exist.
>
> Adding a "bitmaps" key has an advantage that we can use it to test if qemu-img
> supports measuring and copying bitmaps (since both features are expected to
> be delivered at the same time). So we can avoid checking --help learn about
> the capabilities.
>
> I'm ok with both options, can we have only one?
That was the crux of the conversation after v3, where we were trying to
figure out what interface you actually needed. I implemented both to
show the difference, but if you want only one, then my preference is to
delete the --bitmaps option and only expose the optional 'bitmaps size'
field in all cases where bitmaps can be copied.
Here's the diff that would accomplish that:
diff --git i/docs/tools/qemu-img.rst w/docs/tools/qemu-img.rst
index 35050fc51070..69cd9a30373a 100644
--- i/docs/tools/qemu-img.rst
+++ w/docs/tools/qemu-img.rst
@@ -597,7 +597,7 @@ Command description:
For more information, consult ``include/block/block.h`` in QEMU's
source code.
-.. option:: measure [--output=OFMT] [-O OUTPUT_FMT] [-o OPTIONS]
[--size N | [--object OBJECTDEF] [--image-opts] [-f FMT] [--bitmaps] [-l
SNAPSHOT_PARAM] FILENAME]
+.. option:: measure [--output=OFMT] [-O OUTPUT_FMT] [-o OPTIONS]
[--size N | [--object OBJECTDEF] [--image-opts] [-f FMT] [-l
SNAPSHOT_PARAM] FILENAME]
Calculate the file size required for a new image. This information
can be used to size logical volumes or SAN LUNs appropriately for
@@ -634,10 +634,7 @@ Command description:
copy bitmaps from a source image in addition to the guest-visible
data; the line is omitted if either source or destination lacks
bitmap support, or 0 if bitmaps are supported but there is nothing
- to copy. If the ``--bitmaps`` option is in use, the bitmap size is
- instead folded into the required and fully-allocated size for
- convenience, rather than being a separate line item; using the
- option will raise an error if bitmaps are not supported.
+ to copy.
.. option:: snapshot [--object OBJECTDEF] [--image-opts] [-U] [-q] [-l
| -a SNAPSHOT | -c SNAPSHOT | -d SNAPSHOT] FILENAME
diff --git i/qemu-img.c w/qemu-img.c
index 1494d8f5c409..8dc4cae2c274 100644
--- i/qemu-img.c
+++ w/qemu-img.c
@@ -5207,7 +5207,6 @@ static int img_measure(int argc, char **argv)
{"output", required_argument, 0, OPTION_OUTPUT},
{"size", required_argument, 0, OPTION_SIZE},
{"force-share", no_argument, 0, 'U'},
- {"bitmaps", no_argument, 0, OPTION_BITMAPS},
{0, 0, 0, 0}
};
OutputFormat output_format = OFORMAT_HUMAN;
@@ -5224,7 +5223,6 @@ static int img_measure(int argc, char **argv)
QemuOpts *sn_opts = NULL;
QemuOptsList *create_opts = NULL;
bool image_opts = false;
- bool bitmaps = false;
uint64_t img_size = UINT64_MAX;
BlockMeasureInfo *info = NULL;
Error *local_err = NULL;
@@ -5297,10 +5295,6 @@ static int img_measure(int argc, char **argv)
img_size = (uint64_t)sval;
}
break;
- case OPTION_BITMAPS:
- bitmaps = true;
- break;
- }
}
if (qemu_opts_foreach(&qemu_object_opts,
@@ -5328,10 +5322,6 @@ static int img_measure(int argc, char **argv)
error_report("Either --size N or one filename must be
specified.");
goto out;
}
- if (!filename && bitmaps) {
- error_report("--bitmaps is only supported with a filename.");
- goto out;
- }
if (filename) {
in_blk = img_open(image_opts, filename, fmt, 0,
@@ -5387,18 +5377,6 @@ 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;
- }
- }
-
if (output_format == OFORMAT_HUMAN) {
printf("required size: %" PRIu64 "\n", info->required);
printf("fully allocated size: %" PRIu64 "\n",
info->fully_allocated);
diff --git i/qemu-img-cmds.hx w/qemu-img-cmds.hx
index e9beb15e614e..10b910b67cf8 100644
--- i/qemu-img-cmds.hx
+++ w/qemu-img-cmds.hx
@@ -76,9 +76,9 @@ SRST
ERST
DEF("measure", img_measure,
-"measure [--output=ofmt] [-O output_fmt] [-o options] [--size N |
[--object objectdef] [--image-opts] [-f fmt] [--bitmaps] [-l
snapshot_param] filename]")
+"measure [--output=ofmt] [-O output_fmt] [-o options] [--size N |
[--object objectdef] [--image-opts] [-f fmt] [-l snapshot_param] filename]")
SRST
-.. option:: measure [--output=OFMT] [-O OUTPUT_FMT] [-o OPTIONS]
[--size N | [--object OBJECTDEF] [--image-opts] [-f FMT] [--bitmaps] [-l
SNAPSHOT_PARAM] FILENAME]
+.. option:: measure [--output=OFMT] [-O OUTPUT_FMT] [-o OPTIONS]
[--size N | [--object OBJECTDEF] [--image-opts] [-f FMT] [-l
SNAPSHOT_PARAM] FILENAME]
ERST
DEF("snapshot", img_snapshot,
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v5 2/5] qcow2: Expose bitmaps' size during measure
2020-05-21 13:08 ` Eric Blake
@ 2020-05-21 13:17 ` Nir Soffer
2020-05-21 13:29 ` Eric Blake
0 siblings, 1 reply; 17+ messages in thread
From: Nir Soffer @ 2020-05-21 13:17 UTC (permalink / raw)
To: Eric Blake
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block,
Markus Armbruster, QEMU Developers, Max Reitz, John Snow
On Thu, May 21, 2020 at 4:08 PM Eric Blake <eblake@redhat.com> wrote:
>
> On 5/20/20 6:00 PM, Nir Soffer wrote:
>
> >>
> >> On the command-line side, 'qemu-img measure' gains a new --bitmaps
> >> flag. When present, the bitmap size is rolled into the two existing
> >> measures (or errors if either the source image or destination format
> >> lacks bitmaps); when absent, there is never an error (for
> >> back-compat), but the output will instead include a new line item for
> >> bitmaps (which you would have to manually add), with that line being
> >> omitted in the same cases where passing --bitmaps would error.
> >
> > Supporting 2 ways to measure, one by specifying --bitmaps, and another
> > by adding bitmaps key is not a good idea. We really need one way.
> >
> > Each one has advantages. adding --bitmaps flag is consistent with
> > "qemu-img convert"
> > and future extensions that may require new flag, and adding "bitmaps"
> > key is consistent
> > with "qmeu-img info", showing bitmaps when they exist.
> >
> > Adding a "bitmaps" key has an advantage that we can use it to test if qemu-img
> > supports measuring and copying bitmaps (since both features are expected to
> > be delivered at the same time). So we can avoid checking --help learn about
> > the capabilities.
> >
> > I'm ok with both options, can we have only one?
>
> That was the crux of the conversation after v3, where we were trying to
> figure out what interface you actually needed. I implemented both to
> show the difference, but if you want only one, then my preference is to
> delete the --bitmaps option and only expose the optional 'bitmaps size'
> field in all cases where bitmaps can be copied.
I'm fine with this approach - but the bitmaps optional field should be displayed
even if there are no bitmaps in the source, so I can tell if tihs
version of qemu-img
supports measuring/copying bitmaps.
If measure reports bitmaps size we will create a large enough disk and
copy the bitmaps,
and if not we will have to drop the relevant backup history for this
vm, since the copy
will not include the bitmaps. The next backup for this vm will have to
be a full backup.
> Here's the diff that would accomplish that:
Diff does not eliminate the "bitmaps: 0" outputs, so it looks good.
>
> diff --git i/docs/tools/qemu-img.rst w/docs/tools/qemu-img.rst
> index 35050fc51070..69cd9a30373a 100644
> --- i/docs/tools/qemu-img.rst
> +++ w/docs/tools/qemu-img.rst
> @@ -597,7 +597,7 @@ Command description:
> For more information, consult ``include/block/block.h`` in QEMU's
> source code.
>
> -.. option:: measure [--output=OFMT] [-O OUTPUT_FMT] [-o OPTIONS]
> [--size N | [--object OBJECTDEF] [--image-opts] [-f FMT] [--bitmaps] [-l
> SNAPSHOT_PARAM] FILENAME]
> +.. option:: measure [--output=OFMT] [-O OUTPUT_FMT] [-o OPTIONS]
> [--size N | [--object OBJECTDEF] [--image-opts] [-f FMT] [-l
> SNAPSHOT_PARAM] FILENAME]
>
> Calculate the file size required for a new image. This information
> can be used to size logical volumes or SAN LUNs appropriately for
> @@ -634,10 +634,7 @@ Command description:
> copy bitmaps from a source image in addition to the guest-visible
> data; the line is omitted if either source or destination lacks
> bitmap support, or 0 if bitmaps are supported but there is nothing
> - to copy. If the ``--bitmaps`` option is in use, the bitmap size is
> - instead folded into the required and fully-allocated size for
> - convenience, rather than being a separate line item; using the
> - option will raise an error if bitmaps are not supported.
> + to copy.
>
> .. option:: snapshot [--object OBJECTDEF] [--image-opts] [-U] [-q] [-l
> | -a SNAPSHOT | -c SNAPSHOT | -d SNAPSHOT] FILENAME
>
> diff --git i/qemu-img.c w/qemu-img.c
> index 1494d8f5c409..8dc4cae2c274 100644
> --- i/qemu-img.c
> +++ w/qemu-img.c
> @@ -5207,7 +5207,6 @@ static int img_measure(int argc, char **argv)
> {"output", required_argument, 0, OPTION_OUTPUT},
> {"size", required_argument, 0, OPTION_SIZE},
> {"force-share", no_argument, 0, 'U'},
> - {"bitmaps", no_argument, 0, OPTION_BITMAPS},
> {0, 0, 0, 0}
> };
> OutputFormat output_format = OFORMAT_HUMAN;
> @@ -5224,7 +5223,6 @@ static int img_measure(int argc, char **argv)
> QemuOpts *sn_opts = NULL;
> QemuOptsList *create_opts = NULL;
> bool image_opts = false;
> - bool bitmaps = false;
> uint64_t img_size = UINT64_MAX;
> BlockMeasureInfo *info = NULL;
> Error *local_err = NULL;
> @@ -5297,10 +5295,6 @@ static int img_measure(int argc, char **argv)
> img_size = (uint64_t)sval;
> }
> break;
> - case OPTION_BITMAPS:
> - bitmaps = true;
> - break;
> - }
> }
>
> if (qemu_opts_foreach(&qemu_object_opts,
> @@ -5328,10 +5322,6 @@ static int img_measure(int argc, char **argv)
> error_report("Either --size N or one filename must be
> specified.");
> goto out;
> }
> - if (!filename && bitmaps) {
> - error_report("--bitmaps is only supported with a filename.");
> - goto out;
> - }
>
> if (filename) {
> in_blk = img_open(image_opts, filename, fmt, 0,
> @@ -5387,18 +5377,6 @@ 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;
> - }
> - }
> -
> if (output_format == OFORMAT_HUMAN) {
> printf("required size: %" PRIu64 "\n", info->required);
> printf("fully allocated size: %" PRIu64 "\n",
> info->fully_allocated);
> diff --git i/qemu-img-cmds.hx w/qemu-img-cmds.hx
> index e9beb15e614e..10b910b67cf8 100644
> --- i/qemu-img-cmds.hx
> +++ w/qemu-img-cmds.hx
> @@ -76,9 +76,9 @@ SRST
> ERST
>
> DEF("measure", img_measure,
> -"measure [--output=ofmt] [-O output_fmt] [-o options] [--size N |
> [--object objectdef] [--image-opts] [-f fmt] [--bitmaps] [-l
> snapshot_param] filename]")
> +"measure [--output=ofmt] [-O output_fmt] [-o options] [--size N |
> [--object objectdef] [--image-opts] [-f fmt] [-l snapshot_param] filename]")
> SRST
> -.. option:: measure [--output=OFMT] [-O OUTPUT_FMT] [-o OPTIONS]
> [--size N | [--object OBJECTDEF] [--image-opts] [-f FMT] [--bitmaps] [-l
> SNAPSHOT_PARAM] FILENAME]
> +.. option:: measure [--output=OFMT] [-O OUTPUT_FMT] [-o OPTIONS]
> [--size N | [--object OBJECTDEF] [--image-opts] [-f FMT] [-l
> SNAPSHOT_PARAM] FILENAME]
> ERST
>
> DEF("snapshot", img_snapshot,
>
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc. +1-919-301-3226
> Virtualization: qemu.org | libvirt.org
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 2/5] qcow2: Expose bitmaps' size during measure
2020-05-21 13:17 ` Nir Soffer
@ 2020-05-21 13:29 ` Eric Blake
0 siblings, 0 replies; 17+ messages in thread
From: Eric Blake @ 2020-05-21 13:29 UTC (permalink / raw)
To: Nir Soffer
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block,
Markus Armbruster, QEMU Developers, Max Reitz, John Snow
On 5/21/20 8:17 AM, Nir Soffer wrote:
>>> I'm ok with both options, can we have only one?
>>
>> That was the crux of the conversation after v3, where we were trying to
>> figure out what interface you actually needed. I implemented both to
>> show the difference, but if you want only one, then my preference is to
>> delete the --bitmaps option and only expose the optional 'bitmaps size'
>> field in all cases where bitmaps can be copied.
>
> I'm fine with this approach - but the bitmaps optional field should be displayed
> even if there are no bitmaps in the source, so I can tell if tihs
> version of qemu-img
> supports measuring/copying bitmaps.
The optional field appears when both source and destination have
bitmaps. That is, the field is present in all cases in which 'qemu-img
convert --bitmaps' will succeed, even when the field is 0 because there
is nothing to copy.
> If measure reports bitmaps size we will create a large enough disk and
> copy the bitmaps,
> and if not we will have to drop the relevant backup history for this
> vm, since the copy
> will not include the bitmaps. The next backup for this vm will have to
> be a full backup.
If you are always using qcow2 v3 images for both source and destination,
then the field will be present, even when it is 0. It will only be
absent when there is nothing to copy (either the source or destination
format is not qcow2, or you used measure on a size rather than on a
source image).
>
>> Here's the diff that would accomplish that:
>
> Diff does not eliminate the "bitmaps: 0" outputs, so it looks good.
Okay, I'll revise the patch to get rid of the extra option (that is,
getting it back to what it was in v3, before we had the discussion that
prompted the addition of --bitmaps for comparison). (I also have to
touch up the iotests here and in 5/5)
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 2/5] qcow2: Expose bitmaps' size during measure
2020-05-21 11:40 ` Vladimir Sementsov-Ogievskiy
@ 2020-05-21 13:29 ` Nir Soffer
2020-05-21 15:09 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 17+ messages in thread
From: Nir Soffer @ 2020-05-21 13:29 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: Kevin Wolf, qemu-block, QEMU Developers, Max Reitz, John Snow,
Markus Armbruster
On Thu, May 21, 2020 at 2:40 PM Vladimir Sementsov-Ogievskiy
<vsementsov@virtuozzo.com> wrote:
>
> 21.05.2020 02:00, Nir Soffer wrote:
> > On Thu, May 21, 2020 at 1:01 AM 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 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.
> >>
> >> On the command-line side, 'qemu-img measure' gains a new --bitmaps
> >> flag. When present, the bitmap size is rolled into the two existing
> >> measures (or errors if either the source image or destination format
> >> lacks bitmaps); when absent, there is never an error (for
> >> back-compat), but the output will instead include a new line item for
> >> bitmaps (which you would have to manually add), with that line being
> >> omitted in the same cases where passing --bitmaps would error.
> >
> > Supporting 2 ways to measure, one by specifying --bitmaps, and another
> > by adding bitmaps key is not a good idea. We really need one way.
> >
> > Each one has advantages. adding --bitmaps flag is consistent with
> > "qemu-img convert"
> > and future extensions that may require new flag, and adding "bitmaps"
> > key is consistent
> > with "qmeu-img info", showing bitmaps when they exist.
> >
> > Adding a "bitmaps" key has an advantage that we can use it to test if qemu-img
> > supports measuring and copying bitmaps (since both features are expected to
> > be delivered at the same time). So we can avoid checking --help learn about
> > the capabilities.
> >
> > I'm ok with both options, can we have only one?
>
> Hi! What are your scenarios? Are you using qemu-img by hand, or it is used from some management tool? For management tool, I'd recommend to use qmp interface, which is a lot more strict, reliable and stable, and documented. You just need to run qemu binary in stopped mode for it.
The use case is oVirt - it is a management system but it uses qemu-img
to perform various
operations, like copying disks around. The specific use case is
cloning qcow2 image chain
from one server to another, or cloning on the same server.
In the case of qcow2 images on logical volumes, we need to create a
large enough logical
volume to copy an image, and for this we use "qemu-img measure". With
the current patches
we will be able to create large enough logical volume and copy the
image data and the bitmps
to the destination.
qmp interface is nice but to use it we have to rewrite all the code
using qemu-img to start qemu
with the relevant disks and perform operation via qmp or via libvirt.
This a huge rewrite and I'm
not sure it's worth the effort.
This approach also has limitations, for example using qemu-img we can
copy disks in parallel on
multiple hosts, while using libvirt and qemu will limit us to using
one host since qemu takes locks
that we control.
Also using libvirt means that all new features take more time to
complete since now we have
a new layer between oVirt and qemu.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 2/5] qcow2: Expose bitmaps' size during measure
2020-05-21 13:29 ` Nir Soffer
@ 2020-05-21 15:09 ` Vladimir Sementsov-Ogievskiy
2020-05-21 16:28 ` Nir Soffer
0 siblings, 1 reply; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-21 15:09 UTC (permalink / raw)
To: Nir Soffer
Cc: Kevin Wolf, qemu-block, QEMU Developers, Max Reitz, John Snow,
Markus Armbruster
21.05.2020 16:29, Nir Soffer wrote:
> On Thu, May 21, 2020 at 2:40 PM Vladimir Sementsov-Ogievskiy
> <vsementsov@virtuozzo.com> wrote:
>>
>> 21.05.2020 02:00, Nir Soffer wrote:
>>> On Thu, May 21, 2020 at 1:01 AM 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 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.
>>>>
>>>> On the command-line side, 'qemu-img measure' gains a new --bitmaps
>>>> flag. When present, the bitmap size is rolled into the two existing
>>>> measures (or errors if either the source image or destination format
>>>> lacks bitmaps); when absent, there is never an error (for
>>>> back-compat), but the output will instead include a new line item for
>>>> bitmaps (which you would have to manually add), with that line being
>>>> omitted in the same cases where passing --bitmaps would error.
>>>
>>> Supporting 2 ways to measure, one by specifying --bitmaps, and another
>>> by adding bitmaps key is not a good idea. We really need one way.
>>>
>>> Each one has advantages. adding --bitmaps flag is consistent with
>>> "qemu-img convert"
>>> and future extensions that may require new flag, and adding "bitmaps"
>>> key is consistent
>>> with "qmeu-img info", showing bitmaps when they exist.
>>>
>>> Adding a "bitmaps" key has an advantage that we can use it to test if qemu-img
>>> supports measuring and copying bitmaps (since both features are expected to
>>> be delivered at the same time). So we can avoid checking --help learn about
>>> the capabilities.
>>>
>>> I'm ok with both options, can we have only one?
>>
>> Hi! What are your scenarios? Are you using qemu-img by hand, or it is used from some management tool? For management tool, I'd recommend to use qmp interface, which is a lot more strict, reliable and stable, and documented. You just need to run qemu binary in stopped mode for it.
>
> The use case is oVirt - it is a management system but it uses qemu-img
> to perform various
> operations, like copying disks around. The specific use case is
> cloning qcow2 image chain
> from one server to another, or cloning on the same server.
>
> In the case of qcow2 images on logical volumes, we need to create a
> large enough logical
> volume to copy an image, and for this we use "qemu-img measure". With
> the current patches
> we will be able to create large enough logical volume and copy the
> image data and the bitmps
> to the destination.
>
> qmp interface is nice but to use it we have to rewrite all the code
> using qemu-img to start qemu
> with the relevant disks and perform operation via qmp or via libvirt.
> This a huge rewrite and I'm
> not sure it's worth the effort.
OK, I'm not familiar with oVirt.. But maybe, you don't need to rewrite everything, but just add a possibility to use qmp interface, so, keep old features working on qemu-img, while adding new features using qmp?
>
> This approach also has limitations, for example using qemu-img we can
> copy disks in parallel on
> multiple hosts, while using libvirt and qemu will limit us to using
> one host since qemu takes locks
> that we control.
hmm.. do you open same image RW in several qemu-img processes? Or I don't follow, what behavior is blocked by Qemu locks... qemu-img takes locks as well..
>
> Also using libvirt means that all new features take more time to
> complete since now we have
> a new layer between oVirt and qemu.
May be, using qmp directly is still a good option.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 4/5] qemu-img: Add convert --bitmaps option
2020-05-20 22:01 ` [PATCH v5 4/5] qemu-img: Add convert --bitmaps option Eric Blake
@ 2020-05-21 15:11 ` Nir Soffer
2020-05-21 15:19 ` Eric Blake
0 siblings, 1 reply; 17+ messages in thread
From: Nir Soffer @ 2020-05-21 15:11 UTC (permalink / raw)
To: Eric Blake
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, QEMU Developers,
qemu-block, Max Reitz
On Thu, May 21, 2020 at 1:01 AM Eric Blake <eblake@redhat.com> wrote:
>
> Make it easier to copy all the persistent bitmaps of (the top layer
> of) a source image along with its guest-visible contents, by adding a
> boolean flag for use with qemu-img convert. This is basically
> shorthand, as the same effect could be accomplished with a series of
> 'qemu-img bitmap --add' and 'qemu-img bitmap --merge -b source'
> commands, or by QMP commands.
>
> Note that this command will fail in the same scenarios where 'qemu-img
> measure --bitmaps' fails, when either the source or the destanation
> lacks persistent bitmap support altogether.
If we remove --bitmaps option from qemu-img measure, we need to remove
this note.
>
> See also https://bugzilla.redhat.com/show_bug.cgi?id=1779893
>
> While touching this, clean up a couple coding issues spotted in the
> same function: an extra blank line, and merging back-to-back 'if
> (!skip_create)' blocks.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Message-Id: <20200513011648.166876-9-eblake@redhat.com>
> ---
> docs/tools/qemu-img.rst | 6 +++-
> qemu-img.c | 77 +++++++++++++++++++++++++++++++++++++++--
> qemu-img-cmds.hx | 4 +--
> 3 files changed, 81 insertions(+), 6 deletions(-)
>
> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
> index 9a8112fc9f58..35050fc51070 100644
> --- a/docs/tools/qemu-img.rst
> +++ b/docs/tools/qemu-img.rst
> @@ -162,6 +162,10 @@ Parameters to convert subcommand:
>
> .. program:: qemu-img-convert
>
> +.. option:: --bitmaps
> +
> + Additionally copy all persistent bitmaps from the top layer of the source
> +
> .. option:: -n
>
> Skip the creation of the target volume
> @@ -397,7 +401,7 @@ Command description:
> 4
> Error on reading data
>
> -.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [--target-is-zero] [-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-m NUM_COROUTINES] [-W] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME
> +.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [--target-is-zero] [--bitmaps] [-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-m NUM_COROUTINES] [-W] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME
>
> Convert the disk image *FILENAME* or a snapshot *SNAPSHOT_PARAM*
> to disk image *OUTPUT_FILENAME* using format *OUTPUT_FMT*. It can
> diff --git a/qemu-img.c b/qemu-img.c
> index c1bafb57023a..1494d8f5c409 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -192,6 +192,7 @@ static void QEMU_NORETURN help(void)
> " hiding corruption that has already occurred.\n"
> "\n"
> "Parameters to convert subcommand:\n"
> + " '--bitmaps' copies all top-level persistent bitmaps to destination\n"
> " '-m' specifies how many coroutines work in parallel during the convert\n"
> " process (defaults to 8)\n"
> " '-W' allow to write to the target out of order rather than sequential\n"
> @@ -2139,6 +2140,39 @@ static int convert_do_copy(ImgConvertState *s)
> return s->ret;
> }
>
> +static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst)
> +{
> + BdrvDirtyBitmap *bm;
> + Error *err = NULL;
> +
> + FOR_EACH_DIRTY_BITMAP(src, bm) {
> + const char *name;
> +
> + if (!bdrv_dirty_bitmap_get_persistence(bm)) {
> + continue;
> + }
> + name = bdrv_dirty_bitmap_name(bm);
> + qmp_block_dirty_bitmap_add(dst->node_name, name,
> + true, bdrv_dirty_bitmap_granularity(bm),
> + true, true,
> + true, !bdrv_dirty_bitmap_enabled(bm),
> + &err);
> + if (err) {
> + error_reportf_err(err, "Failed to create bitmap %s: ", name);
> + return -1;
> + }
> +
> + do_dirty_bitmap_merge(dst->node_name, name, src->node_name, name,
> + &err);
> + if (err) {
> + error_reportf_err(err, "Failed to populate bitmap %s: ", name);
> + return -1;
> + }
> + }
> +
> + return 0;
> +}
> +
> #define MAX_BUF_SECTORS 32768
>
> static int img_convert(int argc, char **argv)
> @@ -2160,6 +2194,8 @@ static int img_convert(int argc, char **argv)
> int64_t ret = -EINVAL;
> bool force_share = false;
> bool explict_min_sparse = false;
> + bool bitmaps = false;
> + size_t nbitmaps = 0;
>
> ImgConvertState s = (ImgConvertState) {
> /* Need at least 4k of zeros for sparse detection */
> @@ -2179,6 +2215,7 @@ static int img_convert(int argc, char **argv)
> {"target-image-opts", no_argument, 0, OPTION_TARGET_IMAGE_OPTS},
> {"salvage", no_argument, 0, OPTION_SALVAGE},
> {"target-is-zero", no_argument, 0, OPTION_TARGET_IS_ZERO},
> + {"bitmaps", no_argument, 0, OPTION_BITMAPS},
> {0, 0, 0, 0}
> };
> c = getopt_long(argc, argv, ":hf:O:B:Cco:l:S:pt:T:qnm:WU",
> @@ -2304,6 +2341,9 @@ static int img_convert(int argc, char **argv)
> */
> s.has_zero_init = true;
> break;
> + case OPTION_BITMAPS:
> + bitmaps = true;
> + break;
> }
> }
>
> @@ -2365,7 +2405,6 @@ static int img_convert(int argc, char **argv)
> goto fail_getopt;
> }
>
> -
> /* ret is still -EINVAL until here */
> ret = bdrv_parse_cache_mode(src_cache, &src_flags, &src_writethrough);
> if (ret < 0) {
> @@ -2525,6 +2564,27 @@ static int img_convert(int argc, char **argv)
> }
> }
>
> + /* Determine how many bitmaps need copying */
> + if (bitmaps) {
> + BdrvDirtyBitmap *bm;
> +
> + if (s.src_num > 1) {
> + error_report("Copying bitmaps only possible with single source");
> + ret = -1;
> + goto out;
> + }
> + if (!bdrv_supports_persistent_dirty_bitmap(blk_bs(s.src[0]))) {
> + error_report("Source lacks bitmap support");
> + ret = -1;
> + goto out;
> + }
> + FOR_EACH_DIRTY_BITMAP(blk_bs(s.src[0]), bm) {
> + if (bdrv_dirty_bitmap_get_persistence(bm)) {
> + nbitmaps++;
> + }
> + }
> + }
> +
> /*
> * The later open call will need any decryption secrets, and
> * bdrv_create() will purge "opts", so extract them now before
> @@ -2533,9 +2593,7 @@ static int img_convert(int argc, char **argv)
> if (!skip_create) {
> open_opts = qdict_new();
> qemu_opt_foreach(opts, img_add_key_secrets, open_opts, &error_abort);
> - }
>
> - if (!skip_create) {
> /* Create the new image */
> ret = bdrv_create(drv, out_filename, opts, &local_err);
> if (ret < 0) {
> @@ -2573,6 +2631,13 @@ static int img_convert(int argc, char **argv)
> }
> out_bs = blk_bs(s.target);
>
> + if (nbitmaps > 0 && !bdrv_supports_persistent_dirty_bitmap(out_bs)) {
> + error_report("Format driver '%s' does not support bitmaps",
> + out_fmt);
> + ret = -1;
> + goto out;
> + }
> +
> if (s.compressed && !block_driver_can_compress(out_bs->drv)) {
> error_report("Compression not supported for this file format");
> ret = -1;
> @@ -2632,6 +2697,12 @@ static int img_convert(int argc, char **argv)
> }
>
> ret = convert_do_copy(&s);
> +
> + /* Now copy the bitmaps */
> + if (nbitmaps > 0 && ret == 0) {
> + ret = convert_copy_bitmaps(blk_bs(s.src[0]), out_bs);
> + }
> +
> out:
> if (!ret) {
> qemu_progress_print(100, 0);
> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> index 235cc5fffadc..e9beb15e614e 100644
> --- a/qemu-img-cmds.hx
> +++ b/qemu-img-cmds.hx
> @@ -46,9 +46,9 @@ SRST
> ERST
>
> DEF("convert", img_convert,
> - "convert [--object objectdef] [--image-opts] [--target-image-opts] [--target-is-zero] [-U] [-C] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-B backing_file] [-o options] [-l snapshot_param] [-S sparse_size] [-m num_coroutines] [-W] [--salvage] filename [filename2 [...]] output_filename")
> + "convert [--object objectdef] [--image-opts] [--target-image-opts] [--target-is-zero] [--bitmaps] [-U] [-C] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-B backing_file] [-o options] [-l snapshot_param] [-S sparse_size] [-m num_coroutines] [-W] [--salvage] filename [filename2 [...]] output_filename")
> SRST
> -.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [--target-is-zero] [-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-m NUM_COROUTINES] [-W] [--salvage] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME
> +.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [--target-is-zero] [--bitmaps] [-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-m NUM_COROUTINES] [-W] [--salvage] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME
> ERST
>
> DEF("create", img_create,
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 4/5] qemu-img: Add convert --bitmaps option
2020-05-21 15:11 ` Nir Soffer
@ 2020-05-21 15:19 ` Eric Blake
0 siblings, 0 replies; 17+ messages in thread
From: Eric Blake @ 2020-05-21 15:19 UTC (permalink / raw)
To: Nir Soffer
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, QEMU Developers,
qemu-block, Max Reitz
On 5/21/20 10:11 AM, Nir Soffer wrote:
> On Thu, May 21, 2020 at 1:01 AM Eric Blake <eblake@redhat.com> wrote:
>>
>> Make it easier to copy all the persistent bitmaps of (the top layer
>> of) a source image along with its guest-visible contents, by adding a
>> boolean flag for use with qemu-img convert. This is basically
>> shorthand, as the same effect could be accomplished with a series of
>> 'qemu-img bitmap --add' and 'qemu-img bitmap --merge -b source'
>> commands, or by QMP commands.
>>
>> Note that this command will fail in the same scenarios where 'qemu-img
>> measure --bitmaps' fails, when either the source or the destanation
destination
>> lacks persistent bitmap support altogether.
>
> If we remove --bitmaps option from qemu-img measure, we need to remove
> this note.
Or rather, adjust it to state that "this command will fail in the
scenarios where 'qemu-img measure' omits a 'bitmaps size:' output line.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 2/5] qcow2: Expose bitmaps' size during measure
2020-05-21 15:09 ` Vladimir Sementsov-Ogievskiy
@ 2020-05-21 16:28 ` Nir Soffer
0 siblings, 0 replies; 17+ messages in thread
From: Nir Soffer @ 2020-05-21 16:28 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: Kevin Wolf, qemu-block, QEMU Developers, Max Reitz, John Snow,
Markus Armbruster
On Thu, May 21, 2020 at 6:09 PM Vladimir Sementsov-Ogievskiy
<vsementsov@virtuozzo.com> wrote:
>
> 21.05.2020 16:29, Nir Soffer wrote:
> > On Thu, May 21, 2020 at 2:40 PM Vladimir Sementsov-Ogievskiy
> > <vsementsov@virtuozzo.com> wrote:
> >>
> >> 21.05.2020 02:00, Nir Soffer wrote:
> >>> On Thu, May 21, 2020 at 1:01 AM 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 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.
> >>>>
> >>>> On the command-line side, 'qemu-img measure' gains a new --bitmaps
> >>>> flag. When present, the bitmap size is rolled into the two existing
> >>>> measures (or errors if either the source image or destination format
> >>>> lacks bitmaps); when absent, there is never an error (for
> >>>> back-compat), but the output will instead include a new line item for
> >>>> bitmaps (which you would have to manually add), with that line being
> >>>> omitted in the same cases where passing --bitmaps would error.
> >>>
> >>> Supporting 2 ways to measure, one by specifying --bitmaps, and another
> >>> by adding bitmaps key is not a good idea. We really need one way.
> >>>
> >>> Each one has advantages. adding --bitmaps flag is consistent with
> >>> "qemu-img convert"
> >>> and future extensions that may require new flag, and adding "bitmaps"
> >>> key is consistent
> >>> with "qmeu-img info", showing bitmaps when they exist.
> >>>
> >>> Adding a "bitmaps" key has an advantage that we can use it to test if qemu-img
> >>> supports measuring and copying bitmaps (since both features are expected to
> >>> be delivered at the same time). So we can avoid checking --help learn about
> >>> the capabilities.
> >>>
> >>> I'm ok with both options, can we have only one?
> >>
> >> Hi! What are your scenarios? Are you using qemu-img by hand, or it is used from some management tool? For management tool, I'd recommend to use qmp interface, which is a lot more strict, reliable and stable, and documented. You just need to run qemu binary in stopped mode for it.
> >
> > The use case is oVirt - it is a management system but it uses qemu-img
> > to perform various
> > operations, like copying disks around. The specific use case is
> > cloning qcow2 image chain
> > from one server to another, or cloning on the same server.
> >
> > In the case of qcow2 images on logical volumes, we need to create a
> > large enough logical
> > volume to copy an image, and for this we use "qemu-img measure". With
> > the current patches
> > we will be able to create large enough logical volume and copy the
> > image data and the bitmps
> > to the destination.
> >
> > qmp interface is nice but to use it we have to rewrite all the code
> > using qemu-img to start qemu
> > with the relevant disks and perform operation via qmp or via libvirt.
> > This a huge rewrite and I'm
> > not sure it's worth the effort.
>
> OK, I'm not familiar with oVirt.. But maybe, you don't need to rewrite everything, but just add a possibility to use qmp interface, so, keep old features working on qemu-img, while adding new features using qmp?
This is always the case, we always need to support old code for 2
versions when we add
new code. This is even worse. It makes sense only if this enables
stuff which is not possible
with qemu-img.
One case that we did not handle yet is copying bitmaps down after
"qemu-img commit",
which should be solved now in libvirt when using blockCommit(). If use
libvirt also for
cold-commit by running qemu paused, it makes sense to add this.
Another case we did not handle yet is backing up non-running vms,
which will also work
transparently if we run the vm paused just for the backup. So using
paused vm via
libvirt seems like a good direction.
> > This approach also has limitations, for example using qemu-img we can
> > copy disks in parallel on
> > multiple hosts, while using libvirt and qemu will limit us to using
> > one host since qemu takes locks
> > that we control.
>
> hmm.. do you open same image RW in several qemu-img processes? Or I don't follow, what behavior is blocked by Qemu locks... qemu-img takes locks as well..
We don't copy images in parallel yet. Currently we just copy the image
one by one,
starting at the base image in the chain. If we start to copy in
parallel, we will run multiple
qemu-img processes, each copying one layer in the chain. It should be
possible since
only the destination image is writable, and qemu does not need to
read the backing file
not on the source or destination. I think we tried this but maybe it
was before qemu-img
added locking.
> > Also using libvirt means that all new features take more time to
> > complete since now we have
> > a new layer between oVirt and qemu.
>
> May be, using qmp directly is still a good option.
We use qmp for testing our nbd client, it is awesome. This is another option
we need to consider.
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-05-21 16:29 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-20 22:01 [PATCH v5 0/5] qemu-img: Add convert --bitmaps Eric Blake
2020-05-20 22:01 ` [PATCH v5 1/5] iotests: Fix test 178 Eric Blake
2020-05-21 11:31 ` Vladimir Sementsov-Ogievskiy
2020-05-20 22:01 ` [PATCH v5 2/5] qcow2: Expose bitmaps' size during measure Eric Blake
2020-05-20 23:00 ` Nir Soffer
2020-05-21 11:40 ` Vladimir Sementsov-Ogievskiy
2020-05-21 13:29 ` Nir Soffer
2020-05-21 15:09 ` Vladimir Sementsov-Ogievskiy
2020-05-21 16:28 ` Nir Soffer
2020-05-21 13:08 ` Eric Blake
2020-05-21 13:17 ` Nir Soffer
2020-05-21 13:29 ` Eric Blake
2020-05-20 22:01 ` [PATCH v5 3/5] qemu-img: Factor out code for merging bitmaps Eric Blake
2020-05-20 22:01 ` [PATCH v5 4/5] qemu-img: Add convert --bitmaps option Eric Blake
2020-05-21 15:11 ` Nir Soffer
2020-05-21 15:19 ` Eric Blake
2020-05-20 22:01 ` [PATCH v5 5/5] iotests: Add test 291 to for qemu-img bitmap coverage Eric Blake
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.