* [PATCH v3 0/9] qemu-img: Add convert --bitmaps
@ 2020-05-08 18:03 Eric Blake
2020-05-08 18:03 ` [PATCH v3 1/9] docs: Sort sections on qemu-img subcommand parameters Eric Blake
` (8 more replies)
0 siblings, 9 replies; 30+ messages in thread
From: Eric Blake @ 2020-05-08 18:03 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, qemu-block, mreitz
v2 was here, to see the original cover letter:
https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg03464.html
Since then:
- patch 2 was previously posted in isolation but fits well in this series
- patch 3 is new
- patch 4 and 5 were split from v2:2/6 [Max]
- new filename chosen in patch 5 is saner [Kevin]
- patch 6 allows list of operations on one bitmap name [Max]
- patch 7 exposes 0 when measuring v3 image without bitmaps [Max]
- patch 7 and 9 iotests beefed up [Max]
- patch 8 had enough rebase churn that I dropped R-b
- compilation passes on mingw [patchew]
001/9:[----] [--] 'docs: Sort sections on qemu-img subcommand parameters'
002/9:[down] 'qemu-img: Fix stale comments on doc location'
003/9:[down] 'block: Make it easier to learn which BDS support bitmaps'
004/9:[down] 'blockdev: Promote several bitmap functions to non-static'
005/9:[0076] [FC] 'blockdev: Split off basic bitmap operations for qemu-img'
006/9:[0199] [FC] 'qemu-img: Add bitmap sub-command'
007/9:[0078] [FC] 'qcow2: Expose bitmaps' size during measure'
008/9:[0017] [FC] 'qemu-img: Add convert --bitmaps option'
009/9:[0033] [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-v3
Eric Blake (9):
docs: Sort sections on qemu-img subcommand parameters
qemu-img: Fix stale comments on doc location
block: Make it easier to learn which BDS support bitmaps
blockdev: Promote several bitmap functions to non-static
blockdev: Split off basic bitmap operations for qemu-img
qemu-img: Add bitmap sub-command
qcow2: Expose bitmaps' size during measure
qemu-img: Add convert --bitmaps option
iotests: Add test 291 to for qemu-img bitmap coverage
docs/tools/qemu-img.rst | 77 ++++---
Makefile.objs | 3 +-
qapi/block-core.json | 15 +-
block/qcow2.h | 1 +
include/block/block_int.h | 13 ++
include/block/dirty-bitmap.h | 1 +
block/crypto.c | 2 +-
block/dirty-bitmap.c | 9 +
block/monitor/bitmap-qmp-cmds.c | 323 +++++++++++++++++++++++++++++
block/qcow2-bitmap.c | 7 +
block/qcow2.c | 38 +++-
block/raw-format.c | 2 +-
blockdev.c | 303 +--------------------------
qemu-img.c | 340 ++++++++++++++++++++++++++++++-
MAINTAINERS | 1 +
block/monitor/Makefile.objs | 1 +
qemu-img-cmds.hx | 13 +-
tests/qemu-iotests/178.out.qcow2 | 16 ++
tests/qemu-iotests/190 | 43 +++-
tests/qemu-iotests/190.out | 23 ++-
tests/qemu-iotests/291 | 112 ++++++++++
tests/qemu-iotests/291.out | 78 +++++++
tests/qemu-iotests/group | 1 +
23 files changed, 1077 insertions(+), 345 deletions(-)
create mode 100644 block/monitor/bitmap-qmp-cmds.c
create mode 100755 tests/qemu-iotests/291
create mode 100644 tests/qemu-iotests/291.out
--
2.26.2
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v3 1/9] docs: Sort sections on qemu-img subcommand parameters
2020-05-08 18:03 [PATCH v3 0/9] qemu-img: Add convert --bitmaps Eric Blake
@ 2020-05-08 18:03 ` Eric Blake
2020-05-08 18:03 ` [PATCH v3 2/9] qemu-img: Fix stale comments on doc location Eric Blake
` (7 subsequent siblings)
8 siblings, 0 replies; 30+ messages in thread
From: Eric Blake @ 2020-05-08 18:03 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, qemu-block, mreitz
We already list the subcommand summaries alphabetically, we should do
the same for the documentation related to subcommand-specific
parameters.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
docs/tools/qemu-img.rst | 48 ++++++++++++++++++++---------------------
1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index 0080f83a76c9..7d08c48d308f 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -142,30 +142,6 @@ by the used format or see the format descriptions below for details.
the documentation of the emulator's ``-drive cache=...`` option for allowed
values.
-Parameters to snapshot subcommand:
-
-.. program:: qemu-img-snapshot
-
-.. option:: snapshot
-
- Is the name of the snapshot to create, apply or delete
-
-.. option:: -a
-
- Applies a snapshot (revert disk to saved state)
-
-.. option:: -c
-
- Creates a snapshot
-
-.. option:: -d
-
- Deletes a snapshot
-
-.. option:: -l
-
- Lists all snapshots in the given image
-
Parameters to compare subcommand:
.. program:: qemu-img-compare
@@ -245,6 +221,30 @@ Parameters to dd subcommand:
Sets the number of input blocks to skip
+Parameters to snapshot subcommand:
+
+.. program:: qemu-img-snapshot
+
+.. option:: snapshot
+
+ Is the name of the snapshot to create, apply or delete
+
+.. option:: -a
+
+ Applies a snapshot (revert disk to saved state)
+
+.. option:: -c
+
+ Creates a snapshot
+
+.. option:: -d
+
+ Deletes a snapshot
+
+.. option:: -l
+
+ Lists all snapshots in the given image
+
Command description:
.. program:: qemu-img-commands
--
2.26.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 2/9] qemu-img: Fix stale comments on doc location
2020-05-08 18:03 [PATCH v3 0/9] qemu-img: Add convert --bitmaps Eric Blake
2020-05-08 18:03 ` [PATCH v3 1/9] docs: Sort sections on qemu-img subcommand parameters Eric Blake
@ 2020-05-08 18:03 ` Eric Blake
2020-05-11 9:05 ` Max Reitz
2020-05-08 18:03 ` [PATCH v3 3/9] block: Make it easier to learn which BDS support bitmaps Eric Blake
` (6 subsequent siblings)
8 siblings, 1 reply; 30+ messages in thread
From: Eric Blake @ 2020-05-08 18:03 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, qemu-block, mreitz
Missed in commit e13c59fa.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
qemu-img.c | 2 +-
qemu-img-cmds.hx | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 6a4327aaba56..b6e8af9202a5 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -108,7 +108,7 @@ static void QEMU_NORETURN unrecognized_option(const char *option)
error_exit("unrecognized option '%s'", option);
}
-/* Please keep in synch with qemu-img.texi */
+/* Please keep in synch with docs/tools/qemu-img.rst */
static void QEMU_NORETURN help(void)
{
const char *help_msg =
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index c9c54de1df40..e0886437b1f2 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -7,7 +7,7 @@ HXCOMM command structures and help message.
HXCOMM HXCOMM can be used for comments, discarded from both rST and C
HXCOMM When amending the rST sections, please remember to copy the usage
-HXCOMM over to the per-command sections in qemu-img.texi.
+HXCOMM over to the per-command sections in docs/tools/qemu-img.rst.
DEF("amend", img_amend,
"amend [--object objectdef] [--image-opts] [-p] [-q] [-f fmt] [-t cache] -o options filename")
--
2.26.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 3/9] block: Make it easier to learn which BDS support bitmaps
2020-05-08 18:03 [PATCH v3 0/9] qemu-img: Add convert --bitmaps Eric Blake
2020-05-08 18:03 ` [PATCH v3 1/9] docs: Sort sections on qemu-img subcommand parameters Eric Blake
2020-05-08 18:03 ` [PATCH v3 2/9] qemu-img: Fix stale comments on doc location Eric Blake
@ 2020-05-08 18:03 ` Eric Blake
2020-05-11 9:21 ` Max Reitz
2020-05-08 18:03 ` [PATCH v3 4/9] blockdev: Promote several bitmap functions to non-static Eric Blake
` (5 subsequent siblings)
8 siblings, 1 reply; 30+ messages in thread
From: Eric Blake @ 2020-05-08 18:03 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, Vladimir Sementsov-Ogievskiy, John Snow, qemu-block, mreitz
Upcoming patches will enhance bitmap support in qemu-img, but in doing
so, it turns out to be nice to suppress output when bitmaps make no
sense (such as on a qcow2 v2 image). Add a hook to make this easier
to query.
In the future, when we improve the ability to look up bitmaps through
a filter, we will probably also want to teach the block layer to
automatically let filters pass this request on through.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
block/qcow2.h | 1 +
include/block/block_int.h | 1 +
include/block/dirty-bitmap.h | 1 +
block/dirty-bitmap.c | 9 +++++++++
block/qcow2-bitmap.c | 7 +++++++
block/qcow2.c | 1 +
6 files changed, 20 insertions(+)
diff --git a/block/qcow2.h b/block/qcow2.h
index f4de0a27d5c3..fb2b2b5a7b4d 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -764,6 +764,7 @@ bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
int qcow2_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
const char *name,
Error **errp);
+bool qcow2_dirty_bitmap_supported(BlockDriverState *bs);
ssize_t coroutine_fn
qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index df6d0273d679..cb1082da4c43 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -560,6 +560,7 @@ struct BlockDriver {
uint64_t parent_perm, uint64_t parent_shared,
uint64_t *nperm, uint64_t *nshared);
+ bool (*bdrv_dirty_bitmap_supported)(BlockDriverState *bs);
bool (*bdrv_co_can_store_new_dirty_bitmap)(BlockDriverState *bs,
const char *name,
uint32_t granularity,
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 8a1002941892..6d2e1707639f 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -16,6 +16,7 @@ typedef enum BitmapCheckFlags {
#define BDRV_BITMAP_MAX_NAME_SIZE 1023
+bool bdrv_dirty_bitmap_supported(BlockDriverState *bs);
BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
uint32_t granularity,
const char *name,
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 063793e31606..89869c483c44 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -478,6 +478,15 @@ int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
}
}
+bool
+bdrv_dirty_bitmap_supported(BlockDriverState *bs)
+{
+ if (bs->drv && bs->drv->bdrv_dirty_bitmap_supported) {
+ return bs->drv->bdrv_dirty_bitmap_supported(bs);
+ }
+ return false;
+}
+
static bool coroutine_fn
bdrv_co_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
uint32_t granularity, Error **errp)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index cb06954b4a5a..b9889c2144cd 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1748,3 +1748,10 @@ fail:
name, bdrv_get_device_or_node_name(bs));
return false;
}
+
+bool qcow2_dirty_bitmap_supported(BlockDriverState *bs)
+{
+ BDRVQcow2State *s = bs->opaque;
+
+ return s->qcow_version >= 3;
+}
diff --git a/block/qcow2.c b/block/qcow2.c
index 1ad95ff04851..838d810ca5ec 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -5663,6 +5663,7 @@ BlockDriver bdrv_qcow2 = {
.bdrv_detach_aio_context = qcow2_detach_aio_context,
.bdrv_attach_aio_context = qcow2_attach_aio_context,
+ .bdrv_dirty_bitmap_supported = qcow2_dirty_bitmap_supported,
.bdrv_co_can_store_new_dirty_bitmap = qcow2_co_can_store_new_dirty_bitmap,
.bdrv_co_remove_persistent_dirty_bitmap =
qcow2_co_remove_persistent_dirty_bitmap,
--
2.26.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 4/9] blockdev: Promote several bitmap functions to non-static
2020-05-08 18:03 [PATCH v3 0/9] qemu-img: Add convert --bitmaps Eric Blake
` (2 preceding siblings ...)
2020-05-08 18:03 ` [PATCH v3 3/9] block: Make it easier to learn which BDS support bitmaps Eric Blake
@ 2020-05-08 18:03 ` Eric Blake
2020-05-11 9:48 ` Max Reitz
2020-05-08 18:03 ` [PATCH v3 5/9] blockdev: Split off basic bitmap operations for qemu-img Eric Blake
` (4 subsequent siblings)
8 siblings, 1 reply; 30+ messages in thread
From: Eric Blake @ 2020-05-08 18:03 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, Markus Armbruster, qemu-block, mreitz
The next patch will split blockdev.c, which will require accessing
some previously-static functions from more than one .c file. But part
of promoting a function to public is picking a naming scheme that does
not reek of exposing too many internals (two of the three functions
were named starting with 'do_'). To make future code motion easier,
perform the function rename and non-static promotion into its own
patch.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
include/block/block_int.h | 12 +++++++++++
blockdev.c | 45 ++++++++++++++++-----------------------
2 files changed, 30 insertions(+), 27 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index cb1082da4c43..e71505951d48 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1344,4 +1344,16 @@ int coroutine_fn bdrv_co_create_opts_simple(BlockDriver *drv,
Error **errp);
extern QemuOptsList bdrv_create_opts_simple;
+BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node,
+ const char *name,
+ BlockDriverState **pbs,
+ Error **errp);
+BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
+ BlockDirtyBitmapMergeSourceList *bms,
+ HBitmap **backup, Error **errp);
+BdrvDirtyBitmap *block_dirty_bitmap_remove(const char *node, const char *name,
+ bool release,
+ BlockDriverState **bitmap_bs,
+ Error **errp);
+
#endif /* BLOCK_INT_H */
diff --git a/blockdev.c b/blockdev.c
index b3c840ec0312..fbeb38437869 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1197,10 +1197,10 @@ out_aio_context:
*
* @return: A bitmap object on success, or NULL on failure.
*/
-static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node,
- const char *name,
- BlockDriverState **pbs,
- Error **errp)
+BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node,
+ const char *name,
+ BlockDriverState **pbs,
+ Error **errp)
{
BlockDriverState *bs;
BdrvDirtyBitmap *bitmap;
@@ -2171,11 +2171,6 @@ static void block_dirty_bitmap_disable_abort(BlkActionState *common)
}
}
-static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(
- const char *node, const char *target,
- BlockDirtyBitmapMergeSourceList *bitmaps,
- HBitmap **backup, Error **errp);
-
static void block_dirty_bitmap_merge_prepare(BlkActionState *common,
Error **errp)
{
@@ -2189,15 +2184,11 @@ static void block_dirty_bitmap_merge_prepare(BlkActionState *common,
action = common->action->u.block_dirty_bitmap_merge.data;
- state->bitmap = do_block_dirty_bitmap_merge(action->node, action->target,
- action->bitmaps, &state->backup,
- errp);
+ state->bitmap = block_dirty_bitmap_merge(action->node, action->target,
+ action->bitmaps, &state->backup,
+ errp);
}
-static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
- const char *node, const char *name, bool release,
- BlockDriverState **bitmap_bs, Error **errp);
-
static void block_dirty_bitmap_remove_prepare(BlkActionState *common,
Error **errp)
{
@@ -2211,8 +2202,8 @@ static void block_dirty_bitmap_remove_prepare(BlkActionState *common,
action = common->action->u.block_dirty_bitmap_remove.data;
- state->bitmap = do_block_dirty_bitmap_remove(action->node, action->name,
- false, &state->bs, errp);
+ state->bitmap = block_dirty_bitmap_remove(action->node, action->name,
+ false, &state->bs, errp);
if (state->bitmap) {
bdrv_dirty_bitmap_skip_store(state->bitmap, true);
bdrv_dirty_bitmap_set_busy(state->bitmap, true);
@@ -2504,9 +2495,10 @@ out:
aio_context_release(aio_context);
}
-static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
- const char *node, const char *name, bool release,
- BlockDriverState **bitmap_bs, Error **errp)
+BdrvDirtyBitmap *block_dirty_bitmap_remove(const char *node, const char *name,
+ bool release,
+ BlockDriverState **bitmap_bs,
+ Error **errp)
{
BlockDriverState *bs;
BdrvDirtyBitmap *bitmap;
@@ -2548,7 +2540,7 @@ static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
Error **errp)
{
- do_block_dirty_bitmap_remove(node, name, true, NULL, errp);
+ block_dirty_bitmap_remove(node, name, true, NULL, errp);
}
/**
@@ -2609,10 +2601,9 @@ void qmp_block_dirty_bitmap_disable(const char *node, const char *name,
bdrv_disable_dirty_bitmap(bitmap);
}
-static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(
- const char *node, const char *target,
- BlockDirtyBitmapMergeSourceList *bitmaps,
- HBitmap **backup, Error **errp)
+BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
+ BlockDirtyBitmapMergeSourceList *bitmaps,
+ HBitmap **backup, Error **errp)
{
BlockDriverState *bs;
BdrvDirtyBitmap *dst, *src, *anon;
@@ -2675,7 +2666,7 @@ void qmp_block_dirty_bitmap_merge(const char *node, const char *target,
BlockDirtyBitmapMergeSourceList *bitmaps,
Error **errp)
{
- do_block_dirty_bitmap_merge(node, target, bitmaps, NULL, errp);
+ block_dirty_bitmap_merge(node, target, bitmaps, NULL, errp);
}
BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
--
2.26.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 5/9] blockdev: Split off basic bitmap operations for qemu-img
2020-05-08 18:03 [PATCH v3 0/9] qemu-img: Add convert --bitmaps Eric Blake
` (3 preceding siblings ...)
2020-05-08 18:03 ` [PATCH v3 4/9] blockdev: Promote several bitmap functions to non-static Eric Blake
@ 2020-05-08 18:03 ` Eric Blake
2020-05-11 10:17 ` Max Reitz
2020-05-08 18:03 ` [PATCH v3 6/9] qemu-img: Add bitmap sub-command Eric Blake
` (3 subsequent siblings)
8 siblings, 1 reply; 30+ messages in thread
From: Eric Blake @ 2020-05-08 18:03 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, Vladimir Sementsov-Ogievskiy, qemu-block,
Markus Armbruster, mreitz, John Snow
Upcoming patches want to add some basic bitmap manipulation abilities
to qemu-img. But blockdev.o is too heavyweight to link into qemu-img
(among other things, it would drag in block jobs and transaction
support - qemu-img does offline manipulation, where atomicity is less
important because there are no concurrent modifications to compete
with), so it's time to split off the bare bones of what we will need
into a new file block/monitor/bitmap-qmp-cmds.o.
This is sufficient to expose 6 QMP commands for use by qemu-img (add,
remove, clear, enable, disable, merge), as well as move the three
helper functions touched in the previous patch. Regarding
MAINTAINERS, the new file is automatically part of block core, but
also makes sense as related to other dirty bitmap files.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
Makefile.objs | 3 +-
block/monitor/bitmap-qmp-cmds.c | 323 ++++++++++++++++++++++++++++++++
blockdev.c | 284 ----------------------------
MAINTAINERS | 1 +
block/monitor/Makefile.objs | 1 +
5 files changed, 326 insertions(+), 286 deletions(-)
create mode 100644 block/monitor/bitmap-qmp-cmds.c
diff --git a/Makefile.objs b/Makefile.objs
index a7c967633acf..99774cfd2545 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -13,9 +13,8 @@ chardev-obj-y = chardev/
authz-obj-y = authz/
-block-obj-y = nbd/
+block-obj-y = block/ block/monitor/ nbd/ scsi/
block-obj-y += block.o blockjob.o job.o
-block-obj-y += block/ scsi/
block-obj-y += qemu-io-cmds.o
block-obj-$(CONFIG_REPLICATION) += replication.o
diff --git a/block/monitor/bitmap-qmp-cmds.c b/block/monitor/bitmap-qmp-cmds.c
new file mode 100644
index 000000000000..748e1e682483
--- /dev/null
+++ b/block/monitor/bitmap-qmp-cmds.c
@@ -0,0 +1,323 @@
+/*
+ * QEMU host block device bitmaps
+ *
+ * Copyright (c) 2003-2008 Fabrice Bellard
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * later. See the COPYING file in the top-level directory.
+ *
+ * This file incorporates work covered by the following copyright and
+ * permission notice:
+ *
+ * Copyright (c) 2003-2008 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+
+#include "sysemu/blockdev.h"
+#include "block/block.h"
+#include "block/block_int.h"
+#include "qapi/qapi-commands-block.h"
+#include "qapi/error.h"
+
+/**
+ * block_dirty_bitmap_lookup:
+ * Return a dirty bitmap (if present), after validating
+ * the node reference and bitmap names.
+ *
+ * @node: The name of the BDS node to search for bitmaps
+ * @name: The name of the bitmap to search for
+ * @pbs: Output pointer for BDS lookup, if desired. Can be NULL.
+ * @errp: Output pointer for error information. Can be NULL.
+ *
+ * @return: A bitmap object on success, or NULL on failure.
+ */
+BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node,
+ const char *name,
+ BlockDriverState **pbs,
+ Error **errp)
+{
+ BlockDriverState *bs;
+ BdrvDirtyBitmap *bitmap;
+
+ if (!node) {
+ error_setg(errp, "Node cannot be NULL");
+ return NULL;
+ }
+ if (!name) {
+ error_setg(errp, "Bitmap name cannot be NULL");
+ return NULL;
+ }
+ bs = bdrv_lookup_bs(node, node, NULL);
+ if (!bs) {
+ error_setg(errp, "Node '%s' not found", node);
+ return NULL;
+ }
+
+ bitmap = bdrv_find_dirty_bitmap(bs, name);
+ if (!bitmap) {
+ error_setg(errp, "Dirty bitmap '%s' not found", name);
+ return NULL;
+ }
+
+ if (pbs) {
+ *pbs = bs;
+ }
+
+ return bitmap;
+}
+
+void qmp_block_dirty_bitmap_add(const char *node, const char *name,
+ bool has_granularity, uint32_t granularity,
+ bool has_persistent, bool persistent,
+ bool has_disabled, bool disabled,
+ Error **errp)
+{
+ BlockDriverState *bs;
+ BdrvDirtyBitmap *bitmap;
+ AioContext *aio_context;
+
+ if (!name || name[0] == '\0') {
+ error_setg(errp, "Bitmap name cannot be empty");
+ return;
+ }
+
+ bs = bdrv_lookup_bs(node, node, errp);
+ if (!bs) {
+ return;
+ }
+
+ aio_context = bdrv_get_aio_context(bs);
+ aio_context_acquire(aio_context);
+
+ if (has_granularity) {
+ if (granularity < 512 || !is_power_of_2(granularity)) {
+ error_setg(errp, "Granularity must be power of 2 "
+ "and at least 512");
+ goto out;
+ }
+ } else {
+ /* Default to cluster size, if available: */
+ granularity = bdrv_get_default_bitmap_granularity(bs);
+ }
+
+ if (!has_persistent) {
+ persistent = false;
+ }
+
+ if (!has_disabled) {
+ disabled = false;
+ }
+
+ if (persistent &&
+ !bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp))
+ {
+ goto out;
+ }
+
+ bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
+ if (bitmap == NULL) {
+ goto out;
+ }
+
+ if (disabled) {
+ bdrv_disable_dirty_bitmap(bitmap);
+ }
+
+ bdrv_dirty_bitmap_set_persistence(bitmap, persistent);
+
+out:
+ aio_context_release(aio_context);
+}
+
+BdrvDirtyBitmap *block_dirty_bitmap_remove(const char *node, const char *name,
+ bool release,
+ BlockDriverState **bitmap_bs,
+ Error **errp)
+{
+ BlockDriverState *bs;
+ BdrvDirtyBitmap *bitmap;
+ AioContext *aio_context;
+
+ bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
+ if (!bitmap || !bs) {
+ return NULL;
+ }
+
+ aio_context = bdrv_get_aio_context(bs);
+ aio_context_acquire(aio_context);
+
+ if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_BUSY | BDRV_BITMAP_RO,
+ errp)) {
+ aio_context_release(aio_context);
+ return NULL;
+ }
+
+ if (bdrv_dirty_bitmap_get_persistence(bitmap) &&
+ bdrv_remove_persistent_dirty_bitmap(bs, name, errp) < 0)
+ {
+ aio_context_release(aio_context);
+ return NULL;
+ }
+
+ if (release) {
+ bdrv_release_dirty_bitmap(bitmap);
+ }
+
+ if (bitmap_bs) {
+ *bitmap_bs = bs;
+ }
+
+ aio_context_release(aio_context);
+ return release ? NULL : bitmap;
+}
+
+void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
+ Error **errp)
+{
+ block_dirty_bitmap_remove(node, name, true, NULL, errp);
+}
+
+/**
+ * Completely clear a bitmap, for the purposes of synchronizing a bitmap
+ * immediately after a full backup operation.
+ */
+void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
+ Error **errp)
+{
+ BdrvDirtyBitmap *bitmap;
+ BlockDriverState *bs;
+
+ bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
+ if (!bitmap || !bs) {
+ return;
+ }
+
+ if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, errp)) {
+ return;
+ }
+
+ bdrv_clear_dirty_bitmap(bitmap, NULL);
+}
+
+void qmp_block_dirty_bitmap_enable(const char *node, const char *name,
+ Error **errp)
+{
+ BlockDriverState *bs;
+ BdrvDirtyBitmap *bitmap;
+
+ bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
+ if (!bitmap) {
+ return;
+ }
+
+ if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
+ return;
+ }
+
+ bdrv_enable_dirty_bitmap(bitmap);
+}
+
+void qmp_block_dirty_bitmap_disable(const char *node, const char *name,
+ Error **errp)
+{
+ BlockDriverState *bs;
+ BdrvDirtyBitmap *bitmap;
+
+ bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
+ if (!bitmap) {
+ return;
+ }
+
+ if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
+ return;
+ }
+
+ bdrv_disable_dirty_bitmap(bitmap);
+}
+
+BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
+ BlockDirtyBitmapMergeSourceList *bitmaps,
+ HBitmap **backup, Error **errp)
+{
+ BlockDriverState *bs;
+ BdrvDirtyBitmap *dst, *src, *anon;
+ BlockDirtyBitmapMergeSourceList *lst;
+ Error *local_err = NULL;
+
+ dst = block_dirty_bitmap_lookup(node, target, &bs, errp);
+ if (!dst) {
+ return NULL;
+ }
+
+ anon = bdrv_create_dirty_bitmap(bs, bdrv_dirty_bitmap_granularity(dst),
+ NULL, errp);
+ if (!anon) {
+ return NULL;
+ }
+
+ for (lst = bitmaps; lst; lst = lst->next) {
+ switch (lst->value->type) {
+ const char *name, *node;
+ case QTYPE_QSTRING:
+ name = lst->value->u.local;
+ src = bdrv_find_dirty_bitmap(bs, name);
+ if (!src) {
+ error_setg(errp, "Dirty bitmap '%s' not found", name);
+ dst = NULL;
+ goto out;
+ }
+ break;
+ case QTYPE_QDICT:
+ node = lst->value->u.external.node;
+ name = lst->value->u.external.name;
+ src = block_dirty_bitmap_lookup(node, name, NULL, errp);
+ if (!src) {
+ dst = NULL;
+ goto out;
+ }
+ break;
+ default:
+ abort();
+ }
+
+ bdrv_merge_dirty_bitmap(anon, src, NULL, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ dst = NULL;
+ goto out;
+ }
+ }
+
+ /* Merge into dst; dst is unchanged on failure. */
+ bdrv_merge_dirty_bitmap(dst, anon, backup, errp);
+
+ out:
+ bdrv_release_dirty_bitmap(anon);
+ return dst;
+}
+
+void qmp_block_dirty_bitmap_merge(const char *node, const char *target,
+ BlockDirtyBitmapMergeSourceList *bitmaps,
+ Error **errp)
+{
+ block_dirty_bitmap_merge(node, target, bitmaps, NULL, errp);
+}
diff --git a/blockdev.c b/blockdev.c
index fbeb38437869..72df193ca73b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1185,53 +1185,6 @@ out_aio_context:
return NULL;
}
-/**
- * block_dirty_bitmap_lookup:
- * Return a dirty bitmap (if present), after validating
- * the node reference and bitmap names.
- *
- * @node: The name of the BDS node to search for bitmaps
- * @name: The name of the bitmap to search for
- * @pbs: Output pointer for BDS lookup, if desired. Can be NULL.
- * @errp: Output pointer for error information. Can be NULL.
- *
- * @return: A bitmap object on success, or NULL on failure.
- */
-BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node,
- const char *name,
- BlockDriverState **pbs,
- Error **errp)
-{
- BlockDriverState *bs;
- BdrvDirtyBitmap *bitmap;
-
- if (!node) {
- error_setg(errp, "Node cannot be NULL");
- return NULL;
- }
- if (!name) {
- error_setg(errp, "Bitmap name cannot be NULL");
- return NULL;
- }
- bs = bdrv_lookup_bs(node, node, NULL);
- if (!bs) {
- error_setg(errp, "Node '%s' not found", node);
- return NULL;
- }
-
- bitmap = bdrv_find_dirty_bitmap(bs, name);
- if (!bitmap) {
- error_setg(errp, "Dirty bitmap '%s' not found", name);
- return NULL;
- }
-
- if (pbs) {
- *pbs = bs;
- }
-
- return bitmap;
-}
-
/* New and old BlockDriverState structs for atomic group operations */
typedef struct BlkActionState BlkActionState;
@@ -2432,243 +2385,6 @@ void qmp_block_passwd(bool has_device, const char *device,
"Setting block passwords directly is no longer supported");
}
-void qmp_block_dirty_bitmap_add(const char *node, const char *name,
- bool has_granularity, uint32_t granularity,
- bool has_persistent, bool persistent,
- bool has_disabled, bool disabled,
- Error **errp)
-{
- BlockDriverState *bs;
- BdrvDirtyBitmap *bitmap;
- AioContext *aio_context;
-
- if (!name || name[0] == '\0') {
- error_setg(errp, "Bitmap name cannot be empty");
- return;
- }
-
- bs = bdrv_lookup_bs(node, node, errp);
- if (!bs) {
- return;
- }
-
- aio_context = bdrv_get_aio_context(bs);
- aio_context_acquire(aio_context);
-
- if (has_granularity) {
- if (granularity < 512 || !is_power_of_2(granularity)) {
- error_setg(errp, "Granularity must be power of 2 "
- "and at least 512");
- goto out;
- }
- } else {
- /* Default to cluster size, if available: */
- granularity = bdrv_get_default_bitmap_granularity(bs);
- }
-
- if (!has_persistent) {
- persistent = false;
- }
-
- if (!has_disabled) {
- disabled = false;
- }
-
- if (persistent &&
- !bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp))
- {
- goto out;
- }
-
- bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
- if (bitmap == NULL) {
- goto out;
- }
-
- if (disabled) {
- bdrv_disable_dirty_bitmap(bitmap);
- }
-
- bdrv_dirty_bitmap_set_persistence(bitmap, persistent);
-
-out:
- aio_context_release(aio_context);
-}
-
-BdrvDirtyBitmap *block_dirty_bitmap_remove(const char *node, const char *name,
- bool release,
- BlockDriverState **bitmap_bs,
- Error **errp)
-{
- BlockDriverState *bs;
- BdrvDirtyBitmap *bitmap;
- AioContext *aio_context;
-
- bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
- if (!bitmap || !bs) {
- return NULL;
- }
-
- aio_context = bdrv_get_aio_context(bs);
- aio_context_acquire(aio_context);
-
- if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_BUSY | BDRV_BITMAP_RO,
- errp)) {
- aio_context_release(aio_context);
- return NULL;
- }
-
- if (bdrv_dirty_bitmap_get_persistence(bitmap) &&
- bdrv_remove_persistent_dirty_bitmap(bs, name, errp) < 0)
- {
- aio_context_release(aio_context);
- return NULL;
- }
-
- if (release) {
- bdrv_release_dirty_bitmap(bitmap);
- }
-
- if (bitmap_bs) {
- *bitmap_bs = bs;
- }
-
- aio_context_release(aio_context);
- return release ? NULL : bitmap;
-}
-
-void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
- Error **errp)
-{
- block_dirty_bitmap_remove(node, name, true, NULL, errp);
-}
-
-/**
- * Completely clear a bitmap, for the purposes of synchronizing a bitmap
- * immediately after a full backup operation.
- */
-void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
- Error **errp)
-{
- BdrvDirtyBitmap *bitmap;
- BlockDriverState *bs;
-
- bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
- if (!bitmap || !bs) {
- return;
- }
-
- if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, errp)) {
- return;
- }
-
- bdrv_clear_dirty_bitmap(bitmap, NULL);
-}
-
-void qmp_block_dirty_bitmap_enable(const char *node, const char *name,
- Error **errp)
-{
- BlockDriverState *bs;
- BdrvDirtyBitmap *bitmap;
-
- bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
- if (!bitmap) {
- return;
- }
-
- if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
- return;
- }
-
- bdrv_enable_dirty_bitmap(bitmap);
-}
-
-void qmp_block_dirty_bitmap_disable(const char *node, const char *name,
- Error **errp)
-{
- BlockDriverState *bs;
- BdrvDirtyBitmap *bitmap;
-
- bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
- if (!bitmap) {
- return;
- }
-
- if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
- return;
- }
-
- bdrv_disable_dirty_bitmap(bitmap);
-}
-
-BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
- BlockDirtyBitmapMergeSourceList *bitmaps,
- HBitmap **backup, Error **errp)
-{
- BlockDriverState *bs;
- BdrvDirtyBitmap *dst, *src, *anon;
- BlockDirtyBitmapMergeSourceList *lst;
- Error *local_err = NULL;
-
- dst = block_dirty_bitmap_lookup(node, target, &bs, errp);
- if (!dst) {
- return NULL;
- }
-
- anon = bdrv_create_dirty_bitmap(bs, bdrv_dirty_bitmap_granularity(dst),
- NULL, errp);
- if (!anon) {
- return NULL;
- }
-
- for (lst = bitmaps; lst; lst = lst->next) {
- switch (lst->value->type) {
- const char *name, *node;
- case QTYPE_QSTRING:
- name = lst->value->u.local;
- src = bdrv_find_dirty_bitmap(bs, name);
- if (!src) {
- error_setg(errp, "Dirty bitmap '%s' not found", name);
- dst = NULL;
- goto out;
- }
- break;
- case QTYPE_QDICT:
- node = lst->value->u.external.node;
- name = lst->value->u.external.name;
- src = block_dirty_bitmap_lookup(node, name, NULL, errp);
- if (!src) {
- dst = NULL;
- goto out;
- }
- break;
- default:
- abort();
- }
-
- bdrv_merge_dirty_bitmap(anon, src, NULL, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
- dst = NULL;
- goto out;
- }
- }
-
- /* Merge into dst; dst is unchanged on failure. */
- bdrv_merge_dirty_bitmap(dst, anon, backup, errp);
-
- out:
- bdrv_release_dirty_bitmap(anon);
- return dst;
-}
-
-void qmp_block_dirty_bitmap_merge(const char *node, const char *target,
- BlockDirtyBitmapMergeSourceList *bitmaps,
- Error **errp)
-{
- block_dirty_bitmap_merge(node, target, bitmaps, NULL, errp);
-}
-
BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
const char *name,
Error **errp)
diff --git a/MAINTAINERS b/MAINTAINERS
index 1f84e3ae2c6a..6a8dc1e69d42 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2007,6 +2007,7 @@ L: qemu-block@nongnu.org
S: Supported
F: include/qemu/hbitmap.h
F: include/block/dirty-bitmap.h
+F: block/monitor/bitmap-qmp-cmds.c
F: block/dirty-bitmap.c
F: block/qcow2-bitmap.c
F: migration/block-dirty-bitmap.c
diff --git a/block/monitor/Makefile.objs b/block/monitor/Makefile.objs
index 0a74f9a8b5b7..39acf8502224 100644
--- a/block/monitor/Makefile.objs
+++ b/block/monitor/Makefile.objs
@@ -1 +1,2 @@
common-obj-y += block-hmp-cmds.o
+block-obj-y += bitmap-qmp-cmds.o
--
2.26.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 6/9] qemu-img: Add bitmap sub-command
2020-05-08 18:03 [PATCH v3 0/9] qemu-img: Add convert --bitmaps Eric Blake
` (4 preceding siblings ...)
2020-05-08 18:03 ` [PATCH v3 5/9] blockdev: Split off basic bitmap operations for qemu-img Eric Blake
@ 2020-05-08 18:03 ` Eric Blake
2020-05-11 11:10 ` Max Reitz
2020-05-08 18:03 ` [PATCH v3 7/9] qcow2: Expose bitmaps' size during measure Eric Blake
` (2 subsequent siblings)
8 siblings, 1 reply; 30+ messages in thread
From: Eric Blake @ 2020-05-08 18:03 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, qemu-block, mreitz
Include actions for --add, --remove, --clear, --enable, --disable, and
--merge (note that --clear is a bit of fluff, because the same can be
accomplished by removing a bitmap and then adding a new one in its
place, but it matches what QMP commands exist). Listing is omitted,
because it does not require a bitmap name and because it was already
possible with 'qemu-img info'. A single command line can play one or
more bitmap commands in sequence on the same bitmap name (although all
added bitmaps share the same granularity, and and all merged bitmaps
come from the same source file). Merge defaults to other bitmaps in
the primary image, but can also be told to merge bitmaps from a
distinct image.
While this supports --image-opts for the file being modified, I did
not think it worth the extra complexity to support that for the source
file in a cross-file merges. Likewise, I chose to have --merge only
take a single source rather than following the QMP support for
multiple merges in one go (although you can still use more than one
--merge in the command line); in part because qemu-img is offline and
therefore atomicity is not an issue.
Upcoming patches will add iotest coverage of these commands while
also testing other features.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
docs/tools/qemu-img.rst | 23 ++++
qemu-img.c | 254 ++++++++++++++++++++++++++++++++++++++++
qemu-img-cmds.hx | 7 ++
3 files changed, 284 insertions(+)
diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index 7d08c48d308f..68393c357386 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -281,6 +281,29 @@ Command description:
For write tests, by default a buffer filled with zeros is written. This can be
overridden with a pattern byte specified by *PATTERN*.
+.. option:: bitmap (--merge SOURCE | --add | --remove | --clear | --enable | --disable)... [-b SOURCE_FILE [-F SOURCE_FMT]] [-g GRANULARITY] [--object OBJECTDEF] [--image-opts | -f FMT] FILENAME BITMAP
+
+ Perform one or more modifications of the persistent bitmap *BITMAP*
+ in the disk image *FILENAME*. The various modifications are:
+
+ ``--add`` to create *BITMAP*, enabled to record future edits.
+
+ ``--remove`` to remove *BITMAP*.
+
+ ``--clear`` to clear *BITMAP*.
+
+ ``--enable`` to change *BITMAP* to start recording future edits.
+
+ ``--disable`` to change *BITMAP* to stop recording future edits.
+
+ ``--merge`` to merge the contents of *SOURCE_BITMAP* into *BITMAP*.
+
+ Additional options ``-g`` set a non-default *GRANULARITY* for
+ ``--add``, and ``-b`` and ``-F`` select an alternative source file
+ for all *SOURCE* bitmaps used by ``--merge``.
+
+ To see what bitmaps are present in an image, use ``qemu-img info``.
+
.. option:: check [--object OBJECTDEF] [--image-opts] [-q] [-f FMT] [--output=OFMT] [-r [leaks | all]] [-T SRC_CACHE] [-U] FILENAME
Perform a consistency check on the disk image *FILENAME*. The command can
diff --git a/qemu-img.c b/qemu-img.c
index b6e8af9202a5..7ad86f7b8072 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -28,6 +28,7 @@
#include "qemu-common.h"
#include "qemu-version.h"
#include "qapi/error.h"
+#include "qapi/qapi-commands-block-core.h"
#include "qapi/qapi-visit-block-core.h"
#include "qapi/qobject-output-visitor.h"
#include "qapi/qmp/qjson.h"
@@ -71,6 +72,12 @@ enum {
OPTION_SHRINK = 266,
OPTION_SALVAGE = 267,
OPTION_TARGET_IS_ZERO = 268,
+ OPTION_ADD = 269,
+ OPTION_REMOVE = 270,
+ OPTION_CLEAR = 271,
+ OPTION_ENABLE = 272,
+ OPTION_DISABLE = 273,
+ OPTION_MERGE = 274,
};
typedef enum OutputFormat {
@@ -169,6 +176,14 @@ static void QEMU_NORETURN help(void)
" '-n' skips the target volume creation (useful if the volume is created\n"
" prior to running qemu-img)\n"
"\n"
+ "Parameters to bitmap subcommand:\n"
+ " 'bitmap' is the name of the bitmap to manipulate, through one or more\n"
+ " actions from '--add', '--remove', '--clear', '--enable', '--diable',\n"
+ " or '--merge source'\n"
+ " '-g granularity' sets the granularity for '--add' actions\n"
+ " '-b source' and '-F src_fmt' tell '--merge' actions to find the source\n"
+ " bitmaps from an alternative file\n"
+ "\n"
"Parameters to check subcommand:\n"
" '-r' tries to repair any inconsistencies that are found during the check.\n"
" '-r leaks' repairs only cluster leaks, whereas '-r all' fixes all\n"
@@ -4461,6 +4476,245 @@ out:
return 0;
}
+enum ImgBitmapAct {
+ BITMAP_ADD,
+ BITMAP_REMOVE,
+ BITMAP_CLEAR,
+ BITMAP_ENABLE,
+ BITMAP_DISABLE,
+ BITMAP_MERGE,
+};
+typedef struct ImgBitmapAction {
+ enum ImgBitmapAct act;
+ const char *src; /* only used for merge */
+ QSIMPLEQ_ENTRY(ImgBitmapAction) next;
+} ImgBitmapAction;
+
+static int img_bitmap(int argc, char **argv)
+{
+ Error *err = NULL;
+ int c, ret = -1;
+ QemuOpts *opts = NULL;
+ const char *fmt = NULL, *src_fmt = NULL, *src_filename = NULL;
+ const char *filename, *bitmap;
+ BlockBackend *blk = NULL, *src = NULL;
+ BlockDriverState *bs = NULL, *src_bs = NULL;
+ bool image_opts = false;
+ int64_t granularity = 0;
+ bool add = false, merge = false;
+ QSIMPLEQ_HEAD(, ImgBitmapAction) actions;
+ ImgBitmapAction *act, *act_next;
+ const char *op;
+
+ QSIMPLEQ_INIT(&actions);
+
+ for (;;) {
+ static const struct option long_options[] = {
+ {"help", no_argument, 0, 'h'},
+ {"object", required_argument, 0, OPTION_OBJECT},
+ {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
+ {"add", no_argument, 0, OPTION_ADD},
+ {"remove", no_argument, 0, OPTION_REMOVE},
+ {"clear", no_argument, 0, OPTION_CLEAR},
+ {"enable", no_argument, 0, OPTION_ENABLE},
+ {"disable", no_argument, 0, OPTION_DISABLE},
+ {"merge", required_argument, 0, OPTION_MERGE},
+ {"granularity", required_argument, 0, 'g'},
+ {"source-file", required_argument, 0, 'b'},
+ {"source-format", required_argument, 0, 'F'},
+ {0, 0, 0, 0}
+ };
+ c = getopt_long(argc, argv, ":b:f:F:g:h", long_options, NULL);
+ if (c == -1) {
+ break;
+ }
+
+ switch (c) {
+ case ':':
+ missing_argument(argv[optind - 1]);
+ break;
+ case '?':
+ unrecognized_option(argv[optind - 1]);
+ break;
+ case 'h':
+ help();
+ break;
+ case 'b':
+ src_filename = optarg;
+ break;
+ case 'f':
+ fmt = optarg;
+ break;
+ case 'F':
+ src_fmt = optarg;
+ break;
+ case 'g':
+ granularity = cvtnum(optarg);
+ if (granularity < 0) {
+ error_report("Invalid granularity specified");
+ return 1;
+ }
+ break;
+ case OPTION_ADD:
+ act = g_new0(ImgBitmapAction, 1);
+ act->act = BITMAP_ADD;
+ QSIMPLEQ_INSERT_TAIL(&actions, act, next);
+ add = true;
+ break;
+ case OPTION_REMOVE:
+ act = g_new0(ImgBitmapAction, 1);
+ act->act = BITMAP_REMOVE;
+ QSIMPLEQ_INSERT_TAIL(&actions, act, next);
+ break;
+ case OPTION_CLEAR:
+ act = g_new0(ImgBitmapAction, 1);
+ act->act = BITMAP_CLEAR;
+ QSIMPLEQ_INSERT_TAIL(&actions, act, next);
+ break;
+ case OPTION_ENABLE:
+ act = g_new0(ImgBitmapAction, 1);
+ act->act = BITMAP_ENABLE;
+ QSIMPLEQ_INSERT_TAIL(&actions, act, next);
+ break;
+ case OPTION_DISABLE:
+ act = g_new0(ImgBitmapAction, 1);
+ act->act = BITMAP_DISABLE;
+ QSIMPLEQ_INSERT_TAIL(&actions, act, next);
+ break;
+ case OPTION_MERGE:
+ act = g_new0(ImgBitmapAction, 1);
+ act->act = BITMAP_MERGE;
+ act->src = optarg;
+ QSIMPLEQ_INSERT_TAIL(&actions, act, next);
+ merge = true;
+ break;
+ case OPTION_OBJECT:
+ opts = qemu_opts_parse_noisily(&qemu_object_opts, optarg, true);
+ if (!opts) {
+ goto out;
+ }
+ break;
+ case OPTION_IMAGE_OPTS:
+ image_opts = true;
+ break;
+ }
+ }
+
+ if (qemu_opts_foreach(&qemu_object_opts,
+ user_creatable_add_opts_foreach,
+ qemu_img_object_print_help, &error_fatal)) {
+ goto out;
+ }
+
+ if (QSIMPLEQ_EMPTY(&actions)) {
+ error_report("Need at least one of --add, --remove, --clear, "
+ "--enable, --disable, or --merge");
+ goto out;
+ }
+
+ if (granularity && !add) {
+ error_report("granularity only supported with --add");
+ goto out;
+ }
+ if (src_fmt && !src_filename) {
+ error_report("-F only supported with -b");
+ goto out;
+ }
+ if (src_filename && !merge) {
+ error_report("Merge bitmap source file only supported with "
+ "--merge");
+ goto out;
+ }
+
+ if (optind != argc - 2) {
+ error_report("Expecting filename and bitmap name");
+ goto out;
+ }
+
+ filename = argv[optind];
+ bitmap = argv[optind + 1];
+
+ blk = img_open(image_opts, filename, fmt, BDRV_O_RDWR, false, false,
+ false);
+ if (!blk) {
+ goto out;
+ }
+ bs = blk_bs(blk);
+ if (src_filename) {
+ src = img_open(NULL, src_filename, src_fmt, 0, false, false,
+ false);
+ if (!src) {
+ goto out;
+ }
+ src_bs = blk_bs(src);
+ } else {
+ src_bs = bs;
+ }
+
+ QSIMPLEQ_FOREACH_SAFE(act, &actions, next, act_next) {
+ switch (act->act) {
+ case BITMAP_ADD:
+ qmp_block_dirty_bitmap_add(bs->node_name, bitmap,
+ !!granularity, granularity, true, true,
+ false, false, &err);
+ op = "add";
+ break;
+ case BITMAP_REMOVE:
+ qmp_block_dirty_bitmap_remove(bs->node_name, bitmap, &err);
+ op = "remove";
+ break;
+ case BITMAP_CLEAR:
+ qmp_block_dirty_bitmap_clear(bs->node_name, bitmap, &err);
+ op = "clear";
+ break;
+ case BITMAP_ENABLE:
+ qmp_block_dirty_bitmap_enable(bs->node_name, bitmap, &err);
+ op = "enable";
+ break;
+ case BITMAP_DISABLE:
+ 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);
+ op = "merge";
+ break;
+ }
+ default:
+ g_assert_not_reached();
+ }
+
+ if (err) {
+ error_reportf_err(err, "Operation %s on bitmap %s failed",
+ op, bitmap);
+ ret = -1;
+ goto out;
+ }
+ g_free(act);
+ }
+
+ ret = 0;
+
+ out:
+ blk_unref(src);
+ blk_unref(blk);
+ qemu_opts_del(opts);
+ if (ret) {
+ return 1;
+ }
+ return 0;
+}
+
#define C_BS 01
#define C_COUNT 02
#define C_IF 04
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index e0886437b1f2..011688245668 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -20,6 +20,13 @@ DEF("bench", img_bench,
SRST
.. option:: bench [-c COUNT] [-d DEPTH] [-f FMT] [--flush-interval=FLUSH_INTERVAL] [-i AIO] [-n] [--no-drain] [-o OFFSET] [--pattern=PATTERN] [-q] [-s BUFFER_SIZE] [-S STEP_SIZE] [-t CACHE] [-w] [-U] FILENAME
ERST
+
+DEF("bitmap", img_bitmap,
+ "bitmap (--merge SOURCE | --add | --remove | --clear | --enable | --disable)... [-b source_file [-F source_fmt]] [-g granularity] [--object objectdef] [--image-opts | -f fmt] filename bitmap")
+SRST
+.. option:: bitmap (--merge SOURCE | --add | --remove | --clear | --enable | --disable)... [-b SOURCE_FILE [-F SOURCE_FMT]] [-g GRANULARITY] [--object OBJECTDEF] [--image-opts | -f FMT] FILENAME BITMAP
+ERST
+
DEF("check", img_check,
"check [--object objectdef] [--image-opts] [-q] [-f fmt] [--output=ofmt] [-r [leaks | all]] [-T src_cache] [-U] filename")
SRST
--
2.26.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 7/9] qcow2: Expose bitmaps' size during measure
2020-05-08 18:03 [PATCH v3 0/9] qemu-img: Add convert --bitmaps Eric Blake
` (5 preceding siblings ...)
2020-05-08 18:03 ` [PATCH v3 6/9] qemu-img: Add bitmap sub-command Eric Blake
@ 2020-05-08 18:03 ` Eric Blake
2020-05-11 11:50 ` Max Reitz
2020-05-12 10:17 ` Nir Soffer
2020-05-08 18:03 ` [PATCH v3 8/9] qemu-img: Add convert --bitmaps option Eric Blake
2020-05-08 18:03 ` [PATCH v3 9/9] iotests: Add test 291 to for qemu-img bitmap coverage Eric Blake
8 siblings, 2 replies; 30+ messages in thread
From: Eric Blake @ 2020-05-08 18:03 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, Nir Soffer, Markus Armbruster, qemu-block, mreitz
It's useful to know how much space can be occupied by qcow2 persistent
bitmaps, even though such metadata is unrelated to the guest-visible
data. Report this value as an additional field, present when
measuring an existing image and the output format supports bitmaps.
Update iotest 178 and 190 to updated output, as well as new coverage
in 190 demonstrating non-zero values made possible with the
recently-added qemu-img bitmap command.
The addition of a new field demonstrates why we should always
zero-initialize qapi C structs; while the qcow2 driver still fully
populates all fields, the raw and crypto drivers had to be tweaked to
avoid uninitialized data.
See also: https://bugzilla.redhat.com/1779904
Reported-by: Nir Soffer <nsoffer@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
qapi/block-core.json | 15 +++++++----
block/crypto.c | 2 +-
block/qcow2.c | 37 ++++++++++++++++++++++++---
block/raw-format.c | 2 +-
qemu-img.c | 3 +++
tests/qemu-iotests/178.out.qcow2 | 16 ++++++++++++
tests/qemu-iotests/190 | 43 ++++++++++++++++++++++++++++++--
tests/qemu-iotests/190.out | 23 ++++++++++++++++-
8 files changed, 128 insertions(+), 13 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 943df1926a91..9a7a388c7ad3 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -633,18 +633,23 @@
# efficiently so file size may be smaller than virtual disk size.
#
# The values are upper bounds that are guaranteed to fit the new image file.
-# Subsequent modification, such as internal snapshot or bitmap creation, may
-# require additional space and is not covered here.
+# Subsequent modification, such as internal snapshot or further bitmap
+# creation, may require additional space and is not covered here.
#
-# @required: Size required for a new image file, in bytes.
+# @required: Size required for a new image file, in bytes, when copying just
+# guest-visible contents.
#
# @fully-allocated: Image file size, in bytes, once data has been written
-# to all sectors.
+# to all sectors, when copying just guest-visible contents.
+#
+# @bitmaps: Additional size required for bitmap metadata in a source image,
+# if that bitmap metadata can be copied in addition to guest
+# contents. (since 5.1)
#
# Since: 2.10
##
{ 'struct': 'BlockMeasureInfo',
- 'data': {'required': 'int', 'fully-allocated': 'int'} }
+ 'data': {'required': 'int', 'fully-allocated': 'int', '*bitmaps': 'int'} }
##
# @query-block:
diff --git a/block/crypto.c b/block/crypto.c
index 6b21d6bf6c01..eadbcb248563 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -552,7 +552,7 @@ static BlockMeasureInfo *block_crypto_measure(QemuOpts *opts,
* Unallocated blocks are still encrypted so allocation status makes no
* difference to the file size.
*/
- info = g_new(BlockMeasureInfo, 1);
+ info = g_new0(BlockMeasureInfo, 1);
info->fully_allocated = luks_payload_size + size;
info->required = luks_payload_size + size;
return info;
diff --git a/block/qcow2.c b/block/qcow2.c
index 838d810ca5ec..f836a6047879 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4721,6 +4721,7 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
PreallocMode prealloc;
bool has_backing_file;
bool has_luks;
+ uint64_t bitmaps_size = 0; /* size occupied by bitmaps in in_bs */
/* Parse image creation options */
cluster_size = qcow2_opt_get_cluster_size_del(opts, &local_err);
@@ -4796,13 +4797,38 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
/* Account for input image */
if (in_bs) {
+ BdrvDirtyBitmap *bm;
+ size_t bitmap_dir_size = 0;
int64_t ssize = bdrv_getlength(in_bs);
+
if (ssize < 0) {
error_setg_errno(&local_err, -ssize,
"Unable to get image virtual_size");
goto err;
}
+ FOR_EACH_DIRTY_BITMAP(in_bs, bm) {
+ if (bdrv_dirty_bitmap_get_persistence(bm)) {
+ const char *name = bdrv_dirty_bitmap_name(bm);
+ uint32_t granularity = bdrv_dirty_bitmap_granularity(bm);
+ uint64_t bmbits = DIV_ROUND_UP(bdrv_dirty_bitmap_size(bm),
+ granularity);
+ uint64_t bmclusters = DIV_ROUND_UP(DIV_ROUND_UP(bmbits,
+ CHAR_BIT),
+ cluster_size);
+
+ /* Assume the entire bitmap is allocated */
+ bitmaps_size += bmclusters * cluster_size;
+ /* Also reserve space for the bitmap table entries */
+ bitmaps_size += ROUND_UP(bmclusters * sizeof(uint64_t),
+ cluster_size);
+ /* And space for contribution to bitmap directory size */
+ bitmap_dir_size += ROUND_UP(strlen(name) + 24,
+ sizeof(uint64_t));
+ }
+ }
+ bitmaps_size += ROUND_UP(bitmap_dir_size, cluster_size);
+
virtual_size = ROUND_UP(ssize, cluster_size);
if (has_backing_file) {
@@ -4849,16 +4875,21 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
required = virtual_size;
}
- info = g_new(BlockMeasureInfo, 1);
+ info = g_new0(BlockMeasureInfo, 1);
info->fully_allocated =
qcow2_calc_prealloc_size(virtual_size, cluster_size,
ctz32(refcount_bits)) + luks_payload_size;
- /* Remove data clusters that are not required. This overestimates the
+ /*
+ * Remove data clusters that are not required. This overestimates the
* required size because metadata needed for the fully allocated file is
- * still counted.
+ * still counted. Show bitmaps only if both source and destination
+ * would support them.
*/
info->required = info->fully_allocated - virtual_size + required;
+ info->has_bitmaps = version >= 3 && in_bs &&
+ bdrv_dirty_bitmap_supported(in_bs);
+ info->bitmaps = bitmaps_size;
return info;
err:
diff --git a/block/raw-format.c b/block/raw-format.c
index 9108e4369628..a134b1954ca2 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -346,7 +346,7 @@ static BlockMeasureInfo *raw_measure(QemuOpts *opts, BlockDriverState *in_bs,
BDRV_SECTOR_SIZE);
}
- info = g_new(BlockMeasureInfo, 1);
+ info = g_new0(BlockMeasureInfo, 1);
info->required = required;
/* Unallocated sectors count towards the file size in raw images */
diff --git a/qemu-img.c b/qemu-img.c
index 7ad86f7b8072..0e747247e0c5 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -5278,6 +5278,9 @@ static int img_measure(int argc, char **argv)
if (output_format == OFORMAT_HUMAN) {
printf("required size: %" PRIu64 "\n", info->required);
printf("fully allocated size: %" PRIu64 "\n", info->fully_allocated);
+ if (info->has_bitmaps) {
+ printf("bitmaps size: %" PRIu64 "\n", info->bitmaps);
+ }
} else {
dump_json_block_measure_info(info);
}
diff --git a/tests/qemu-iotests/178.out.qcow2 b/tests/qemu-iotests/178.out.qcow2
index f59bf4b2fbc4..0c6ca5e05713 100644
--- a/tests/qemu-iotests/178.out.qcow2
+++ b/tests/qemu-iotests/178.out.qcow2
@@ -37,6 +37,7 @@ qemu-img: The image size is too large (try using a larger cluster size)
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=0
required size: 196608
fully allocated size: 196608
+bitmaps size: 0
converted image file size in bytes: 196608
@@ -45,6 +46,7 @@ converted image file size in bytes: 196608
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
required size: 393216
fully allocated size: 1074135040
+bitmaps size: 0
wrote 512/512 bytes at offset 512
512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
wrote 65536/65536 bytes at offset 65536
@@ -53,6 +55,7 @@ wrote 64512/64512 bytes at offset 134217728
63 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
required size: 589824
fully allocated size: 1074135040
+bitmaps size: 0
converted image file size in bytes: 524288
@@ -60,6 +63,7 @@ converted image file size in bytes: 524288
required size: 524288
fully allocated size: 1074135040
+bitmaps size: 0
converted image file size in bytes: 458752
@@ -67,16 +71,19 @@ converted image file size in bytes: 458752
required size: 1074135040
fully allocated size: 1074135040
+bitmaps size: 0
== qcow2 input image and LUKS encryption ==
required size: 2686976
fully allocated size: 1076232192
+bitmaps size: 0
== qcow2 input image and preallocation (human) ==
required size: 1074135040
fully allocated size: 1074135040
+bitmaps size: 0
converted image file size in bytes: 1074135040
@@ -87,6 +94,7 @@ wrote 8388608/8388608 bytes at offset 0
8 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
required size: 8716288
fully allocated size: 8716288
+bitmaps size: 0
converted image file size in bytes: 8716288
@@ -173,6 +181,7 @@ qemu-img: The image size is too large (try using a larger cluster size)
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=0
{
+ "bitmaps": 0,
"required": 196608,
"fully-allocated": 196608
}
@@ -183,6 +192,7 @@ converted image file size in bytes: 196608
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
{
+ "bitmaps": 0,
"required": 393216,
"fully-allocated": 1074135040
}
@@ -193,6 +203,7 @@ wrote 65536/65536 bytes at offset 65536
wrote 64512/64512 bytes at offset 134217728
63 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
{
+ "bitmaps": 0,
"required": 589824,
"fully-allocated": 1074135040
}
@@ -202,6 +213,7 @@ converted image file size in bytes: 524288
== qcow2 input image with internal snapshot (json) ==
{
+ "bitmaps": 0,
"required": 524288,
"fully-allocated": 1074135040
}
@@ -211,6 +223,7 @@ converted image file size in bytes: 458752
== qcow2 input image and a backing file (json) ==
{
+ "bitmaps": 0,
"required": 1074135040,
"fully-allocated": 1074135040
}
@@ -218,6 +231,7 @@ converted image file size in bytes: 458752
== qcow2 input image and LUKS encryption ==
{
+ "bitmaps": 0,
"required": 2686976,
"fully-allocated": 1076232192
}
@@ -225,6 +239,7 @@ converted image file size in bytes: 458752
== qcow2 input image and preallocation (json) ==
{
+ "bitmaps": 0,
"required": 1074135040,
"fully-allocated": 1074135040
}
@@ -237,6 +252,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=8388608
wrote 8388608/8388608 bytes at offset 0
8 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
{
+ "bitmaps": 0,
"required": 8716288,
"fully-allocated": 8716288
}
diff --git a/tests/qemu-iotests/190 b/tests/qemu-iotests/190
index 6d41650438e1..1b5fff45bfcd 100755
--- a/tests/qemu-iotests/190
+++ b/tests/qemu-iotests/190
@@ -2,7 +2,7 @@
#
# qemu-img measure sub-command tests on huge qcow2 files
#
-# Copyright (C) 2017 Red Hat, Inc.
+# Copyright (C) 2017-2020 Red Hat, Inc.
#
# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
@@ -42,7 +42,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
_supported_fmt qcow2
_supported_proto file
-echo "== Huge file =="
+echo "== Huge file without bitmaps =="
echo
_make_test_img -o 'cluster_size=2M' 2T
@@ -51,6 +51,45 @@ $QEMU_IMG measure -O raw -f qcow2 "$TEST_IMG"
$QEMU_IMG measure -O qcow2 -o cluster_size=64k -f qcow2 "$TEST_IMG"
$QEMU_IMG measure -O qcow2 -o cluster_size=2M -f qcow2 "$TEST_IMG"
+echo
+echo "== Huge file with bitmaps =="
+echo
+
+$QEMU_IMG bitmap --add --granularity 512 -f qcow2 "$TEST_IMG" b1
+$QEMU_IMG bitmap --add -g 2M -f qcow2 "$TEST_IMG" b2
+
+# No bitmap output, since raw does not support it
+$QEMU_IMG measure -O raw -f qcow2 "$TEST_IMG"
+# No bitmap output, since no bitmaps on raw source
+$QEMU_IMG measure -O qcow2 -f raw "$TEST_IMG"
+# No bitmap output, since v2 does not support it
+$QEMU_IMG measure -O qcow2 -o compat=0.10 -f qcow2 "$TEST_IMG"
+
+# Compute expected output:
+echo
+val2T=$((2*1024*1024*1024*1024))
+cluster=$((64*1024))
+b1clusters=$(( (val2T/512/8 + cluster - 1) / cluster ))
+b2clusters=$(( (val2T/2/1024/1024/8 + cluster - 1) / cluster ))
+echo expected bitmap $((b1clusters * cluster +
+ (b1clusters * 8 + cluster - 1) / cluster * cluster +
+ b2clusters * cluster +
+ (b2clusters * 8 + cluster - 1) / cluster * cluster +
+ cluster))
+$QEMU_IMG measure -O qcow2 -o cluster_size=64k -f qcow2 "$TEST_IMG"
+
+# Compute expected output:
+echo
+cluster=$((2*1024*1024))
+b1clusters=$(( (val2T/512/8 + cluster - 1) / cluster ))
+b2clusters=$(( (val2T/2/1024/1024/8 + cluster - 1) / cluster ))
+echo expected bitmap $((b1clusters * cluster +
+ (b1clusters * 8 + cluster - 1) / cluster * cluster +
+ b2clusters * cluster +
+ (b2clusters * 8 + cluster - 1) / cluster * cluster +
+ cluster))
+$QEMU_IMG measure -O qcow2 -o cluster_size=2M -f qcow2 "$TEST_IMG"
+
# success, all done
echo "*** done"
rm -f $seq.full
diff --git a/tests/qemu-iotests/190.out b/tests/qemu-iotests/190.out
index d001942002db..6d5a25e9de2f 100644
--- a/tests/qemu-iotests/190.out
+++ b/tests/qemu-iotests/190.out
@@ -1,11 +1,32 @@
QA output created by 190
-== Huge file ==
+== Huge file without bitmaps ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2199023255552
required size: 2199023255552
fully allocated size: 2199023255552
required size: 335806464
fully allocated size: 2199359062016
+bitmaps size: 0
required size: 18874368
fully allocated size: 2199042129920
+bitmaps size: 0
+
+== Huge file with bitmaps ==
+
+required size: 2199023255552
+fully allocated size: 2199023255552
+required size: 7012352
+fully allocated size: 17170432
+required size: 335806464
+fully allocated size: 2199359062016
+
+expected bitmap 537198592
+required size: 335806464
+fully allocated size: 2199359062016
+bitmaps size: 537198592
+
+expected bitmap 545259520
+required size: 18874368
+fully allocated size: 2199042129920
+bitmaps size: 545259520
*** done
--
2.26.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 8/9] qemu-img: Add convert --bitmaps option
2020-05-08 18:03 [PATCH v3 0/9] qemu-img: Add convert --bitmaps Eric Blake
` (6 preceding siblings ...)
2020-05-08 18:03 ` [PATCH v3 7/9] qcow2: Expose bitmaps' size during measure Eric Blake
@ 2020-05-08 18:03 ` Eric Blake
2020-05-11 11:58 ` Max Reitz
2020-05-08 18:03 ` [PATCH v3 9/9] iotests: Add test 291 to for qemu-img bitmap coverage Eric Blake
8 siblings, 1 reply; 30+ messages in thread
From: Eric Blake @ 2020-05-08 18:03 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, 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.
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>
---
docs/tools/qemu-img.rst | 6 ++-
qemu-img.c | 81 +++++++++++++++++++++++++++++++++++++++--
qemu-img-cmds.hx | 4 +-
3 files changed, 85 insertions(+), 6 deletions(-)
diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index 68393c357386..444030861cd7 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
@@ -396,7 +400,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 0e747247e0c5..5c60d0fc8c39 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 {
@@ -191,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"
@@ -2108,6 +2110,47 @@ static int convert_do_copy(ImgConvertState *s)
return s->ret;
}
+static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst)
+{
+ BdrvDirtyBitmap *bm;
+ Error *err = NULL;
+ BlockDirtyBitmapMergeSource *merge;
+ BlockDirtyBitmapMergeSourceList *list;
+
+ 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;
+ }
+
+ merge = g_new0(BlockDirtyBitmapMergeSource, 1);
+ merge->type = QTYPE_QDICT;
+ merge->u.external.node = g_strdup(src->node_name);
+ merge->u.external.name = g_strdup(name);
+ list = g_new0(BlockDirtyBitmapMergeSourceList, 1);
+ list->value = merge;
+ qmp_block_dirty_bitmap_merge(dst->node_name, name, list, &err);
+ qapi_free_BlockDirtyBitmapMergeSourceList(list);
+ 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)
@@ -2129,6 +2172,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 */
@@ -2148,6 +2193,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",
@@ -2271,6 +2317,9 @@ static int img_convert(int argc, char **argv)
*/
s.has_zero_init = true;
break;
+ case OPTION_BITMAPS:
+ bitmaps = true;
+ break;
}
}
@@ -2332,7 +2381,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) {
@@ -2492,6 +2540,22 @@ 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;
+ }
+ 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
@@ -2500,9 +2564,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) {
@@ -2540,6 +2602,13 @@ static int img_convert(int argc, char **argv)
}
out_bs = blk_bs(s.target);
+ if (nbitmaps > 0 && !bdrv_dirty_bitmap_supported(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;
@@ -2599,6 +2668,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 011688245668..5eb9daa55dc3 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] 30+ messages in thread
* [PATCH v3 9/9] iotests: Add test 291 to for qemu-img bitmap coverage
2020-05-08 18:03 [PATCH v3 0/9] qemu-img: Add convert --bitmaps Eric Blake
` (7 preceding siblings ...)
2020-05-08 18:03 ` [PATCH v3 8/9] qemu-img: Add convert --bitmaps option Eric Blake
@ 2020-05-08 18:03 ` Eric Blake
2020-05-11 12:03 ` Max Reitz
8 siblings, 1 reply; 30+ messages in thread
From: Eric Blake @ 2020-05-08 18:03 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, 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>
---
tests/qemu-iotests/291 | 112 +++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/291.out | 78 ++++++++++++++++++++++++++
tests/qemu-iotests/group | 1 +
3 files changed, 191 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..14e5cfc96589
--- /dev/null
+++ b/tests/qemu-iotests/291.out
@@ -0,0 +1,78 @@
+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
+ 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
+ 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 fe649c5b736e..206a23292688 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -298,4 +298,5 @@
288 quick
289 rw quick
290 rw auto quick
+291 rw quick
292 rw auto quick
--
2.26.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v3 2/9] qemu-img: Fix stale comments on doc location
2020-05-08 18:03 ` [PATCH v3 2/9] qemu-img: Fix stale comments on doc location Eric Blake
@ 2020-05-11 9:05 ` Max Reitz
0 siblings, 0 replies; 30+ messages in thread
From: Max Reitz @ 2020-05-11 9:05 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: kwolf, qemu-block
[-- Attachment #1.1: Type: text/plain, Size: 285 bytes --]
On 08.05.20 20:03, Eric Blake wrote:
> Missed in commit e13c59fa.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> qemu-img.c | 2 +-
> qemu-img-cmds.hx | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 3/9] block: Make it easier to learn which BDS support bitmaps
2020-05-08 18:03 ` [PATCH v3 3/9] block: Make it easier to learn which BDS support bitmaps Eric Blake
@ 2020-05-11 9:21 ` Max Reitz
2020-05-11 18:16 ` Eric Blake
0 siblings, 1 reply; 30+ messages in thread
From: Max Reitz @ 2020-05-11 9:21 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: kwolf, Vladimir Sementsov-Ogievskiy, John Snow, qemu-block
[-- Attachment #1.1: Type: text/plain, Size: 3182 bytes --]
On 08.05.20 20:03, Eric Blake wrote:
> Upcoming patches will enhance bitmap support in qemu-img, but in doing
> so, it turns out to be nice to suppress output when bitmaps make no
> sense (such as on a qcow2 v2 image). Add a hook to make this easier
> to query.
>
> In the future, when we improve the ability to look up bitmaps through
> a filter, we will probably also want to teach the block layer to
> automatically let filters pass this request on through.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> block/qcow2.h | 1 +
> include/block/block_int.h | 1 +
> include/block/dirty-bitmap.h | 1 +
> block/dirty-bitmap.c | 9 +++++++++
> block/qcow2-bitmap.c | 7 +++++++
> block/qcow2.c | 1 +
> 6 files changed, 20 insertions(+)
>
> diff --git a/block/qcow2.h b/block/qcow2.h
> index f4de0a27d5c3..fb2b2b5a7b4d 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -764,6 +764,7 @@ bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
> int qcow2_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
> const char *name,
> Error **errp);
> +bool qcow2_dirty_bitmap_supported(BlockDriverState *bs);
>
> ssize_t coroutine_fn
> qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index df6d0273d679..cb1082da4c43 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -560,6 +560,7 @@ struct BlockDriver {
> uint64_t parent_perm, uint64_t parent_shared,
> uint64_t *nperm, uint64_t *nshared);
>
> + bool (*bdrv_dirty_bitmap_supported)(BlockDriverState *bs);
All BDSs support bitmaps, but only some support persistent dirty
bitmaps, so I think the name should reflect that.
Conceptually, this looks reasonable. This information might indeed be
nice to have, and I’m not sure whether we should extend any existing
interface to return it.
(The interfaces that come to my mind are:
(1) bdrv_can_store_new_dirty_bitmap() below, which we could make accept
a NULL @name to return basically the same information. But it’s still a
bit different, because I’d expect that function to return whether any
bitmap can be stored then, not whether the node supports bitmaps at all.
So e.g. if there are already too many bitmaps, it should return false,
even though the node itself does support bitmaps.
(2) bdrv_get_info()/BlockDriverInfo: This information would fit in very
nicely here, but do we have to put it here just because it does? I
don’t think so. This patch adds 20 lines of code, that shows that it’s
very simple to add a dedicated method, and it’s certainly a bit easier
to use than to invoke bdrv_get_info() and throw away all the other
information. Perhaps this patch only shows that BlockDriverInfo doesn’t
make much sense in the first place, and most of its fields should have
been scalar return values from dedicated functions.)
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 4/9] blockdev: Promote several bitmap functions to non-static
2020-05-08 18:03 ` [PATCH v3 4/9] blockdev: Promote several bitmap functions to non-static Eric Blake
@ 2020-05-11 9:48 ` Max Reitz
0 siblings, 0 replies; 30+ messages in thread
From: Max Reitz @ 2020-05-11 9:48 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: kwolf, Markus Armbruster, qemu-block
[-- Attachment #1.1: Type: text/plain, Size: 1631 bytes --]
On 08.05.20 20:03, Eric Blake wrote:
> The next patch will split blockdev.c, which will require accessing
> some previously-static functions from more than one .c file. But part
> of promoting a function to public is picking a naming scheme that does
> not reek of exposing too many internals (two of the three functions
> were named starting with 'do_'). To make future code motion easier,
> perform the function rename and non-static promotion into its own
> patch.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> include/block/block_int.h | 12 +++++++++++
> blockdev.c | 45 ++++++++++++++++-----------------------
> 2 files changed, 30 insertions(+), 27 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
> diff --git a/blockdev.c b/blockdev.c
> index b3c840ec0312..fbeb38437869 100644
> --- a/blockdev.c
> +++ b/blockdev.c
[...]
> @@ -2504,9 +2495,10 @@ out:
> aio_context_release(aio_context);
> }
>
> -static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
> - const char *node, const char *name, bool release,
> - BlockDriverState **bitmap_bs, Error **errp)
> +BdrvDirtyBitmap *block_dirty_bitmap_remove(const char *node, const char *name,
> + bool release,
> + BlockDriverState **bitmap_bs,
I’m not entirely sure what this parameter is even used for, but it
obviously doesn’t concern this patch.
Max
> + Error **errp)
> {
> BlockDriverState *bs;
> BdrvDirtyBitmap *bitmap;
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 5/9] blockdev: Split off basic bitmap operations for qemu-img
2020-05-08 18:03 ` [PATCH v3 5/9] blockdev: Split off basic bitmap operations for qemu-img Eric Blake
@ 2020-05-11 10:17 ` Max Reitz
0 siblings, 0 replies; 30+ messages in thread
From: Max Reitz @ 2020-05-11 10:17 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: kwolf, Vladimir Sementsov-Ogievskiy, John Snow,
Markus Armbruster, qemu-block
[-- Attachment #1.1: Type: text/plain, Size: 2016 bytes --]
On 08.05.20 20:03, Eric Blake wrote:
> Upcoming patches want to add some basic bitmap manipulation abilities
> to qemu-img. But blockdev.o is too heavyweight to link into qemu-img
> (among other things, it would drag in block jobs and transaction
> support - qemu-img does offline manipulation, where atomicity is less
> important because there are no concurrent modifications to compete
> with), so it's time to split off the bare bones of what we will need
> into a new file block/monitor/bitmap-qmp-cmds.o.
>
> This is sufficient to expose 6 QMP commands for use by qemu-img (add,
> remove, clear, enable, disable, merge), as well as move the three
> helper functions touched in the previous patch. Regarding
> MAINTAINERS, the new file is automatically part of block core, but
> also makes sense as related to other dirty bitmap files.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> Makefile.objs | 3 +-
> block/monitor/bitmap-qmp-cmds.c | 323 ++++++++++++++++++++++++++++++++
> blockdev.c | 284 ----------------------------
> MAINTAINERS | 1 +
> block/monitor/Makefile.objs | 1 +
> 5 files changed, 326 insertions(+), 286 deletions(-)
> create mode 100644 block/monitor/bitmap-qmp-cmds.c
>
> diff --git a/Makefile.objs b/Makefile.objs
> index a7c967633acf..99774cfd2545 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -13,9 +13,8 @@ chardev-obj-y = chardev/
>
> authz-obj-y = authz/
>
> -block-obj-y = nbd/
> +block-obj-y = block/ block/monitor/ nbd/ scsi/
This reads weird because it’s precisely the monitor that we don’t want
in qemu-img.
But I suppose block/monitor is the natural place for block functions
that are to be used by the monitor (and maybe other parties like
qemu-img). And the monitor itself would never be placed under block/.
So I suppose it does make sense and I have no better suggestion.
Reviewed-by: Max Reitz <mreitz@redhat.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 6/9] qemu-img: Add bitmap sub-command
2020-05-08 18:03 ` [PATCH v3 6/9] qemu-img: Add bitmap sub-command Eric Blake
@ 2020-05-11 11:10 ` Max Reitz
2020-05-11 18:22 ` Eric Blake
0 siblings, 1 reply; 30+ messages in thread
From: Max Reitz @ 2020-05-11 11:10 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: kwolf, qemu-block
[-- Attachment #1.1: Type: text/plain, Size: 2196 bytes --]
On 08.05.20 20:03, Eric Blake wrote:
> Include actions for --add, --remove, --clear, --enable, --disable, and
> --merge (note that --clear is a bit of fluff, because the same can be
> accomplished by removing a bitmap and then adding a new one in its
> place, but it matches what QMP commands exist). Listing is omitted,
> because it does not require a bitmap name and because it was already
> possible with 'qemu-img info'. A single command line can play one or
> more bitmap commands in sequence on the same bitmap name (although all
> added bitmaps share the same granularity, and and all merged bitmaps
> come from the same source file). Merge defaults to other bitmaps in
> the primary image, but can also be told to merge bitmaps from a
> distinct image.
For the record: Yes, my comment was mostly about my confusion around the
{}. So just replacing them by () would have pacified me.
But this is more fun, of course.
> While this supports --image-opts for the file being modified, I did
> not think it worth the extra complexity to support that for the source
> file in a cross-file merges. Likewise, I chose to have --merge only
> take a single source rather than following the QMP support for
> multiple merges in one go (although you can still use more than one
> --merge in the command line); in part because qemu-img is offline and
> therefore atomicity is not an issue.
>
> Upcoming patches will add iotest coverage of these commands while
> also testing other features.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> docs/tools/qemu-img.rst | 23 ++++
> qemu-img.c | 254 ++++++++++++++++++++++++++++++++++++++++
> qemu-img-cmds.hx | 7 ++
> 3 files changed, 284 insertions(+)
>
> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
> index 7d08c48d308f..68393c357386 100644
> --- a/docs/tools/qemu-img.rst
> +++ b/docs/tools/qemu-img.rst
> @@ -281,6 +281,29 @@ Command description:
[...]
> + Additional options ``-g`` set a non-default *GRANULARITY* for
sets?
With that fixed (or maybe not, you know that better than me):
Reviewed-by: Max Reitz <mreitz@redhat.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 7/9] qcow2: Expose bitmaps' size during measure
2020-05-08 18:03 ` [PATCH v3 7/9] qcow2: Expose bitmaps' size during measure Eric Blake
@ 2020-05-11 11:50 ` Max Reitz
2020-05-11 18:29 ` Eric Blake
2020-05-12 10:17 ` Nir Soffer
1 sibling, 1 reply; 30+ messages in thread
From: Max Reitz @ 2020-05-11 11:50 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: kwolf, Nir Soffer, Markus Armbruster, qemu-block
[-- Attachment #1.1: Type: text/plain, Size: 7712 bytes --]
On 08.05.20 20:03, Eric Blake wrote:
> It's useful to know how much space can be occupied by qcow2 persistent
> bitmaps, even though such metadata is unrelated to the guest-visible
> data. Report this value as an additional field, present when
> measuring an existing image and the output format supports bitmaps.
> Update iotest 178 and 190 to updated output, as well as new coverage
> in 190 demonstrating non-zero values made possible with the
> recently-added qemu-img bitmap command.
>
> The addition of a new field demonstrates why we should always
> zero-initialize qapi C structs; while the qcow2 driver still fully
> populates all fields, the raw and crypto drivers had to be tweaked to
> avoid uninitialized data.
>
> See also: https://bugzilla.redhat.com/1779904
>
> Reported-by: Nir Soffer <nsoffer@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> qapi/block-core.json | 15 +++++++----
> block/crypto.c | 2 +-
> block/qcow2.c | 37 ++++++++++++++++++++++++---
> block/raw-format.c | 2 +-
> qemu-img.c | 3 +++
> tests/qemu-iotests/178.out.qcow2 | 16 ++++++++++++
> tests/qemu-iotests/190 | 43 ++++++++++++++++++++++++++++++--
> tests/qemu-iotests/190.out | 23 ++++++++++++++++-
> 8 files changed, 128 insertions(+), 13 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 943df1926a91..9a7a388c7ad3 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -633,18 +633,23 @@
> # efficiently so file size may be smaller than virtual disk size.
> #
> # The values are upper bounds that are guaranteed to fit the new image file.
> -# Subsequent modification, such as internal snapshot or bitmap creation, may
> -# require additional space and is not covered here.
> +# Subsequent modification, such as internal snapshot or further bitmap
> +# creation, may require additional space and is not covered here.
> #
> -# @required: Size required for a new image file, in bytes.
> +# @required: Size required for a new image file, in bytes, when copying just
> +# guest-visible contents.
> #
> # @fully-allocated: Image file size, in bytes, once data has been written
> -# to all sectors.
> +# to all sectors, when copying just guest-visible contents.
> +#
> +# @bitmaps: Additional size required for bitmap metadata in a source image,
s/in/from/? Otherwise it sounds like this would be about allocation in
the source, which it clear can’t be, but, well.
> +# if that bitmap metadata can be copied in addition to guest
> +# contents. (since 5.1)
[...]
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 838d810ca5ec..f836a6047879 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
[...]
> @@ -4849,16 +4875,21 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
> required = virtual_size;
> }
>
> - info = g_new(BlockMeasureInfo, 1);
> + info = g_new0(BlockMeasureInfo, 1);
> info->fully_allocated =
> qcow2_calc_prealloc_size(virtual_size, cluster_size,
> ctz32(refcount_bits)) + luks_payload_size;
>
> - /* Remove data clusters that are not required. This overestimates the
> + /*
> + * Remove data clusters that are not required. This overestimates the
> * required size because metadata needed for the fully allocated file is
> - * still counted.
> + * still counted. Show bitmaps only if both source and destination
> + * would support them.
> */
> info->required = info->fully_allocated - virtual_size + required;
> + info->has_bitmaps = version >= 3 && in_bs &&
> + bdrv_dirty_bitmap_supported(in_bs);
Why is it important whether the source format supports persistent dirty
bitmaps?
I’m asking because I’d like there to be some concise reason when and why
the @bitmaps field appears. “Whenever the target supports bitmaps” is
more concise than “When both source and target support bitmaps”. Also,
the latter is not really different from “When any bitmap data can be
copied”, but in the latter case we should not show it when there are no
bitmaps in the source (even though the format supports them).
Or from the other perspective: As a user, I would never be annoyed by
the @bitmaps field being present. I don’t mind a “0”.
OTOH, what information can it convey to me that it it’s optional and
sometimes not present?
I can see these cases:
- That the source format doesn’t support bitmaps? I want to convert it
to something else anyway, so I don’t really care about what the source
format can or can’t do.
- That the destination doesn’t support bitmaps? Ah, yes, the fact that
the bitmap field is missing might be a warning sign for this.
- That qemu is too old to copy bitmaps? Same here.
- That there are no bitmaps in the source? OK, but then I disregard the
@bitmaps field anyway, present or not.
So from that standpoint, the best use seems to me to take “The @bitmaps
field isn’t present” as kind of a warning that something in the convert
process won’t support copying bitmaps. If it’s present, all is well.
So basically there’d be an iff relationship between “measure reports
@bitmaps” and “convert --bitmap can work”.
But the distinction between “the source format doesn’t support bitmaps”
and “the source image doesn’t have bitmaps” doesn’t seem that important
to me to make it visible in the interface.
[...]
> diff --git a/tests/qemu-iotests/190 b/tests/qemu-iotests/190
> index 6d41650438e1..1b5fff45bfcd 100755
> --- a/tests/qemu-iotests/190
> +++ b/tests/qemu-iotests/190
[...]
> @@ -51,6 +51,45 @@ $QEMU_IMG measure -O raw -f qcow2 "$TEST_IMG"
> $QEMU_IMG measure -O qcow2 -o cluster_size=64k -f qcow2 "$TEST_IMG"
> $QEMU_IMG measure -O qcow2 -o cluster_size=2M -f qcow2 "$TEST_IMG"
>
> +echo
> +echo "== Huge file with bitmaps =="
> +echo
> +
> +$QEMU_IMG bitmap --add --granularity 512 -f qcow2 "$TEST_IMG" b1
> +$QEMU_IMG bitmap --add -g 2M -f qcow2 "$TEST_IMG" b2
> +
> +# No bitmap output, since raw does not support it
> +$QEMU_IMG measure -O raw -f qcow2 "$TEST_IMG"
> +# No bitmap output, since no bitmaps on raw source
> +$QEMU_IMG measure -O qcow2 -f raw "$TEST_IMG"
> +# No bitmap output, since v2 does not support it
> +$QEMU_IMG measure -O qcow2 -o compat=0.10 -f qcow2 "$TEST_IMG"
> +
> +# Compute expected output:
> +echo
> +val2T=$((2*1024*1024*1024*1024))
> +cluster=$((64*1024))
> +b1clusters=$(( (val2T/512/8 + cluster - 1) / cluster ))
> +b2clusters=$(( (val2T/2/1024/1024/8 + cluster - 1) / cluster ))
> +echo expected bitmap $((b1clusters * cluster +
> + (b1clusters * 8 + cluster - 1) / cluster * cluster +
> + b2clusters * cluster +
> + (b2clusters * 8 + cluster - 1) / cluster * cluster +
> + cluster))
> +$QEMU_IMG measure -O qcow2 -o cluster_size=64k -f qcow2 "$TEST_IMG"
> +
> +# Compute expected output:
> +echo
> +cluster=$((2*1024*1024))
> +b1clusters=$(( (val2T/512/8 + cluster - 1) / cluster ))
> +b2clusters=$(( (val2T/2/1024/1024/8 + cluster - 1) / cluster ))
> +echo expected bitmap $((b1clusters * cluster +
> + (b1clusters * 8 + cluster - 1) / cluster * cluster +
> + b2clusters * cluster +
> + (b2clusters * 8 + cluster - 1) / cluster * cluster +
> + cluster))
> +$QEMU_IMG measure -O qcow2 -o cluster_size=2M -f qcow2 "$TEST_IMG"
Thanks!
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 8/9] qemu-img: Add convert --bitmaps option
2020-05-08 18:03 ` [PATCH v3 8/9] qemu-img: Add convert --bitmaps option Eric Blake
@ 2020-05-11 11:58 ` Max Reitz
0 siblings, 0 replies; 30+ messages in thread
From: Max Reitz @ 2020-05-11 11:58 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: kwolf, qemu-block
[-- Attachment #1.1: Type: text/plain, Size: 968 bytes --]
On 08.05.20 20:03, Eric Blake 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.
>
> 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>
> ---
> docs/tools/qemu-img.rst | 6 ++-
> qemu-img.c | 81 +++++++++++++++++++++++++++++++++++++++--
> qemu-img-cmds.hx | 4 +-
> 3 files changed, 85 insertions(+), 6 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 9/9] iotests: Add test 291 to for qemu-img bitmap coverage
2020-05-08 18:03 ` [PATCH v3 9/9] iotests: Add test 291 to for qemu-img bitmap coverage Eric Blake
@ 2020-05-11 12:03 ` Max Reitz
0 siblings, 0 replies; 30+ messages in thread
From: Max Reitz @ 2020-05-11 12:03 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: kwolf, qemu-block
[-- Attachment #1.1: Type: text/plain, Size: 594 bytes --]
On 08.05.20 20:03, Eric Blake wrote:
> 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>
> ---
> tests/qemu-iotests/291 | 112 +++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/291.out | 78 ++++++++++++++++++++++++++
> tests/qemu-iotests/group | 1 +
> 3 files changed, 191 insertions(+)
> create mode 100755 tests/qemu-iotests/291
> create mode 100644 tests/qemu-iotests/291.out
Reviewed-by: Max Reitz <mreitz@redhat.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 3/9] block: Make it easier to learn which BDS support bitmaps
2020-05-11 9:21 ` Max Reitz
@ 2020-05-11 18:16 ` Eric Blake
2020-05-11 21:21 ` Eric Blake
2020-05-12 6:19 ` Max Reitz
0 siblings, 2 replies; 30+ messages in thread
From: Eric Blake @ 2020-05-11 18:16 UTC (permalink / raw)
To: Max Reitz, qemu-devel
Cc: kwolf, Vladimir Sementsov-Ogievskiy, John Snow, qemu-block
On 5/11/20 4:21 AM, Max Reitz wrote:
> On 08.05.20 20:03, Eric Blake wrote:
>> Upcoming patches will enhance bitmap support in qemu-img, but in doing
>> so, it turns out to be nice to suppress output when bitmaps make no
>> sense (such as on a qcow2 v2 image). Add a hook to make this easier
>> to query.
>>
>> In the future, when we improve the ability to look up bitmaps through
>> a filter, we will probably also want to teach the block layer to
>> automatically let filters pass this request on through.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>> block/qcow2.h | 1 +
>> include/block/block_int.h | 1 +
>> include/block/dirty-bitmap.h | 1 +
>> block/dirty-bitmap.c | 9 +++++++++
>> block/qcow2-bitmap.c | 7 +++++++
>> block/qcow2.c | 1 +
>> 6 files changed, 20 insertions(+)
>>
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index f4de0a27d5c3..fb2b2b5a7b4d 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -764,6 +764,7 @@ bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
>> int qcow2_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>> const char *name,
>> Error **errp);
>> +bool qcow2_dirty_bitmap_supported(BlockDriverState *bs);
>>
>> ssize_t coroutine_fn
>> qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index df6d0273d679..cb1082da4c43 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -560,6 +560,7 @@ struct BlockDriver {
>> uint64_t parent_perm, uint64_t parent_shared,
>> uint64_t *nperm, uint64_t *nshared);
>>
>> + bool (*bdrv_dirty_bitmap_supported)(BlockDriverState *bs);
>
> All BDSs support bitmaps, but only some support persistent dirty
> bitmaps, so I think the name should reflect that.
How about .bdrv_dirty_bitmap_supports_persistent?
>
> Conceptually, this looks reasonable. This information might indeed be
> nice to have, and I’m not sure whether we should extend any existing
> interface to return it.
>
> (The interfaces that come to my mind are:
> (1) bdrv_can_store_new_dirty_bitmap() below, which we could make accept
> a NULL @name to return basically the same information. But it’s still a
> bit different, because I’d expect that function to return whether any
> bitmap can be stored then, not whether the node supports bitmaps at all.
> So e.g. if there are already too many bitmaps, it should return false,
> even though the node itself does support bitmaps.
[which reminds me - a while ago, we had patches for qcow2 handling with
64k bitmaps, or whatever insane number it took to overflow data
structures, and I don't know if those ever got applied...]
>
> (2) bdrv_get_info()/BlockDriverInfo: This information would fit in very
> nicely here, but do we have to put it here just because it does? I
> don’t think so. This patch adds 20 lines of code, that shows that it’s
> very simple to add a dedicated method, and it’s certainly a bit easier
> to use than to invoke bdrv_get_info() and throw away all the other
> information. Perhaps this patch only shows that BlockDriverInfo doesn’t
> make much sense in the first place, and most of its fields should have
> been scalar return values from dedicated functions.)
Indeed, you (re-)discovered some of the very reasons why I chose to make
a new interface. I could tweak the commit message to mention
alternatives, if that would help.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 6/9] qemu-img: Add bitmap sub-command
2020-05-11 11:10 ` Max Reitz
@ 2020-05-11 18:22 ` Eric Blake
2020-05-12 6:46 ` Max Reitz
0 siblings, 1 reply; 30+ messages in thread
From: Eric Blake @ 2020-05-11 18:22 UTC (permalink / raw)
To: Max Reitz, qemu-devel; +Cc: kwolf, qemu-block
On 5/11/20 6:10 AM, Max Reitz wrote:
> On 08.05.20 20:03, Eric Blake wrote:
>> Include actions for --add, --remove, --clear, --enable, --disable, and
>> --merge (note that --clear is a bit of fluff, because the same can be
>> accomplished by removing a bitmap and then adding a new one in its
>> place, but it matches what QMP commands exist). Listing is omitted,
>> because it does not require a bitmap name and because it was already
>> possible with 'qemu-img info'. A single command line can play one or
>> more bitmap commands in sequence on the same bitmap name (although all
>> added bitmaps share the same granularity, and and all merged bitmaps
>> come from the same source file). Merge defaults to other bitmaps in
>> the primary image, but can also be told to merge bitmaps from a
>> distinct image.
>
> For the record: Yes, my comment was mostly about my confusion around the
> {}. So just replacing them by () would have pacified me.
>
> But this is more fun, of course.
>
>> +++ b/docs/tools/qemu-img.rst
>> @@ -281,6 +281,29 @@ Command description:
>
> [...]
>
>> + Additional options ``-g`` set a non-default *GRANULARITY* for
>
> sets?
Or maybe:
Additional options include ``-g`` which sets a non-default *GRANULARITY*
for ``--add``, and ``-b`` and ``-F`` which select an alternative source
file for all *SOURCE* bitmaps used by ``--merge``.
And in writing this, I just realized - even though you _can_ use --add
more than once in a command line, the command is still limited to
operating on a single bitmap name, so unless you write contortions like:
qemu-img bitmap --add --remove --add -g 1024 file.qcow2 bitmapname
there will normally be at most one --add operation for a -g to be used
with (because otherwise the second --add will fail when attempting to
create an already-existing bitmap name).
>
> With that fixed (or maybe not, you know that better than me):
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 7/9] qcow2: Expose bitmaps' size during measure
2020-05-11 11:50 ` Max Reitz
@ 2020-05-11 18:29 ` Eric Blake
0 siblings, 0 replies; 30+ messages in thread
From: Eric Blake @ 2020-05-11 18:29 UTC (permalink / raw)
To: Max Reitz, qemu-devel; +Cc: kwolf, Nir Soffer, Markus Armbruster, qemu-block
On 5/11/20 6:50 AM, Max Reitz wrote:
> On 08.05.20 20:03, Eric Blake wrote:
>> It's useful to know how much space can be occupied by qcow2 persistent
>> bitmaps, even though such metadata is unrelated to the guest-visible
>> data. Report this value as an additional field, present when
>> measuring an existing image and the output format supports bitmaps.
>> Update iotest 178 and 190 to updated output, as well as new coverage
>> in 190 demonstrating non-zero values made possible with the
>> recently-added qemu-img bitmap command.
>>
>> The addition of a new field demonstrates why we should always
>> zero-initialize qapi C structs; while the qcow2 driver still fully
>> populates all fields, the raw and crypto drivers had to be tweaked to
>> avoid uninitialized data.
>>
>> See also: https://bugzilla.redhat.com/1779904
>>
>> Reported-by: Nir Soffer <nsoffer@redhat.com>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>> +#
>> +# @bitmaps: Additional size required for bitmap metadata in a source image,
>
> s/in/from/? Otherwise it sounds like this would be about allocation in
> the source, which it clear can’t be, but, well.
>
Yes, 'from' sounds nicer, especially since the size requirements being
measured depend on the destination's cluster size (which may be
different from the source's cluster size).
>> +# if that bitmap metadata can be copied in addition to guest
>> +# contents. (since 5.1)
>
> [...]
>
>> + /*
>> + * Remove data clusters that are not required. This overestimates the
>> * required size because metadata needed for the fully allocated file is
>> - * still counted.
>> + * still counted. Show bitmaps only if both source and destination
>> + * would support them.
>> */
>> info->required = info->fully_allocated - virtual_size + required;
>> + info->has_bitmaps = version >= 3 && in_bs &&
>> + bdrv_dirty_bitmap_supported(in_bs);
>
> Why is it important whether the source format supports persistent dirty
> bitmaps?
If the source format does not support bitmaps, there is nothing to copy
over. Reporting '0' would work, but adds verbosity. It also becomes a
question as to whether 'qemu-img convert --bitmaps' should silently
ignore such sources, or loudly error out that the option is unsupported
because the source lacks bitmaps. I could lean either way.
>
> I’m asking because I’d like there to be some concise reason when and why
> the @bitmaps field appears. “Whenever the target supports bitmaps” is
> more concise than “When both source and target support bitmaps”. Also,
> the latter is not really different from “When any bitmap data can be
> copied”, but in the latter case we should not show it when there are no
> bitmaps in the source (even though the format supports them).
>
> Or from the other perspective: As a user, I would never be annoyed by
> the @bitmaps field being present. I don’t mind a “0”.
> OTOH, what information can it convey to me that it it’s optional and
> sometimes not present?
The impact to the iotests .out files is larger if I do not require that
the source supports bitmaps (more lines of 'bitmaps: 0' added). I'm
fine doing that, if we decide we're okay with the simpler definition of
'"bitmaps" is present if the destination supports them' (rather than
this version's implementation of '"bitmaps" is present if both the
source and destination support them').
> I can see these cases:
>
> - That the source format doesn’t support bitmaps? I want to convert it
> to something else anyway, so I don’t really care about what the source
> format can or can’t do.
>
> - That the destination doesn’t support bitmaps? Ah, yes, the fact that
> the bitmap field is missing might be a warning sign for this.
>
> - That qemu is too old to copy bitmaps? Same here.
In fact, that argument is a GOOD reason to output 'bitmaps: 0' in as
many cases as possible, because it then becomes a side-effect witness of
whether 'qemu-img convert --bitmaps' is even understood.
>
> - That there are no bitmaps in the source? OK, but then I disregard the
> @bitmaps field anyway, present or not.
>
> So from that standpoint, the best use seems to me to take “The @bitmaps
> field isn’t present” as kind of a warning that something in the convert
> process won’t support copying bitmaps. If it’s present, all is well.
> So basically there’d be an iff relationship between “measure reports
> @bitmaps” and “convert --bitmap can work”.
Yes, I can make that tweak for v4.
>
> But the distinction between “the source format doesn’t support bitmaps”
> and “the source image doesn’t have bitmaps” doesn’t seem that important
> to me to make it visible in the interface.
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 3/9] block: Make it easier to learn which BDS support bitmaps
2020-05-11 18:16 ` Eric Blake
@ 2020-05-11 21:21 ` Eric Blake
2020-05-12 6:19 ` Max Reitz
1 sibling, 0 replies; 30+ messages in thread
From: Eric Blake @ 2020-05-11 21:21 UTC (permalink / raw)
To: Max Reitz, qemu-devel; +Cc: kwolf, Vladimir Sementsov-Ogievskiy, qemu-block
On 5/11/20 1:16 PM, Eric Blake wrote:
> On 5/11/20 4:21 AM, Max Reitz wrote:
>>> +++ b/include/block/block_int.h
>>> @@ -560,6 +560,7 @@ struct BlockDriver {
>>> uint64_t parent_perm, uint64_t
>>> parent_shared,
>>> uint64_t *nperm, uint64_t *nshared);
>>>
>>> + bool (*bdrv_dirty_bitmap_supported)(BlockDriverState *bs);
>>
>> All BDSs support bitmaps, but only some support persistent dirty
>> bitmaps, so I think the name should reflect that.
>
> How about .bdrv_dirty_bitmap_supports_persistent?
Bike-shedding myself, it looks like
.bdrv_supports_persistent_dirty_bitmap is better (if you go by the
naming convention 'noun-verb-details', it makes more sense that a 'bdrv'
supports 'persistent dirty bitmaps', than that a 'bdrv dirty bitmap'
supports 'persistence', particularly when the parameter is a
BlockDriverState rather than a BdrvDirtyBitmap).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 3/9] block: Make it easier to learn which BDS support bitmaps
2020-05-11 18:16 ` Eric Blake
2020-05-11 21:21 ` Eric Blake
@ 2020-05-12 6:19 ` Max Reitz
1 sibling, 0 replies; 30+ messages in thread
From: Max Reitz @ 2020-05-12 6:19 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: kwolf, Vladimir Sementsov-Ogievskiy, John Snow, qemu-block
[-- Attachment #1.1: Type: text/plain, Size: 4465 bytes --]
On 11.05.20 20:16, Eric Blake wrote:
> On 5/11/20 4:21 AM, Max Reitz wrote:
>> On 08.05.20 20:03, Eric Blake wrote:
>>> Upcoming patches will enhance bitmap support in qemu-img, but in doing
>>> so, it turns out to be nice to suppress output when bitmaps make no
>>> sense (such as on a qcow2 v2 image). Add a hook to make this easier
>>> to query.
>>>
>>> In the future, when we improve the ability to look up bitmaps through
>>> a filter, we will probably also want to teach the block layer to
>>> automatically let filters pass this request on through.
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>> ---
>>> block/qcow2.h | 1 +
>>> include/block/block_int.h | 1 +
>>> include/block/dirty-bitmap.h | 1 +
>>> block/dirty-bitmap.c | 9 +++++++++
>>> block/qcow2-bitmap.c | 7 +++++++
>>> block/qcow2.c | 1 +
>>> 6 files changed, 20 insertions(+)
>>>
>>> diff --git a/block/qcow2.h b/block/qcow2.h
>>> index f4de0a27d5c3..fb2b2b5a7b4d 100644
>>> --- a/block/qcow2.h
>>> +++ b/block/qcow2.h
>>> @@ -764,6 +764,7 @@ bool
>>> qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
>>> int qcow2_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>>> const char *name,
>>> Error **errp);
>>> +bool qcow2_dirty_bitmap_supported(BlockDriverState *bs);
>>>
>>> ssize_t coroutine_fn
>>> qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
>>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>>> index df6d0273d679..cb1082da4c43 100644
>>> --- a/include/block/block_int.h
>>> +++ b/include/block/block_int.h
>>> @@ -560,6 +560,7 @@ struct BlockDriver {
>>> uint64_t parent_perm, uint64_t
>>> parent_shared,
>>> uint64_t *nperm, uint64_t *nshared);
>>>
>>> + bool (*bdrv_dirty_bitmap_supported)(BlockDriverState *bs);
>>
>> All BDSs support bitmaps, but only some support persistent dirty
>> bitmaps, so I think the name should reflect that.
>
> How about .bdrv_dirty_bitmap_supports_persistent?
Sure. Or .bdrv_supports_persistent_dirty_bitmaps. Or
.bdrv_persistent_dirty_bitmaps_supported. You decide what sounds best. :)
>> Conceptually, this looks reasonable. This information might indeed be
>> nice to have, and I’m not sure whether we should extend any existing
>> interface to return it.
>>
>> (The interfaces that come to my mind are:
>> (1) bdrv_can_store_new_dirty_bitmap() below, which we could make accept
>> a NULL @name to return basically the same information. But it’s still a
>> bit different, because I’d expect that function to return whether any
>> bitmap can be stored then, not whether the node supports bitmaps at all.
>> So e.g. if there are already too many bitmaps, it should return false,
>> even though the node itself does support bitmaps.
>
> [which reminds me - a while ago, we had patches for qcow2 handling with
> 64k bitmaps, or whatever insane number it took to overflow data
> structures, and I don't know if those ever got applied...]
I think that was a1db8733d28. As far as I remember I just didn’t merge
the test, because it took like five minutes to run.
>> (2) bdrv_get_info()/BlockDriverInfo: This information would fit in very
>> nicely here, but do we have to put it here just because it does? I
>> don’t think so. This patch adds 20 lines of code, that shows that it’s
>> very simple to add a dedicated method, and it’s certainly a bit easier
>> to use than to invoke bdrv_get_info() and throw away all the other
>> information. Perhaps this patch only shows that BlockDriverInfo doesn’t
>> make much sense in the first place, and most of its fields should have
>> been scalar return values from dedicated functions.)
>
> Indeed, you (re-)discovered some of the very reasons why I chose to make
> a new interface. I could tweak the commit message to mention
> alternatives, if that would help.
I suppose it wouldn’t help me, but it can’t harm either. So if it isn’t
too difficult, why not.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 6/9] qemu-img: Add bitmap sub-command
2020-05-11 18:22 ` Eric Blake
@ 2020-05-12 6:46 ` Max Reitz
0 siblings, 0 replies; 30+ messages in thread
From: Max Reitz @ 2020-05-12 6:46 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: kwolf, qemu-block
[-- Attachment #1.1: Type: text/plain, Size: 2067 bytes --]
On 11.05.20 20:22, Eric Blake wrote:
> On 5/11/20 6:10 AM, Max Reitz wrote:
>> On 08.05.20 20:03, Eric Blake wrote:
>>> Include actions for --add, --remove, --clear, --enable, --disable, and
>>> --merge (note that --clear is a bit of fluff, because the same can be
>>> accomplished by removing a bitmap and then adding a new one in its
>>> place, but it matches what QMP commands exist). Listing is omitted,
>>> because it does not require a bitmap name and because it was already
>>> possible with 'qemu-img info'. A single command line can play one or
>>> more bitmap commands in sequence on the same bitmap name (although all
>>> added bitmaps share the same granularity, and and all merged bitmaps
>>> come from the same source file). Merge defaults to other bitmaps in
>>> the primary image, but can also be told to merge bitmaps from a
>>> distinct image.
>>
>> For the record: Yes, my comment was mostly about my confusion around the
>> {}. So just replacing them by () would have pacified me.
>>
>> But this is more fun, of course.
>>
>
>>> +++ b/docs/tools/qemu-img.rst
>>> @@ -281,6 +281,29 @@ Command description:
>>
>> [...]
>>
>>> + Additional options ``-g`` set a non-default *GRANULARITY* for
>>
>> sets?
>
> Or maybe:
>
> Additional options include ``-g`` which sets a non-default *GRANULARITY*
> for ``--add``, and ``-b`` and ``-F`` which select an alternative source
> file for all *SOURCE* bitmaps used by ``--merge``.
Sounds good.
> And in writing this, I just realized - even though you _can_ use --add
> more than once in a command line, the command is still limited to
> operating on a single bitmap name, so unless you write contortions like:
>
> qemu-img bitmap --add --remove --add -g 1024 file.qcow2 bitmapname
>
> there will normally be at most one --add operation for a -g to be used
> with (because otherwise the second --add will fail when attempting to
> create an already-existing bitmap name).
Sure, but that’s a semantic problem, not a syntactic one. :)
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 7/9] qcow2: Expose bitmaps' size during measure
2020-05-08 18:03 ` [PATCH v3 7/9] qcow2: Expose bitmaps' size during measure Eric Blake
2020-05-11 11:50 ` Max Reitz
@ 2020-05-12 10:17 ` Nir Soffer
2020-05-12 11:10 ` Max Reitz
1 sibling, 1 reply; 30+ messages in thread
From: Nir Soffer @ 2020-05-12 10:17 UTC (permalink / raw)
To: Eric Blake
Cc: Kevin Wolf, Markus Armbruster, QEMU Developers, qemu-block, Max Reitz
On Fri, May 8, 2020 at 9:03 PM Eric Blake <eblake@redhat.com> wrote:
>
> It's useful to know how much space can be occupied by qcow2 persistent
> bitmaps, even though such metadata is unrelated to the guest-visible
> data. Report this value as an additional field, present when
> measuring an existing image and the output format supports bitmaps.
> Update iotest 178 and 190 to updated output, as well as new coverage
> in 190 demonstrating non-zero values made possible with the
> recently-added qemu-img bitmap command.
>
> The addition of a new field demonstrates why we should always
> zero-initialize qapi C structs; while the qcow2 driver still fully
> populates all fields, the raw and crypto drivers had to be tweaked to
> avoid uninitialized data.
>
> See also: https://bugzilla.redhat.com/1779904
>
> Reported-by: Nir Soffer <nsoffer@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> qapi/block-core.json | 15 +++++++----
> block/crypto.c | 2 +-
> block/qcow2.c | 37 ++++++++++++++++++++++++---
> block/raw-format.c | 2 +-
> qemu-img.c | 3 +++
> tests/qemu-iotests/178.out.qcow2 | 16 ++++++++++++
> tests/qemu-iotests/190 | 43 ++++++++++++++++++++++++++++++--
> tests/qemu-iotests/190.out | 23 ++++++++++++++++-
> 8 files changed, 128 insertions(+), 13 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 943df1926a91..9a7a388c7ad3 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -633,18 +633,23 @@
> # efficiently so file size may be smaller than virtual disk size.
> #
> # The values are upper bounds that are guaranteed to fit the new image file.
> -# Subsequent modification, such as internal snapshot or bitmap creation, may
> -# require additional space and is not covered here.
> +# Subsequent modification, such as internal snapshot or further bitmap
> +# creation, may require additional space and is not covered here.
> #
> -# @required: Size required for a new image file, in bytes.
> +# @required: Size required for a new image file, in bytes, when copying just
> +# guest-visible contents.
> #
> # @fully-allocated: Image file size, in bytes, once data has been written
> -# to all sectors.
> +# to all sectors, when copying just guest-visible contents.
This does not break old code since previously we always reported only
guest visible content
here, but it changes the semantics, and now you cannot allocate
"required" size, you need
to allocate "required" size with "bitmaps" size. If we add a new
extension all users will have to
change the calculation again.
I think keeping the semantics that "required" and "fully-allocated"
are the size you need based
on the parameters of the command is easier to use and more consistent.
Current the measure
command contract is that invoking it with similar parameters as used
in convert will give
the right measurement needed for the convert command.
> +#
> +# @bitmaps: Additional size required for bitmap metadata in a source image,
> +# if that bitmap metadata can be copied in addition to guest
> +# contents. (since 5.1)
Reporting bitmaps without specifying that bitmaps are needed is less consistent
with "qemu-img convert", but has the advantage that we don't need to
check if measure
supports bitmaps. But this will work only if new versions always
report "bitmaps" even when
the value is zero.
With the current way, to measure an image we need to do:
qemu-img info --output json ...
check if image contains bitmaps
qemu-img measure --output json ...
check if bitmaps are reported.
If image has bitmaps and bitmaps are not reported, we know that we have an old
version of qemu-img that does not know how to measure bitmaps.
If bitmaps are reported in both commands we will use the value when creating
block devices.
If we always report bitmaps even when they are zero, we don't need to
run "qemu-img info".
But my preferred interface is:
qemu-img measure --bitmaps ...
And report bitmaps only if the user asked to get the value. In this
case the required and
fully-allocated values will include bitmaps.
To learn if qemu-img measure understand bitmaps we can check --help
output, like we did
in the past until we can require the version on all platforms.
An advantage is not having to change old tests.
Nir
> #
> # Since: 2.10
> ##
> { 'struct': 'BlockMeasureInfo',
> - 'data': {'required': 'int', 'fully-allocated': 'int'} }
> + 'data': {'required': 'int', 'fully-allocated': 'int', '*bitmaps': 'int'} }
>
> ##
> # @query-block:
> diff --git a/block/crypto.c b/block/crypto.c
> index 6b21d6bf6c01..eadbcb248563 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -552,7 +552,7 @@ static BlockMeasureInfo *block_crypto_measure(QemuOpts *opts,
> * Unallocated blocks are still encrypted so allocation status makes no
> * difference to the file size.
> */
> - info = g_new(BlockMeasureInfo, 1);
> + info = g_new0(BlockMeasureInfo, 1);
> info->fully_allocated = luks_payload_size + size;
> info->required = luks_payload_size + size;
> return info;
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 838d810ca5ec..f836a6047879 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -4721,6 +4721,7 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
> PreallocMode prealloc;
> bool has_backing_file;
> bool has_luks;
> + uint64_t bitmaps_size = 0; /* size occupied by bitmaps in in_bs */
>
> /* Parse image creation options */
> cluster_size = qcow2_opt_get_cluster_size_del(opts, &local_err);
> @@ -4796,13 +4797,38 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
>
> /* Account for input image */
> if (in_bs) {
> + BdrvDirtyBitmap *bm;
> + size_t bitmap_dir_size = 0;
> int64_t ssize = bdrv_getlength(in_bs);
> +
> if (ssize < 0) {
> error_setg_errno(&local_err, -ssize,
> "Unable to get image virtual_size");
> goto err;
> }
>
> + FOR_EACH_DIRTY_BITMAP(in_bs, bm) {
> + if (bdrv_dirty_bitmap_get_persistence(bm)) {
> + const char *name = bdrv_dirty_bitmap_name(bm);
> + uint32_t granularity = bdrv_dirty_bitmap_granularity(bm);
> + uint64_t bmbits = DIV_ROUND_UP(bdrv_dirty_bitmap_size(bm),
> + granularity);
> + uint64_t bmclusters = DIV_ROUND_UP(DIV_ROUND_UP(bmbits,
> + CHAR_BIT),
> + cluster_size);
> +
> + /* Assume the entire bitmap is allocated */
> + bitmaps_size += bmclusters * cluster_size;
> + /* Also reserve space for the bitmap table entries */
> + bitmaps_size += ROUND_UP(bmclusters * sizeof(uint64_t),
> + cluster_size);
> + /* And space for contribution to bitmap directory size */
> + bitmap_dir_size += ROUND_UP(strlen(name) + 24,
> + sizeof(uint64_t));
> + }
> + }
> + bitmaps_size += ROUND_UP(bitmap_dir_size, cluster_size);
> +
> virtual_size = ROUND_UP(ssize, cluster_size);
>
> if (has_backing_file) {
> @@ -4849,16 +4875,21 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
> required = virtual_size;
> }
>
> - info = g_new(BlockMeasureInfo, 1);
> + info = g_new0(BlockMeasureInfo, 1);
> info->fully_allocated =
> qcow2_calc_prealloc_size(virtual_size, cluster_size,
> ctz32(refcount_bits)) + luks_payload_size;
>
> - /* Remove data clusters that are not required. This overestimates the
> + /*
> + * Remove data clusters that are not required. This overestimates the
> * required size because metadata needed for the fully allocated file is
> - * still counted.
> + * still counted. Show bitmaps only if both source and destination
> + * would support them.
> */
> info->required = info->fully_allocated - virtual_size + required;
> + info->has_bitmaps = version >= 3 && in_bs &&
> + bdrv_dirty_bitmap_supported(in_bs);
> + info->bitmaps = bitmaps_size;
> return info;
>
> err:
> diff --git a/block/raw-format.c b/block/raw-format.c
> index 9108e4369628..a134b1954ca2 100644
> --- a/block/raw-format.c
> +++ b/block/raw-format.c
> @@ -346,7 +346,7 @@ static BlockMeasureInfo *raw_measure(QemuOpts *opts, BlockDriverState *in_bs,
> BDRV_SECTOR_SIZE);
> }
>
> - info = g_new(BlockMeasureInfo, 1);
> + info = g_new0(BlockMeasureInfo, 1);
> info->required = required;
>
> /* Unallocated sectors count towards the file size in raw images */
> diff --git a/qemu-img.c b/qemu-img.c
> index 7ad86f7b8072..0e747247e0c5 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -5278,6 +5278,9 @@ static int img_measure(int argc, char **argv)
> if (output_format == OFORMAT_HUMAN) {
> printf("required size: %" PRIu64 "\n", info->required);
> printf("fully allocated size: %" PRIu64 "\n", info->fully_allocated);
> + if (info->has_bitmaps) {
> + printf("bitmaps size: %" PRIu64 "\n", info->bitmaps);
> + }
> } else {
> dump_json_block_measure_info(info);
> }
> diff --git a/tests/qemu-iotests/178.out.qcow2 b/tests/qemu-iotests/178.out.qcow2
> index f59bf4b2fbc4..0c6ca5e05713 100644
> --- a/tests/qemu-iotests/178.out.qcow2
> +++ b/tests/qemu-iotests/178.out.qcow2
> @@ -37,6 +37,7 @@ qemu-img: The image size is too large (try using a larger cluster size)
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=0
> required size: 196608
> fully allocated size: 196608
> +bitmaps size: 0
>
> converted image file size in bytes: 196608
>
> @@ -45,6 +46,7 @@ converted image file size in bytes: 196608
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
> required size: 393216
> fully allocated size: 1074135040
> +bitmaps size: 0
> wrote 512/512 bytes at offset 512
> 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> wrote 65536/65536 bytes at offset 65536
> @@ -53,6 +55,7 @@ wrote 64512/64512 bytes at offset 134217728
> 63 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> required size: 589824
> fully allocated size: 1074135040
> +bitmaps size: 0
>
> converted image file size in bytes: 524288
>
> @@ -60,6 +63,7 @@ converted image file size in bytes: 524288
>
> required size: 524288
> fully allocated size: 1074135040
> +bitmaps size: 0
>
> converted image file size in bytes: 458752
>
> @@ -67,16 +71,19 @@ converted image file size in bytes: 458752
>
> required size: 1074135040
> fully allocated size: 1074135040
> +bitmaps size: 0
>
> == qcow2 input image and LUKS encryption ==
>
> required size: 2686976
> fully allocated size: 1076232192
> +bitmaps size: 0
>
> == qcow2 input image and preallocation (human) ==
>
> required size: 1074135040
> fully allocated size: 1074135040
> +bitmaps size: 0
>
> converted image file size in bytes: 1074135040
>
> @@ -87,6 +94,7 @@ wrote 8388608/8388608 bytes at offset 0
> 8 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> required size: 8716288
> fully allocated size: 8716288
> +bitmaps size: 0
>
> converted image file size in bytes: 8716288
>
> @@ -173,6 +181,7 @@ qemu-img: The image size is too large (try using a larger cluster size)
>
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=0
> {
> + "bitmaps": 0,
> "required": 196608,
> "fully-allocated": 196608
> }
> @@ -183,6 +192,7 @@ converted image file size in bytes: 196608
>
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
> {
> + "bitmaps": 0,
> "required": 393216,
> "fully-allocated": 1074135040
> }
> @@ -193,6 +203,7 @@ wrote 65536/65536 bytes at offset 65536
> wrote 64512/64512 bytes at offset 134217728
> 63 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> {
> + "bitmaps": 0,
> "required": 589824,
> "fully-allocated": 1074135040
> }
> @@ -202,6 +213,7 @@ converted image file size in bytes: 524288
> == qcow2 input image with internal snapshot (json) ==
>
> {
> + "bitmaps": 0,
> "required": 524288,
> "fully-allocated": 1074135040
> }
> @@ -211,6 +223,7 @@ converted image file size in bytes: 458752
> == qcow2 input image and a backing file (json) ==
>
> {
> + "bitmaps": 0,
> "required": 1074135040,
> "fully-allocated": 1074135040
> }
> @@ -218,6 +231,7 @@ converted image file size in bytes: 458752
> == qcow2 input image and LUKS encryption ==
>
> {
> + "bitmaps": 0,
> "required": 2686976,
> "fully-allocated": 1076232192
> }
> @@ -225,6 +239,7 @@ converted image file size in bytes: 458752
> == qcow2 input image and preallocation (json) ==
>
> {
> + "bitmaps": 0,
> "required": 1074135040,
> "fully-allocated": 1074135040
> }
> @@ -237,6 +252,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=8388608
> wrote 8388608/8388608 bytes at offset 0
> 8 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> {
> + "bitmaps": 0,
> "required": 8716288,
> "fully-allocated": 8716288
> }
> diff --git a/tests/qemu-iotests/190 b/tests/qemu-iotests/190
> index 6d41650438e1..1b5fff45bfcd 100755
> --- a/tests/qemu-iotests/190
> +++ b/tests/qemu-iotests/190
> @@ -2,7 +2,7 @@
> #
> # qemu-img measure sub-command tests on huge qcow2 files
> #
> -# Copyright (C) 2017 Red Hat, Inc.
> +# Copyright (C) 2017-2020 Red Hat, Inc.
> #
> # This program is free software; you can redistribute it and/or modify
> # it under the terms of the GNU General Public License as published by
> @@ -42,7 +42,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
> _supported_fmt qcow2
> _supported_proto file
>
> -echo "== Huge file =="
> +echo "== Huge file without bitmaps =="
> echo
>
> _make_test_img -o 'cluster_size=2M' 2T
> @@ -51,6 +51,45 @@ $QEMU_IMG measure -O raw -f qcow2 "$TEST_IMG"
> $QEMU_IMG measure -O qcow2 -o cluster_size=64k -f qcow2 "$TEST_IMG"
> $QEMU_IMG measure -O qcow2 -o cluster_size=2M -f qcow2 "$TEST_IMG"
>
> +echo
> +echo "== Huge file with bitmaps =="
> +echo
> +
> +$QEMU_IMG bitmap --add --granularity 512 -f qcow2 "$TEST_IMG" b1
> +$QEMU_IMG bitmap --add -g 2M -f qcow2 "$TEST_IMG" b2
> +
> +# No bitmap output, since raw does not support it
> +$QEMU_IMG measure -O raw -f qcow2 "$TEST_IMG"
> +# No bitmap output, since no bitmaps on raw source
> +$QEMU_IMG measure -O qcow2 -f raw "$TEST_IMG"
> +# No bitmap output, since v2 does not support it
> +$QEMU_IMG measure -O qcow2 -o compat=0.10 -f qcow2 "$TEST_IMG"
> +
> +# Compute expected output:
> +echo
> +val2T=$((2*1024*1024*1024*1024))
> +cluster=$((64*1024))
> +b1clusters=$(( (val2T/512/8 + cluster - 1) / cluster ))
> +b2clusters=$(( (val2T/2/1024/1024/8 + cluster - 1) / cluster ))
> +echo expected bitmap $((b1clusters * cluster +
> + (b1clusters * 8 + cluster - 1) / cluster * cluster +
> + b2clusters * cluster +
> + (b2clusters * 8 + cluster - 1) / cluster * cluster +
> + cluster))
> +$QEMU_IMG measure -O qcow2 -o cluster_size=64k -f qcow2 "$TEST_IMG"
> +
> +# Compute expected output:
> +echo
> +cluster=$((2*1024*1024))
> +b1clusters=$(( (val2T/512/8 + cluster - 1) / cluster ))
> +b2clusters=$(( (val2T/2/1024/1024/8 + cluster - 1) / cluster ))
> +echo expected bitmap $((b1clusters * cluster +
> + (b1clusters * 8 + cluster - 1) / cluster * cluster +
> + b2clusters * cluster +
> + (b2clusters * 8 + cluster - 1) / cluster * cluster +
> + cluster))
> +$QEMU_IMG measure -O qcow2 -o cluster_size=2M -f qcow2 "$TEST_IMG"
> +
> # success, all done
> echo "*** done"
> rm -f $seq.full
> diff --git a/tests/qemu-iotests/190.out b/tests/qemu-iotests/190.out
> index d001942002db..6d5a25e9de2f 100644
> --- a/tests/qemu-iotests/190.out
> +++ b/tests/qemu-iotests/190.out
> @@ -1,11 +1,32 @@
> QA output created by 190
> -== Huge file ==
> +== Huge file without bitmaps ==
>
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2199023255552
> required size: 2199023255552
> fully allocated size: 2199023255552
> required size: 335806464
> fully allocated size: 2199359062016
> +bitmaps size: 0
> required size: 18874368
> fully allocated size: 2199042129920
> +bitmaps size: 0
> +
> +== Huge file with bitmaps ==
> +
> +required size: 2199023255552
> +fully allocated size: 2199023255552
> +required size: 7012352
> +fully allocated size: 17170432
> +required size: 335806464
> +fully allocated size: 2199359062016
> +
> +expected bitmap 537198592
> +required size: 335806464
> +fully allocated size: 2199359062016
> +bitmaps size: 537198592
> +
> +expected bitmap 545259520
> +required size: 18874368
> +fully allocated size: 2199042129920
> +bitmaps size: 545259520
> *** done
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 7/9] qcow2: Expose bitmaps' size during measure
2020-05-12 10:17 ` Nir Soffer
@ 2020-05-12 11:10 ` Max Reitz
2020-05-12 19:39 ` Eric Blake
0 siblings, 1 reply; 30+ messages in thread
From: Max Reitz @ 2020-05-12 11:10 UTC (permalink / raw)
To: Nir Soffer, Eric Blake
Cc: Kevin Wolf, QEMU Developers, qemu-block, Markus Armbruster
[-- Attachment #1.1: Type: text/plain, Size: 8461 bytes --]
On 12.05.20 12:17, Nir Soffer wrote:
> On Fri, May 8, 2020 at 9:03 PM Eric Blake <eblake@redhat.com> wrote:
>>
>> It's useful to know how much space can be occupied by qcow2 persistent
>> bitmaps, even though such metadata is unrelated to the guest-visible
>> data. Report this value as an additional field, present when
>> measuring an existing image and the output format supports bitmaps.
>> Update iotest 178 and 190 to updated output, as well as new coverage
>> in 190 demonstrating non-zero values made possible with the
>> recently-added qemu-img bitmap command.
>>
>> The addition of a new field demonstrates why we should always
>> zero-initialize qapi C structs; while the qcow2 driver still fully
>> populates all fields, the raw and crypto drivers had to be tweaked to
>> avoid uninitialized data.
>>
>> See also: https://bugzilla.redhat.com/1779904
>>
>> Reported-by: Nir Soffer <nsoffer@redhat.com>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>> qapi/block-core.json | 15 +++++++----
>> block/crypto.c | 2 +-
>> block/qcow2.c | 37 ++++++++++++++++++++++++---
>> block/raw-format.c | 2 +-
>> qemu-img.c | 3 +++
>> tests/qemu-iotests/178.out.qcow2 | 16 ++++++++++++
>> tests/qemu-iotests/190 | 43 ++++++++++++++++++++++++++++++--
>> tests/qemu-iotests/190.out | 23 ++++++++++++++++-
>> 8 files changed, 128 insertions(+), 13 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 943df1926a91..9a7a388c7ad3 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -633,18 +633,23 @@
>> # efficiently so file size may be smaller than virtual disk size.
>> #
>> # The values are upper bounds that are guaranteed to fit the new image file.
>> -# Subsequent modification, such as internal snapshot or bitmap creation, may
>> -# require additional space and is not covered here.
>> +# Subsequent modification, such as internal snapshot or further bitmap
>> +# creation, may require additional space and is not covered here.
>> #
>> -# @required: Size required for a new image file, in bytes.
>> +# @required: Size required for a new image file, in bytes, when copying just
>> +# guest-visible contents.
>> #
>> # @fully-allocated: Image file size, in bytes, once data has been written
>> -# to all sectors.
>> +# to all sectors, when copying just guest-visible contents.
>
> This does not break old code since previously we always reported only
> guest visible content
> here, but it changes the semantics, and now you cannot allocate
> "required" size, you need
> to allocate "required" size with "bitmaps" size.
Only if you copy the bitmaps, though, which requires using the --bitmaps
switch for convert.
> If we add a new
> extension all users will have to
> change the calculation again.
It was my impression that existing users won’t have to do that, because
they don’t use --bitmaps yet.
In contrast, if we included the bitmap size in @required or
@fully-allocated, then previous users that didn’t copy bitmaps to the
destination (which they couldn’t) would allocate too much space.
...revisiting this after reading more of your mail: With a --bitmaps
switch, existing users wouldn’t suffer from such compatibility problems.
However, then users (that now want to copy bitmaps) will have to pass
the new --bitmaps flag in the command line, and I don’t see how that’s
less complicated than just adding @bitmaps to @required.
> I think keeping the semantics that "required" and "fully-allocated"
> are the size you need based
> on the parameters of the command is easier to use and more consistent.
> Current the measure
> command contract is that invoking it with similar parameters as used
> in convert will give
> the right measurement needed for the convert command.
>
>> +#
>> +# @bitmaps: Additional size required for bitmap metadata in a source image,
>> +# if that bitmap metadata can be copied in addition to guest
>> +# contents. (since 5.1)
>
> Reporting bitmaps without specifying that bitmaps are needed is less consistent
> with "qemu-img convert", but has the advantage that we don't need to
> check if measure
> supports bitmaps. But this will work only if new versions always
> report "bitmaps" even when
> the value is zero.
>
> With the current way, to measure an image we need to do:
>
> qemu-img info --output json ...
> check if image contains bitmaps
> qemu-img measure --output json ...
> check if bitmaps are reported.
>
> If image has bitmaps and bitmaps are not reported, we know that we have an old
> version of qemu-img that does not know how to measure bitmaps.
Well, but you’ll also see that convert --bitmaps will fail. But I can
see that you probably want to know about that before you create the
target image.
> If bitmaps are reported in both commands we will use the value when creating
> block devices.
And otherwise? Do you error out, or do you just omit --bitmaps from
convert?
> If we always report bitmaps even when they are zero, we don't need to
> run "qemu-img info".
>
> But my preferred interface is:
>
> qemu-img measure --bitmaps ...
>
> And report bitmaps only if the user asked to get the value. In this
> case the required and
> fully-allocated values will include bitmaps.
Well, it would be consistent with the convert interface. If you specify
it for one, you specify it for the other.
OTOH, this would mean having to pass around the @bitmaps bool in the
block layer, which is a bit more difficult than just adding a new field
in BlockMeasureInfo. It would also mean to add a new bool every time we
add a new extension (which you hinted at above that it might happen).
(We could also let img_measure() in qemu-img add @bitmaps to @required
if --bitmaps was given, so we wouldn’t have to pass the bool around; but
I think letting qemu-img fix up a QAPI object filled by the block driver
sounds wrong. (Because that means the block driver didn’t fill it
correctly.))
And I don’t see how the interface proposed here by Eric (or rather, what
I think we had agreed on for the next version) poses any problems for
users. If you want to copy bitmaps, you just use @required + @bitmaps.
(If @bitmaps isn’t present, you can’t copy bitmaps, so that should be
an error.) If you don’t want to copy bitmaps, you just use @required.
(And if you want to copy bitmaps if present, you use @required +
@bitmaps, and treat @bitmaps as 0 if not present.)
> To learn if qemu-img measure understand bitmaps we can check --help
> output, like we did
> in the past until we can require the version on all platforms.
>
> An advantage is not having to change old tests.
I personally don’t really consider that a significant advantage... (On
the contrary, seeing the field in all old tests means the code path is
exercised more often, even though it’s supposed to always just report 0.)
So all in all the main benefit I see in your proposal, i.e. having
@bitmaps be included in @required with --bitmaps given, is that it would
give a symmetrical interface between measure and convert: For simple
cases, you can just replace the “convert” in your command line by
“measure”, retrieve @required/@fully-allocated, create the target image
based on that, and then re-run the same command line, but with “convert”
this time.
But I’m not sure whether that’s really an advantage in practice or more
of a gimmick. With Eric’s proposal, if you want to convert with
bitmaps, just add @bitmaps to the target size. If you don’t, don’t. If
you’d prefer to but don’t really care, add “@bitmaps ?: 0”.
The benefit of Eric’s proposal (not including @bitmaps in @required or
@fully-allocated) is that it doesn’t require passing an additional
parameter to the block driver. It also makes the definition of
BlockMeasureInfo simpler. With your proposal, it would need to be
parameterized. (As in, @required sometimes includes the bitmaps,
sometimes it doesn’t, depending on the parameter used to retrieve
BlockMeasureInfo.) I’m not sure whether that even makes sense in the
QAPI definition.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 7/9] qcow2: Expose bitmaps' size during measure
2020-05-12 11:10 ` Max Reitz
@ 2020-05-12 19:39 ` Eric Blake
2020-05-12 20:35 ` Nir Soffer
2020-05-13 8:53 ` Max Reitz
0 siblings, 2 replies; 30+ messages in thread
From: Eric Blake @ 2020-05-12 19:39 UTC (permalink / raw)
To: Max Reitz, Nir Soffer
Cc: Kevin Wolf, QEMU Developers, qemu-block, Markus Armbruster
On 5/12/20 6:10 AM, Max Reitz wrote:
>> This does not break old code since previously we always reported only
>> guest visible content
>> here, but it changes the semantics, and now you cannot allocate
>> "required" size, you need
>> to allocate "required" size with "bitmaps" size.
>
> Only if you copy the bitmaps, though, which requires using the --bitmaps
> switch for convert.
First, a usage question: would you rather that 'qemu-img convert
--bitmaps' silently succeeds even when the image being converted has no
bitmaps, or would you rather that it fails loudly when there are no
bitmaps to be copied over? As implemented in this patch series, patch 8
currently silently succeeds. But in order to make patch 7 and 8
consistent with one another, I need to know which behavior is easier to
use: failing convert if the source lacks bitmaps (and thus measure would
suppress the bitmaps:0 output), or treating lack of bitmaps as nothing
additional to copy and thereby succeeding (and thus measure should
output bitmaps:0 to show that no additional space is needed because
nothing else will be copied, successfully).
>> If we add a new
>> extension all users will have to
>> change the calculation again.
>
> It was my impression that existing users won’t have to do that, because
> they don’t use --bitmaps yet.
>
> In contrast, if we included the bitmap size in @required or
> @fully-allocated, then previous users that didn’t copy bitmaps to the
> destination (which they couldn’t) would allocate too much space.
>
> ...revisiting this after reading more of your mail: With a --bitmaps
> switch, existing users wouldn’t suffer from such compatibility problems.
> However, then users (that now want to copy bitmaps) will have to pass
> the new --bitmaps flag in the command line, and I don’t see how that’s
> less complicated than just adding @bitmaps to @required.
More concretely, suppose down the road that we add the ability to copy
internal snapshots (not that you want to mix internal and external
snapshots, but that such information already exists and therefore can be
used as an example). Which is easier:
$ qemu-img measure -f qcow2 image.qcow2
required size: 8716288
fully allocated size: 8716288
bitmaps size: 1024
internal snapshot size: 2048
where you now have to add three numbers prior to creating dest.qcow2 and
calling:
$ qemu-img convert image.qcow2 -f dest.qcow2 --bitmaps --snapshots
or using:
$ qemu-img measure --bitmaps --snapshots -f qcow2 image.qcow2
required size: 8719360
fully allocated size: 8719360
where you then call:
$ qemu-img convert image.qcow2 -f dest.qcow2 --bitmaps --snapshots
with a single size that matches the same arguments you pass to qemu-img
convert? What about failure cases? What happens when qemu-img doesn't
understand --snapshots but does understand --bitmaps? Do you have to
try a series of up to three calls to find how much information is supported:
$ qemu-img measure -f qcow2 image.qcow2 --bitmaps --snapshots
error: unknown argument
$ qemu-img measure -f qcow2 image.qcow2 --bitmaps
error: unknown argument
$ qemu-img measure -f qcow2 image.qcow2
data given, now you know that neither --bitmaps nor --snapshots will work
or is it nicer to issue just one measure without options, getting
separate output lines, and seeing which output lines exist to know which
convert options are supported, at the minor expense of having to add
values yourself?
And then back to my question: should 'measure --bitmaps' fail if there
are no bitmaps to be measured, or silently succeed and not change the
output size?
>> With the current way, to measure an image we need to do:
>>
>> qemu-img info --output json ...
>> check if image contains bitmaps
>> qemu-img measure --output json ...
>> check if bitmaps are reported.
Why do you have to check 'qemu-img info' first? If measure reports
bitmaps, then you know bitmaps can be copied; if it doesn't, then you
can check info as a fallback path to compute size yourself - but
computing the size yourself doesn't help unless you also have fallbacks
to copy the bitmaps via QMP commands, because that qemu-img will also
lack 'qemu-img convert --bitmaps' or 'qemu-img bitmaps' to do it via
qemu-img.
>>
>> If image has bitmaps and bitmaps are not reported, we know that we have an old
>> version of qemu-img that does not know how to measure bitmaps.
>
> Well, but you’ll also see that convert --bitmaps will fail. But I can
> see that you probably want to know about that before you create the
> target image.
>
>> If bitmaps are reported in both commands we will use the value when creating
>> block devices.
>
> And otherwise? Do you error out, or do you just omit --bitmaps from
> convert?
>
>> If we always report bitmaps even when they are zero, we don't need to
>> run "qemu-img info".
>>
>> But my preferred interface is:
>>
>> qemu-img measure --bitmaps ...
>>
>> And report bitmaps only if the user asked to get the value. In this
>> case the required and
>> fully-allocated values will include bitmaps.
>
> Well, it would be consistent with the convert interface. If you specify
> it for one, you specify it for the other.
>
> OTOH, this would mean having to pass around the @bitmaps bool in the
> block layer, which is a bit more difficult than just adding a new field
> in BlockMeasureInfo. It would also mean to add a new bool every time we
> add a new extension (which you hinted at above that it might happen).
Or, that could be a CLI-only feature: the QMP interface _always_ reports
bitmaps separately, but if 'qemu-img measure --bitmaps' is used, the CLI
then adds that value in on your behalf after the QMP command but before
printing to the end user.
>
> (We could also let img_measure() in qemu-img add @bitmaps to @required
> if --bitmaps was given, so we wouldn’t have to pass the bool around; but
> I think letting qemu-img fix up a QAPI object filled by the block driver
> sounds wrong. (Because that means the block driver didn’t fill it
> correctly.))
If we only touch it up in the CLI, then we would have two forms of CLI
output:
$ qemu-img measure -f qcow2 image.qcow2
required size: 8716288
fully allocated size: 8716288
bitmaps size: 1024
$ qemu-img measure -f qcow2 image.qcow2 --bitmaps
required size: 8717312
fully allocated size: 8717312
>
> And I don’t see how the interface proposed here by Eric (or rather, what
> I think we had agreed on for the next version) poses any problems for
> users. If you want to copy bitmaps, you just use @required + @bitmaps.
> (If @bitmaps isn’t present, you can’t copy bitmaps, so that should be
> an error.) If you don’t want to copy bitmaps, you just use @required.
>
> (And if you want to copy bitmaps if present, you use @required +
> @bitmaps, and treat @bitmaps as 0 if not present.)
>
>> To learn if qemu-img measure understand bitmaps we can check --help
>> output, like we did
>> in the past until we can require the version on all platforms.
>>
>> An advantage is not having to change old tests.
> I personally don’t really consider that a significant advantage... (On
> the contrary, seeing the field in all old tests means the code path is
> exercised more often, even though it’s supposed to always just report 0.)
>
> So all in all the main benefit I see in your proposal, i.e. having
> @bitmaps be included in @required with --bitmaps given, is that it would
> give a symmetrical interface between measure and convert: For simple
> cases, you can just replace the “convert” in your command line by
> “measure”, retrieve @required/@fully-allocated, create the target image
> based on that, and then re-run the same command line, but with “convert”
> this time.
>
> But I’m not sure whether that’s really an advantage in practice or more
> of a gimmick. With Eric’s proposal, if you want to convert with
> bitmaps, just add @bitmaps to the target size. If you don’t, don’t. If
> you’d prefer to but don’t really care, add “@bitmaps ?: 0”.
>
> The benefit of Eric’s proposal (not including @bitmaps in @required or
> @fully-allocated) is that it doesn’t require passing an additional
> parameter to the block driver. It also makes the definition of
> BlockMeasureInfo simpler. With your proposal, it would need to be
> parameterized. (As in, @required sometimes includes the bitmaps,
> sometimes it doesn’t, depending on the parameter used to retrieve
> BlockMeasureInfo.) I’m not sure whether that even makes sense in the
> QAPI definition.
I'm leaning towards making v4 try a CLI-only 'measure --bitmaps', to see
if I can speed the discussion along with concrete patches for comparison.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 7/9] qcow2: Expose bitmaps' size during measure
2020-05-12 19:39 ` Eric Blake
@ 2020-05-12 20:35 ` Nir Soffer
2020-05-12 21:04 ` Eric Blake
2020-05-13 8:53 ` Max Reitz
1 sibling, 1 reply; 30+ messages in thread
From: Nir Soffer @ 2020-05-12 20:35 UTC (permalink / raw)
To: Eric Blake
Cc: Kevin Wolf, Markus Armbruster, QEMU Developers, qemu-block, Max Reitz
On Tue, May 12, 2020 at 10:39 PM Eric Blake <eblake@redhat.com> wrote:
>
> On 5/12/20 6:10 AM, Max Reitz wrote:
>
>
> >> This does not break old code since previously we always reported only
> >> guest visible content
> >> here, but it changes the semantics, and now you cannot allocate
> >> "required" size, you need
> >> to allocate "required" size with "bitmaps" size.
> >
> > Only if you copy the bitmaps, though, which requires using the --bitmaps
> > switch for convert.
>
> First, a usage question: would you rather that 'qemu-img convert
> --bitmaps' silently succeeds even when the image being converted has no
> bitmaps, or would you rather that it fails loudly when there are no
> bitmaps to be copied over?
I think the meaning of --bitmaps should be "copy also bitmaps".
This request makes sense only for qcow2 images, since other images do
not have bitmaps, so failing loudly when the source format does not support
bitmaps seems right.
Same for the target format does not support bitmaps, this is invalid request
and it should fail loudly.
If the source and target are qcow2, and there are no bitmaps in source, I don't
see any reason to fail. We don't want to check if an image has bitmaps before
we copy the image, it does not help us.
> As implemented in this patch series, patch 8
> currently silently succeeds.
Sounds good for qcow2 format.
> But in order to make patch 7 and 8
> consistent with one another, I need to know which behavior is easier to
> use: failing convert if the source lacks bitmaps (and thus measure would
> suppress the bitmaps:0 output), or treating lack of bitmaps as nothing
> additional to copy and thereby succeeding (and thus measure should
> output bitmaps:0 to show that no additional space is needed because
> nothing else will be copied, successfully).
I don't think showing "bitmaps: 0" in measure is related to how --bitmaps
behave in convert. If we will have --bitmaps in measure, we don't need to
show "bitmaps" at all since "required" will include it.
If we want to show bitmaps in measure, I think using the same logic is fine:
- if format does not support bitmaps - fail
- if format suppots bitmaps, show what we have - zero is valid result when
image does not have any bitmap.
> >> If we add a new
> >> extension all users will have to
> >> change the calculation again.
> >
> > It was my impression that existing users won’t have to do that, because
> > they don’t use --bitmaps yet.
> >
> > In contrast, if we included the bitmap size in @required or
> > @fully-allocated, then previous users that didn’t copy bitmaps to the
> > destination (which they couldn’t) would allocate too much space.
> >
> > ...revisiting this after reading more of your mail: With a --bitmaps
> > switch, existing users wouldn’t suffer from such compatibility problems.
> > However, then users (that now want to copy bitmaps) will have to pass
> > the new --bitmaps flag in the command line, and I don’t see how that’s
> > less complicated than just adding @bitmaps to @required.
>
> More concretely, suppose down the road that we add the ability to copy
> internal snapshots (not that you want to mix internal and external
> snapshots, but that such information already exists and therefore can be
> used as an example). Which is easier:
>
> $ qemu-img measure -f qcow2 image.qcow2
> required size: 8716288
> fully allocated size: 8716288
> bitmaps size: 1024
> internal snapshot size: 2048
>
> where you now have to add three numbers prior to creating dest.qcow2 and
> calling:
>
> $ qemu-img convert image.qcow2 -f dest.qcow2 --bitmaps --snapshots
>
> or using:
>
> $ qemu-img measure --bitmaps --snapshots -f qcow2 image.qcow2
> required size: 8719360
> fully allocated size: 8719360
>
> where you then call:
>
> $ qemu-img convert image.qcow2 -f dest.qcow2 --bitmaps --snapshots
>
> with a single size that matches the same arguments you pass to qemu-img
> convert?
Yes, the second form is a good example of using --bitmaps consistently.
> What about failure cases? What happens when qemu-img doesn't
> understand --snapshots but does understand --bitmaps? Do you have to
> try a series of up to three calls to find how much information is supported:
>
> $ qemu-img measure -f qcow2 image.qcow2 --bitmaps --snapshots
> error: unknown argument
> $ qemu-img measure -f qcow2 image.qcow2 --bitmaps
> error: unknown argument
> $ qemu-img measure -f qcow2 image.qcow2
> data given, now you know that neither --bitmaps nor --snapshots will work
Assuming that I cannot require a version that support both features, which is
usually the case when we have to support different platforms with
different versions
of qemu-img, I will check the capabilities using qemu-img --help and cache
the result. For vdsm case we control the host so qemu-img should not be upgraded
behind vdsm back.
Then I will use the supported features. If both are missing, this
convert will drop the
the bitmaps and the snaphosts, and the next incremetnal backup will
fail or fallback to
full backup.
> or is it nicer to issue just one measure without options, getting
> separate output lines, and seeing which output lines exist to know which
> convert options are supported, at the minor expense of having to add
> values yourself?
It is a little nicer since we don't need to check the capabilities,
but we need to
check them anyway for qemu-img convert, so this does not help much.
But it introduces other issues:
- we need to calculate the required size using required + bitmap or
required + bitmaps + snapshots
- what if measuring a snapshot is expensive - let's say take 20
seconds. This is still fast enough
if the entire copy takes several minutes, so having a way to measure
is useful. But users that do not
care about the snapshot have to pay for this one every call. So we
would end with a --snapshot flag
to avoid this, and inconsistent API.
> And then back to my question: should 'measure --bitmaps' fail if there
> are no bitmaps to be measured, or silently succeed and not change the
> output size?
For raw file yes (invalid request), for qcow2 file no, it should just
add 0 since this is the actual
size required for bitmaps in this image.
> >> With the current way, to measure an image we need to do:
> >>
> >> qemu-img info --output json ...
> >> check if image contains bitmaps
> >> qemu-img measure --output json ...
> >> check if bitmaps are reported.
>
> Why do you have to check 'qemu-img info' first? If measure reports
> bitmaps, then you know bitmaps can be copied;
This works only if qemu-img measure will report "bitmaps": 0 when there
are no bitmaps. Otherwise I don't know if this version does not report bitmaps
because it does not understand them, or because there are no bitmaps.
Using qemu-img info I can tell the difference if measure does not report 0.
> if it doesn't, then you
> can check info as a fallback path to compute size yourself - but
> computing the size yourself doesn't help unless you also have fallbacks
> to copy the bitmaps via QMP commands, because that qemu-img will also
> lack 'qemu-img convert --bitmaps' or 'qemu-img bitmaps' to do it via
> qemu-img.
When we started to work on this it was not clear that we will have a
way to measure
bitmaps. If we are going to have support both in convert and measure,
we can check
capability only in convert or only in measure.
> >> If image has bitmaps and bitmaps are not reported, we know that we have an old
> >> version of qemu-img that does not know how to measure bitmaps.
> >
> > Well, but you’ll also see that convert --bitmaps will fail. But I can
> > see that you probably want to know about that before you create the
> > target image.
> >
> >> If bitmaps are reported in both commands we will use the value when creating
> >> block devices.
> >
> > And otherwise? Do you error out, or do you just omit --bitmaps from
> > convert?
> >
> >> If we always report bitmaps even when they are zero, we don't need to
> >> run "qemu-img info".
> >>
> >> But my preferred interface is:
> >>
> >> qemu-img measure --bitmaps ...
> >>
> >> And report bitmaps only if the user asked to get the value. In this
> >> case the required and
> >> fully-allocated values will include bitmaps.
> >
> > Well, it would be consistent with the convert interface. If you specify
> > it for one, you specify it for the other.
> >
> > OTOH, this would mean having to pass around the @bitmaps bool in the
> > block layer, which is a bit more difficult than just adding a new field
> > in BlockMeasureInfo. It would also mean to add a new bool every time we
> > add a new extension (which you hinted at above that it might happen).
>
> Or, that could be a CLI-only feature: the QMP interface _always_ reports
> bitmaps separately, but if 'qemu-img measure --bitmaps' is used, the CLI
> then adds that value in on your behalf after the QMP command but before
> printing to the end user.
>
> >
> > (We could also let img_measure() in qemu-img add @bitmaps to @required
> > if --bitmaps was given, so we wouldn’t have to pass the bool around; but
> > I think letting qemu-img fix up a QAPI object filled by the block driver
> > sounds wrong. (Because that means the block driver didn’t fill it
> > correctly.))
>
> If we only touch it up in the CLI, then we would have two forms of CLI
> output:
>
> $ qemu-img measure -f qcow2 image.qcow2
> required size: 8716288
> fully allocated size: 8716288
> bitmaps size: 1024
> $ qemu-img measure -f qcow2 image.qcow2 --bitmaps
> required size: 8717312
> fully allocated size: 8717312
I hope we will not have 2 forms. qemu-img is complicated enough ;-)
> > And I don’t see how the interface proposed here by Eric (or rather, what
> > I think we had agreed on for the next version) poses any problems for
> > users. If you want to copy bitmaps, you just use @required + @bitmaps.
> > (If @bitmaps isn’t present, you can’t copy bitmaps, so that should be
> > an error.) If you don’t want to copy bitmaps, you just use @required.
> >
> > (And if you want to copy bitmaps if present, you use @required +
> > @bitmaps, and treat @bitmaps as 0 if not present.)
> >
> >> To learn if qemu-img measure understand bitmaps we can check --help
> >> output, like we did
> >> in the past until we can require the version on all platforms.
> >>
> >> An advantage is not having to change old tests.
> > I personally don’t really consider that a significant advantage... (On
> > the contrary, seeing the field in all old tests means the code path is
> > exercised more often, even though it’s supposed to always just report 0.)
> >
> > So all in all the main benefit I see in your proposal, i.e. having
> > @bitmaps be included in @required with --bitmaps given, is that it would
> > give a symmetrical interface between measure and convert: For simple
> > cases, you can just replace the “convert” in your command line by
> > “measure”, retrieve @required/@fully-allocated, create the target image
> > based on that, and then re-run the same command line, but with “convert”
> > this time.
> >
> > But I’m not sure whether that’s really an advantage in practice or more
> > of a gimmick. With Eric’s proposal, if you want to convert with
> > bitmaps, just add @bitmaps to the target size. If you don’t, don’t. If
> > you’d prefer to but don’t really care, add “@bitmaps ?: 0”.
> >
> > The benefit of Eric’s proposal (not including @bitmaps in @required or
> > @fully-allocated) is that it doesn’t require passing an additional
> > parameter to the block driver. It also makes the definition of
> > BlockMeasureInfo simpler. With your proposal, it would need to be
> > parameterized. (As in, @required sometimes includes the bitmaps,
> > sometimes it doesn’t, depending on the parameter used to retrieve
> > BlockMeasureInfo.) I’m not sure whether that even makes sense in the
> > QAPI definition.
>
> I'm leaning towards making v4 try a CLI-only 'measure --bitmaps', to see
> if I can speed the discussion along with concrete patches for comparison.
Thanks, that would be useful.
Nir
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 7/9] qcow2: Expose bitmaps' size during measure
2020-05-12 20:35 ` Nir Soffer
@ 2020-05-12 21:04 ` Eric Blake
0 siblings, 0 replies; 30+ messages in thread
From: Eric Blake @ 2020-05-12 21:04 UTC (permalink / raw)
To: Nir Soffer
Cc: Kevin Wolf, Markus Armbruster, QEMU Developers, qemu-block, Max Reitz
On 5/12/20 3:35 PM, Nir Soffer wrote:
>> First, a usage question: would you rather that 'qemu-img convert
>> --bitmaps' silently succeeds even when the image being converted has no
>> bitmaps, or would you rather that it fails loudly when there are no
>> bitmaps to be copied over?
>
> I think the meaning of --bitmaps should be "copy also bitmaps".
>
> This request makes sense only for qcow2 images, since other images do
> not have bitmaps, so failing loudly when the source format does not support
> bitmaps seems right.
>
> Same for the target format does not support bitmaps, this is invalid request
> and it should fail loudly.
>
> If the source and target are qcow2, and there are no bitmaps in source, I don't
> see any reason to fail. We don't want to check if an image has bitmaps before
> we copy the image, it does not help us.
>
>> As implemented in this patch series, patch 8
>> currently silently succeeds.
>
> Sounds good for qcow2 format.
So, to make sure I'm clear, you'll be happy with something like:
- no source
$ qemu-img measure --bitmaps -O qcow2 1G
error: --bitmaps requires source file
- src cannot support bitmaps
$ qemu-img measure --bitmaps -f raw -O qcow2 file.raw
error: raw source does not support bitmaps
$ qemu-img measure -f raw -O qcow2 file.raw
success, totals are unchanged (since there are no bitmaps)
$ qemu-img convert --bitmaps -f raw -O qcow2 file.raw file.qcow2
error: raw source does not support bitmaps
- dest cannot support bitmaps
$ qemu-img measure --bitmaps -f qcow2 -O raw file.qcow2
error: raw destination does not support bitmaps
$ qemu-img measure -f qcow2 -O raw file.qcow2
success, totals are unchanged (since there are no bitmaps)
$ qemu-img convert --bitmaps -f qcow2 -O raw file.qcow2 file.raw
error: raw destination does not support bitmaps
- another way dest cannot support bitmaps
$ qemu-img convert --bitmaps -f qcow2 -O qcow2 -o compat=0.10 \
src.qcow2 dst.qcow2
error: v2 qcow2 destination does not support bitmaps
- src and dest support bitmaps, but there are none
$ qemu-img measure --bitmaps -f qcow2 -O qcow2 file.qcow2
success, no modification to totals since no bitmaps to copy
$ qemu-img measure -f qcow2 -O qcow2 file.qcow2
success, separate line item for 'bitmap size: 0'
$ qemu-img convert --bitmaps -f qcow2 -O qcow2 src.qcow2 dst.qcow2
success, even though no bitmaps to copy
- src and dest support bitmaps, and there are some
$ qemu-img measure --bitmaps -f qcow2 -O qcow2 file.qcow2
success, totals modified in place to include bitmaps without separate line
$ qemu-img measure -f qcow2 -O qcow2 file.qcow2
success, separate line item added for 'bitmap size: NNN'
$ qemu-img convert --bitmaps -f qcow2 -O qcow2 src.qcow2 dst.qcow2
success, and bitmaps were copied
>
>> But in order to make patch 7 and 8
>> consistent with one another, I need to know which behavior is easier to
>> use: failing convert if the source lacks bitmaps (and thus measure would
>> suppress the bitmaps:0 output), or treating lack of bitmaps as nothing
>> additional to copy and thereby succeeding (and thus measure should
>> output bitmaps:0 to show that no additional space is needed because
>> nothing else will be copied, successfully).
>
> I don't think showing "bitmaps: 0" in measure is related to how --bitmaps
> behave in convert. If we will have --bitmaps in measure, we don't need to
> show "bitmaps" at all since "required" will include it.
Showing 'bitmaps: 0' in the QMP is how the CLI will know whether it was
an error to request measuring with bitmaps.
>
> If we want to show bitmaps in measure, I think using the same logic is fine:
> - if format does not support bitmaps - fail
> - if format suppots bitmaps, show what we have - zero is valid result when
> image does not have any bitmap.
More precisely, fail in QMP if either the source being measured cannot
support bitmaps at all, or if the destination that determines the
measurements cannot support bitmaps. But if both support bitmaps, then
the QMP will output 0 if there happen to be no bitmaps to copy, and the
CLI can add 'measure --bitmaps' to silently fold in a present bitmaps
number (be it 0 or non-zero) into the other fields, or error if QMP did
not include a bitmaps field.
>> And then back to my question: should 'measure --bitmaps' fail if there
>> are no bitmaps to be measured, or silently succeed and not change the
>> output size?
>
> For raw file yes (invalid request), for qcow2 file no, it should just
> add 0 since this is the actual
> size required for bitmaps in this image.
Okay, that is slightly different than what Max was describing (which was
to output 'bitmaps: 0' in QMP even if the source is raw), but easy
enough to be consistent on, and as this is your feature request, I can
make v4 along those lines.
>
>>>> With the current way, to measure an image we need to do:
>>>>
>>>> qemu-img info --output json ...
>>>> check if image contains bitmaps
>>>> qemu-img measure --output json ...
>>>> check if bitmaps are reported.
>>
>> Why do you have to check 'qemu-img info' first? If measure reports
>> bitmaps, then you know bitmaps can be copied;
>
> This works only if qemu-img measure will report "bitmaps": 0 when there
> are no bitmaps. Otherwise I don't know if this version does not report bitmaps
> because it does not understand them, or because there are no bitmaps.
>
> Using qemu-img info I can tell the difference if measure does not report 0.
But similarly, with the above rules, 'qemu-img measure --bitmaps' will
either fail because qemu is too old, or fail because the source format
cannot support bitmaps to be copied; and succeed if 'qemu-img convert
--bitmaps' will also succeed. All without having to check 'qemu-img
info' to see if there were bitmaps.
>
>> if it doesn't, then you
>> can check info as a fallback path to compute size yourself - but
>> computing the size yourself doesn't help unless you also have fallbacks
>> to copy the bitmaps via QMP commands, because that qemu-img will also
>> lack 'qemu-img convert --bitmaps' or 'qemu-img bitmaps' to do it via
>> qemu-img.
>
> When we started to work on this it was not clear that we will have a
> way to measure
> bitmaps. If we are going to have support both in convert and measure,
> we can check
> capability only in convert or only in measure.
Yes, the point of this series is that it is an all-or-none improvement
to qemu-img: all of the bitmap manipulations you are wanting will be
included at the same time in qemu 5.1, and likely backported at the same
time by whatever downstream distros that want backports included.
Existing qemu-img 5.0 cannot manipulate bitmaps in any reasonable
manner, at most it has 'qemu-img info' that tells you that bitmaps
exist, but you're still stuck with a non-qemu-img fallback if there is
no qemu-img support for manipulating bitmaps at all (including the
fallback of just declaring incremental backups unsupported, and using
full backups instead).
>>> (We could also let img_measure() in qemu-img add @bitmaps to @required
>>> if --bitmaps was given, so we wouldn’t have to pass the bool around; but
>>> I think letting qemu-img fix up a QAPI object filled by the block driver
>>> sounds wrong. (Because that means the block driver didn’t fill it
>>> correctly.))
>>
>> If we only touch it up in the CLI, then we would have two forms of CLI
>> output:
>>
>> $ qemu-img measure -f qcow2 image.qcow2
>> required size: 8716288
>> fully allocated size: 8716288
>> bitmaps size: 1024
>> $ qemu-img measure -f qcow2 image.qcow2 --bitmaps
>> required size: 8717312
>> fully allocated size: 8717312
>
> I hope we will not have 2 forms. qemu-img is complicated enough ;-)
2 forms is the easiest way to add it. And it is not that hard to
explain. Either you pass --bitmaps up front (no extra lines, but
success only if measure saw bitmap support, even if the bitmaps occupy 0
spze), or you don't pass --bitmaps up front (an extra bitmaps: line if
bitmaps are supported, even if the size is 0, and no change to existing
lines). Just because 2 forms exist does not mean you are required to
use both forms.
>> I'm leaning towards making v4 try a CLI-only 'measure --bitmaps', to see
>> if I can speed the discussion along with concrete patches for comparison.
>
> Thanks, that would be useful.
Okay, that's the 2-form approach.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 7/9] qcow2: Expose bitmaps' size during measure
2020-05-12 19:39 ` Eric Blake
2020-05-12 20:35 ` Nir Soffer
@ 2020-05-13 8:53 ` Max Reitz
1 sibling, 0 replies; 30+ messages in thread
From: Max Reitz @ 2020-05-13 8:53 UTC (permalink / raw)
To: Eric Blake, Nir Soffer
Cc: Kevin Wolf, QEMU Developers, qemu-block, Markus Armbruster
[-- Attachment #1.1: Type: text/plain, Size: 8114 bytes --]
On 12.05.20 21:39, Eric Blake wrote:
> On 5/12/20 6:10 AM, Max Reitz wrote:
>
>
>>> This does not break old code since previously we always reported only
>>> guest visible content
>>> here, but it changes the semantics, and now you cannot allocate
>>> "required" size, you need
>>> to allocate "required" size with "bitmaps" size.
>>
>> Only if you copy the bitmaps, though, which requires using the --bitmaps
>> switch for convert.
>
> First, a usage question: would you rather that 'qemu-img convert
> --bitmaps' silently succeeds even when the image being converted has no
> bitmaps, or would you rather that it fails loudly when there are no
> bitmaps to be copied over? As implemented in this patch series, patch 8
> currently silently succeeds. But in order to make patch 7 and 8
> consistent with one another, I need to know which behavior is easier to
> use: failing convert if the source lacks bitmaps (and thus measure would
> suppress the bitmaps:0 output), or treating lack of bitmaps as nothing
> additional to copy and thereby succeeding (and thus measure should
> output bitmaps:0 to show that no additional space is needed because
> nothing else will be copied, successfully).
“Easier to use” in my opinion would be if it never fails, but the
question is whether that’s also the most useful approach.
I think it should match what I thought when @bitmaps should be reported,
i.e. it should be accepted whenever the target format (and qemu-img
convert, obviously) supports bitmaps. I don’t think there’s value in
aborting just because the source doesn’t have bitmaps. What’s the user
going to do in response? “Oops, thanks, qemu-img, I accidentally tried
to convert the wrong image”?
(Whereas if we reject --bitmaps because the target format doesn’t
support it, there’s actually value, because the user is reminded that
they should choose a different target format.)
>>> If we add a new
>>> extension all users will have to
>>> change the calculation again.
>>
>> It was my impression that existing users won’t have to do that, because
>> they don’t use --bitmaps yet.
>>
>> In contrast, if we included the bitmap size in @required or
>> @fully-allocated, then previous users that didn’t copy bitmaps to the
>> destination (which they couldn’t) would allocate too much space.
>>
>> ...revisiting this after reading more of your mail: With a --bitmaps
>> switch, existing users wouldn’t suffer from such compatibility problems.
>> However, then users (that now want to copy bitmaps) will have to pass
>> the new --bitmaps flag in the command line, and I don’t see how that’s
>> less complicated than just adding @bitmaps to @required.
>
> More concretely, suppose down the road that we add the ability to copy
> internal snapshots (not that you want to mix internal and external
> snapshots, but that such information already exists and therefore can be
> used as an example). Which is easier:
>
> $ qemu-img measure -f qcow2 image.qcow2
> required size: 8716288
> fully allocated size: 8716288
> bitmaps size: 1024
> internal snapshot size: 2048
>
> where you now have to add three numbers prior to creating dest.qcow2and
> calling:
>
> $ qemu-img convert image.qcow2 -f dest.qcow2 --bitmaps --snapshots
>
> or using:
>
> $ qemu-img measure --bitmaps --snapshots -f qcow2 image.qcow2
> required size: 8719360
> fully allocated size: 8719360
>
> where you then call:
>
> $ qemu-img convert image.qcow2 -f dest.qcow2 --bitmaps --snapshots
“Which is easier” is misleading, because both are actually trivial.
Adding three numbers is what a computer does best.
It’s a question of style, not of “which is easier”.
> with a single size that matches the same arguments you pass to qemu-img
> convert? What about failure cases? What happens when qemu-img doesn't
> understand --snapshots but does understand --bitmaps? Do you have to
> try a series of up to three calls to find how much information is
> supported:
>
> $ qemu-img measure -f qcow2 image.qcow2 --bitmaps --snapshots
> error: unknown argument
> $ qemu-img measure -f qcow2 image.qcow2 --bitmaps
> error: unknown argument
> $ qemu-img measure -f qcow2 image.qcow2
> data given, now you know that neither --bitmaps nor --snapshots will work
>
> or is it nicer to issue just one measure without options, getting
> separate output lines, and seeing which output lines exist to know which
> convert options are supported, at the minor expense of having to add
> values yourself?
Sounds good to me, but then again, calling a command to see whether it
fails isn’t exactly hard either.
So it remains a question of style to me.
From my perspective, measure is a query function similar to info. It
should return all information that’s trivial to obtain. Switches to
select what information to return how only make sense when that optional
information is hard to obtain (think qemu-img info --backing-chain).
Switches make sense on data-modifying operations like convert. Not so
much for querying operations, because why not just return all
information you have and let the caller sort it out?
[...]
>>> If we always report bitmaps even when they are zero, we don't need to
>>> run "qemu-img info".
>>>
>>> But my preferred interface is:
>>>
>>> qemu-img measure --bitmaps ...
>>>
>>> And report bitmaps only if the user asked to get the value. In this
>>> case the required and
>>> fully-allocated values will include bitmaps.
>>
>> Well, it would be consistent with the convert interface. If you specify
>> it for one, you specify it for the other.
>>
>> OTOH, this would mean having to pass around the @bitmaps bool in the
>> block layer, which is a bit more difficult than just adding a new field
>> in BlockMeasureInfo. It would also mean to add a new bool every time we
>> add a new extension (which you hinted at above that it might happen).
>
> Or, that could be a CLI-only feature: the QMP interface _always_ reports
> bitmaps separately, but if 'qemu-img measure --bitmaps' is used, the CLI
> then adds that value in on your behalf after the QMP command but before
> printing to the end user.
>
>>
>> (We could also let img_measure() in qemu-img add @bitmaps to @required
>> if --bitmaps was given, so we wouldn’t have to pass the bool around; but
>> I think letting qemu-img fix up a QAPI object filled by the block driver
>> sounds wrong. (Because that means the block driver didn’t fill it
>> correctly.))
>
> If we only touch it up in the CLI, then we would have two forms of CLI
> output:
>
> $ qemu-img measure -f qcow2 image.qcow2
> required size: 8716288
> fully allocated size: 8716288
> bitmaps size: 1024
> $ qemu-img measure -f qcow2 image.qcow2 --bitmaps
> required size: 8717312
> fully allocated size: 8717312
I would hope that programs consuming qemu-img measure’s output use
--output=json, and then you definitely can’t just ignore the QAPI object
definition.
Furthermore, I don’t like that 2-form at all, for two reasons. First,
again, I can’t see a reasonable QAPI definition to match it; and second,
I don’t like it as a consistent interface. We should decide on one
thing and do that. Either you need to pass --bitmaps to make them
included in required, or they aren’t in the output at all.
The way you propose it with this 2-form approach makes --bitmap make
qemu-img be a calculator. But why? It’s absolutely trivial for
consumers to:
irb(main):002:1` result = JSON.parse(`./qemu-img measure --output=json \
irb(main):003:0> -O qcow2 src.qcow2`)
irb(main):004:0* result['required'] +
irb(main):005:0> (result['bitmaps'] ? result['bitmaps'] : 0)
=> 524288
(Example in Ruby, but I don’t think that matters much.)
There is no value in qemu-img optionally performing that operation for
the caller. Adding two integers is the simplest thing there is.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2020-05-13 8:53 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-08 18:03 [PATCH v3 0/9] qemu-img: Add convert --bitmaps Eric Blake
2020-05-08 18:03 ` [PATCH v3 1/9] docs: Sort sections on qemu-img subcommand parameters Eric Blake
2020-05-08 18:03 ` [PATCH v3 2/9] qemu-img: Fix stale comments on doc location Eric Blake
2020-05-11 9:05 ` Max Reitz
2020-05-08 18:03 ` [PATCH v3 3/9] block: Make it easier to learn which BDS support bitmaps Eric Blake
2020-05-11 9:21 ` Max Reitz
2020-05-11 18:16 ` Eric Blake
2020-05-11 21:21 ` Eric Blake
2020-05-12 6:19 ` Max Reitz
2020-05-08 18:03 ` [PATCH v3 4/9] blockdev: Promote several bitmap functions to non-static Eric Blake
2020-05-11 9:48 ` Max Reitz
2020-05-08 18:03 ` [PATCH v3 5/9] blockdev: Split off basic bitmap operations for qemu-img Eric Blake
2020-05-11 10:17 ` Max Reitz
2020-05-08 18:03 ` [PATCH v3 6/9] qemu-img: Add bitmap sub-command Eric Blake
2020-05-11 11:10 ` Max Reitz
2020-05-11 18:22 ` Eric Blake
2020-05-12 6:46 ` Max Reitz
2020-05-08 18:03 ` [PATCH v3 7/9] qcow2: Expose bitmaps' size during measure Eric Blake
2020-05-11 11:50 ` Max Reitz
2020-05-11 18:29 ` Eric Blake
2020-05-12 10:17 ` Nir Soffer
2020-05-12 11:10 ` Max Reitz
2020-05-12 19:39 ` Eric Blake
2020-05-12 20:35 ` Nir Soffer
2020-05-12 21:04 ` Eric Blake
2020-05-13 8:53 ` Max Reitz
2020-05-08 18:03 ` [PATCH v3 8/9] qemu-img: Add convert --bitmaps option Eric Blake
2020-05-11 11:58 ` Max Reitz
2020-05-08 18:03 ` [PATCH v3 9/9] iotests: Add test 291 to for qemu-img bitmap coverage Eric Blake
2020-05-11 12:03 ` Max Reitz
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.