All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 0/9] qemu-img: Add convert --bitmaps
@ 2020-05-13  1:16 Eric Blake
  2020-05-13  1:16 ` [PATCH v4 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-13  1:16 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, nsoffer, qemu-block, mreitz

v3 was here:
https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg02139.html
original cover letter here:
https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg03464.html

Since then:
- add R-b where appropriate
- patch 3: rename new driver hook [Max]
- patch 6: typo and grammar fix [Max]
- patch 7: add 'measure --bitmaps' convenience option [Nir]
- patch 7: more documentation, enhance test to cover more cases
- patch 8: rebase to earlier changes, ensure that both 'measure --bitmaps'
and 'convert --bitmaps' pass or fail on the same criteria

001/9:[----] [--] 'docs: Sort sections on qemu-img subcommand parameters'
002/9:[----] [--] 'qemu-img: Fix stale comments on doc location'
003/9:[0017] [FC] 'block: Make it easier to learn which BDS support bitmaps'
004/9:[----] [--] 'blockdev: Promote several bitmap functions to non-static'
005/9:[----] [--] 'blockdev: Split off basic bitmap operations for qemu-img'
006/9:[0009] [FC] 'qemu-img: Add bitmap sub-command'
007/9:[0084] [FC] 'qcow2: Expose bitmaps' size during measure'
008/9:[0008] [FC] 'qemu-img: Add convert --bitmaps option'
009/9:[----] [--] '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-v4

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          |  88 +++++---
 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                    |  39 +++-
 block/raw-format.c               |   2 +-
 blockdev.c                       | 303 +------------------------
 qemu-img.c                       | 366 ++++++++++++++++++++++++++++++-
 MAINTAINERS                      |   1 +
 block/monitor/Makefile.objs      |   1 +
 qemu-img-cmds.hx                 |  17 +-
 tests/qemu-iotests/178.out.qcow2 |  16 ++
 tests/qemu-iotests/190           |  57 ++++-
 tests/qemu-iotests/190.out       |  35 ++-
 tests/qemu-iotests/291           | 112 ++++++++++
 tests/qemu-iotests/291.out       |  78 +++++++
 tests/qemu-iotests/group         |   1 +
 23 files changed, 1142 insertions(+), 348 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 v4 1/9] docs: Sort sections on qemu-img subcommand parameters
  2020-05-13  1:16 [PATCH v4 0/9] qemu-img: Add convert --bitmaps Eric Blake
@ 2020-05-13  1:16 ` Eric Blake
  2020-05-14  5:02   ` Vladimir Sementsov-Ogievskiy
  2020-05-13  1:16 ` [PATCH v4 2/9] qemu-img: Fix stale comments on doc location Eric Blake
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 30+ messages in thread
From: Eric Blake @ 2020-05-13  1:16 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, nsoffer, 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 v4 2/9] qemu-img: Fix stale comments on doc location
  2020-05-13  1:16 [PATCH v4 0/9] qemu-img: Add convert --bitmaps Eric Blake
  2020-05-13  1:16 ` [PATCH v4 1/9] docs: Sort sections on qemu-img subcommand parameters Eric Blake
@ 2020-05-13  1:16 ` Eric Blake
  2020-05-14  5:06   ` Vladimir Sementsov-Ogievskiy
  2020-05-13  1:16 ` [PATCH v4 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-13  1:16 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, nsoffer, qemu-block, mreitz

Missed in commit e13c59fa.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@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 v4 3/9] block: Make it easier to learn which BDS support bitmaps
  2020-05-13  1:16 [PATCH v4 0/9] qemu-img: Add convert --bitmaps Eric Blake
  2020-05-13  1:16 ` [PATCH v4 1/9] docs: Sort sections on qemu-img subcommand parameters Eric Blake
  2020-05-13  1:16 ` [PATCH v4 2/9] qemu-img: Fix stale comments on doc location Eric Blake
@ 2020-05-13  1:16 ` Eric Blake
  2020-05-14  5:19   ` Vladimir Sementsov-Ogievskiy
  2020-05-13  1:16 ` [PATCH v4 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-13  1:16 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, Vladimir Sementsov-Ogievskiy, qemu-block, mreitz, nsoffer,
	John Snow

Upcoming patches will enhance bitmap support in qemu-img, but in doing
so, it turns out to be nice to suppress output when persistent bitmaps
make no sense (such as on a qcow2 v2 image).  Add a hook to make this
easier to query.

This patch adds a new callback .bdrv_supports_persistent_dirty_bitmap,
rather than trying to shoehorn the answer in via existing callbacks.
In particular, while it might have been possible to overload
.bdrv_co_can_store_new_dirty_bitmap to special-case a NULL input to
answer whether any persistent bitmaps are supported, that is at odds
with whether a particular bitmap can be stored (for example, even on
an image that supports persistent bitmaps but has currently filled up
the maximum number of bitmaps, attempts to store another one should
fail); and the new functionality doesn't require coroutine safety.
Similarly, we could have added one more piece of information to
.bdrv_get_info, but then again, most callers to that function tend to
already discard extraneous information, and making it a catch-all
rather than a series of dedicated scalar queries hasn't really
simplified life.

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                | 2 ++
 6 files changed, 21 insertions(+)

diff --git a/block/qcow2.h b/block/qcow2.h
index f4de0a27d5c3..c94beb7f8716 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_supports_persistent_dirty_bitmap(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..5bcd6aa39f6c 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_supports_persistent_dirty_bitmap)(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..5a8d52e4deaf 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_supports_persistent_dirty_bitmap(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..f9bfc77985e8 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_supports_persistent_dirty_bitmap(BlockDriverState *bs)
+{
+    if (bs->drv && bs->drv->bdrv_supports_persistent_dirty_bitmap) {
+        return bs->drv->bdrv_supports_persistent_dirty_bitmap(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..1cf6d2ab77a3 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_supports_persistent_dirty_bitmap(BlockDriverState *bs)
+{
+    BDRVQcow2State *s = bs->opaque;
+
+    return s->qcow_version >= 3;
+}
diff --git a/block/qcow2.c b/block/qcow2.c
index 1ad95ff04851..1c8f3ab8ae68 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -5663,6 +5663,8 @@ BlockDriver bdrv_qcow2 = {
     .bdrv_detach_aio_context  = qcow2_detach_aio_context,
     .bdrv_attach_aio_context  = qcow2_attach_aio_context,

+    .bdrv_supports_persistent_dirty_bitmap =
+            qcow2_supports_persistent_dirty_bitmap,
     .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 v4 4/9] blockdev: Promote several bitmap functions to non-static
  2020-05-13  1:16 [PATCH v4 0/9] qemu-img: Add convert --bitmaps Eric Blake
                   ` (2 preceding siblings ...)
  2020-05-13  1:16 ` [PATCH v4 3/9] block: Make it easier to learn which BDS support bitmaps Eric Blake
@ 2020-05-13  1:16 ` Eric Blake
  2020-05-14  5:45   ` Vladimir Sementsov-Ogievskiy
  2020-05-14 11:45   ` Vladimir Sementsov-Ogievskiy
  2020-05-13  1:16 ` [PATCH v4 5/9] blockdev: Split off basic bitmap operations for qemu-img Eric Blake
                   ` (4 subsequent siblings)
  8 siblings, 2 replies; 30+ messages in thread
From: Eric Blake @ 2020-05-13  1:16 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, nsoffer, 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>
Reviewed-by: Max Reitz <mreitz@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 5bcd6aa39f6c..cd85899ea2b5 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 v4 5/9] blockdev: Split off basic bitmap operations for qemu-img
  2020-05-13  1:16 [PATCH v4 0/9] qemu-img: Add convert --bitmaps Eric Blake
                   ` (3 preceding siblings ...)
  2020-05-13  1:16 ` [PATCH v4 4/9] blockdev: Promote several bitmap functions to non-static Eric Blake
@ 2020-05-13  1:16 ` Eric Blake
  2020-05-14  6:21   ` Vladimir Sementsov-Ogievskiy
  2020-05-13  1:16 ` [PATCH v4 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-13  1:16 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, Vladimir Sementsov-Ogievskiy, qemu-block,
	Markus Armbruster, mreitz, nsoffer, 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>
Reviewed-by: Max Reitz <mreitz@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 v4 6/9] qemu-img: Add bitmap sub-command
  2020-05-13  1:16 [PATCH v4 0/9] qemu-img: Add convert --bitmaps Eric Blake
                   ` (4 preceding siblings ...)
  2020-05-13  1:16 ` [PATCH v4 5/9] blockdev: Split off basic bitmap operations for qemu-img Eric Blake
@ 2020-05-13  1:16 ` Eric Blake
  2020-05-14  6:45   ` Vladimir Sementsov-Ogievskiy
  2020-05-18 11:42   ` Vladimir Sementsov-Ogievskiy
  2020-05-13  1:16 ` [PATCH v4 7/9] qcow2: Expose bitmaps' size during measure Eric Blake
                   ` (2 subsequent siblings)
  8 siblings, 2 replies; 30+ messages in thread
From: Eric Blake @ 2020-05-13  1:16 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, nsoffer, 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>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 docs/tools/qemu-img.rst |  24 ++++
 qemu-img.c              | 254 ++++++++++++++++++++++++++++++++++++++++
 qemu-img-cmds.hx        |   7 ++
 3 files changed, 285 insertions(+)

diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index 7d08c48d308f..219483cec279 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -281,6 +281,30 @@ 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 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``.
+
+  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..8c99e68ba8aa 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', '--disable',\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 v4 7/9] qcow2: Expose bitmaps' size during measure
  2020-05-13  1:16 [PATCH v4 0/9] qemu-img: Add convert --bitmaps Eric Blake
                   ` (5 preceding siblings ...)
  2020-05-13  1:16 ` [PATCH v4 6/9] qemu-img: Add bitmap sub-command Eric Blake
@ 2020-05-13  1:16 ` Eric Blake
  2020-05-18 13:07   ` Vladimir Sementsov-Ogievskiy
  2020-05-13  1:16 ` [PATCH v4 8/9] qemu-img: Add convert --bitmaps option Eric Blake
  2020-05-13  1:16 ` [PATCH v4 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-13  1:16 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, nsoffer, 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 QMP field, present when
measuring an existing image and output format that both support
bitmaps.  Update iotest 178 and 190 to updated output, as well as new
coverage in 190 demonstrating non-zero values made possible with the
recently-added qemu-img bitmap command.

On the command-line side, 'qemu-img measure' gains a new --bitmaps
flag.  When present, the bitmap size is rolled into the two existing
measures (or errors if either the source image or destination format
lacks bitmaps); when absent, there is never an error (for
back-compat), but the output will instead include a new line item for
bitmaps (which you would have to manually add), with that line being
omitted in the same cases where passing --bitmaps would error.

The behavior chosen here is symmetrical with the upcoming 'qemu-img
convert --bitmaps' being added in the next patch: that is, either both
commands will succeed (your qemu-img was new enough to do bitmap
manipulations, AND you correctly measured and copied the bitmaps, even
if that measurement was 0 because there was nothing to copy) or both
fail (either your qemu-img is too old to understand --bitmaps, or it
understands it but your choice of images do not support seamless
transition of bitmaps because either source, destination, or both lack
bitmap support).

The addition of a new field demonstrates why we should always
zero-initialize qapi C structs; while the qcow2 driver still fully
populates all fields, the raw and crypto drivers had to be tweaked to
avoid uninitialized data.

See also: https://bugzilla.redhat.com/1779904

Reported-by: Nir Soffer <nsoffer@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 docs/tools/qemu-img.rst          | 10 +++++-
 qapi/block-core.json             | 15 ++++++---
 block/crypto.c                   |  2 +-
 block/qcow2.c                    | 37 +++++++++++++++++++--
 block/raw-format.c               |  2 +-
 qemu-img.c                       | 25 ++++++++++++++
 qemu-img-cmds.hx                 |  4 +--
 tests/qemu-iotests/178.out.qcow2 | 16 +++++++++
 tests/qemu-iotests/190           | 57 ++++++++++++++++++++++++++++++--
 tests/qemu-iotests/190.out       | 35 +++++++++++++++++++-
 10 files changed, 187 insertions(+), 16 deletions(-)

diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index 219483cec279..b6f87ec6d3c0 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -593,7 +593,7 @@ Command description:
   For more information, consult ``include/block/block.h`` in QEMU's
   source code.

-.. option:: measure [--output=OFMT] [-O OUTPUT_FMT] [-o OPTIONS] [--size N | [--object OBJECTDEF] [--image-opts] [-f FMT] [-l SNAPSHOT_PARAM] FILENAME]
+.. option:: measure [--output=OFMT] [-O OUTPUT_FMT] [-o OPTIONS] [--size N | [--object OBJECTDEF] [--image-opts] [-f FMT] [--bitmaps] [-l SNAPSHOT_PARAM] FILENAME]

   Calculate the file size required for a new image.  This information
   can be used to size logical volumes or SAN LUNs appropriately for
@@ -616,6 +616,7 @@ Command description:

     required size: 524288
     fully allocated size: 1074069504
+    bitmaps: 0

   The ``required size`` is the file size of the new image.  It may be smaller
   than the virtual disk size if the image format supports compact representation.
@@ -625,6 +626,13 @@ Command description:
   occupy with the exception of internal snapshots, dirty bitmaps, vmstate data,
   and other advanced image format features.

+  The ``bitmaps size`` is the additional size required if the
+  destination supports persistent bitmaps, in order to additionally
+  copy bitmaps in addition to the guest-visible data.  If the
+  ``--bitmaps`` option was in use, the bitmap size is folded into the
+  required and fully-allocated size for convenience, rather than being
+  a separate line item.
+
 .. option:: snapshot [--object OBJECTDEF] [--image-opts] [-U] [-q] [-l | -a SNAPSHOT | -c SNAPSHOT | -d SNAPSHOT] FILENAME

   List, apply, create or delete snapshots in image *FILENAME*.
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 943df1926a91..65280eb9847d 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 if all the top-level bitmap metadata in
+#           the source image were to be copied to the destination, present
+#           when the destination supports persistent bitmaps. (since 5.1)
 #
 # Since: 2.10
 ##
 { 'struct': 'BlockMeasureInfo',
-  'data': {'required': 'int', 'fully-allocated': 'int'} }
+  'data': {'required': 'int', 'fully-allocated': 'int', '*bitmaps': 'int'} }

 ##
 # @query-block:
diff --git a/block/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 1c8f3ab8ae68..be0950016365 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_supports_persistent_dirty_bitmap(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 8c99e68ba8aa..0a326993d7ac 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 {
@@ -5096,6 +5097,7 @@ static int img_measure(int argc, char **argv)
         {"output", required_argument, 0, OPTION_OUTPUT},
         {"size", required_argument, 0, OPTION_SIZE},
         {"force-share", no_argument, 0, 'U'},
+        {"bitmaps", no_argument, 0, OPTION_BITMAPS},
         {0, 0, 0, 0}
     };
     OutputFormat output_format = OFORMAT_HUMAN;
@@ -5112,6 +5114,7 @@ static int img_measure(int argc, char **argv)
     QemuOpts *sn_opts = NULL;
     QemuOptsList *create_opts = NULL;
     bool image_opts = false;
+    bool bitmaps = false;
     uint64_t img_size = UINT64_MAX;
     BlockMeasureInfo *info = NULL;
     Error *local_err = NULL;
@@ -5192,6 +5195,9 @@ static int img_measure(int argc, char **argv)
             img_size = (uint64_t)sval;
         }
         break;
+        case OPTION_BITMAPS:
+            bitmaps = true;
+            break;
         }
     }

@@ -5220,6 +5226,10 @@ static int img_measure(int argc, char **argv)
         error_report("Either --size N or one filename must be specified.");
         goto out;
     }
+    if (!filename && bitmaps) {
+        error_report("--bitmaps is only supported with a filename.");
+        goto out;
+    }

     if (filename) {
         in_blk = img_open(image_opts, filename, fmt, 0,
@@ -5275,9 +5285,24 @@ static int img_measure(int argc, char **argv)
         goto out;
     }

+    if (bitmaps) {
+        if (!info->has_bitmaps) {
+            error_report("no bitmaps measured, either source or destination "
+                         "format lacks bitmap support");
+            goto out;
+        } else {
+            info->required += info->bitmaps;
+            info->fully_allocated += info->bitmaps;
+            info->has_bitmaps = false;
+        }
+    }
+
     if (output_format == OFORMAT_HUMAN) {
         printf("required size: %" PRIu64 "\n", info->required);
         printf("fully allocated size: %" PRIu64 "\n", info->fully_allocated);
+        if (info->has_bitmaps) {
+            printf("bitmaps size: %" PRIu64 "\n", info->bitmaps);
+        }
     } else {
         dump_json_block_measure_info(info);
     }
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 011688245668..bfcd9f32dddf 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -76,9 +76,9 @@ SRST
 ERST

 DEF("measure", img_measure,
-"measure [--output=ofmt] [-O output_fmt] [-o options] [--size N | [--object objectdef] [--image-opts] [-f fmt] [-l snapshot_param] filename]")
+"measure [--output=ofmt] [-O output_fmt] [-o options] [--size N | [--object objectdef] [--image-opts] [-f fmt] [--bitmaps] [-l snapshot_param] filename]")
 SRST
-.. option:: measure [--output=OFMT] [-O OUTPUT_FMT] [-o OPTIONS] [--size N | [--object OBJECTDEF] [--image-opts] [-f FMT] [-l SNAPSHOT_PARAM] FILENAME]
+.. option:: measure [--output=OFMT] [-O OUTPUT_FMT] [-o OPTIONS] [--size N | [--object OBJECTDEF] [--image-opts] [-f FMT] [--bitmaps] [-l SNAPSHOT_PARAM] FILENAME]
 ERST

 DEF("snapshot", img_snapshot,
diff --git a/tests/qemu-iotests/178.out.qcow2 b/tests/qemu-iotests/178.out.qcow2
index 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..4e71e29f2df3 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,59 @@ $QEMU_IMG measure -O raw -f qcow2 "$TEST_IMG"
 $QEMU_IMG measure -O qcow2 -o cluster_size=64k -f qcow2 "$TEST_IMG"
 $QEMU_IMG measure -O qcow2 -o cluster_size=2M -f qcow2 "$TEST_IMG"

+echo
+echo "== Huge file with bitmaps =="
+echo
+
+$QEMU_IMG bitmap --add --granularity 512 -f qcow2 "$TEST_IMG" b1
+$QEMU_IMG bitmap --add -g 2M -f qcow2 "$TEST_IMG" b2
+
+# No bitmap without a source
+$QEMU_IMG measure --bitmaps -O qcow2 --size 10M
+# No bitmap output, since raw does not support it
+$QEMU_IMG measure -O raw -f qcow2 "$TEST_IMG" ||
+    echo "unexpected failure"
+$QEMU_IMG measure --bitmaps -O raw -f qcow2 "$TEST_IMG" &&
+    echo "unexpected success"
+# No bitmap output, since no bitmaps on raw source
+$QEMU_IMG measure -O qcow2 -f raw "$TEST_IMG" ||
+    echo "unexpected failure"
+$QEMU_IMG measure --bitmaps -O qcow2 -f raw "$TEST_IMG" &&
+    echo "unexpected success"
+# No bitmap output, since v2 does not support it
+$QEMU_IMG measure -O qcow2 -o compat=0.10 -f qcow2 "$TEST_IMG" ||
+    echo "unexpected failure"
+$QEMU_IMG measure --bitmaps -O qcow2 -o compat=0.10 -f qcow2 "$TEST_IMG" &&
+    echo "unexpected success"
+
+# Compute expected output:
+echo
+val2T=$((2*1024*1024*1024*1024))
+cluster=$((64*1024))
+b1clusters=$(( (val2T/512/8 + cluster - 1) / cluster ))
+b2clusters=$(( (val2T/2/1024/1024/8 + cluster - 1) / cluster ))
+echo expected bitmap $((b1clusters * cluster +
+			(b1clusters * 8 + cluster - 1) / cluster * cluster +
+			b2clusters * cluster +
+			(b2clusters * 8 + cluster - 1) / cluster * cluster +
+			cluster))
+$QEMU_IMG measure -O qcow2 -o cluster_size=64k -f qcow2 "$TEST_IMG"
+$QEMU_IMG measure --bitmaps -O qcow2 -o cluster_size=64k -f qcow2 "$TEST_IMG"
+
+# Compute expected output:
+echo
+cluster=$((2*1024*1024))
+b1clusters=$(( (val2T/512/8 + cluster - 1) / cluster ))
+b2clusters=$(( (val2T/2/1024/1024/8 + cluster - 1) / cluster ))
+echo expected bitmap $((b1clusters * cluster +
+			(b1clusters * 8 + cluster - 1) / cluster * cluster +
+			b2clusters * cluster +
+			(b2clusters * 8 + cluster - 1) / cluster * cluster +
+			cluster))
+$QEMU_IMG measure --output=json -O qcow2 -o cluster_size=2M -f qcow2 "$TEST_IMG"
+$QEMU_IMG measure --output=json --bitmaps -O qcow2 -o cluster_size=2M \
+     -f qcow2 "$TEST_IMG"
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/190.out b/tests/qemu-iotests/190.out
index d001942002db..5c35f9268068 100644
--- a/tests/qemu-iotests/190.out
+++ b/tests/qemu-iotests/190.out
@@ -1,11 +1,44 @@
 QA output created by 190
-== Huge file ==
+== Huge file without bitmaps ==

 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2199023255552
 required size: 2199023255552
 fully allocated size: 2199023255552
 required size: 335806464
 fully allocated size: 2199359062016
+bitmaps size: 0
 required size: 18874368
 fully allocated size: 2199042129920
+bitmaps size: 0
+
+== Huge file with bitmaps ==
+
+qemu-img: --bitmaps is only supported with a filename.
+required size: 2199023255552
+fully allocated size: 2199023255552
+qemu-img: no bitmaps measured, either source or destination format lacks bitmap support
+required size: 7012352
+fully allocated size: 17170432
+qemu-img: no bitmaps measured, either source or destination format lacks bitmap support
+required size: 335806464
+fully allocated size: 2199359062016
+qemu-img: no bitmaps measured, either source or destination format lacks bitmap support
+
+expected bitmap 537198592
+required size: 335806464
+fully allocated size: 2199359062016
+bitmaps size: 537198592
+required size: 873005056
+fully allocated size: 2199896260608
+
+expected bitmap 545259520
+{
+    "bitmaps": 545259520,
+    "required": 18874368,
+    "fully-allocated": 2199042129920
+}
+{
+    "required": 564133888,
+    "fully-allocated": 2199587389440
+}
 *** done
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v4 8/9] qemu-img: Add convert --bitmaps option
  2020-05-13  1:16 [PATCH v4 0/9] qemu-img: Add convert --bitmaps Eric Blake
                   ` (6 preceding siblings ...)
  2020-05-13  1:16 ` [PATCH v4 7/9] qcow2: Expose bitmaps' size during measure Eric Blake
@ 2020-05-13  1:16 ` Eric Blake
  2020-05-18 13:33   ` Vladimir Sementsov-Ogievskiy
  2020-05-13  1:16 ` [PATCH v4 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-13  1:16 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, nsoffer, qemu-block, mreitz

Make it easier to copy all the persistent bitmaps of (the top layer
of) a source image along with its guest-visible contents, by adding a
boolean flag for use with qemu-img convert.  This is basically
shorthand, as the same effect could be accomplished with a series of
'qemu-img bitmap --add' and 'qemu-img bitmap --merge -b source'
commands, or by QMP commands.

Note that this command will fail in the same scenarios where 'qemu-img
measure --bitmaps' fails, when either the source or the destanation
lacks persistent bitmap support altogether.

See also https://bugzilla.redhat.com/show_bug.cgi?id=1779893

While touching this, clean up a couple coding issues spotted in the
same function: an extra blank line, and merging back-to-back 'if
(!skip_create)' blocks.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 docs/tools/qemu-img.rst |  6 ++-
 qemu-img.c              | 85 +++++++++++++++++++++++++++++++++++++++--
 qemu-img-cmds.hx        |  4 +-
 3 files changed, 89 insertions(+), 6 deletions(-)

diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index b6f87ec6d3c0..25156f489696 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -162,6 +162,10 @@ Parameters to convert subcommand:

 .. program:: qemu-img-convert

+.. option:: --bitmaps
+
+  Additionally copy all persistent bitmaps from the top layer of the source
+
 .. option:: -n

   Skip the creation of the target volume
@@ -397,7 +401,7 @@ Command description:
   4
     Error on reading data

-.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [--target-is-zero] [-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-m NUM_COROUTINES] [-W] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME
+.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [--target-is-zero] [--bitmaps] [-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-m NUM_COROUTINES] [-W] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME

   Convert the disk image *FILENAME* or a snapshot *SNAPSHOT_PARAM*
   to disk image *OUTPUT_FILENAME* using format *OUTPUT_FMT*. It can
diff --git a/qemu-img.c b/qemu-img.c
index 0a326993d7ac..a90d1eeefcba 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -192,6 +192,7 @@ static void QEMU_NORETURN help(void)
            "       hiding corruption that has already occurred.\n"
            "\n"
            "Parameters to convert subcommand:\n"
+           "  '--bitmaps' copies all top-level persistent bitmaps to destination\n"
            "  '-m' specifies how many coroutines work in parallel during the convert\n"
            "       process (defaults to 8)\n"
            "  '-W' allow to write to the target out of order rather than sequential\n"
@@ -2109,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)
@@ -2130,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 */
@@ -2149,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",
@@ -2272,6 +2317,9 @@ static int img_convert(int argc, char **argv)
              */
             s.has_zero_init = true;
             break;
+        case OPTION_BITMAPS:
+            bitmaps = true;
+            break;
         }
     }

@@ -2333,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) {
@@ -2493,6 +2540,27 @@ static int img_convert(int argc, char **argv)
         }
     }

+    /* Determine how many bitmaps need copying */
+    if (bitmaps) {
+        BdrvDirtyBitmap *bm;
+
+        if (s.src_num > 1) {
+            error_report("Copying bitmaps only possible with single source");
+            ret = -1;
+            goto out;
+        }
+        if (!bdrv_supports_persistent_dirty_bitmap(blk_bs(s.src[0]))) {
+            error_report("Source lacks bitmap support");
+            ret = -1;
+            goto out;
+        }
+        FOR_EACH_DIRTY_BITMAP(blk_bs(s.src[0]), bm) {
+            if (bdrv_dirty_bitmap_get_persistence(bm)) {
+                nbitmaps++;
+            }
+        }
+    }
+
     /*
      * The later open call will need any decryption secrets, and
      * bdrv_create() will purge "opts", so extract them now before
@@ -2501,9 +2569,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) {
@@ -2541,6 +2607,13 @@ static int img_convert(int argc, char **argv)
     }
     out_bs = blk_bs(s.target);

+    if (nbitmaps > 0 && !bdrv_supports_persistent_dirty_bitmap(out_bs)) {
+        error_report("Format driver '%s' does not support bitmaps",
+                     out_fmt);
+        ret = -1;
+        goto out;
+    }
+
     if (s.compressed && !block_driver_can_compress(out_bs->drv)) {
         error_report("Compression not supported for this file format");
         ret = -1;
@@ -2600,6 +2673,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 bfcd9f32dddf..2791c4f58ddd 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 v4 9/9] iotests: Add test 291 to for qemu-img bitmap coverage
  2020-05-13  1:16 [PATCH v4 0/9] qemu-img: Add convert --bitmaps Eric Blake
                   ` (7 preceding siblings ...)
  2020-05-13  1:16 ` [PATCH v4 8/9] qemu-img: Add convert --bitmaps option Eric Blake
@ 2020-05-13  1:16 ` Eric Blake
  2020-05-18 14:43   ` Vladimir Sementsov-Ogievskiy
  8 siblings, 1 reply; 30+ messages in thread
From: Eric Blake @ 2020-05-13  1:16 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, nsoffer, qemu-block, mreitz

Add a new test covering the 'qemu-img bitmap' subcommand, as well as
'qemu-img convert --bitmaps', both added in recent patches.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 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 v4 1/9] docs: Sort sections on qemu-img subcommand parameters
  2020-05-13  1:16 ` [PATCH v4 1/9] docs: Sort sections on qemu-img subcommand parameters Eric Blake
@ 2020-05-14  5:02   ` Vladimir Sementsov-Ogievskiy
  0 siblings, 0 replies; 30+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-14  5:02 UTC (permalink / raw)
  To: Eric Blake, qemu-devel; +Cc: kwolf, nsoffer, qemu-block, mreitz

13.05.2020 04:16, Eric Blake wrote:
> 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>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v4 2/9] qemu-img: Fix stale comments on doc location
  2020-05-13  1:16 ` [PATCH v4 2/9] qemu-img: Fix stale comments on doc location Eric Blake
@ 2020-05-14  5:06   ` Vladimir Sementsov-Ogievskiy
  0 siblings, 0 replies; 30+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-14  5:06 UTC (permalink / raw)
  To: Eric Blake, qemu-devel; +Cc: kwolf, nsoffer, qemu-block, mreitz

13.05.2020 04:16, Eric Blake wrote:
> Missed in commit e13c59fa.
> 
> Signed-off-by: Eric Blake<eblake@redhat.com>
> Reviewed-by: Max Reitz<mreitz@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v4 3/9] block: Make it easier to learn which BDS support bitmaps
  2020-05-13  1:16 ` [PATCH v4 3/9] block: Make it easier to learn which BDS support bitmaps Eric Blake
@ 2020-05-14  5:19   ` Vladimir Sementsov-Ogievskiy
  2020-05-14 14:09     ` Eric Blake
  0 siblings, 1 reply; 30+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-14  5:19 UTC (permalink / raw)
  To: Eric Blake, qemu-devel; +Cc: kwolf, nsoffer, John Snow, qemu-block, mreitz

13.05.2020 04:16, 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 persistent bitmaps
> make no sense (such as on a qcow2 v2 image).  Add a hook to make this
> easier to query.
> 
> This patch adds a new callback .bdrv_supports_persistent_dirty_bitmap,
> rather than trying to shoehorn the answer in via existing callbacks.
> In particular, while it might have been possible to overload
> .bdrv_co_can_store_new_dirty_bitmap to special-case a NULL input to
> answer whether any persistent bitmaps are supported, that is at odds
> with whether a particular bitmap can be stored (for example, even on
> an image that supports persistent bitmaps but has currently filled up
> the maximum number of bitmaps, attempts to store another one should
> fail); and the new functionality doesn't require coroutine safety.
> Similarly, we could have added one more piece of information to
> .bdrv_get_info, but then again, most callers to that function tend to
> already discard extraneous information, and making it a catch-all
> rather than a series of dedicated scalar queries hasn't really
> simplified life.
> 
> 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.

Hm. I think that bitmap at filter bs is a valid thing (moreover I have a plan to use it for one issue), so I'm not sure that it's good idea to do any generic logic around bitmaps work through filters, better to always address the exact node you mean..

> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v4 4/9] blockdev: Promote several bitmap functions to non-static
  2020-05-13  1:16 ` [PATCH v4 4/9] blockdev: Promote several bitmap functions to non-static Eric Blake
@ 2020-05-14  5:45   ` Vladimir Sementsov-Ogievskiy
  2020-05-14 11:45   ` Vladimir Sementsov-Ogievskiy
  1 sibling, 0 replies; 30+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-14  5:45 UTC (permalink / raw)
  To: Eric Blake, qemu-devel
  Cc: kwolf, nsoffer, Markus Armbruster, qemu-block, mreitz

13.05.2020 04:16, 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>
> Reviewed-by: Max Reitz<mreitz@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v4 5/9] blockdev: Split off basic bitmap operations for qemu-img
  2020-05-13  1:16 ` [PATCH v4 5/9] blockdev: Split off basic bitmap operations for qemu-img Eric Blake
@ 2020-05-14  6:21   ` Vladimir Sementsov-Ogievskiy
  2020-05-14 14:15     ` Eric Blake
  0 siblings, 1 reply; 30+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-14  6:21 UTC (permalink / raw)
  To: Eric Blake, qemu-devel
  Cc: kwolf, qemu-block, Markus Armbruster, mreitz, nsoffer, John Snow

13.05.2020 04:16, 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>
> Reviewed-by: Max Reitz <mreitz@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

Hmm, shouldn't transaction bitmap actions be moved here too? May be, not in these series..

> @@ -0,0 +1,323 @@
> +/*
> + * QEMU host block device bitmaps

A bit conflicts with tha fact that they are not of block-device level but of node-level.

May be just "Block dirty bitmap qmp commands" ?

> + *
> + * Copyright (c) 2003-2008 Fabrice Bellard

Does it really apply here? block-dirty-bitmap-add was added in 2015.. May be Red Hat and Virtuozzo copyrights would be more appropriate.

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

compiles for with only four:

   #include "qemu/osdep.h"
                                                                                  
   #include "block/block_int.h"
   #include "qapi/qapi-commands-block.h"
   #include "qapi/error.h"

with at least extra includes dropped:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v4 6/9] qemu-img: Add bitmap sub-command
  2020-05-13  1:16 ` [PATCH v4 6/9] qemu-img: Add bitmap sub-command Eric Blake
@ 2020-05-14  6:45   ` Vladimir Sementsov-Ogievskiy
  2020-05-14 14:20     ` Eric Blake
  2020-05-18 11:42   ` Vladimir Sementsov-Ogievskiy
  1 sibling, 1 reply; 30+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-14  6:45 UTC (permalink / raw)
  To: Eric Blake, qemu-devel; +Cc: kwolf, nsoffer, qemu-block, mreitz

13.05.2020 04:16, 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.
> 
> 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>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

I'm sorry for asking it only now on v4.. But still. Why do we need it? We can instead run qemu binary (or even new qemu-storage-daemon) and just use existing qmp commands. Is there a real benefit in developing qemu-img, maintaining two interfaces for the same thing? Of-course, just run qmp commands from terminal is a lot less comfortable than just a qemu img command.. But may be we need some wrapper, which make it simple to run one qmp command on an image?

It's simple to make a python wrapper working like

qemu-qmp block-dirty-bitmap-add '{node: self, name: bitmap0, persistent: true}' /path/to/x.qcow2


-- 
Best regards,
Vladimir


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v4 4/9] blockdev: Promote several bitmap functions to non-static
  2020-05-13  1:16 ` [PATCH v4 4/9] blockdev: Promote several bitmap functions to non-static Eric Blake
  2020-05-14  5:45   ` Vladimir Sementsov-Ogievskiy
@ 2020-05-14 11:45   ` Vladimir Sementsov-Ogievskiy
  2020-05-14 14:10     ` Eric Blake
  1 sibling, 1 reply; 30+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-14 11:45 UTC (permalink / raw)
  To: Eric Blake, qemu-devel
  Cc: kwolf, nsoffer, Markus Armbruster, qemu-block, mreitz

13.05.2020 04:16, Eric Blake wrote:
> -        HBitmap **backup, Error **errp)
> +BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
> +                                          BlockDirtyBitmapMergeSourceList *bitmaps,
> +                                          HBitmap **backup, Error **errp)
>   {
>       BlockDriverState *bs;

s/bitmaps/bms/ to match declaration and fit into 80 characters

-- 
Best regards,
Vladimir


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v4 3/9] block: Make it easier to learn which BDS support bitmaps
  2020-05-14  5:19   ` Vladimir Sementsov-Ogievskiy
@ 2020-05-14 14:09     ` Eric Blake
  0 siblings, 0 replies; 30+ messages in thread
From: Eric Blake @ 2020-05-14 14:09 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy, qemu-devel
  Cc: kwolf, nsoffer, John Snow, qemu-block, mreitz

On 5/14/20 12:19 AM, Vladimir Sementsov-Ogievskiy wrote:

>> 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.
> 
> Hm. I think that bitmap at filter bs is a valid thing (moreover I have a 
> plan to use it for one issue), so I'm not sure that it's good idea to do 
> any generic logic around bitmaps work through filters, better to always 
> address the exact node you mean..

In the immediate term, the only user of this new function is qemu-img, 
and qemu-img is not setting up filters, so whether this callback can see 
through filters is moot (that is, we are always addressing the exact 
node the user passed on the command line, for whatever bitmap operations 
qemu-img does).  In the long term, being able to see bitmaps through 
filters is better addressed after we finally include Max's series that 
include filter handling overall.

> 
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Thanks.

-- 
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 v4 4/9] blockdev: Promote several bitmap functions to non-static
  2020-05-14 11:45   ` Vladimir Sementsov-Ogievskiy
@ 2020-05-14 14:10     ` Eric Blake
  0 siblings, 0 replies; 30+ messages in thread
From: Eric Blake @ 2020-05-14 14:10 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy, qemu-devel
  Cc: kwolf, nsoffer, Markus Armbruster, qemu-block, mreitz

On 5/14/20 6:45 AM, Vladimir Sementsov-Ogievskiy wrote:
> 13.05.2020 04:16, Eric Blake wrote:
>> -        HBitmap **backup, Error **errp)
>> +BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const 
>> char *target,
>> +                                          
>> BlockDirtyBitmapMergeSourceList *bitmaps,
>> +                                          HBitmap **backup, Error 
>> **errp)
>>   {
>>       BlockDriverState *bs;
> 
> s/bitmaps/bms/ to match declaration and fit into 80 characters

Can do, although it has ripple effects to 5/9 (as I wanted that to be 
pure code motion).

-- 
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 v4 5/9] blockdev: Split off basic bitmap operations for qemu-img
  2020-05-14  6:21   ` Vladimir Sementsov-Ogievskiy
@ 2020-05-14 14:15     ` Eric Blake
  0 siblings, 0 replies; 30+ messages in thread
From: Eric Blake @ 2020-05-14 14:15 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy, qemu-devel
  Cc: kwolf, qemu-block, Markus Armbruster, mreitz, nsoffer, John Snow

On 5/14/20 1:21 AM, Vladimir Sementsov-Ogievskiy wrote:
> 13.05.2020 04:16, 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>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>> ---

>> +++ b/block/monitor/bitmap-qmp-cmds.c
> 
> Hmm, shouldn't transaction bitmap actions be moved here too? May be, not 
> in these series..

No, the very reason that we split this file off of blockdev.c is that 
transactions are NOT needed in qemu-img.  You have to have separate .o 
files for what qemu-img needs, vs. what the rest of qemu proper needs, 
and transactions are only needed in qemu proper at the moment.  If we 
ever need transactions in Kevin's qemu-storage-daemon but not all of 
blockdev.c, then we'd need yet another .c file independent from either 
blockdev.c or this patch's new bitmap-qmp-cmds.c.

> 
>> @@ -0,0 +1,323 @@
>> +/*
>> + * QEMU host block device bitmaps
> 
> A bit conflicts with tha fact that they are not of block-device level 
> but of node-level.
> 
> May be just "Block dirty bitmap qmp commands" ?

Copy-and-paste from blockdev.c, tweaked by adding one word.  Your 
wording is also fine.

> 
>> + *
>> + * Copyright (c) 2003-2008 Fabrice Bellard
> 
> Does it really apply here? block-dirty-bitmap-add was added in 2015.. 
> May be Red Hat and Virtuozzo copyrights would be more appropriate.

When splitting a file, the safest course of action is to preserve ALL 
copyright from the original file into the new file.

Adding additional copyright lines is okay as part of submitting new 
functionality, but in this case, I don't feel comfortable adding Red Hat 
copyright (my split isn't adding new functionality), and I don't have 
authorization to add Virtuozzo copyright (as that is not my employer).


>> +#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"
> 
> compiles for with only four:
> 
>    #include "qemu/osdep.h"
>    #include "block/block_int.h"
>    #include "qapi/qapi-commands-block.h"
>    #include "qapi/error.h"

Thanks. I blame rebase; an earlier version used a different header in 
patch 4/9; when I moved things to block_int.h in that patch, I didn't 
realize that this patch could be improved.

> 
> with at least extra includes dropped:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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 v4 6/9] qemu-img: Add bitmap sub-command
  2020-05-14  6:45   ` Vladimir Sementsov-Ogievskiy
@ 2020-05-14 14:20     ` Eric Blake
  2020-05-14 15:09       ` Vladimir Sementsov-Ogievskiy
  0 siblings, 1 reply; 30+ messages in thread
From: Eric Blake @ 2020-05-14 14:20 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy, qemu-devel
  Cc: kwolf, nsoffer, qemu-block, mreitz

On 5/14/20 1:45 AM, Vladimir Sementsov-Ogievskiy wrote:
> 13.05.2020 04:16, 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.
>>

> 
> I'm sorry for asking it only now on v4.. But still. Why do we need it? 

Ease of use.

> We can instead run qemu binary (or even new qemu-storage-daemon) and 
> just use existing qmp commands. Is there a real benefit in developing 
> qemu-img, maintaining two interfaces for the same thing?

If it makes someone's life easier, and is not hard to maintain, then 
yes.  A command line interface that calls into QMP is not hard to 
maintain.  And _I_ certainly found it easier to write iotests with this 
patch in place, so it already has at least one client.

> Of-course, just 
> run qmp commands from terminal is a lot less comfortable than just a 
> qemu img command.. But may be we need some wrapper, which make it simple 
> to run one qmp command on an image?
> 
> It's simple to make a python wrapper working like
> 
> qemu-qmp block-dirty-bitmap-add '{node: self, name: bitmap0, persistent: 
> true}' /path/to/x.qcow2

This _IS_ such a wrapper.  The whole point of this patch is that it is 
now simpler to run one (or more) QMP command on an offline image from 
the command line.  Just because I wrote it in C instead of python, and 
attached it to an existing tool instead of writing a new tool, doesn't 
change the fact that it is just a wrapper around the existing QMP commands.

-- 
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 v4 6/9] qemu-img: Add bitmap sub-command
  2020-05-14 14:20     ` Eric Blake
@ 2020-05-14 15:09       ` Vladimir Sementsov-Ogievskiy
  0 siblings, 0 replies; 30+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-14 15:09 UTC (permalink / raw)
  To: Eric Blake, qemu-devel; +Cc: kwolf, nsoffer, qemu-block, mreitz

14.05.2020 17:20, Eric Blake wrote:
> On 5/14/20 1:45 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 13.05.2020 04:16, 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.
>>>
> 
>>
>> I'm sorry for asking it only now on v4.. But still. Why do we need it? 
> 
> Ease of use.
> 
>> We can instead run qemu binary (or even new qemu-storage-daemon) and just use existing qmp commands. Is there a real benefit in developing qemu-img, maintaining two interfaces for the same thing?
> 
> If it makes someone's life easier, and is not hard to maintain, then yes.  A command line interface that calls into QMP is not hard to maintain.  And _I_ certainly found it easier to write iotests with this patch in place, so it already has at least one client.
> 
>> Of-course, just run qmp commands from terminal is a lot less comfortable than just a qemu img command.. But may be we need some wrapper, which make it simple to run one qmp command on an image?
>>
>> It's simple to make a python wrapper working like
>>
>> qemu-qmp block-dirty-bitmap-add '{node: self, name: bitmap0, persistent: true}' /path/to/x.qcow2
> 
> This _IS_ such a wrapper.  The whole point of this patch is that it is now simpler to run one (or more) QMP command on an offline image from the command line.  Just because I wrote it in C instead of python, and attached it to an existing tool instead of writing a new tool, doesn't change the fact that it is just a wrapper around the existing QMP commands.
> 

OK, that's right.

The thing that I didn't like is that we have to make cli-to-qapi interface mapping by hand. But I see now that interface you implementing is prepared to make several actions with same bitmap-name, which can't be achieved with some kind of automatic interface matching anyway, so my proposal don't match your needs, sorry for my haste)


-- 
Best regards,
Vladimir


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v4 6/9] qemu-img: Add bitmap sub-command
  2020-05-13  1:16 ` [PATCH v4 6/9] qemu-img: Add bitmap sub-command Eric Blake
  2020-05-14  6:45   ` Vladimir Sementsov-Ogievskiy
@ 2020-05-18 11:42   ` Vladimir Sementsov-Ogievskiy
  2020-05-18 19:07     ` Eric Blake
  1 sibling, 1 reply; 30+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-18 11:42 UTC (permalink / raw)
  To: Eric Blake, qemu-devel; +Cc: kwolf, nsoffer, qemu-block, mreitz

13.05.2020 04:16, 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.
> 
> 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>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>   docs/tools/qemu-img.rst |  24 ++++
>   qemu-img.c              | 254 ++++++++++++++++++++++++++++++++++++++++
>   qemu-img-cmds.hx        |   7 ++
>   3 files changed, 285 insertions(+)
> 
> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
> index 7d08c48d308f..219483cec279 100644
> --- a/docs/tools/qemu-img.rst
> +++ b/docs/tools/qemu-img.rst
> @@ -281,6 +281,30 @@ 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 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``.
> +
> +  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..8c99e68ba8aa 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', '--disable',\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);

fit in one line

> +    if (!blk) {
> +        goto out;
> +    }
> +    bs = blk_bs(blk);
> +    if (src_filename) {
> +        src = img_open(NULL, src_filename, src_fmt, 0, false, false,
> +                       false);

s/NULL/false/

also, fit in one line

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

s/failed/failed: /

> +                              op, bitmap);
> +            ret = -1;

dead assignment: you never set ret after first initialization to -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;

Hmm, as it's the only usage of ret, you may initialize it to 1 at function start, and here just "return ret;"

> +}
> +
>   #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

Not about this patch, but it's a pity that we have triple duplications (triplications ?) of such lines..

> +ERST
> +
>   DEF("check", img_check,
>       "check [--object objectdef] [--image-opts] [-q] [-f fmt] [--output=ofmt] [-r [leaks | all]] [-T src_cache] [-U] filename")
>   SRST
> 

With at least s/NULL/false/ and s/failed/failed: / (or with all my tiny suggestions):
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v4 7/9] qcow2: Expose bitmaps' size during measure
  2020-05-13  1:16 ` [PATCH v4 7/9] qcow2: Expose bitmaps' size during measure Eric Blake
@ 2020-05-18 13:07   ` Vladimir Sementsov-Ogievskiy
  2020-05-18 19:17     ` Eric Blake
  0 siblings, 1 reply; 30+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-18 13:07 UTC (permalink / raw)
  To: Eric Blake, qemu-devel
  Cc: kwolf, nsoffer, Markus Armbruster, qemu-block, mreitz

13.05.2020 04:16, Eric Blake wrote:
> It's useful to know how much space can be occupied by qcow2 persistent
> bitmaps, even though such metadata is unrelated to the guest-visible
> data.  Report this value as an additional QMP field, present when
> measuring an existing image and output format that both support
> bitmaps.  Update iotest 178 and 190 to updated output, as well as new
> coverage in 190 demonstrating non-zero values made possible with the
> recently-added qemu-img bitmap command.
> 
> On the command-line side, 'qemu-img measure' gains a new --bitmaps
> flag.  When present, the bitmap size is rolled into the two existing
> measures (or errors if either the source image or destination format
> lacks bitmaps); when absent, there is never an error (for
> back-compat), but the output will instead include a new line item for
> bitmaps (which you would have to manually add), with that line being
> omitted in the same cases where passing --bitmaps would error.
> 
> The behavior chosen here is symmetrical with the upcoming 'qemu-img
> convert --bitmaps' being added in the next patch: that is, either both
> commands will succeed (your qemu-img was new enough to do bitmap
> manipulations, AND you correctly measured and copied the bitmaps, even
> if that measurement was 0 because there was nothing to copy) or both
> fail (either your qemu-img is too old to understand --bitmaps, or it
> understands it but your choice of images do not support seamless
> transition of bitmaps because either source, destination, or both lack
> bitmap support).
> 
> The addition of a new field demonstrates why we should always
> zero-initialize qapi C structs; while the qcow2 driver still fully
> populates all fields, the raw and crypto drivers had to be tweaked to
> avoid uninitialized data.
> 
> See also: https://bugzilla.redhat.com/1779904
> 
> Reported-by: Nir Soffer <nsoffer@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   docs/tools/qemu-img.rst          | 10 +++++-
>   qapi/block-core.json             | 15 ++++++---
>   block/crypto.c                   |  2 +-
>   block/qcow2.c                    | 37 +++++++++++++++++++--
>   block/raw-format.c               |  2 +-
>   qemu-img.c                       | 25 ++++++++++++++
>   qemu-img-cmds.hx                 |  4 +--
>   tests/qemu-iotests/178.out.qcow2 | 16 +++++++++
>   tests/qemu-iotests/190           | 57 ++++++++++++++++++++++++++++++--
>   tests/qemu-iotests/190.out       | 35 +++++++++++++++++++-
>   10 files changed, 187 insertions(+), 16 deletions(-)
> 
> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
> index 219483cec279..b6f87ec6d3c0 100644
> --- a/docs/tools/qemu-img.rst
> +++ b/docs/tools/qemu-img.rst
> @@ -593,7 +593,7 @@ Command description:
>     For more information, consult ``include/block/block.h`` in QEMU's
>     source code.
> 
> -.. option:: measure [--output=OFMT] [-O OUTPUT_FMT] [-o OPTIONS] [--size N | [--object OBJECTDEF] [--image-opts] [-f FMT] [-l SNAPSHOT_PARAM] FILENAME]
> +.. option:: measure [--output=OFMT] [-O OUTPUT_FMT] [-o OPTIONS] [--size N | [--object OBJECTDEF] [--image-opts] [-f FMT] [--bitmaps] [-l SNAPSHOT_PARAM] FILENAME]
> 
>     Calculate the file size required for a new image.  This information
>     can be used to size logical volumes or SAN LUNs appropriately for
> @@ -616,6 +616,7 @@ Command description:
> 
>       required size: 524288
>       fully allocated size: 1074069504
> +    bitmaps: 0
> 
>     The ``required size`` is the file size of the new image.  It may be smaller
>     than the virtual disk size if the image format supports compact representation.
> @@ -625,6 +626,13 @@ Command description:
>     occupy with the exception of internal snapshots, dirty bitmaps, vmstate data,
>     and other advanced image format features.
> 
> +  The ``bitmaps size`` is the additional size required if the

you called it "bitmaps" in example output above. Should it be consistent? Either "``bitmaps``" here, or "bitmaps size: 0" above?

> +  destination supports persistent bitmaps, in order to additionally
> +  copy bitmaps in addition to the guest-visible data.  If the
> +  ``--bitmaps`` option was in use, the bitmap size is folded into the
> +  required and fully-allocated size for convenience, rather than being
> +  a separate line item.
> +
>   .. option:: snapshot [--object OBJECTDEF] [--image-opts] [-U] [-q] [-l | -a SNAPSHOT | -c SNAPSHOT | -d SNAPSHOT] FILENAME
> 
>     List, apply, create or delete snapshots in image *FILENAME*.
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 943df1926a91..65280eb9847d 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.

"copying just guest-visible" sounds like something less than "all fully-allocated sectors"..
But I don't have better suggestion.. Just, "not including bitmaps" sounds weird too.

> +#
> +# @bitmaps: Additional size required if all the top-level bitmap metadata in
> +#           the source image were to be copied to the destination, present
> +#           when the destination supports persistent bitmaps. (since 5.1)
>   #
>   # Since: 2.10
>   ##
>   { 'struct': 'BlockMeasureInfo',
> -  'data': {'required': 'int', 'fully-allocated': 'int'} }
> +  'data': {'required': 'int', 'fully-allocated': 'int', '*bitmaps': 'int'} }
> 
>   ##
>   # @query-block:
> diff --git a/block/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 1c8f3ab8ae68..be0950016365 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));

Could we instead reuse code from qcow2_co_can_store_new_dirty_bitmap(), which calls calc_dir_entry_size() for this thing?
Possibly, make a function qcow2_measure_bitmaps in block/qcow2-bitmaps.c with this FOR_EACH? All details about qcow2 bitmap structures sounds better in block/qcow2-bitmaps.c

> +            }
> +        }
> +        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_supports_persistent_dirty_bitmap(in_bs);
> +    info->bitmaps = bitmaps_size;

AFAIK, in QAPI, if has_<something> field is false, than <something> must be zero. Maybe, it's only about nested structured fields, not about simple numbers, but I think it's better keep bitmaps 0 in case when has_bitmaps is false.

Also, it seems a bit better to check version earlier, and don't do all the calculations, if we are not going to use them.. But it's a rare backward-compatibility case, I don't care.

>       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 8c99e68ba8aa..0a326993d7ac 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 {
> @@ -5096,6 +5097,7 @@ static int img_measure(int argc, char **argv)
>           {"output", required_argument, 0, OPTION_OUTPUT},
>           {"size", required_argument, 0, OPTION_SIZE},
>           {"force-share", no_argument, 0, 'U'},
> +        {"bitmaps", no_argument, 0, OPTION_BITMAPS},
>           {0, 0, 0, 0}
>       };
>       OutputFormat output_format = OFORMAT_HUMAN;
> @@ -5112,6 +5114,7 @@ static int img_measure(int argc, char **argv)
>       QemuOpts *sn_opts = NULL;
>       QemuOptsList *create_opts = NULL;
>       bool image_opts = false;
> +    bool bitmaps = false;
>       uint64_t img_size = UINT64_MAX;
>       BlockMeasureInfo *info = NULL;
>       Error *local_err = NULL;
> @@ -5192,6 +5195,9 @@ static int img_measure(int argc, char **argv)
>               img_size = (uint64_t)sval;
>           }
>           break;
> +        case OPTION_BITMAPS:
> +            bitmaps = true;
> +            break;
>           }
>       }
> 
> @@ -5220,6 +5226,10 @@ static int img_measure(int argc, char **argv)
>           error_report("Either --size N or one filename must be specified.");
>           goto out;
>       }
> +    if (!filename && bitmaps) {
> +        error_report("--bitmaps is only supported with a filename.");
> +        goto out;
> +    }
> 
>       if (filename) {
>           in_blk = img_open(image_opts, filename, fmt, 0,
> @@ -5275,9 +5285,24 @@ static int img_measure(int argc, char **argv)
>           goto out;
>       }
> 
> +    if (bitmaps) {
> +        if (!info->has_bitmaps) {
> +            error_report("no bitmaps measured, either source or destination "
> +                         "format lacks bitmap support");
> +            goto out;
> +        } else {
> +            info->required += info->bitmaps;
> +            info->fully_allocated += info->bitmaps;
> +            info->has_bitmaps = false;

And here, I think better to zero info->bitmaps as well.

> +        }
> +    }
> +
>       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);
>       }

[..]

> --- 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,59 @@ $QEMU_IMG measure -O raw -f qcow2 "$TEST_IMG"
>   $QEMU_IMG measure -O qcow2 -o cluster_size=64k -f qcow2 "$TEST_IMG"
>   $QEMU_IMG measure -O qcow2 -o cluster_size=2M -f qcow2 "$TEST_IMG"
> 
> +echo
> +echo "== Huge file with bitmaps =="
> +echo
> +
> +$QEMU_IMG bitmap --add --granularity 512 -f qcow2 "$TEST_IMG" b1
> +$QEMU_IMG bitmap --add -g 2M -f qcow2 "$TEST_IMG" b2
> +
> +# No bitmap without a source
> +$QEMU_IMG measure --bitmaps -O qcow2 --size 10M

should this be ored to  'echo "unexpected success"' as following failures?


> +# No bitmap output, since raw does not support it
> +$QEMU_IMG measure -O raw -f qcow2 "$TEST_IMG" ||
> +    echo "unexpected failure"
> +$QEMU_IMG measure --bitmaps -O raw -f qcow2 "$TEST_IMG" &&
> +    echo "unexpected success"
> +# No bitmap output, since no bitmaps on raw source
> +$QEMU_IMG measure -O qcow2 -f raw "$TEST_IMG" ||
> +    echo "unexpected failure"
> +$QEMU_IMG measure --bitmaps -O qcow2 -f raw "$TEST_IMG" &&
> +    echo "unexpected success"
> +# No bitmap output, since v2 does not support it
> +$QEMU_IMG measure -O qcow2 -o compat=0.10 -f qcow2 "$TEST_IMG" ||
> +    echo "unexpected failure"
> +$QEMU_IMG measure --bitmaps -O qcow2 -o compat=0.10 -f qcow2 "$TEST_IMG" &&
> +    echo "unexpected success"
> +
> +# Compute expected output:
> +echo
> +val2T=$((2*1024*1024*1024*1024))
> +cluster=$((64*1024))
> +b1clusters=$(( (val2T/512/8 + cluster - 1) / cluster ))
> +b2clusters=$(( (val2T/2/1024/1024/8 + cluster - 1) / cluster ))

comment on the following calculations won't hurt, at least something like
  "bitmap clusters + bitmap tables + bitmaps directory"

> +echo expected bitmap $((b1clusters * cluster +
> +			(b1clusters * 8 + cluster - 1) / cluster * cluster +
> +			b2clusters * cluster +
> +			(b2clusters * 8 + cluster - 1) / cluster * cluster +
> +			cluster))
> +$QEMU_IMG measure -O qcow2 -o cluster_size=64k -f qcow2 "$TEST_IMG"
> +$QEMU_IMG measure --bitmaps -O qcow2 -o cluster_size=64k -f qcow2 "$TEST_IMG"
> +
> +# Compute expected output:
> +echo
> +cluster=$((2*1024*1024))
> +b1clusters=$(( (val2T/512/8 + cluster - 1) / cluster ))
> +b2clusters=$(( (val2T/2/1024/1024/8 + cluster - 1) / cluster ))
> +echo expected bitmap $((b1clusters * cluster +
> +			(b1clusters * 8 + cluster - 1) / cluster * cluster +
> +			b2clusters * cluster +
> +			(b2clusters * 8 + cluster - 1) / cluster * cluster +
> +			cluster))
> +$QEMU_IMG measure --output=json -O qcow2 -o cluster_size=2M -f qcow2 "$TEST_IMG"
> +$QEMU_IMG measure --output=json --bitmaps -O qcow2 -o cluster_size=2M \
> +     -f qcow2 "$TEST_IMG"
> +
>   # success, all done
>   echo "*** done"
>   rm -f $seq.full
> diff --git a/tests/qemu-iotests/190.out b/tests/qemu-iotests/190.out
> index d001942002db..5c35f9268068 100644
> --- a/tests/qemu-iotests/190.out
> +++ b/tests/qemu-iotests/190.out
> @@ -1,11 +1,44 @@
>   QA output created by 190
> -== Huge file ==
> +== Huge file without bitmaps ==
> 
>   Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2199023255552
>   required size: 2199023255552
>   fully allocated size: 2199023255552
>   required size: 335806464
>   fully allocated size: 2199359062016
> +bitmaps size: 0
>   required size: 18874368
>   fully allocated size: 2199042129920
> +bitmaps size: 0
> +
> +== Huge file with bitmaps ==
> +
> +qemu-img: --bitmaps is only supported with a filename.
> +required size: 2199023255552> +fully allocated size: 2199023255552

  - same as for "without bitmaps" sections, good

> +qemu-img: no bitmaps measured, either source or destination format lacks bitmap support
> +required size: 7012352
> +fully allocated size: 17170432
> +qemu-img: no bitmaps measured, either source or destination format lacks bitmap support
> +required size: 335806464
> +fully allocated size: 2199359062016

matches "without bitmaps" section (hmm, v2 image needs same space? Ok)

> +qemu-img: no bitmaps measured, either source or destination format lacks bitmap support
> +
> +expected bitmap 537198592
> +required size: 335806464
> +fully allocated size: 2199359062016

matches "without bitmaps" section.

> +bitmaps size: 537198592

match expectations

> +required size: 873005056
> +fully allocated size: 2199896260608

matches "without bitmaps" section + expected

> +
> +expected bitmap 545259520
> +{
> +    "bitmaps": 545259520,

match expected

> +    "required": 18874368,
> +    "fully-allocated": 2199042129920

matches "without bitmaps" section.
> +}
> +{
> +    "required": 564133888,
> +    "fully-allocated": 2199587389440
> +}

matches "without bitmaps" section + expected

>   *** done
> 


-- 
Best regards,
Vladimir


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v4 8/9] qemu-img: Add convert --bitmaps option
  2020-05-13  1:16 ` [PATCH v4 8/9] qemu-img: Add convert --bitmaps option Eric Blake
@ 2020-05-18 13:33   ` Vladimir Sementsov-Ogievskiy
  0 siblings, 0 replies; 30+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-18 13:33 UTC (permalink / raw)
  To: Eric Blake, qemu-devel; +Cc: kwolf, nsoffer, qemu-block, mreitz

13.05.2020 04:16, 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.
> 
> Note that this command will fail in the same scenarios where 'qemu-img
> measure --bitmaps' fails, when either the source or the destanation
> lacks persistent bitmap support altogether.
> 
> See also https://bugzilla.redhat.com/show_bug.cgi?id=1779893
> 
> While touching this, clean up a couple coding issues spotted in the
> same function: an extra blank line, and merging back-to-back 'if
> (!skip_create)' blocks.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   docs/tools/qemu-img.rst |  6 ++-
>   qemu-img.c              | 85 +++++++++++++++++++++++++++++++++++++++--
>   qemu-img-cmds.hx        |  4 +-
>   3 files changed, 89 insertions(+), 6 deletions(-)
> 

[..]

> 
> +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);

^ duplicated code chunk with "case BITMAP_MERGE". Small helper function will not hurt.

> +        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)
> @@ -2130,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 */
> @@ -2149,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",
> @@ -2272,6 +2317,9 @@ static int img_convert(int argc, char **argv)
>                */
>               s.has_zero_init = true;
>               break;
> +        case OPTION_BITMAPS:
> +            bitmaps = true;
> +            break;
>           }
>       }
> 
> @@ -2333,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) {
> @@ -2493,6 +2540,27 @@ static int img_convert(int argc, char **argv)
>           }
>       }
> 
> +    /* Determine how many bitmaps need copying */
> +    if (bitmaps) {
> +        BdrvDirtyBitmap *bm;
> +
> +        if (s.src_num > 1) {
> +            error_report("Copying bitmaps only possible with single source");
> +            ret = -1;
> +            goto out;
> +        }
> +        if (!bdrv_supports_persistent_dirty_bitmap(blk_bs(s.src[0]))) {
> +            error_report("Source lacks bitmap support");
> +            ret = -1;
> +            goto out;
> +        }
> +        FOR_EACH_DIRTY_BITMAP(blk_bs(s.src[0]), bm) {
> +            if (bdrv_dirty_bitmap_get_persistence(bm)) {

Just note, that we can't have not persistent bitmaps here.. But the check doesn't hurt.

> +                nbitmaps++;
> +            }
> +        }
> +    }
> +
>       /*
>        * The later open call will need any decryption secrets, and
>        * bdrv_create() will purge "opts", so extract them now before
> @@ -2501,9 +2569,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) {
> @@ -2541,6 +2607,13 @@ static int img_convert(int argc, char **argv)
>       }
>       out_bs = blk_bs(s.target);
> 
> +    if (nbitmaps > 0 && !bdrv_supports_persistent_dirty_bitmap(out_bs)) {
> +        error_report("Format driver '%s' does not support bitmaps",
> +                     out_fmt);
> +        ret = -1;
> +        goto out;
> +    }
> +
>       if (s.compressed && !block_driver_can_compress(out_bs->drv)) {
>           error_report("Compression not supported for this file format");
>           ret = -1;
> @@ -2600,6 +2673,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 bfcd9f32dddf..2791c4f58ddd 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,
> 


-- 
Best regards,
Vladimir


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v4 9/9] iotests: Add test 291 to for qemu-img bitmap coverage
  2020-05-13  1:16 ` [PATCH v4 9/9] iotests: Add test 291 to for qemu-img bitmap coverage Eric Blake
@ 2020-05-18 14:43   ` Vladimir Sementsov-Ogievskiy
  0 siblings, 0 replies; 30+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-18 14:43 UTC (permalink / raw)
  To: Eric Blake, qemu-devel; +Cc: kwolf, nsoffer, qemu-block, mreitz

13.05.2020 04:16, 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>
> Reviewed-by: Max Reitz <mreitz@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> ---
>   tests/qemu-iotests/291     | 112 +++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/291.out |  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

[..]

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

Actually '-f $IMGFMT' option is already inside $QEMU_IO. Still it's a kind of standard for iotests to duplicate -f option, so I don't care. I hope one day we'll restrict it in qemu-io interface :)

-- 
Best regards,
Vladimir


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v4 6/9] qemu-img: Add bitmap sub-command
  2020-05-18 11:42   ` Vladimir Sementsov-Ogievskiy
@ 2020-05-18 19:07     ` Eric Blake
  2020-05-18 19:38       ` Vladimir Sementsov-Ogievskiy
  0 siblings, 1 reply; 30+ messages in thread
From: Eric Blake @ 2020-05-18 19:07 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy, qemu-devel
  Cc: kwolf, nsoffer, qemu-block, mreitz

On 5/18/20 6:42 AM, Vladimir Sementsov-Ogievskiy wrote:
> 13.05.2020 04:16, 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.
>>
>> 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.
>>

>> +
>> +    blk = img_open(image_opts, filename, fmt, BDRV_O_RDWR, false, false,
>> +                   false);
> 
> fit in one line

That line would be exactly 80; I tend to wrap at 79 or earlier rather 
than exactly on 80.

> 
>> +    if (!blk) {
>> +        goto out;
>> +    }
>> +    bs = blk_bs(blk);
>> +    if (src_filename) {
>> +        src = img_open(NULL, src_filename, src_fmt, 0, false, false,
>> +                       false);
> 
> s/NULL/false/

D'oh.  And yet it still compiles.  Fixing.

> 
> also, fit in one line

Yes, this one's shorter.  Fixing.


>> +
>> +        if (err) {
>> +            error_reportf_err(err, "Operation %s on bitmap %s failed",
> 
> s/failed/failed: /
> 
>> +                              op, bitmap);
>> +            ret = -1;
> 
> dead assignment: you never set ret after first initialization to -1.

Fixing both.

> 
>> +            goto out;
>> +        }
>> +        g_free(act);
>> +    }
>> +
>> +    ret = 0;
>> +
>> + out:
>> +    blk_unref(src);
>> +    blk_unref(blk);
>> +    qemu_opts_del(opts);
>> +    if (ret) {
>> +        return 1;
>> +    }
>> +    return 0;
> 
> Hmm, as it's the only usage of ret, you may initialize it to 1 at 
> function start, and here just "return ret;"

Yep, done.

>> +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
> 
> Not about this patch, but it's a pity that we have triple duplications 
> (triplications ?) of such lines..

Yes, it is annoying.  But as you say, it's a cleanup for another day, 
for someone who is interested.

> 
> With at least s/NULL/false/ and s/failed/failed: / (or with all my tiny 
> suggestions):
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Thanks; squashing in:

diff --git i/qemu-img.c w/qemu-img.c
index 8c99e68ba8aa..f940137cb0e5 100644
--- i/qemu-img.c
+++ w/qemu-img.c
@@ -4493,7 +4493,7 @@ typedef struct ImgBitmapAction {
  static int img_bitmap(int argc, char **argv)
  {
      Error *err = NULL;
-    int c, ret = -1;
+    int c, ret = 1;
      QemuOpts *opts = NULL;
      const char *fmt = NULL, *src_fmt = NULL, *src_filename = NULL;
      const char *filename, *bitmap;
@@ -4641,8 +4641,7 @@ static int img_bitmap(int argc, char **argv)
      }
      bs = blk_bs(blk);
      if (src_filename) {
-        src = img_open(NULL, src_filename, src_fmt, 0, false, false,
-                       false);
+        src = img_open(false, src_filename, src_fmt, 0, false, false, 
false);
          if (!src) {
              goto out;
          }
@@ -4695,9 +4694,8 @@ static int img_bitmap(int argc, char **argv)
          }

          if (err) {
-            error_reportf_err(err, "Operation %s on bitmap %s failed",
+            error_reportf_err(err, "Operation %s on bitmap %s failed: ",
                                op, bitmap);
-            ret = -1;
              goto out;
          }
          g_free(act);
@@ -4709,10 +4707,7 @@ static int img_bitmap(int argc, char **argv)
      blk_unref(src);
      blk_unref(blk);
      qemu_opts_del(opts);
-    if (ret) {
-        return 1;
-    }
-    return 0;
+    return ret;
  }

  #define C_BS      01

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org



^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [PATCH v4 7/9] qcow2: Expose bitmaps' size during measure
  2020-05-18 13:07   ` Vladimir Sementsov-Ogievskiy
@ 2020-05-18 19:17     ` Eric Blake
  2020-05-18 19:47       ` Vladimir Sementsov-Ogievskiy
  0 siblings, 1 reply; 30+ messages in thread
From: Eric Blake @ 2020-05-18 19:17 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy, qemu-devel
  Cc: kwolf, nsoffer, Markus Armbruster, qemu-block, mreitz

On 5/18/20 8:07 AM, Vladimir Sementsov-Ogievskiy wrote:
> 13.05.2020 04:16, Eric Blake wrote:
>> It's useful to know how much space can be occupied by qcow2 persistent
>> bitmaps, even though such metadata is unrelated to the guest-visible
>> data.  Report this value as an additional QMP field, present when
>> measuring an existing image and output format that both support
>> bitmaps.  Update iotest 178 and 190 to updated output, as well as new
>> coverage in 190 demonstrating non-zero values made possible with the
>> recently-added qemu-img bitmap command.
>>

>> @@ -616,6 +616,7 @@ Command description:
>>
>>       required size: 524288
>>       fully allocated size: 1074069504
>> +    bitmaps: 0
>>
>>     The ``required size`` is the file size of the new image.  It may 
>> be smaller
>>     than the virtual disk size if the image format supports compact 
>> representation.
>> @@ -625,6 +626,13 @@ Command description:
>>     occupy with the exception of internal snapshots, dirty bitmaps, 
>> vmstate data,
>>     and other advanced image format features.
>>
>> +  The ``bitmaps size`` is the additional size required if the
> 
> you called it "bitmaps" in example output above. Should it be 
> consistent? Either "``bitmaps``" here, or "bitmaps size: 0" above?

"bitmaps size: 0" is better. Will fix the description above.

>> +++ b/qapi/block-core.json
>> @@ -633,18 +633,23 @@
>>   # efficiently so file size may be smaller than virtual disk size.
>>   #
>>   # The values are upper bounds that are guaranteed to fit the new 
>> image file.
>> -# Subsequent modification, such as internal snapshot or bitmap 
>> creation, may
>> -# require additional space and is not covered here.
>> +# Subsequent modification, such as internal snapshot or further bitmap
>> +# creation, may require additional space and is not covered here.
>>   #
>> -# @required: Size required for a new image file, in bytes.
>> +# @required: Size required for a new image file, in bytes, when 
>> copying just
>> +#            guest-visible contents.
>>   #
>>   # @fully-allocated: Image file size, in bytes, once data has been 
>> written
>> -#                   to all sectors.
>> +#                   to all sectors, when copying just guest-visible 
>> contents.
> 
> "copying just guest-visible" sounds like something less than "all 
> fully-allocated sectors"..
> But I don't have better suggestion.. Just, "not including bitmaps" 
> sounds weird too.

If we ever add support for copying internal snapshots, that would not be 
included either.  Maybe "copying just allocated guest-visible contents" 
for @required, and no change to the wording for @fully-allocated.


>> @@ -4796,13 +4797,38 @@ static BlockMeasureInfo 
>> *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,

>>
>> +        FOR_EACH_DIRTY_BITMAP(in_bs, bm) {
>> +            if (bdrv_dirty_bitmap_get_persistence(bm)) {
>> +                const char *name = bdrv_dirty_bitmap_name(bm);
>> +                uint32_t granularity = 
>> bdrv_dirty_bitmap_granularity(bm);
>> +                uint64_t bmbits = 
>> DIV_ROUND_UP(bdrv_dirty_bitmap_size(bm),
>> +                                               granularity);
>> +                uint64_t bmclusters = DIV_ROUND_UP(DIV_ROUND_UP(bmbits,
>> +                                                                
>> CHAR_BIT),
>> +                                                   cluster_size);
>> +
>> +                /* Assume the entire bitmap is allocated */
>> +                bitmaps_size += bmclusters * cluster_size;
>> +                /* Also reserve space for the bitmap table entries */
>> +                bitmaps_size += ROUND_UP(bmclusters * sizeof(uint64_t),
>> +                                         cluster_size);
>> +                /* And space for contribution to bitmap directory 
>> size */
>> +                bitmap_dir_size += ROUND_UP(strlen(name) + 24,
>> +                                            sizeof(uint64_t));
> 
> Could we instead reuse code from qcow2_co_can_store_new_dirty_bitmap(), 
> which calls calc_dir_entry_size() for this thing?
> Possibly, make a function qcow2_measure_bitmaps in block/qcow2-bitmaps.c 
> with this FOR_EACH? All details about qcow2 bitmap structures sounds 
> better in block/qcow2-bitmaps.c

Could do.  Sounds like I'm better off submitting a v5 for this patch, 
although I'll go ahead and stage 1-6 for pull request today to minimize 
future rebase churn.


>> +    info->has_bitmaps = version >= 3 && in_bs &&
>> +        bdrv_supports_persistent_dirty_bitmap(in_bs);
>> +    info->bitmaps = bitmaps_size;
> 
> AFAIK, in QAPI, if has_<something> field is false, than <something> must 
> be zero. Maybe, it's only about nested structured fields, not about 
> simple numbers, but I think it's better keep bitmaps 0 in case when 
> has_bitmaps is false.

During creation (including when parsing QMP from the user over the 
monitor), everything is indeed guaranteed to be zero-initialized.  But 
we don't have any requirement that things remain zero-initialized even 
when has_FOO is false; at the same time, it's easy enough to make this 
code conditional.

> 
> Also, it seems a bit better to check version earlier, and don't do all 
> the calculations, if we are not going to use them.. But it's a rare 
> backward-compatibility case, I don't care.

I'll see how easy or hard it is for my v5 patch.


>> @@ -5275,9 +5285,24 @@ static int img_measure(int argc, char **argv)
>>           goto out;
>>       }
>>
>> +    if (bitmaps) {
>> +        if (!info->has_bitmaps) {
>> +            error_report("no bitmaps measured, either source or 
>> destination "
>> +                         "format lacks bitmap support");
>> +            goto out;
>> +        } else {
>> +            info->required += info->bitmaps;
>> +            info->fully_allocated += info->bitmaps;
>> +            info->has_bitmaps = false;
> 
> And here, I think better to zero info->bitmaps as well.

Here, the object is going to be subsequently freed; I'm less worried 
about wasting time doing local cleanups than I am about the earlier case 
about letting an object escape immediate scope in a different state than 
the usual preconditions.


>> +$QEMU_IMG bitmap --add --granularity 512 -f qcow2 "$TEST_IMG" b1
>> +$QEMU_IMG bitmap --add -g 2M -f qcow2 "$TEST_IMG" b2
>> +
>> +# No bitmap without a source
>> +$QEMU_IMG measure --bitmaps -O qcow2 --size 10M
> 
> should this be ored to  'echo "unexpected success"' as following failures?
> 

Can't hurt.


>> +# Compute expected output:
>> +echo
>> +val2T=$((2*1024*1024*1024*1024))
>> +cluster=$((64*1024))
>> +b1clusters=$(( (val2T/512/8 + cluster - 1) / cluster ))
>> +b2clusters=$(( (val2T/2/1024/1024/8 + cluster - 1) / cluster ))
> 
> comment on the following calculations won't hurt, at least something like
>   "bitmap clusters + bitmap tables + bitmaps directory"

Sure.

> 
>> +echo expected bitmap $((b1clusters * cluster +
>> +            (b1clusters * 8 + cluster - 1) / cluster * cluster +
>> +            b2clusters * cluster +
>> +            (b2clusters * 8 + cluster - 1) / cluster * cluster +
>> +            cluster))
>> +$QEMU_IMG measure -O qcow2 -o cluster_size=64k -f qcow2 "$TEST_IMG"
>> +$QEMU_IMG measure --bitmaps -O qcow2 -o cluster_size=64k -f qcow2 
>> "$TEST_IMG"
>> +

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org



^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v4 6/9] qemu-img: Add bitmap sub-command
  2020-05-18 19:07     ` Eric Blake
@ 2020-05-18 19:38       ` Vladimir Sementsov-Ogievskiy
  0 siblings, 0 replies; 30+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-18 19:38 UTC (permalink / raw)
  To: Eric Blake, qemu-devel; +Cc: kwolf, nsoffer, qemu-block, mreitz

18.05.2020 22:07, Eric Blake wrote:
> On 5/18/20 6:42 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 13.05.2020 04:16, 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.
>>>
>>> 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.
>>>
> 
>>> +
>>> +    blk = img_open(image_opts, filename, fmt, BDRV_O_RDWR, false, false,
>>> +                   false);
>>
>> fit in one line
> 
> That line would be exactly 80; I tend to wrap at 79 or earlier rather than exactly on 80.
> 
>>
>>> +    if (!blk) {
>>> +        goto out;
>>> +    }
>>> +    bs = blk_bs(blk);
>>> +    if (src_filename) {
>>> +        src = img_open(NULL, src_filename, src_fmt, 0, false, false,
>>> +                       false);
>>
>> s/NULL/false/
> 
> D'oh.  And yet it still compiles.  Fixing.
> 
>>
>> also, fit in one line
> 
> Yes, this one's shorter.  Fixing.
> 
> 
>>> +
>>> +        if (err) {
>>> +            error_reportf_err(err, "Operation %s on bitmap %s failed",
>>
>> s/failed/failed: /
>>
>>> +                              op, bitmap);
>>> +            ret = -1;
>>
>> dead assignment: you never set ret after first initialization to -1.
> 
> Fixing both.
> 
>>
>>> +            goto out;
>>> +        }
>>> +        g_free(act);
>>> +    }
>>> +
>>> +    ret = 0;
>>> +
>>> + out:
>>> +    blk_unref(src);
>>> +    blk_unref(blk);
>>> +    qemu_opts_del(opts);
>>> +    if (ret) {
>>> +        return 1;
>>> +    }
>>> +    return 0;
>>
>> Hmm, as it's the only usage of ret, you may initialize it to 1 at function start, and here just "return ret;"
> 
> Yep, done.
> 
>>> +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
>>
>> Not about this patch, but it's a pity that we have triple duplications (triplications ?) of such lines..
> 
> Yes, it is annoying.  But as you say, it's a cleanup for another day, for someone who is interested.
> 
>>
>> With at least s/NULL/false/ and s/failed/failed: / (or with all my tiny suggestions):
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> Thanks; squashing in:
> 
> diff --git i/qemu-img.c w/qemu-img.c
> index 8c99e68ba8aa..f940137cb0e5 100644
> --- i/qemu-img.c
> +++ w/qemu-img.c
> @@ -4493,7 +4493,7 @@ typedef struct ImgBitmapAction {
>   static int img_bitmap(int argc, char **argv)
>   {
>       Error *err = NULL;
> -    int c, ret = -1;
> +    int c, ret = 1;
>       QemuOpts *opts = NULL;
>       const char *fmt = NULL, *src_fmt = NULL, *src_filename = NULL;
>       const char *filename, *bitmap;
> @@ -4641,8 +4641,7 @@ static int img_bitmap(int argc, char **argv)
>       }
>       bs = blk_bs(blk);
>       if (src_filename) {
> -        src = img_open(NULL, src_filename, src_fmt, 0, false, false,
> -                       false);
> +        src = img_open(false, src_filename, src_fmt, 0, false, false, false);
>           if (!src) {
>               goto out;
>           }
> @@ -4695,9 +4694,8 @@ static int img_bitmap(int argc, char **argv)
>           }
> 
>           if (err) {
> -            error_reportf_err(err, "Operation %s on bitmap %s failed",
> +            error_reportf_err(err, "Operation %s on bitmap %s failed: ",
>                                 op, bitmap);
> -            ret = -1;
>               goto out;
>           }
>           g_free(act);
> @@ -4709,10 +4707,7 @@ static int img_bitmap(int argc, char **argv)
>       blk_unref(src);
>       blk_unref(blk);
>       qemu_opts_del(opts);
> -    if (ret) {
> -        return 1;
> -    }
> -    return 0;
> +    return ret;
>   }
> 
>   #define C_BS      01
> 

Great, keep my r-b with this.

-- 
Best regards,
Vladimir


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v4 7/9] qcow2: Expose bitmaps' size during measure
  2020-05-18 19:17     ` Eric Blake
@ 2020-05-18 19:47       ` Vladimir Sementsov-Ogievskiy
  0 siblings, 0 replies; 30+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-18 19:47 UTC (permalink / raw)
  To: Eric Blake, qemu-devel
  Cc: kwolf, nsoffer, Markus Armbruster, qemu-block, mreitz

18.05.2020 22:17, Eric Blake wrote:
> On 5/18/20 8:07 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 13.05.2020 04:16, Eric Blake wrote:
>>> It's useful to know how much space can be occupied by qcow2 persistent
>>> bitmaps, even though such metadata is unrelated to the guest-visible
>>> data.  Report this value as an additional QMP field, present when
>>> measuring an existing image and output format that both support
>>> bitmaps.  Update iotest 178 and 190 to updated output, as well as new
>>> coverage in 190 demonstrating non-zero values made possible with the
>>> recently-added qemu-img bitmap command.
>>>
> 
>>> @@ -616,6 +616,7 @@ Command description:
>>>
>>>       required size: 524288
>>>       fully allocated size: 1074069504
>>> +    bitmaps: 0
>>>
>>>     The ``required size`` is the file size of the new image.  It may be smaller
>>>     than the virtual disk size if the image format supports compact representation.
>>> @@ -625,6 +626,13 @@ Command description:
>>>     occupy with the exception of internal snapshots, dirty bitmaps, vmstate data,
>>>     and other advanced image format features.
>>>
>>> +  The ``bitmaps size`` is the additional size required if the
>>
>> you called it "bitmaps" in example output above. Should it be consistent? Either "``bitmaps``" here, or "bitmaps size: 0" above?
> 
> "bitmaps size: 0" is better. Will fix the description above.
> 
>>> +++ b/qapi/block-core.json
>>> @@ -633,18 +633,23 @@
>>>   # efficiently so file size may be smaller than virtual disk size.
>>>   #
>>>   # The values are upper bounds that are guaranteed to fit the new image file.
>>> -# Subsequent modification, such as internal snapshot or bitmap creation, may
>>> -# require additional space and is not covered here.
>>> +# Subsequent modification, such as internal snapshot or further bitmap
>>> +# creation, may require additional space and is not covered here.
>>>   #
>>> -# @required: Size required for a new image file, in bytes.
>>> +# @required: Size required for a new image file, in bytes, when copying just
>>> +#            guest-visible contents.
>>>   #
>>>   # @fully-allocated: Image file size, in bytes, once data has been written
>>> -#                   to all sectors.
>>> +#                   to all sectors, when copying just guest-visible contents.
>>
>> "copying just guest-visible" sounds like something less than "all fully-allocated sectors"..
>> But I don't have better suggestion.. Just, "not including bitmaps" sounds weird too.
> 
> If we ever add support for copying internal snapshots, that would not be included either.  Maybe "copying just allocated guest-visible contents" for @required, and no change to the wording for @fully-allocated.

Not sure is it better or not, so, I'm OK with any variant.

> 
> 
>>> @@ -4796,13 +4797,38 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
> 
>>>
>>> +        FOR_EACH_DIRTY_BITMAP(in_bs, bm) {
>>> +            if (bdrv_dirty_bitmap_get_persistence(bm)) {
>>> +                const char *name = bdrv_dirty_bitmap_name(bm);
>>> +                uint32_t granularity = bdrv_dirty_bitmap_granularity(bm);
>>> +                uint64_t bmbits = DIV_ROUND_UP(bdrv_dirty_bitmap_size(bm),
>>> +                                               granularity);
>>> +                uint64_t bmclusters = DIV_ROUND_UP(DIV_ROUND_UP(bmbits,
>>> + CHAR_BIT),
>>> +                                                   cluster_size);
>>> +
>>> +                /* Assume the entire bitmap is allocated */
>>> +                bitmaps_size += bmclusters * cluster_size;
>>> +                /* Also reserve space for the bitmap table entries */
>>> +                bitmaps_size += ROUND_UP(bmclusters * sizeof(uint64_t),
>>> +                                         cluster_size);
>>> +                /* And space for contribution to bitmap directory size */
>>> +                bitmap_dir_size += ROUND_UP(strlen(name) + 24,
>>> +                                            sizeof(uint64_t));
>>
>> Could we instead reuse code from qcow2_co_can_store_new_dirty_bitmap(), which calls calc_dir_entry_size() for this thing?
>> Possibly, make a function qcow2_measure_bitmaps in block/qcow2-bitmaps.c with this FOR_EACH? All details about qcow2 bitmap structures sounds better in block/qcow2-bitmaps.c
> 
> Could do.  Sounds like I'm better off submitting a v5 for this patch, although I'll go ahead and stage 1-6 for pull request today to minimize future rebase churn.

Thanks!

> 
> 
>>> +    info->has_bitmaps = version >= 3 && in_bs &&
>>> +        bdrv_supports_persistent_dirty_bitmap(in_bs);
>>> +    info->bitmaps = bitmaps_size;
>>
>> AFAIK, in QAPI, if has_<something> field is false, than <something> must be zero. Maybe, it's only about nested structured fields, not about simple numbers, but I think it's better keep bitmaps 0 in case when has_bitmaps is false.
> 
> During creation (including when parsing QMP from the user over the monitor), everything is indeed guaranteed to be zero-initialized.  But we don't have any requirement that things remain zero-initialized even when has_FOO is false; at the same time, it's easy enough to make this code conditional.
> 
>>
>> Also, it seems a bit better to check version earlier, and don't do all the calculations, if we are not going to use them.. But it's a rare backward-compatibility case, I don't care.
> 
> I'll see how easy or hard it is for my v5 patch.
> 
> 
>>> @@ -5275,9 +5285,24 @@ static int img_measure(int argc, char **argv)
>>>           goto out;
>>>       }
>>>
>>> +    if (bitmaps) {
>>> +        if (!info->has_bitmaps) {
>>> +            error_report("no bitmaps measured, either source or destination "
>>> +                         "format lacks bitmap support");
>>> +            goto out;
>>> +        } else {
>>> +            info->required += info->bitmaps;
>>> +            info->fully_allocated += info->bitmaps;
>>> +            info->has_bitmaps = false;
>>
>> And here, I think better to zero info->bitmaps as well.
> 
> Here, the object is going to be subsequently freed; I'm less worried about wasting time doing local cleanups than I am about the earlier case about letting an object escape immediate scope in a different state than the usual preconditions.
> 
> 
>>> +$QEMU_IMG bitmap --add --granularity 512 -f qcow2 "$TEST_IMG" b1
>>> +$QEMU_IMG bitmap --add -g 2M -f qcow2 "$TEST_IMG" b2
>>> +
>>> +# No bitmap without a source
>>> +$QEMU_IMG measure --bitmaps -O qcow2 --size 10M
>>
>> should this be ored to  'echo "unexpected success"' as following failures?
>>
> 
> Can't hurt.
> 
> 
>>> +# Compute expected output:
>>> +echo
>>> +val2T=$((2*1024*1024*1024*1024))
>>> +cluster=$((64*1024))
>>> +b1clusters=$(( (val2T/512/8 + cluster - 1) / cluster ))
>>> +b2clusters=$(( (val2T/2/1024/1024/8 + cluster - 1) / cluster ))
>>
>> comment on the following calculations won't hurt, at least something like
>>   "bitmap clusters + bitmap tables + bitmaps directory"
> 
> Sure.
> 
>>
>>> +echo expected bitmap $((b1clusters * cluster +
>>> +            (b1clusters * 8 + cluster - 1) / cluster * cluster +
>>> +            b2clusters * cluster +
>>> +            (b2clusters * 8 + cluster - 1) / cluster * cluster +
>>> +            cluster))
>>> +$QEMU_IMG measure -O qcow2 -o cluster_size=64k -f qcow2 "$TEST_IMG"
>>> +$QEMU_IMG measure --bitmaps -O qcow2 -o cluster_size=64k -f qcow2 "$TEST_IMG"
>>> +
> 


-- 
Best regards,
Vladimir


^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2020-05-18 20:00 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-13  1:16 [PATCH v4 0/9] qemu-img: Add convert --bitmaps Eric Blake
2020-05-13  1:16 ` [PATCH v4 1/9] docs: Sort sections on qemu-img subcommand parameters Eric Blake
2020-05-14  5:02   ` Vladimir Sementsov-Ogievskiy
2020-05-13  1:16 ` [PATCH v4 2/9] qemu-img: Fix stale comments on doc location Eric Blake
2020-05-14  5:06   ` Vladimir Sementsov-Ogievskiy
2020-05-13  1:16 ` [PATCH v4 3/9] block: Make it easier to learn which BDS support bitmaps Eric Blake
2020-05-14  5:19   ` Vladimir Sementsov-Ogievskiy
2020-05-14 14:09     ` Eric Blake
2020-05-13  1:16 ` [PATCH v4 4/9] blockdev: Promote several bitmap functions to non-static Eric Blake
2020-05-14  5:45   ` Vladimir Sementsov-Ogievskiy
2020-05-14 11:45   ` Vladimir Sementsov-Ogievskiy
2020-05-14 14:10     ` Eric Blake
2020-05-13  1:16 ` [PATCH v4 5/9] blockdev: Split off basic bitmap operations for qemu-img Eric Blake
2020-05-14  6:21   ` Vladimir Sementsov-Ogievskiy
2020-05-14 14:15     ` Eric Blake
2020-05-13  1:16 ` [PATCH v4 6/9] qemu-img: Add bitmap sub-command Eric Blake
2020-05-14  6:45   ` Vladimir Sementsov-Ogievskiy
2020-05-14 14:20     ` Eric Blake
2020-05-14 15:09       ` Vladimir Sementsov-Ogievskiy
2020-05-18 11:42   ` Vladimir Sementsov-Ogievskiy
2020-05-18 19:07     ` Eric Blake
2020-05-18 19:38       ` Vladimir Sementsov-Ogievskiy
2020-05-13  1:16 ` [PATCH v4 7/9] qcow2: Expose bitmaps' size during measure Eric Blake
2020-05-18 13:07   ` Vladimir Sementsov-Ogievskiy
2020-05-18 19:17     ` Eric Blake
2020-05-18 19:47       ` Vladimir Sementsov-Ogievskiy
2020-05-13  1:16 ` [PATCH v4 8/9] qemu-img: Add convert --bitmaps option Eric Blake
2020-05-18 13:33   ` Vladimir Sementsov-Ogievskiy
2020-05-13  1:16 ` [PATCH v4 9/9] iotests: Add test 291 to for qemu-img bitmap coverage Eric Blake
2020-05-18 14:43   ` Vladimir Sementsov-Ogievskiy

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.