[Qemu-devel] [PATCH v2 0/6] qmp dirty bitmap API

[Qemu-devel] [PATCH v2 0/6] qmp dirty bitmap API

[Vladimir Sementsov-Ogievskiy]

Hi all.

There are three qmp commands, needed to implement external backup API.

Using these three commands, client may do all needed bitmap management by
hand:

on backup start we need to do a transaction:
 {disable old bitmap, create new bitmap}

on backup success:
 drop old bitmap

on backup fail:
 enable old bitmap
 merge new bitmap to old bitmap
 drop new bitmap

v2: fix merge command deadlock
  add new patches: 1 and 6

Vladimir Sementsov-Ogievskiy (6):
  block: maintain persistent disabled bitmaps
  block/dirty-bitmap: add lock to bdrv_enable/disable_dirty_bitmap
  qapi: add block-dirty-bitmap-enable/disable
  qmp: transaction support for block-dirty-bitmap-enable/disable
  qapi: add block-dirty-bitmap-merge
  qapi: add disabled parameter to block-dirty-bitmap-add

 qapi/block-core.json         |  92 ++++++++++++++++++++++-
 qapi/transaction.json        |   4 +
 block/qcow2.h                |   2 +-
 include/block/dirty-bitmap.h |   3 +-
 block/dirty-bitmap.c         |  42 ++++++-----
 block/qcow2-bitmap.c         |  12 +--
 block/qcow2.c                |   2 +-
 blockdev.c                   | 169 +++++++++++++++++++++++++++++++++++++++++--
 8 files changed, 287 insertions(+), 39 deletions(-)

-- 
2.11.1

[Qemu-devel] [PATCH v2 1/6] block: maintain persistent disabled bitmaps

[Vladimir Sementsov-Ogievskiy]

To maintain load/store disabled bitmap there is new approach:

 - deprecate @autoload flag of block-dirty-bitmap-add, make it ignored
 - store enabled bitmaps as "auto" to qcow2
 - store disabled bitmaps without "auto" flag to qcow2
 - on qcow2 open load "auto" bitmaps as enabled and others
   as disabled (except in_use bitmaps)

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block-core.json         |  6 +++---
 block/qcow2.h                |  2 +-
 include/block/dirty-bitmap.h |  1 -
 block/dirty-bitmap.c         | 18 ------------------
 block/qcow2-bitmap.c         | 12 +++++++-----
 block/qcow2.c                |  2 +-
 blockdev.c                   | 10 ++--------
 7 files changed, 14 insertions(+), 37 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index ab96e348e6..827254db22 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1593,9 +1593,9 @@
 #              Qcow2 disks support persistent bitmaps. Default is false for
 #              block-dirty-bitmap-add. (Since: 2.10)
 #
-# @autoload: the bitmap will be automatically loaded when the image it is stored
-#            in is opened. This flag may only be specified for persistent
-#            bitmaps. Default is false for block-dirty-bitmap-add. (Since: 2.10)
+# @autoload: ignored and deprecated since 2.12.
+#            Currently, all dirty tracking bitmaps are loaded from Qcow2 on
+#            open.
 #
 # Since: 2.4
 ##
diff --git a/block/qcow2.h b/block/qcow2.h
index 782a206ecb..a3e29276fc 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -672,7 +672,7 @@ void qcow2_cache_discard(BlockDriverState *bs, Qcow2Cache *c, void *table);
 int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
                                   void **refcount_table,
                                   int64_t *refcount_table_size);
-bool qcow2_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp);
+bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
 void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 3579a7597c..144e77a879 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -65,7 +65,6 @@ void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap,
 void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
 
 void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap, bool value);
-void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload);
 void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
                                        bool persistent);
 
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index bd04e991b1..3777be1985 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -52,8 +52,6 @@ struct BdrvDirtyBitmap {
                                    Such operations must fail and both the image
                                    and this bitmap must remain unchanged while
                                    this flag is set. */
-    bool autoload;              /* For persistent bitmaps: bitmap must be
-                                   autoloaded on image opening */
     bool persistent;            /* bitmap must be saved to owner disk image */
     QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
@@ -104,7 +102,6 @@ void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap)
     g_free(bitmap->name);
     bitmap->name = NULL;
     bitmap->persistent = false;
-    bitmap->autoload = false;
 }
 
 /* Called with BQL taken.  */
@@ -261,8 +258,6 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
     bitmap->successor = NULL;
     successor->persistent = bitmap->persistent;
     bitmap->persistent = false;
-    successor->autoload = bitmap->autoload;
-    bitmap->autoload = false;
     bdrv_release_dirty_bitmap(bs, bitmap);
 
     return successor;
@@ -667,19 +662,6 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs)
 }
 
 /* Called with BQL taken. */
-void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload)
-{
-    qemu_mutex_lock(bitmap->mutex);
-    bitmap->autoload = autoload;
-    qemu_mutex_unlock(bitmap->mutex);
-}
-
-bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap)
-{
-    return bitmap->autoload;
-}
-
-/* Called with BQL taken. */
 void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, bool persistent)
 {
     qemu_mutex_lock(bitmap->mutex);
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index f45e46cfbd..ae14464de6 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -933,14 +933,14 @@ static void set_readonly_helper(gpointer bitmap, gpointer value)
     bdrv_dirty_bitmap_set_readonly(bitmap, (bool)value);
 }
 
-/* qcow2_load_autoloading_dirty_bitmaps()
+/* qcow2_load_dirty_bitmaps()
  * Return value is a hint for caller: true means that the Qcow2 header was
  * updated. (false doesn't mean that the header should be updated by the
  * caller, it just means that updating was not needed or the image cannot be
  * written to).
  * On failure the function returns false.
  */
-bool qcow2_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp)
+bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
 {
     BDRVQcow2State *s = bs->opaque;
     Qcow2BitmapList *bm_list;
@@ -960,14 +960,16 @@ bool qcow2_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp)
     }
 
     QSIMPLEQ_FOREACH(bm, bm_list, entry) {
-        if ((bm->flags & BME_FLAG_AUTO) && !(bm->flags & BME_FLAG_IN_USE)) {
+        if (!(bm->flags & BME_FLAG_IN_USE)) {
             BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
             if (bitmap == NULL) {
                 goto fail;
             }
 
+            if (!(bm->flags & BME_FLAG_AUTO)) {
+                bdrv_disable_dirty_bitmap(bitmap);
+            }
             bdrv_dirty_bitmap_set_persistance(bitmap, true);
-            bdrv_dirty_bitmap_set_autoload(bitmap, true);
             bm->flags |= BME_FLAG_IN_USE;
             created_dirty_bitmaps =
                     g_slist_append(created_dirty_bitmaps, bitmap);
@@ -1369,7 +1371,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
             bm->table.size = 0;
             QSIMPLEQ_INSERT_TAIL(&drop_tables, tb, entry);
         }
-        bm->flags = bdrv_dirty_bitmap_get_autoload(bitmap) ? BME_FLAG_AUTO : 0;
+        bm->flags = bdrv_dirty_bitmap_enabled(bitmap) ? BME_FLAG_AUTO : 0;
         bm->granularity_bits = ctz32(bdrv_dirty_bitmap_granularity(bitmap));
         bm->dirty_bitmap = bitmap;
     }
diff --git a/block/qcow2.c b/block/qcow2.c
index 92cb9f9bfa..93c3a97cfe 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1441,7 +1441,7 @@ static int qcow2_do_open(BlockDriverState *bs, QDict *options, int flags,
         s->autoclear_features &= QCOW2_AUTOCLEAR_MASK;
     }
 
-    if (qcow2_load_autoloading_dirty_bitmaps(bs, &local_err)) {
+    if (qcow2_load_dirty_bitmaps(bs, &local_err)) {
         update_header = false;
     }
     if (local_err != NULL) {
diff --git a/blockdev.c b/blockdev.c
index 56a6b24a0b..8068cbd606 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2738,14 +2738,9 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
     if (!has_persistent) {
         persistent = false;
     }
-    if (!has_autoload) {
-        autoload = false;
-    }
 
-    if (has_autoload && !persistent) {
-        error_setg(errp, "Autoload flag must be used only for persistent "
-                         "bitmaps");
-        return;
+    if (has_autoload) {
+        warn_report("Autoload option is deprected and its value is ignored");
     }
 
     if (persistent &&
@@ -2760,7 +2755,6 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
     }
 
     bdrv_dirty_bitmap_set_persistance(bitmap, persistent);
-    bdrv_dirty_bitmap_set_autoload(bitmap, autoload);
 }
 
 void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
-- 
2.11.1

[Qemu-devel] [PATCH v2 2/6] block/dirty-bitmap: add lock to bdrv_enable/disable_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

Add locks and remove comments about BQL accordingly to
dirty_bitmap_mutex definition in block_int.h.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/dirty-bitmap.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 3777be1985..d0a10c4f5d 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -389,18 +389,20 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
     }
 }
 
-/* Called with BQL taken.  */
 void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
 {
+    bdrv_dirty_bitmap_lock(bitmap);
     assert(!bdrv_dirty_bitmap_frozen(bitmap));
     bitmap->disabled = true;
+    bdrv_dirty_bitmap_unlock(bitmap);
 }
 
-/* Called with BQL taken.  */
 void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
 {
+    bdrv_dirty_bitmap_lock(bitmap);
     assert(!bdrv_dirty_bitmap_frozen(bitmap));
     bitmap->disabled = false;
+    bdrv_dirty_bitmap_unlock(bitmap);
 }
 
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
-- 
2.11.1

[Qemu-devel] [PATCH v2 3/6] qapi: add block-dirty-bitmap-enable/disable

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block-core.json | 42 ++++++++++++++++++++++++++++++++++++++++++
 blockdev.c           | 42 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 827254db22..b3851ee760 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1672,6 +1672,48 @@
   'data': 'BlockDirtyBitmap' }
 
 ##
+# @block-dirty-bitmap-enable:
+#
+# Enable dirty bitmap, so that it will continue tracking disk changes.
+#
+# Returns: nothing on success
+#          If @node is not a valid block device, DeviceNotFound
+#          If @name is not found, GenericError with an explanation
+#
+# Since: 2.12
+#
+# Example:
+#
+# -> { "execute": "block-dirty-bitmap-enable",
+#      "arguments": { "node": "drive0", "name": "bitmap0" } }
+# <- { "return": {} }
+#
+##
+  { 'command': 'block-dirty-bitmap-enable',
+    'data': 'BlockDirtyBitmap' }
+
+##
+# @block-dirty-bitmap-disable:
+#
+# Disable dirty bitmap, so that it will stop tracking disk changes.
+#
+# Returns: nothing on success
+#          If @node is not a valid block device, DeviceNotFound
+#          If @name is not found, GenericError with an explanation
+#
+# Since: 2.12
+#
+# Example:
+#
+# -> { "execute": "block-dirty-bitmap-disable",
+#      "arguments": { "node": "drive0", "name": "bitmap0" } }
+# <- { "return": {} }
+#
+##
+    { 'command': 'block-dirty-bitmap-disable',
+      'data': 'BlockDirtyBitmap' }
+
+##
 # @BlockDirtyBitmapSha256:
 #
 # SHA256 hash of dirty bitmap data
diff --git a/blockdev.c b/blockdev.c
index 8068cbd606..997a6f514c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2821,6 +2821,48 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
     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 || !bs) {
+        return;
+    }
+
+    if (bdrv_dirty_bitmap_frozen(bitmap)) {
+        error_setg(errp,
+                   "Bitmap '%s' is currently frozen and cannot be enabled",
+                   name);
+        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 || !bs) {
+        return;
+    }
+
+    if (bdrv_dirty_bitmap_frozen(bitmap)) {
+        error_setg(errp,
+                   "Bitmap '%s' is currently frozen and cannot be disabled",
+                   name);
+        return;
+    }
+
+    bdrv_disable_dirty_bitmap(bitmap);
+}
+
 BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
                                                               const char *name,
                                                               Error **errp)
-- 
2.11.1

[Qemu-devel] [PATCH v2 4/6] qmp: transaction support for block-dirty-bitmap-enable/disable

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/transaction.json |  4 +++
 blockdev.c            | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 83 insertions(+)

diff --git a/qapi/transaction.json b/qapi/transaction.json
index bd312792da..b643d848f8 100644
--- a/qapi/transaction.json
+++ b/qapi/transaction.json
@@ -46,6 +46,8 @@
 # - @abort: since 1.6
 # - @block-dirty-bitmap-add: since 2.5
 # - @block-dirty-bitmap-clear: since 2.5
+# - @block-dirty-bitmap-enable: since 2.12
+# - @block-dirty-bitmap-disable: since 2.12
 # - @blockdev-backup: since 2.3
 # - @blockdev-snapshot: since 2.5
 # - @blockdev-snapshot-internal-sync: since 1.7
@@ -59,6 +61,8 @@
        'abort': 'Abort',
        'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
        'block-dirty-bitmap-clear': 'BlockDirtyBitmap',
+       'block-dirty-bitmap-enable': 'BlockDirtyBitmap',
+       'block-dirty-bitmap-disable': 'BlockDirtyBitmap',
        'blockdev-backup': 'BlockdevBackup',
        'blockdev-snapshot': 'BlockdevSnapshot',
        'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
diff --git a/blockdev.c b/blockdev.c
index 997a6f514c..d338363d78 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1962,6 +1962,7 @@ typedef struct BlockDirtyBitmapState {
     AioContext *aio_context;
     HBitmap *backup;
     bool prepared;
+    bool was_enabled;
 } BlockDirtyBitmapState;
 
 static void block_dirty_bitmap_add_prepare(BlkActionState *common,
@@ -2069,6 +2070,74 @@ static void block_dirty_bitmap_clear_clean(BlkActionState *common)
     }
 }
 
+static void block_dirty_bitmap_enable_prepare(BlkActionState *common,
+                                              Error **errp)
+{
+    BlockDirtyBitmap *action;
+    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+                                             common, common);
+
+    if (action_check_completion_mode(common, errp) < 0) {
+        return;
+    }
+
+    action = common->action->u.block_dirty_bitmap_enable.data;
+    state->bitmap = block_dirty_bitmap_lookup(action->node,
+                                              action->name,
+                                              &state->bs,
+                                              errp);
+    if (!state->bitmap) {
+        return;
+    }
+
+    state->was_enabled = bdrv_dirty_bitmap_enabled(state->bitmap);
+    bdrv_enable_dirty_bitmap(state->bitmap);
+}
+
+static void block_dirty_bitmap_enable_abort(BlkActionState *common)
+{
+    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+                                             common, common);
+
+    if (!state->was_enabled) {
+        bdrv_disable_dirty_bitmap(state->bitmap);
+    }
+}
+
+static void block_dirty_bitmap_disable_prepare(BlkActionState *common,
+                                               Error **errp)
+{
+    BlockDirtyBitmap *action;
+    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+                                             common, common);
+
+    if (action_check_completion_mode(common, errp) < 0) {
+        return;
+    }
+
+    action = common->action->u.block_dirty_bitmap_disable.data;
+    state->bitmap = block_dirty_bitmap_lookup(action->node,
+                                              action->name,
+                                              &state->bs,
+                                              errp);
+    if (!state->bitmap) {
+        return;
+    }
+
+    state->was_enabled = bdrv_dirty_bitmap_enabled(state->bitmap);
+    bdrv_disable_dirty_bitmap(state->bitmap);
+}
+
+static void block_dirty_bitmap_disable_abort(BlkActionState *common)
+{
+    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+                                             common, common);
+
+    if (state->was_enabled) {
+        bdrv_enable_dirty_bitmap(state->bitmap);
+    }
+}
+
 static void abort_prepare(BlkActionState *common, Error **errp)
 {
     error_setg(errp, "Transaction aborted using Abort action");
@@ -2130,6 +2199,16 @@ static const BlkActionOps actions[] = {
         .commit = block_dirty_bitmap_clear_commit,
         .abort = block_dirty_bitmap_clear_abort,
         .clean = block_dirty_bitmap_clear_clean,
+    },
+    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ENABLE] = {
+        .instance_size = sizeof(BlockDirtyBitmapState),
+        .prepare = block_dirty_bitmap_enable_prepare,
+        .abort = block_dirty_bitmap_enable_abort,
+    },
+    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_DISABLE] = {
+        .instance_size = sizeof(BlockDirtyBitmapState),
+        .prepare = block_dirty_bitmap_disable_prepare,
+        .abort = block_dirty_bitmap_disable_abort,
     }
 };
 
-- 
2.11.1

[Qemu-devel] [PATCH v2 5/6] qapi: add block-dirty-bitmap-merge

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block-core.json         | 38 ++++++++++++++++++++++++++++++++++++++
 include/block/dirty-bitmap.h |  2 ++
 block/dirty-bitmap.c         | 18 ++++++++++++++++++
 blockdev.c                   | 30 ++++++++++++++++++++++++++++++
 4 files changed, 88 insertions(+)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index b3851ee760..9f9cfa0a44 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1604,6 +1604,20 @@
             '*persistent': 'bool', '*autoload': 'bool' } }
 
 ##
+# @BlockDirtyBitmapMerge:
+#
+# @node: name of device/node which the bitmap is tracking
+#
+# @dst_name: name of the destination dirty bitmap
+#
+# @src_name: name of the source dirty bitmap
+#
+# Since: 2.12
+##
+{ 'struct': 'BlockDirtyBitmapMerge',
+  'data': { 'node': 'str', 'dst_name': 'str', 'src_name': 'str' } }
+
+##
 # @block-dirty-bitmap-add:
 #
 # Create a dirty bitmap with a name on the node, and start tracking the writes.
@@ -1714,6 +1728,30 @@
       'data': 'BlockDirtyBitmap' }
 
 ##
+# @block-dirty-bitmap-merge:
+#
+# Merge @src_name dirty bitmap to @dst_name dirty bitmap. @src_name dirty
+# bitmap is unchanged.
+#
+# Returns: nothing on success
+#          If @node is not a valid block device, DeviceNotFound
+#          If @dst_name or @src_name is not found, GenericError
+#          If bitmaps has different sizes or granularities, GenericError
+#
+# Since: 2.12
+#
+# Example:
+#
+# -> { "execute": "block-dirty-bitmap-merge",
+#      "arguments": { "node": "drive0", "dst_name": "bitmap0",
+#                     "src_name": "bitmap1" } }
+# <- { "return": {} }
+#
+##
+      { 'command': 'block-dirty-bitmap-merge',
+        'data': 'BlockDirtyBitmapMerge' }
+
+##
 # @BlockDirtyBitmapSha256:
 #
 # SHA256 hash of dirty bitmap data
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 144e77a879..92a1041368 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -67,6 +67,8 @@ void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap, bool value);
 void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
                                        bool persistent);
+void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
+                             Error **errp);
 
 /* Functions that require manual locking.  */
 void bdrv_dirty_bitmap_lock(BdrvDirtyBitmap *bitmap);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index d0a10c4f5d..afcb3f541e 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -699,3 +699,21 @@ char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp)
 {
     return hbitmap_sha256(bitmap->bitmap, errp);
 }
+
+void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
+                             Error **errp)
+{
+    /* only bitmaps from one bds are supported */
+    assert(dest->mutex == src->mutex);
+
+    qemu_mutex_lock(dest->mutex);
+
+    assert(bdrv_dirty_bitmap_enabled(dest));
+    assert(!bdrv_dirty_bitmap_readonly(dest));
+
+    if (!hbitmap_merge(dest->bitmap, src->bitmap)) {
+        error_setg(errp, "Bitmaps are incompatible and can't be merged");
+    }
+
+    qemu_mutex_unlock(dest->mutex);
+}
diff --git a/blockdev.c b/blockdev.c
index d338363d78..1650c31c87 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2942,6 +2942,36 @@ void qmp_block_dirty_bitmap_disable(const char *node, const char *name,
     bdrv_disable_dirty_bitmap(bitmap);
 }
 
+void qmp_block_dirty_bitmap_merge(const char *node, const char *dst_name,
+                                  const char *src_name, Error **errp)
+{
+    BlockDriverState *bs;
+    BdrvDirtyBitmap *dst, *src;
+
+    dst = block_dirty_bitmap_lookup(node, dst_name, &bs, errp);
+    if (!dst || !bs) {
+        return;
+    }
+
+    if (bdrv_dirty_bitmap_frozen(dst)) {
+        error_setg(errp, "Bitmap '%s' is frozen and cannot be modified",
+                   dst_name);
+        return;
+    } else if (bdrv_dirty_bitmap_readonly(dst)) {
+        error_setg(errp, "Bitmap '%s' is readonly and cannot be modified",
+                   dst_name);
+        return;
+    }
+
+    src = bdrv_find_dirty_bitmap(bs, src_name);
+    if (!src) {
+        error_setg(errp, "Dirty bitmap '%s' not found", src_name);
+        return;
+    }
+
+    bdrv_merge_dirty_bitmap(dst, src, errp);
+}
+
 BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
                                                               const char *name,
                                                               Error **errp)
-- 
2.11.1

[Qemu-devel] [PATCH v2 6/6] qapi: add disabled parameter to block-dirty-bitmap-add

[Vladimir Sementsov-Ogievskiy]

This is needed, for example, to create a new bitmap and merge several
disabled bitmaps into a new one. Without this flag we will have to
put block-dirty-bitmap-add and block-dirty-bitmap-disable into one
transaction.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block-core.json |  6 +++++-
 blockdev.c           | 10 ++++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9f9cfa0a44..de8041d11d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1597,11 +1597,15 @@
 #            Currently, all dirty tracking bitmaps are loaded from Qcow2 on
 #            open.
 #
+# @disabled: bitmap is created in disabled state, which means that it will not
+#            track drive changes. The bitmap may be enabled with
+#            block-dirty-bitmap-enable. Default is false. (Since: 2.12)
+#
 # Since: 2.4
 ##
 { 'struct': 'BlockDirtyBitmapAdd',
   'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
-            '*persistent': 'bool', '*autoload': 'bool' } }
+            '*persistent': 'bool', '*autoload': 'bool', '*disabled': 'bool' } }
 
 ##
 # @BlockDirtyBitmapMerge:
diff --git a/blockdev.c b/blockdev.c
index 1650c31c87..444fccaab4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1983,6 +1983,7 @@ static void block_dirty_bitmap_add_prepare(BlkActionState *common,
                                action->has_granularity, action->granularity,
                                action->has_persistent, action->persistent,
                                action->has_autoload, action->autoload,
+                               action->has_disabled, action->disabled,
                                &local_err);
 
     if (!local_err) {
@@ -2788,6 +2789,7 @@ 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_autoload, bool autoload,
+                                bool has_disabled, bool disabled,
                                 Error **errp)
 {
     BlockDriverState *bs;
@@ -2822,6 +2824,10 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
         warn_report("Autoload option is deprected and its value is ignored");
     }
 
+    if (!has_disabled) {
+        disabled = false;
+    }
+
     if (persistent &&
         !bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp))
     {
@@ -2833,6 +2839,10 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
         return;
     }
 
+    if (disabled) {
+        bdrv_disable_dirty_bitmap(bitmap);
+    }
+
     bdrv_dirty_bitmap_set_persistance(bitmap, persistent);
 }
 
-- 
2.11.1

Re: [Qemu-devel] [PATCH v2 4/6] qmp: transaction support for block-dirty-bitmap-enable/disable

[Vladimir Sementsov-Ogievskiy]

16.01.2018 15:54, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   qapi/transaction.json |  4 +++
>   blockdev.c            | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 83 insertions(+)
>
> diff --git a/qapi/transaction.json b/qapi/transaction.json
> index bd312792da..b643d848f8 100644
> --- a/qapi/transaction.json
> +++ b/qapi/transaction.json
> @@ -46,6 +46,8 @@
>   # - @abort: since 1.6
>   # - @block-dirty-bitmap-add: since 2.5
>   # - @block-dirty-bitmap-clear: since 2.5
> +# - @block-dirty-bitmap-enable: since 2.12
> +# - @block-dirty-bitmap-disable: since 2.12
>   # - @blockdev-backup: since 2.3
>   # - @blockdev-snapshot: since 2.5
>   # - @blockdev-snapshot-internal-sync: since 1.7
> @@ -59,6 +61,8 @@
>          'abort': 'Abort',
>          'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
>          'block-dirty-bitmap-clear': 'BlockDirtyBitmap',
> +       'block-dirty-bitmap-enable': 'BlockDirtyBitmap',
> +       'block-dirty-bitmap-disable': 'BlockDirtyBitmap',
>          'blockdev-backup': 'BlockdevBackup',
>          'blockdev-snapshot': 'BlockdevSnapshot',
>          'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
> diff --git a/blockdev.c b/blockdev.c
> index 997a6f514c..d338363d78 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1962,6 +1962,7 @@ typedef struct BlockDirtyBitmapState {
>       AioContext *aio_context;
>       HBitmap *backup;
>       bool prepared;
> +    bool was_enabled;
>   } BlockDirtyBitmapState;
>   
>   static void block_dirty_bitmap_add_prepare(BlkActionState *common,
> @@ -2069,6 +2070,74 @@ static void block_dirty_bitmap_clear_clean(BlkActionState *common)
>       }
>   }
>   
> +static void block_dirty_bitmap_enable_prepare(BlkActionState *common,
> +                                              Error **errp)
> +{
> +    BlockDirtyBitmap *action;
> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> +                                             common, common);
> +
> +    if (action_check_completion_mode(common, errp) < 0) {
> +        return;
> +    }
> +
> +    action = common->action->u.block_dirty_bitmap_enable.data;
> +    state->bitmap = block_dirty_bitmap_lookup(action->node,
> +                                              action->name,
> +                                              &state->bs,
> +                                              errp);
> +    if (!state->bitmap) {
> +        return;
> +    }
> +
> +    state->was_enabled = bdrv_dirty_bitmap_enabled(state->bitmap);
> +    bdrv_enable_dirty_bitmap(state->bitmap);
> +}
> +
> +static void block_dirty_bitmap_enable_abort(BlkActionState *common)
> +{
> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> +                                             common, common);
> +
> +    if (!state->was_enabled) {
> +        bdrv_disable_dirty_bitmap(state->bitmap);
> +    }
> +}
> +
> +static void block_dirty_bitmap_disable_prepare(BlkActionState *common,
> +                                               Error **errp)
> +{
> +    BlockDirtyBitmap *action;
> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> +                                             common, common);
> +
> +    if (action_check_completion_mode(common, errp) < 0) {
> +        return;
> +    }
> +
> +    action = common->action->u.block_dirty_bitmap_disable.data;
> +    state->bitmap = block_dirty_bitmap_lookup(action->node,
> +                                              action->name,
> +                                              &state->bs,
> +                                              errp);
> +    if (!state->bitmap) {
> +        return;
> +    }
> +
> +    state->was_enabled = bdrv_dirty_bitmap_enabled(state->bitmap);
> +    bdrv_disable_dirty_bitmap(state->bitmap);

hm. actually, I just make it like _clear is made. But now I doubt,
why do we need disable here? we can disable in commit, and do not need
state->was_enabled..

and for _clear, we not to clear bitmap in commit? and then, we do not need
bdrv_undo_clear_dirty_bitmap at all?

or what do I miss? if nothing, I'll respin the series including _clear 
refactoring.

> +}
> +
> +static void block_dirty_bitmap_disable_abort(BlkActionState *common)
> +{
> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> +                                             common, common);
> +
> +    if (state->was_enabled) {
> +        bdrv_enable_dirty_bitmap(state->bitmap);
> +    }
> +}
> +
>   static void abort_prepare(BlkActionState *common, Error **errp)
>   {
>       error_setg(errp, "Transaction aborted using Abort action");
> @@ -2130,6 +2199,16 @@ static const BlkActionOps actions[] = {
>           .commit = block_dirty_bitmap_clear_commit,
>           .abort = block_dirty_bitmap_clear_abort,
>           .clean = block_dirty_bitmap_clear_clean,
> +    },
> +    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ENABLE] = {
> +        .instance_size = sizeof(BlockDirtyBitmapState),
> +        .prepare = block_dirty_bitmap_enable_prepare,
> +        .abort = block_dirty_bitmap_enable_abort,
> +    },
> +    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_DISABLE] = {
> +        .instance_size = sizeof(BlockDirtyBitmapState),
> +        .prepare = block_dirty_bitmap_disable_prepare,
> +        .abort = block_dirty_bitmap_disable_abort,
>       }
>   };
>   


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 0/6] qmp dirty bitmap API

[John Snow]

On 01/16/2018 07:54 AM, Vladimir Sementsov-Ogievskiy wrote:
> Hi all.
> 
> There are three qmp commands, needed to implement external backup API.
> 
> Using these three commands, client may do all needed bitmap management by
> hand:
> 
> on backup start we need to do a transaction:
>  {disable old bitmap, create new bitmap}
> 
> on backup success:
>  drop old bitmap
> 
> on backup fail:
>  enable old bitmap
>  merge new bitmap to old bitmap
>  drop new bitmap
> 
> v2: fix merge command deadlock
>   add new patches: 1 and 6
> 
> Vladimir Sementsov-Ogievskiy (6):
>   block: maintain persistent disabled bitmaps
>   block/dirty-bitmap: add lock to bdrv_enable/disable_dirty_bitmap
>   qapi: add block-dirty-bitmap-enable/disable
>   qmp: transaction support for block-dirty-bitmap-enable/disable
>   qapi: add block-dirty-bitmap-merge
>   qapi: add disabled parameter to block-dirty-bitmap-add
> 
>  qapi/block-core.json         |  92 ++++++++++++++++++++++-
>  qapi/transaction.json        |   4 +
>  block/qcow2.h                |   2 +-
>  include/block/dirty-bitmap.h |   3 +-
>  block/dirty-bitmap.c         |  42 ++++++-----
>  block/qcow2-bitmap.c         |  12 +--
>  block/qcow2.c                |   2 +-
>  blockdev.c                   | 169 +++++++++++++++++++++++++++++++++++++++++--
>  8 files changed, 287 insertions(+), 39 deletions(-)
> 

Fails to apply to master (b384cd95) on patch four and five. Only
contextual problems, I've patched it up and I'll review that.

(mirrored here if you want to check my rebase work:
https://github.com/jnsnow/qemu/tree/vlad-review)

Since I was full of such bad and stupid ideas last time, I'd like
someone else to look over this one for design and I'll just review it
for accuracy.

--js

Re: [Qemu-devel] [PATCH v2 1/6] block: maintain persistent disabled bitmaps

[John Snow]

On 01/16/2018 07:54 AM, Vladimir Sementsov-Ogievskiy wrote:
> To maintain load/store disabled bitmap there is new approach:
> 
>  - deprecate @autoload flag of block-dirty-bitmap-add, make it ignored
>  - store enabled bitmaps as "auto" to qcow2
>  - store disabled bitmaps without "auto" flag to qcow2
>  - on qcow2 open load "auto" bitmaps as enabled and others
>    as disabled (except in_use bitmaps)
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  qapi/block-core.json         |  6 +++---
>  block/qcow2.h                |  2 +-
>  include/block/dirty-bitmap.h |  1 -
>  block/dirty-bitmap.c         | 18 ------------------
>  block/qcow2-bitmap.c         | 12 +++++++-----
>  block/qcow2.c                |  2 +-
>  blockdev.c                   | 10 ++--------
>  7 files changed, 14 insertions(+), 37 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index ab96e348e6..827254db22 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1593,9 +1593,9 @@
>  #              Qcow2 disks support persistent bitmaps. Default is false for
>  #              block-dirty-bitmap-add. (Since: 2.10)
>  #
> -# @autoload: the bitmap will be automatically loaded when the image it is stored
> -#            in is opened. This flag may only be specified for persistent
> -#            bitmaps. Default is false for block-dirty-bitmap-add. (Since: 2.10)
> +# @autoload: ignored and deprecated since 2.12.
> +#            Currently, all dirty tracking bitmaps are loaded from Qcow2 on
> +#            open.

Hmm, so we're going to say that *all* persistent bitmaps are loaded into
memory, but they may-or-may-not-be enabled, is that the approach we're
taking now?

>  #
>  # Since: 2.4
>  ##
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 782a206ecb..a3e29276fc 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -672,7 +672,7 @@ void qcow2_cache_discard(BlockDriverState *bs, Qcow2Cache *c, void *table);
>  int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>                                    void **refcount_table,
>                                    int64_t *refcount_table_size);
> -bool qcow2_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp);
> +bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
>  int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
>  void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
>  int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 3579a7597c..144e77a879 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -65,7 +65,6 @@ void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap,
>  void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
>  
>  void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap, bool value);
> -void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload);
>  void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
>                                         bool persistent);
>  
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index bd04e991b1..3777be1985 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -52,8 +52,6 @@ struct BdrvDirtyBitmap {
>                                     Such operations must fail and both the image
>                                     and this bitmap must remain unchanged while
>                                     this flag is set. */
> -    bool autoload;              /* For persistent bitmaps: bitmap must be
> -                                   autoloaded on image opening */
>      bool persistent;            /* bitmap must be saved to owner disk image */
>      QLIST_ENTRY(BdrvDirtyBitmap) list;
>  };
> @@ -104,7 +102,6 @@ void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap)
>      g_free(bitmap->name);
>      bitmap->name = NULL;
>      bitmap->persistent = false;
> -    bitmap->autoload = false;
>  }
>  
>  /* Called with BQL taken.  */
> @@ -261,8 +258,6 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
>      bitmap->successor = NULL;
>      successor->persistent = bitmap->persistent;
>      bitmap->persistent = false;
> -    successor->autoload = bitmap->autoload;
> -    bitmap->autoload = false;
>      bdrv_release_dirty_bitmap(bs, bitmap);
>  
>      return successor;
> @@ -667,19 +662,6 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs)
>  }
>  
>  /* Called with BQL taken. */
> -void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload)
> -{
> -    qemu_mutex_lock(bitmap->mutex);
> -    bitmap->autoload = autoload;
> -    qemu_mutex_unlock(bitmap->mutex);
> -}
> -
> -bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap)
> -{
> -    return bitmap->autoload;
> -}
> -
> -/* Called with BQL taken. */
>  void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, bool persistent)
>  {
>      qemu_mutex_lock(bitmap->mutex);
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index f45e46cfbd..ae14464de6 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -933,14 +933,14 @@ static void set_readonly_helper(gpointer bitmap, gpointer value)
>      bdrv_dirty_bitmap_set_readonly(bitmap, (bool)value);
>  }
>  
> -/* qcow2_load_autoloading_dirty_bitmaps()
> +/* qcow2_load_dirty_bitmaps()
>   * Return value is a hint for caller: true means that the Qcow2 header was
>   * updated. (false doesn't mean that the header should be updated by the
>   * caller, it just means that updating was not needed or the image cannot be
>   * written to).
>   * On failure the function returns false.
>   */
> -bool qcow2_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp)
> +bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>  {
>      BDRVQcow2State *s = bs->opaque;
>      Qcow2BitmapList *bm_list;
> @@ -960,14 +960,16 @@ bool qcow2_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>      }
>  
>      QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> -        if ((bm->flags & BME_FLAG_AUTO) && !(bm->flags & BME_FLAG_IN_USE)) {
> +        if (!(bm->flags & BME_FLAG_IN_USE)) {
>              BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
>              if (bitmap == NULL) {
>                  goto fail;
>              }
>  
> +            if (!(bm->flags & BME_FLAG_AUTO)) {
> +                bdrv_disable_dirty_bitmap(bitmap);
> +            }

So we're re-using this as the enabled flag, basically.

>              bdrv_dirty_bitmap_set_persistance(bitmap, true);
> -            bdrv_dirty_bitmap_set_autoload(bitmap, true);
>              bm->flags |= BME_FLAG_IN_USE;
>              created_dirty_bitmaps =
>                      g_slist_append(created_dirty_bitmaps, bitmap);
> @@ -1369,7 +1371,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>              bm->table.size = 0;
>              QSIMPLEQ_INSERT_TAIL(&drop_tables, tb, entry);
>          }
> -        bm->flags = bdrv_dirty_bitmap_get_autoload(bitmap) ? BME_FLAG_AUTO : 0;
> +        bm->flags = bdrv_dirty_bitmap_enabled(bitmap) ? BME_FLAG_AUTO : 0;
>          bm->granularity_bits = ctz32(bdrv_dirty_bitmap_granularity(bitmap));
>          bm->dirty_bitmap = bitmap;
>      }
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 92cb9f9bfa..93c3a97cfe 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1441,7 +1441,7 @@ static int qcow2_do_open(BlockDriverState *bs, QDict *options, int flags,
>          s->autoclear_features &= QCOW2_AUTOCLEAR_MASK;
>      }
>  
> -    if (qcow2_load_autoloading_dirty_bitmaps(bs, &local_err)) {
> +    if (qcow2_load_dirty_bitmaps(bs, &local_err)) {
>          update_header = false;
>      }
>      if (local_err != NULL) {
> diff --git a/blockdev.c b/blockdev.c
> index 56a6b24a0b..8068cbd606 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2738,14 +2738,9 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
>      if (!has_persistent) {
>          persistent = false;
>      }
> -    if (!has_autoload) {
> -        autoload = false;
> -    }
>  
> -    if (has_autoload && !persistent) {
> -        error_setg(errp, "Autoload flag must be used only for persistent "
> -                         "bitmaps");
> -        return;
> +    if (has_autoload) {
> +        warn_report("Autoload option is deprected and its value is ignored");

"deprecated"

>      }
>  
>      if (persistent &&
> @@ -2760,7 +2755,6 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
>      }
>  
>      bdrv_dirty_bitmap_set_persistance(bitmap, persistent);
> -    bdrv_dirty_bitmap_set_autoload(bitmap, autoload);
>  }
>  
>  void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
> 

Checks out mechanically; I'm not sure yet if we ought to re-use
BME_FLAG_AUTO as the enabled flag. I'll get back to that :)

With spelling error fixed:

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH v2 2/6] block/dirty-bitmap: add lock to bdrv_enable/disable_dirty_bitmap

[John Snow]

On 01/16/2018 07:54 AM, Vladimir Sementsov-Ogievskiy wrote:
> Add locks and remove comments about BQL accordingly to
> dirty_bitmap_mutex definition in block_int.h.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/dirty-bitmap.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 3777be1985..d0a10c4f5d 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -389,18 +389,20 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>      }
>  }
>  
> -/* Called with BQL taken.  */
>  void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>  {
> +    bdrv_dirty_bitmap_lock(bitmap);
>      assert(!bdrv_dirty_bitmap_frozen(bitmap));
>      bitmap->disabled = true;
> +    bdrv_dirty_bitmap_unlock(bitmap);
>  }
>  
> -/* Called with BQL taken.  */
>  void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>  {
> +    bdrv_dirty_bitmap_lock(bitmap);
>      assert(!bdrv_dirty_bitmap_frozen(bitmap));
>      bitmap->disabled = false;
> +    bdrv_dirty_bitmap_unlock(bitmap);
>  }
>  
>  BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
> 

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH v2 3/6] qapi: add block-dirty-bitmap-enable/disable

[John Snow]

On 01/16/2018 07:54 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  qapi/block-core.json | 42 ++++++++++++++++++++++++++++++++++++++++++
>  blockdev.c           | 42 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 84 insertions(+)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 827254db22..b3851ee760 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1672,6 +1672,48 @@
>    'data': 'BlockDirtyBitmap' }
>  
>  ##
> +# @block-dirty-bitmap-enable:
> +#
> +# Enable dirty bitmap, so that it will continue tracking disk changes.
> +#

suggest:
"Enables a dirty bitmap so that it will begin tracking disk changes."

Key item here is "begin" instead of "continue".

> +# Returns: nothing on success
> +#          If @node is not a valid block device, DeviceNotFound
> +#          If @name is not found, GenericError with an explanation
> +#
> +# Since: 2.12
> +#
> +# Example:
> +#
> +# -> { "execute": "block-dirty-bitmap-enable",
> +#      "arguments": { "node": "drive0", "name": "bitmap0" } }
> +# <- { "return": {} }
> +#
> +##
> +  { 'command': 'block-dirty-bitmap-enable',
> +    'data': 'BlockDirtyBitmap' }
> +
> +##
> +# @block-dirty-bitmap-disable:
> +#
> +# Disable dirty bitmap, so that it will stop tracking disk changes.
> +#

suggest:
"Disables a dirty bitmap so that it will stop tracking disk changes."

> +# Returns: nothing on success
> +#          If @node is not a valid block device, DeviceNotFound
> +#          If @name is not found, GenericError with an explanation
> +#
> +# Since: 2.12
> +#
> +# Example:
> +#
> +# -> { "execute": "block-dirty-bitmap-disable",
> +#      "arguments": { "node": "drive0", "name": "bitmap0" } }
> +# <- { "return": {} }
> +#
> +##
> +    { 'command': 'block-dirty-bitmap-disable',
> +      'data': 'BlockDirtyBitmap' }
> +
> +##
>  # @BlockDirtyBitmapSha256:
>  #
>  # SHA256 hash of dirty bitmap data
> diff --git a/blockdev.c b/blockdev.c
> index 8068cbd606..997a6f514c 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2821,6 +2821,48 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
>      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 || !bs) {
> +        return;
> +    }
> +
> +    if (bdrv_dirty_bitmap_frozen(bitmap)) {
> +        error_setg(errp,
> +                   "Bitmap '%s' is currently frozen and cannot be enabled",
> +                   name);
> +        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 || !bs) {
> +        return;
> +    }
> +
> +    if (bdrv_dirty_bitmap_frozen(bitmap)) {
> +        error_setg(errp,
> +                   "Bitmap '%s' is currently frozen and cannot be disabled",
> +                   name);
> +        return;
> +    }
> +
> +    bdrv_disable_dirty_bitmap(bitmap);
> +}
> +
>  BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
>                                                                const char *name,
>                                                                Error **errp)
> 

I have to admit exposing this interface still makes me nervous, but :)

Mechanically correct, and with suggesting phrasing changes:

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH v2 4/6] qmp: transaction support for block-dirty-bitmap-enable/disable

[John Snow]

On 01/17/2018 10:06 AM, Vladimir Sementsov-Ogievskiy wrote:
> 16.01.2018 15:54, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   qapi/transaction.json |  4 +++
>>   blockdev.c            | 79
>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 83 insertions(+)
>>
>> diff --git a/qapi/transaction.json b/qapi/transaction.json
>> index bd312792da..b643d848f8 100644
>> --- a/qapi/transaction.json
>> +++ b/qapi/transaction.json
>> @@ -46,6 +46,8 @@
>>   # - @abort: since 1.6
>>   # - @block-dirty-bitmap-add: since 2.5
>>   # - @block-dirty-bitmap-clear: since 2.5
>> +# - @block-dirty-bitmap-enable: since 2.12
>> +# - @block-dirty-bitmap-disable: since 2.12
>>   # - @blockdev-backup: since 2.3
>>   # - @blockdev-snapshot: since 2.5
>>   # - @blockdev-snapshot-internal-sync: since 1.7
>> @@ -59,6 +61,8 @@
>>          'abort': 'Abort',
>>          'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
>>          'block-dirty-bitmap-clear': 'BlockDirtyBitmap',
>> +       'block-dirty-bitmap-enable': 'BlockDirtyBitmap',
>> +       'block-dirty-bitmap-disable': 'BlockDirtyBitmap',
>>          'blockdev-backup': 'BlockdevBackup',
>>          'blockdev-snapshot': 'BlockdevSnapshot',
>>          'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
>> diff --git a/blockdev.c b/blockdev.c
>> index 997a6f514c..d338363d78 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -1962,6 +1962,7 @@ typedef struct BlockDirtyBitmapState {
>>       AioContext *aio_context;
>>       HBitmap *backup;
>>       bool prepared;
>> +    bool was_enabled;
>>   } BlockDirtyBitmapState;
>>     static void block_dirty_bitmap_add_prepare(BlkActionState *common,
>> @@ -2069,6 +2070,74 @@ static void
>> block_dirty_bitmap_clear_clean(BlkActionState *common)
>>       }
>>   }
>>   +static void block_dirty_bitmap_enable_prepare(BlkActionState *common,
>> +                                              Error **errp)
>> +{
>> +    BlockDirtyBitmap *action;
>> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>> +                                             common, common);
>> +
>> +    if (action_check_completion_mode(common, errp) < 0) {
>> +        return;
>> +    }
>> +
>> +    action = common->action->u.block_dirty_bitmap_enable.data;
>> +    state->bitmap = block_dirty_bitmap_lookup(action->node,
>> +                                              action->name,
>> +                                              &state->bs,
>> +                                              errp);
>> +    if (!state->bitmap) {
>> +        return;
>> +    }
>> +
>> +    state->was_enabled = bdrv_dirty_bitmap_enabled(state->bitmap);
>> +    bdrv_enable_dirty_bitmap(state->bitmap);
>> +}
>> +
>> +static void block_dirty_bitmap_enable_abort(BlkActionState *common)
>> +{
>> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>> +                                             common, common);
>> +
>> +    if (!state->was_enabled) {
>> +        bdrv_disable_dirty_bitmap(state->bitmap);
>> +    }
>> +}
>> +
>> +static void block_dirty_bitmap_disable_prepare(BlkActionState *common,
>> +                                               Error **errp)
>> +{
>> +    BlockDirtyBitmap *action;
>> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>> +                                             common, common);
>> +
>> +    if (action_check_completion_mode(common, errp) < 0) {
>> +        return;
>> +    }
>> +
>> +    action = common->action->u.block_dirty_bitmap_disable.data;
>> +    state->bitmap = block_dirty_bitmap_lookup(action->node,
>> +                                              action->name,
>> +                                              &state->bs,
>> +                                              errp);
>> +    if (!state->bitmap) {

&state->bs should be NULL if we're not going to use it. If we're going
to use it, we should check the error condition here because it might fail.

>> +        return;
>> +    }
>> +
>> +    state->was_enabled = bdrv_dirty_bitmap_enabled(state->bitmap);
>> +    bdrv_disable_dirty_bitmap(state->bitmap);
> 
> hm. actually, I just make it like _clear is made. But now I doubt,
> why do we need disable here? we can disable in commit, and do not need
> state->was_enabled..
> 

You need to make sure that there is no way for bdrv_disable_dirty_bitmap
to fail, so you need to add that frozen check in. Then you're alright,
and you can move the actual disable portion to the commit, and get rid
of the undo call.

Or, we can even do this kind of trick to remove the redundant error
checking:

void qmp_block_dirty_bitmap_enable(const char *node, const char *name,
                                   Error **errp)
{
    BlockDirtyBitmap data = {
	.node = node,
        .name = name
    };
    TransactionAction action = {
        .type = TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ENABLE,
	.u.block_dirty_bitmap_enable.data = &data,
    };
    blockdev_do_action(&action, errp);
}

static void block_dirty_bitmap_enable_prepare(BlkActionState *common,
                                              Error **errp)
{
    BlockDirtyBitmap *action;
    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
                                             common, common);

    if (action_check_completion_mode(common, errp) < 0) {
	return;
    }

    action = common->action->u.block_dirty_bitmap_enable.data;
    state->bitmap = block_dirty_bitmap_lookup(action->node,
                                              action->name,
                                              NULL,
                                              errp);
    if (!state->bitmap) {
        return;
    }

    if (bdrv_dirty_bitmap_frozen(state->bitmap)) {
        error_setg(errp, "Bitmap '%s' is currently frozen and cannot be
                         " enabled", name);
        return;
    }
}

static void block_dirty_bitmap_enable_commit(BlkActionState *common,
                                              Error **errp)
{
    ...blah...
    bdrv_dirty_bitmap_enable(state->bitmap);
}

You can do the same for disable.

> and for _clear, we not to clear bitmap in commit? and then, we do not need
> bdrv_undo_clear_dirty_bitmap at all?
> 

Yeah, you can apply the same technique here too. You can consolidate the
error checking in .prepare(), then recycle that error checking for the
standalone QMP command.

Then we can be assured we're not missing a safeguard in the transaction
version, and it should be safe to move the action to the commit phase,
because _enable, _disable, and _clear all cannot actually fail.

Then you can get rid of the state->backup state, too, and the extra
argument to bdrv_clear_dirty_bitmap.

Nice!

> or what do I miss? if nothing, I'll respin the series including _clear
> refactoring.
> 

Just pay heed to the error checking in .prepare(), and make sure the
error checking is identical between the QMP and transactional versions.

>> +}
>> +
>> +static void block_dirty_bitmap_disable_abort(BlkActionState *common)
>> +{
>> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>> +                                             common, common);
>> +
>> +    if (state->was_enabled) {
>> +        bdrv_enable_dirty_bitmap(state->bitmap);
>> +    }
>> +}
>> +
>>   static void abort_prepare(BlkActionState *common, Error **errp)
>>   {
>>       error_setg(errp, "Transaction aborted using Abort action");
>> @@ -2130,6 +2199,16 @@ static const BlkActionOps actions[] = {
>>           .commit = block_dirty_bitmap_clear_commit,
>>           .abort = block_dirty_bitmap_clear_abort,
>>           .clean = block_dirty_bitmap_clear_clean,
>> +    },
>> +    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ENABLE] = {
>> +        .instance_size = sizeof(BlockDirtyBitmapState),
>> +        .prepare = block_dirty_bitmap_enable_prepare,
>> +        .abort = block_dirty_bitmap_enable_abort,
>> +    },
>> +    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_DISABLE] = {
>> +        .instance_size = sizeof(BlockDirtyBitmapState),
>> +        .prepare = block_dirty_bitmap_disable_prepare,
>> +        .abort = block_dirty_bitmap_disable_abort,
>>       }
>>   };
>>   
> 
> 

Thanks!

Re: [Qemu-devel] [PATCH v2 1/6] block: maintain persistent disabled bitmaps

[Vladimir Sementsov-Ogievskiy]

20.01.2018 02:43, John Snow wrote:
>
> On 01/16/2018 07:54 AM, Vladimir Sementsov-Ogievskiy wrote:
>> To maintain load/store disabled bitmap there is new approach:
>>
>>   - deprecate @autoload flag of block-dirty-bitmap-add, make it ignored
>>   - store enabled bitmaps as "auto" to qcow2
>>   - store disabled bitmaps without "auto" flag to qcow2
>>   - on qcow2 open load "auto" bitmaps as enabled and others
>>     as disabled (except in_use bitmaps)
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   qapi/block-core.json         |  6 +++---
>>   block/qcow2.h                |  2 +-
>>   include/block/dirty-bitmap.h |  1 -
>>   block/dirty-bitmap.c         | 18 ------------------
>>   block/qcow2-bitmap.c         | 12 +++++++-----
>>   block/qcow2.c                |  2 +-
>>   blockdev.c                   | 10 ++--------
>>   7 files changed, 14 insertions(+), 37 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index ab96e348e6..827254db22 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -1593,9 +1593,9 @@
>>   #              Qcow2 disks support persistent bitmaps. Default is false for
>>   #              block-dirty-bitmap-add. (Since: 2.10)
>>   #
>> -# @autoload: the bitmap will be automatically loaded when the image it is stored
>> -#            in is opened. This flag may only be specified for persistent
>> -#            bitmaps. Default is false for block-dirty-bitmap-add. (Since: 2.10)
>> +# @autoload: ignored and deprecated since 2.12.
>> +#            Currently, all dirty tracking bitmaps are loaded from Qcow2 on
>> +#            open.
> Hmm, so we're going to say that *all* persistent bitmaps are loaded into
> memory, but they may-or-may-not-be enabled, is that the approach we're
> taking now?

yes.

>
>>   #
>>   # Since: 2.4
>>   ##
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index 782a206ecb..a3e29276fc 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -672,7 +672,7 @@ void qcow2_cache_discard(BlockDriverState *bs, Qcow2Cache *c, void *table);
>>   int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>>                                     void **refcount_table,
>>                                     int64_t *refcount_table_size);
>> -bool qcow2_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp);
>> +bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
>>   int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
>>   void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
>>   int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>> index 3579a7597c..144e77a879 100644
>> --- a/include/block/dirty-bitmap.h
>> +++ b/include/block/dirty-bitmap.h
>> @@ -65,7 +65,6 @@ void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap,
>>   void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
>>   
>>   void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap, bool value);
>> -void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload);
>>   void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
>>                                          bool persistent);
>>   
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index bd04e991b1..3777be1985 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -52,8 +52,6 @@ struct BdrvDirtyBitmap {
>>                                      Such operations must fail and both the image
>>                                      and this bitmap must remain unchanged while
>>                                      this flag is set. */
>> -    bool autoload;              /* For persistent bitmaps: bitmap must be
>> -                                   autoloaded on image opening */
>>       bool persistent;            /* bitmap must be saved to owner disk image */
>>       QLIST_ENTRY(BdrvDirtyBitmap) list;
>>   };
>> @@ -104,7 +102,6 @@ void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap)
>>       g_free(bitmap->name);
>>       bitmap->name = NULL;
>>       bitmap->persistent = false;
>> -    bitmap->autoload = false;
>>   }
>>   
>>   /* Called with BQL taken.  */
>> @@ -261,8 +258,6 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
>>       bitmap->successor = NULL;
>>       successor->persistent = bitmap->persistent;
>>       bitmap->persistent = false;
>> -    successor->autoload = bitmap->autoload;
>> -    bitmap->autoload = false;
>>       bdrv_release_dirty_bitmap(bs, bitmap);
>>   
>>       return successor;
>> @@ -667,19 +662,6 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs)
>>   }
>>   
>>   /* Called with BQL taken. */
>> -void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload)
>> -{
>> -    qemu_mutex_lock(bitmap->mutex);
>> -    bitmap->autoload = autoload;
>> -    qemu_mutex_unlock(bitmap->mutex);
>> -}
>> -
>> -bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap)
>> -{
>> -    return bitmap->autoload;
>> -}
>> -
>> -/* Called with BQL taken. */
>>   void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, bool persistent)
>>   {
>>       qemu_mutex_lock(bitmap->mutex);
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index f45e46cfbd..ae14464de6 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -933,14 +933,14 @@ static void set_readonly_helper(gpointer bitmap, gpointer value)
>>       bdrv_dirty_bitmap_set_readonly(bitmap, (bool)value);
>>   }
>>   
>> -/* qcow2_load_autoloading_dirty_bitmaps()
>> +/* qcow2_load_dirty_bitmaps()
>>    * Return value is a hint for caller: true means that the Qcow2 header was
>>    * updated. (false doesn't mean that the header should be updated by the
>>    * caller, it just means that updating was not needed or the image cannot be
>>    * written to).
>>    * On failure the function returns false.
>>    */
>> -bool qcow2_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>> +bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>>   {
>>       BDRVQcow2State *s = bs->opaque;
>>       Qcow2BitmapList *bm_list;
>> @@ -960,14 +960,16 @@ bool qcow2_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>>       }
>>   
>>       QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>> -        if ((bm->flags & BME_FLAG_AUTO) && !(bm->flags & BME_FLAG_IN_USE)) {
>> +        if (!(bm->flags & BME_FLAG_IN_USE)) {
>>               BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
>>               if (bitmap == NULL) {
>>                   goto fail;
>>               }
>>   
>> +            if (!(bm->flags & BME_FLAG_AUTO)) {
>> +                bdrv_disable_dirty_bitmap(bitmap);
>> +            }
> So we're re-using this as the enabled flag, basically.
>
>>               bdrv_dirty_bitmap_set_persistance(bitmap, true);
>> -            bdrv_dirty_bitmap_set_autoload(bitmap, true);
>>               bm->flags |= BME_FLAG_IN_USE;
>>               created_dirty_bitmaps =
>>                       g_slist_append(created_dirty_bitmaps, bitmap);
>> @@ -1369,7 +1371,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>>               bm->table.size = 0;
>>               QSIMPLEQ_INSERT_TAIL(&drop_tables, tb, entry);
>>           }
>> -        bm->flags = bdrv_dirty_bitmap_get_autoload(bitmap) ? BME_FLAG_AUTO : 0;
>> +        bm->flags = bdrv_dirty_bitmap_enabled(bitmap) ? BME_FLAG_AUTO : 0;
>>           bm->granularity_bits = ctz32(bdrv_dirty_bitmap_granularity(bitmap));
>>           bm->dirty_bitmap = bitmap;
>>       }
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 92cb9f9bfa..93c3a97cfe 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1441,7 +1441,7 @@ static int qcow2_do_open(BlockDriverState *bs, QDict *options, int flags,
>>           s->autoclear_features &= QCOW2_AUTOCLEAR_MASK;
>>       }
>>   
>> -    if (qcow2_load_autoloading_dirty_bitmaps(bs, &local_err)) {
>> +    if (qcow2_load_dirty_bitmaps(bs, &local_err)) {
>>           update_header = false;
>>       }
>>       if (local_err != NULL) {
>> diff --git a/blockdev.c b/blockdev.c
>> index 56a6b24a0b..8068cbd606 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -2738,14 +2738,9 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
>>       if (!has_persistent) {
>>           persistent = false;
>>       }
>> -    if (!has_autoload) {
>> -        autoload = false;
>> -    }
>>   
>> -    if (has_autoload && !persistent) {
>> -        error_setg(errp, "Autoload flag must be used only for persistent "
>> -                         "bitmaps");
>> -        return;
>> +    if (has_autoload) {
>> +        warn_report("Autoload option is deprected and its value is ignored");
> "deprecated"
>
>>       }
>>   
>>       if (persistent &&
>> @@ -2760,7 +2755,6 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
>>       }
>>   
>>       bdrv_dirty_bitmap_set_persistance(bitmap, persistent);
>> -    bdrv_dirty_bitmap_set_autoload(bitmap, autoload);
>>   }
>>   
>>   void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
>>
> Checks out mechanically; I'm not sure yet if we ought to re-use
> BME_FLAG_AUTO as the enabled flag. I'll get back to that :)
>
> With spelling error fixed:
>
> Reviewed-by: John Snow <jsnow@redhat.com>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 3/6] qapi: add block-dirty-bitmap-enable/disable

[Vladimir Sementsov-Ogievskiy]

20.01.2018 02:50, John Snow wrote:
>
> On 01/16/2018 07:54 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   qapi/block-core.json | 42 ++++++++++++++++++++++++++++++++++++++++++
>>   blockdev.c           | 42 ++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 84 insertions(+)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 827254db22..b3851ee760 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -1672,6 +1672,48 @@
>>     'data': 'BlockDirtyBitmap' }
>>   
>>   ##
>> +# @block-dirty-bitmap-enable:
>> +#
>> +# Enable dirty bitmap, so that it will continue tracking disk changes.
>> +#
> suggest:
> "Enables a dirty bitmap so that it will begin tracking disk changes."
>
> Key item here is "begin" instead of "continue".

agree.

>
>> +# Returns: nothing on success
>> +#          If @node is not a valid block device, DeviceNotFound
>> +#          If @name is not found, GenericError with an explanation
>> +#
>> +# Since: 2.12
>> +#
>> +# Example:
>> +#
>> +# -> { "execute": "block-dirty-bitmap-enable",
>> +#      "arguments": { "node": "drive0", "name": "bitmap0" } }
>> +# <- { "return": {} }
>> +#
>> +##
>> +  { 'command': 'block-dirty-bitmap-enable',
>> +    'data': 'BlockDirtyBitmap' }
>> +
>> +##
>> +# @block-dirty-bitmap-disable:
>> +#
>> +# Disable dirty bitmap, so that it will stop tracking disk changes.
>> +#
> suggest:
> "Disables a dirty bitmap so that it will stop tracking disk changes."
>
>> +# Returns: nothing on success
>> +#          If @node is not a valid block device, DeviceNotFound
>> +#          If @name is not found, GenericError with an explanation
>> +#
>> +# Since: 2.12
>> +#
>> +# Example:
>> +#
>> +# -> { "execute": "block-dirty-bitmap-disable",
>> +#      "arguments": { "node": "drive0", "name": "bitmap0" } }
>> +# <- { "return": {} }
>> +#
>> +##
>> +    { 'command': 'block-dirty-bitmap-disable',
>> +      'data': 'BlockDirtyBitmap' }
>> +
>> +##
>>   # @BlockDirtyBitmapSha256:
>>   #
>>   # SHA256 hash of dirty bitmap data
>> diff --git a/blockdev.c b/blockdev.c
>> index 8068cbd606..997a6f514c 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -2821,6 +2821,48 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
>>       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 || !bs) {
>> +        return;
>> +    }
>> +
>> +    if (bdrv_dirty_bitmap_frozen(bitmap)) {
>> +        error_setg(errp,
>> +                   "Bitmap '%s' is currently frozen and cannot be enabled",
>> +                   name);
>> +        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 || !bs) {
>> +        return;
>> +    }
>> +
>> +    if (bdrv_dirty_bitmap_frozen(bitmap)) {
>> +        error_setg(errp,
>> +                   "Bitmap '%s' is currently frozen and cannot be disabled",
>> +                   name);
>> +        return;
>> +    }
>> +
>> +    bdrv_disable_dirty_bitmap(bitmap);
>> +}
>> +
>>   BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
>>                                                                 const char *name,
>>                                                                 Error **errp)
>>
> I have to admit exposing this interface still makes me nervous, but :)
>
> Mechanically correct, and with suggesting phrasing changes:
>
> Reviewed-by: John Snow <jsnow@redhat.com>



-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 4/6] qmp: transaction support for block-dirty-bitmap-enable/disable

[Vladimir Sementsov-Ogievskiy]

20.01.2018 03:28, John Snow wrote:
>
> On 01/17/2018 10:06 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 16.01.2018 15:54, Vladimir Sementsov-Ogievskiy wrote:
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>    qapi/transaction.json |  4 +++
>>>    blockdev.c            | 79
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>    2 files changed, 83 insertions(+)
>>>
>>> diff --git a/qapi/transaction.json b/qapi/transaction.json
>>> index bd312792da..b643d848f8 100644
>>> --- a/qapi/transaction.json
>>> +++ b/qapi/transaction.json
>>> @@ -46,6 +46,8 @@
>>>    # - @abort: since 1.6
>>>    # - @block-dirty-bitmap-add: since 2.5
>>>    # - @block-dirty-bitmap-clear: since 2.5
>>> +# - @block-dirty-bitmap-enable: since 2.12
>>> +# - @block-dirty-bitmap-disable: since 2.12
>>>    # - @blockdev-backup: since 2.3
>>>    # - @blockdev-snapshot: since 2.5
>>>    # - @blockdev-snapshot-internal-sync: since 1.7
>>> @@ -59,6 +61,8 @@
>>>           'abort': 'Abort',
>>>           'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
>>>           'block-dirty-bitmap-clear': 'BlockDirtyBitmap',
>>> +       'block-dirty-bitmap-enable': 'BlockDirtyBitmap',
>>> +       'block-dirty-bitmap-disable': 'BlockDirtyBitmap',
>>>           'blockdev-backup': 'BlockdevBackup',
>>>           'blockdev-snapshot': 'BlockdevSnapshot',
>>>           'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
>>> diff --git a/blockdev.c b/blockdev.c
>>> index 997a6f514c..d338363d78 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -1962,6 +1962,7 @@ typedef struct BlockDirtyBitmapState {
>>>        AioContext *aio_context;
>>>        HBitmap *backup;
>>>        bool prepared;
>>> +    bool was_enabled;
>>>    } BlockDirtyBitmapState;
>>>      static void block_dirty_bitmap_add_prepare(BlkActionState *common,
>>> @@ -2069,6 +2070,74 @@ static void
>>> block_dirty_bitmap_clear_clean(BlkActionState *common)
>>>        }
>>>    }
>>>    +static void block_dirty_bitmap_enable_prepare(BlkActionState *common,
>>> +                                              Error **errp)
>>> +{
>>> +    BlockDirtyBitmap *action;
>>> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>>> +                                             common, common);
>>> +
>>> +    if (action_check_completion_mode(common, errp) < 0) {
>>> +        return;
>>> +    }
>>> +
>>> +    action = common->action->u.block_dirty_bitmap_enable.data;
>>> +    state->bitmap = block_dirty_bitmap_lookup(action->node,
>>> +                                              action->name,
>>> +                                              &state->bs,
>>> +                                              errp);
>>> +    if (!state->bitmap) {
>>> +        return;
>>> +    }
>>> +
>>> +    state->was_enabled = bdrv_dirty_bitmap_enabled(state->bitmap);
>>> +    bdrv_enable_dirty_bitmap(state->bitmap);
>>> +}
>>> +
>>> +static void block_dirty_bitmap_enable_abort(BlkActionState *common)
>>> +{
>>> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>>> +                                             common, common);
>>> +
>>> +    if (!state->was_enabled) {
>>> +        bdrv_disable_dirty_bitmap(state->bitmap);
>>> +    }
>>> +}
>>> +
>>> +static void block_dirty_bitmap_disable_prepare(BlkActionState *common,
>>> +                                               Error **errp)
>>> +{
>>> +    BlockDirtyBitmap *action;
>>> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>>> +                                             common, common);
>>> +
>>> +    if (action_check_completion_mode(common, errp) < 0) {
>>> +        return;
>>> +    }
>>> +
>>> +    action = common->action->u.block_dirty_bitmap_disable.data;
>>> +    state->bitmap = block_dirty_bitmap_lookup(action->node,
>>> +                                              action->name,
>>> +                                              &state->bs,
>>> +                                              errp);
>>> +    if (!state->bitmap) {
> &state->bs should be NULL if we're not going to use it. If we're going
> to use it, we should check the error condition here because it might fail.

yes, it should be checked. It think it should be checked in other 
commands too.

>
>>> +        return;
>>> +    }
>>> +
>>> +    state->was_enabled = bdrv_dirty_bitmap_enabled(state->bitmap);
>>> +    bdrv_disable_dirty_bitmap(state->bitmap);
>> hm. actually, I just make it like _clear is made. But now I doubt,
>> why do we need disable here? we can disable in commit, and do not need
>> state->was_enabled..
>>
> You need to make sure that there is no way for bdrv_disable_dirty_bitmap
> to fail, so you need to add that frozen check in. Then you're alright,
> and you can move the actual disable portion to the commit, and get rid
> of the undo call.
>
> Or, we can even do this kind of trick to remove the redundant error
> checking:
>
> void qmp_block_dirty_bitmap_enable(const char *node, const char *name,
>                                     Error **errp)
> {
>      BlockDirtyBitmap data = {
> 	.node = node,
>          .name = name
>      };
>      TransactionAction action = {
>          .type = TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ENABLE,
> 	.u.block_dirty_bitmap_enable.data = &data,
>      };
>      blockdev_do_action(&action, errp);
> }
>
> static void block_dirty_bitmap_enable_prepare(BlkActionState *common,
>                                                Error **errp)
> {
>      BlockDirtyBitmap *action;
>      BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>                                               common, common);
>
>      if (action_check_completion_mode(common, errp) < 0) {
> 	return;
>      }
>
>      action = common->action->u.block_dirty_bitmap_enable.data;
>      state->bitmap = block_dirty_bitmap_lookup(action->node,
>                                                action->name,
>                                                NULL,
>                                                errp);
>      if (!state->bitmap) {
>          return;
>      }
>
>      if (bdrv_dirty_bitmap_frozen(state->bitmap)) {
>          error_setg(errp, "Bitmap '%s' is currently frozen and cannot be
>                           " enabled", name);
>          return;
>      }
> }
>
> static void block_dirty_bitmap_enable_commit(BlkActionState *common,
>                                                Error **errp)
> {
>      ...blah...
>      bdrv_dirty_bitmap_enable(state->bitmap);
> }
>
> You can do the same for disable.
>
>> and for _clear, we not to clear bitmap in commit? and then, we do not need
>> bdrv_undo_clear_dirty_bitmap at all?
>>
> Yeah, you can apply the same technique here too. You can consolidate the
> error checking in .prepare(), then recycle that error checking for the
> standalone QMP command.
>
> Then we can be assured we're not missing a safeguard in the transaction
> version, and it should be safe to move the action to the commit phase,
> because _enable, _disable, and _clear all cannot actually fail.
>
> Then you can get rid of the state->backup state, too, and the extra
> argument to bdrv_clear_dirty_bitmap.
>
> Nice!
>
>> or what do I miss? if nothing, I'll respin the series including _clear
>> refactoring.
>>
> Just pay heed to the error checking in .prepare(), and make sure the
> error checking is identical between the QMP and transactional versions.
>
>>> +}
>>> +
>>> +static void block_dirty_bitmap_disable_abort(BlkActionState *common)
>>> +{
>>> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>>> +                                             common, common);
>>> +
>>> +    if (state->was_enabled) {
>>> +        bdrv_enable_dirty_bitmap(state->bitmap);
>>> +    }
>>> +}
>>> +
>>>    static void abort_prepare(BlkActionState *common, Error **errp)
>>>    {
>>>        error_setg(errp, "Transaction aborted using Abort action");
>>> @@ -2130,6 +2199,16 @@ static const BlkActionOps actions[] = {
>>>            .commit = block_dirty_bitmap_clear_commit,
>>>            .abort = block_dirty_bitmap_clear_abort,
>>>            .clean = block_dirty_bitmap_clear_clean,
>>> +    },
>>> +    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ENABLE] = {
>>> +        .instance_size = sizeof(BlockDirtyBitmapState),
>>> +        .prepare = block_dirty_bitmap_enable_prepare,
>>> +        .abort = block_dirty_bitmap_enable_abort,
>>> +    },
>>> +    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_DISABLE] = {
>>> +        .instance_size = sizeof(BlockDirtyBitmapState),
>>> +        .prepare = block_dirty_bitmap_disable_prepare,
>>> +        .abort = block_dirty_bitmap_disable_abort,
>>>        }
>>>    };
>>>    
>>
> Thanks!


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 0/6] qmp dirty bitmap API

[Vladimir Sementsov-Ogievskiy]

20.01.2018 02:30, John Snow wrote:
>
> On 01/16/2018 07:54 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all.
>>
>> There are three qmp commands, needed to implement external backup API.
>>
>> Using these three commands, client may do all needed bitmap management by
>> hand:
>>
>> on backup start we need to do a transaction:
>>   {disable old bitmap, create new bitmap}
>>
>> on backup success:
>>   drop old bitmap
>>
>> on backup fail:
>>   enable old bitmap
>>   merge new bitmap to old bitmap
>>   drop new bitmap
>>
>> v2: fix merge command deadlock
>>    add new patches: 1 and 6
>>
>> Vladimir Sementsov-Ogievskiy (6):
>>    block: maintain persistent disabled bitmaps
>>    block/dirty-bitmap: add lock to bdrv_enable/disable_dirty_bitmap
>>    qapi: add block-dirty-bitmap-enable/disable
>>    qmp: transaction support for block-dirty-bitmap-enable/disable
>>    qapi: add block-dirty-bitmap-merge
>>    qapi: add disabled parameter to block-dirty-bitmap-add
>>
>>   qapi/block-core.json         |  92 ++++++++++++++++++++++-
>>   qapi/transaction.json        |   4 +
>>   block/qcow2.h                |   2 +-
>>   include/block/dirty-bitmap.h |   3 +-
>>   block/dirty-bitmap.c         |  42 ++++++-----
>>   block/qcow2-bitmap.c         |  12 +--
>>   block/qcow2.c                |   2 +-
>>   blockdev.c                   | 169 +++++++++++++++++++++++++++++++++++++++++--
>>   8 files changed, 287 insertions(+), 39 deletions(-)
>>
> Fails to apply to master (b384cd95) on patch four and five. Only
> contextual problems, I've patched it up and I'll review that.
>
> (mirrored here if you want to check my rebase work:
> https://github.com/jnsnow/qemu/tree/vlad-review)
>
> Since I was full of such bad and stupid ideas last time, I'd like
> someone else to look over this one for design and I'll just review it
> for accuracy.
>
> --js

Thank you for review, John!

Ok, so, I'll going to:

- take patch 1 into migration and respin it today (I hope) with test 
about qcow2-based bitmap migration disabled.
- separate fixes and refactoring from here (locking + _bitmap_clear 
transaction), send them separately
- than, make test for external backup and respin these series with it

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 0/6] qmp dirty bitmap API

[Vladimir Sementsov-Ogievskiy]

22.01.2018 12:20, Vladimir Sementsov-Ogievskiy wrote:
> 20.01.2018 02:30, John Snow wrote:
>>
>> On 01/16/2018 07:54 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Hi all.
>>>
>>> There are three qmp commands, needed to implement external backup API.
>>>
>>> Using these three commands, client may do all needed bitmap 
>>> management by
>>> hand:
>>>
>>> on backup start we need to do a transaction:
>>>   {disable old bitmap, create new bitmap}
>>>
>>> on backup success:
>>>   drop old bitmap
>>>
>>> on backup fail:
>>>   enable old bitmap
>>>   merge new bitmap to old bitmap
>>>   drop new bitmap
>>>
>>> v2: fix merge command deadlock
>>>    add new patches: 1 and 6
>>>
>>> Vladimir Sementsov-Ogievskiy (6):
>>>    block: maintain persistent disabled bitmaps
>>>    block/dirty-bitmap: add lock to bdrv_enable/disable_dirty_bitmap
>>>    qapi: add block-dirty-bitmap-enable/disable
>>>    qmp: transaction support for block-dirty-bitmap-enable/disable
>>>    qapi: add block-dirty-bitmap-merge
>>>    qapi: add disabled parameter to block-dirty-bitmap-add
>>>
>>>   qapi/block-core.json         |  92 ++++++++++++++++++++++-
>>>   qapi/transaction.json        |   4 +
>>>   block/qcow2.h                |   2 +-
>>>   include/block/dirty-bitmap.h |   3 +-
>>>   block/dirty-bitmap.c         |  42 ++++++-----
>>>   block/qcow2-bitmap.c         |  12 +--
>>>   block/qcow2.c                |   2 +-
>>>   blockdev.c                   | 169 
>>> +++++++++++++++++++++++++++++++++++++++++--
>>>   8 files changed, 287 insertions(+), 39 deletions(-)
>>>
>> Fails to apply to master (b384cd95) on patch four and five. Only
>> contextual problems, I've patched it up and I'll review that.
>>
>> (mirrored here if you want to check my rebase work:
>> https://github.com/jnsnow/qemu/tree/vlad-review)
>>
>> Since I was full of such bad and stupid ideas last time, I'd like
>> someone else to look over this one for design and I'll just review it
>> for accuracy.
>>
>> --js
>
> Thank you for review, John!
>
> Ok, so, I'll going to:
>
> - take patch 1 into migration and respin it today (I hope) with test 
> about qcow2-based bitmap migration disabled.
> - separate fixes and refactoring from here (locking + _bitmap_clear 
> transaction), send them separately
> - than, make test for external backup and respin these series with it
>

changed to:

1. send patch 1/6 separately with the whole reasoning[done], as it 
blocks two series, wait for accepting
2. respin postcopy series
3. finish up discussion on bitmap locking under "[PATCH v9 03/13] 
block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap"
4. separate fixes and refactoring from here (locking + _bitmap_clear 
transaction), send them separately
5. make test for external backup and respin these series with it

2 depends on 1
4 depends on 3
5 depends on 1 and 4

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 0/6] qmp dirty bitmap API

[John Snow]

On 01/22/2018 07:22 AM, Vladimir Sementsov-Ogievskiy wrote:
> 22.01.2018 12:20, Vladimir Sementsov-Ogievskiy wrote:
>> 20.01.2018 02:30, John Snow wrote:
>>>
>>> On 01/16/2018 07:54 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> Hi all.
>>>>
>>>> There are three qmp commands, needed to implement external backup API.
>>>>
>>>> Using these three commands, client may do all needed bitmap
>>>> management by
>>>> hand:
>>>>
>>>> on backup start we need to do a transaction:
>>>>   {disable old bitmap, create new bitmap}
>>>>
>>>> on backup success:
>>>>   drop old bitmap
>>>>
>>>> on backup fail:
>>>>   enable old bitmap
>>>>   merge new bitmap to old bitmap
>>>>   drop new bitmap
>>>>
>>>> v2: fix merge command deadlock
>>>>    add new patches: 1 and 6
>>>>
>>>> Vladimir Sementsov-Ogievskiy (6):
>>>>    block: maintain persistent disabled bitmaps
>>>>    block/dirty-bitmap: add lock to bdrv_enable/disable_dirty_bitmap
>>>>    qapi: add block-dirty-bitmap-enable/disable
>>>>    qmp: transaction support for block-dirty-bitmap-enable/disable
>>>>    qapi: add block-dirty-bitmap-merge
>>>>    qapi: add disabled parameter to block-dirty-bitmap-add
>>>>
>>>>   qapi/block-core.json         |  92 ++++++++++++++++++++++-
>>>>   qapi/transaction.json        |   4 +
>>>>   block/qcow2.h                |   2 +-
>>>>   include/block/dirty-bitmap.h |   3 +-
>>>>   block/dirty-bitmap.c         |  42 ++++++-----
>>>>   block/qcow2-bitmap.c         |  12 +--
>>>>   block/qcow2.c                |   2 +-
>>>>   blockdev.c                   | 169
>>>> +++++++++++++++++++++++++++++++++++++++++--
>>>>   8 files changed, 287 insertions(+), 39 deletions(-)
>>>>
>>> Fails to apply to master (b384cd95) on patch four and five. Only
>>> contextual problems, I've patched it up and I'll review that.
>>>
>>> (mirrored here if you want to check my rebase work:
>>> https://github.com/jnsnow/qemu/tree/vlad-review)
>>>
>>> Since I was full of such bad and stupid ideas last time, I'd like
>>> someone else to look over this one for design and I'll just review it
>>> for accuracy.
>>>
>>> --js
>>
>> Thank you for review, John!
>>
>> Ok, so, I'll going to:
>>
>> - take patch 1 into migration and respin it today (I hope) with test
>> about qcow2-based bitmap migration disabled.
>> - separate fixes and refactoring from here (locking + _bitmap_clear
>> transaction), send them separately
>> - than, make test for external backup and respin these series with it
>>
> 
> changed to:
> 
> 1. send patch 1/6 separately with the whole reasoning[done], as it
> blocks two series, wait for accepting
> 2. respin postcopy series
> 3. finish up discussion on bitmap locking under "[PATCH v9 03/13]
> block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap"
> 4. separate fixes and refactoring from here (locking + _bitmap_clear
> transaction), send them separately
> 5. make test for external backup and respin these series with it
> 
> 2 depends on 1
> 4 depends on 3
> 5 depends on 1 and 4
> 

Great, thanks!

Re: [Qemu-devel] [PATCH v2 3/6] qapi: add block-dirty-bitmap-enable/disable

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 605 bytes --]

On 01/22/2018 03:09 AM, Vladimir Sementsov-Ogievskiy wrote:

>>>
>> I have to admit exposing this interface still makes me nervous, but :)
>>
>> Mechanically correct, and with suggesting phrasing changes:
>>
>> Reviewed-by: John Snow <jsnow@redhat.com>

Should we go with an x- prefix for now, and let things settle for a
release before promoting it to fully supported, just in case we find
something that justifies your nervousness in accepting the interface?

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2 3/6] qapi: add block-dirty-bitmap-enable/disable

[John Snow]

On 01/22/2018 02:51 PM, Eric Blake wrote:
> On 01/22/2018 03:09 AM, Vladimir Sementsov-Ogievskiy wrote:
> 
>>>>
>>> I have to admit exposing this interface still makes me nervous, but :)
>>>
>>> Mechanically correct, and with suggesting phrasing changes:
>>>
>>> Reviewed-by: John Snow <jsnow@redhat.com>
> 
> Should we go with an x- prefix for now, and let things settle for a
> release before promoting it to fully supported, just in case we find
> something that justifies your nervousness in accepting the interface?
> 

I proposed as much in a reply to the cover letter; I'm willing to take
patch one now for the sake of migration; the rest of this series I want
a test suite that helps document the anticipated use case, or otherwise
an x- prefix for the command names.

Re: [Qemu-devel] [PATCH v2 0/6] qmp dirty bitmap API

[Vladimir Sementsov-Ogievskiy]

22.01.2018 20:23, John Snow wrote:
>
> On 01/22/2018 07:22 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 22.01.2018 12:20, Vladimir Sementsov-Ogievskiy wrote:
>>> 20.01.2018 02:30, John Snow wrote:
>>>> On 01/16/2018 07:54 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>>> Hi all.
>>>>>
>>>>> There are three qmp commands, needed to implement external backup API.
>>>>>
>>>>> Using these three commands, client may do all needed bitmap
>>>>> management by
>>>>> hand:
>>>>>
>>>>> on backup start we need to do a transaction:
>>>>>    {disable old bitmap, create new bitmap}
>>>>>
>>>>> on backup success:
>>>>>    drop old bitmap
>>>>>
>>>>> on backup fail:
>>>>>    enable old bitmap
>>>>>    merge new bitmap to old bitmap
>>>>>    drop new bitmap
>>>>>
>>>>> v2: fix merge command deadlock
>>>>>     add new patches: 1 and 6
>>>>>
>>>>> Vladimir Sementsov-Ogievskiy (6):
>>>>>     block: maintain persistent disabled bitmaps
>>>>>     block/dirty-bitmap: add lock to bdrv_enable/disable_dirty_bitmap
>>>>>     qapi: add block-dirty-bitmap-enable/disable
>>>>>     qmp: transaction support for block-dirty-bitmap-enable/disable
>>>>>     qapi: add block-dirty-bitmap-merge
>>>>>     qapi: add disabled parameter to block-dirty-bitmap-add
>>>>>
>>>>>    qapi/block-core.json         |  92 ++++++++++++++++++++++-
>>>>>    qapi/transaction.json        |   4 +
>>>>>    block/qcow2.h                |   2 +-
>>>>>    include/block/dirty-bitmap.h |   3 +-
>>>>>    block/dirty-bitmap.c         |  42 ++++++-----
>>>>>    block/qcow2-bitmap.c         |  12 +--
>>>>>    block/qcow2.c                |   2 +-
>>>>>    blockdev.c                   | 169
>>>>> +++++++++++++++++++++++++++++++++++++++++--
>>>>>    8 files changed, 287 insertions(+), 39 deletions(-)
>>>>>
>>>> Fails to apply to master (b384cd95) on patch four and five. Only
>>>> contextual problems, I've patched it up and I'll review that.
>>>>
>>>> (mirrored here if you want to check my rebase work:
>>>> https://github.com/jnsnow/qemu/tree/vlad-review)
>>>>
>>>> Since I was full of such bad and stupid ideas last time, I'd like
>>>> someone else to look over this one for design and I'll just review it
>>>> for accuracy.
>>>>
>>>> --js
>>> Thank you for review, John!
>>>
>>> Ok, so, I'll going to:
>>>
>>> - take patch 1 into migration and respin it today (I hope) with test
>>> about qcow2-based bitmap migration disabled.
>>> - separate fixes and refactoring from here (locking + _bitmap_clear
>>> transaction), send them separately
>>> - than, make test for external backup and respin these series with it
>>>
>> changed to:
>>
>> 1. send patch 1/6 separately with the whole reasoning[done], as it
>> blocks two series, wait for accepting
>> 2. respin postcopy series
>> 3. finish up discussion on bitmap locking under "[PATCH v9 03/13]
>> block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap"
>> 4. separate fixes and refactoring from here (locking + _bitmap_clear
>> transaction), send them separately
>> 5. make test for external backup and respin these series with it
>>
>> 2 depends on 1
>> 4 depends on 3
>> 5 depends on 1 and 4
>>
> Great, thanks!

Sorry for long delay, I was ill. Now I'm returning to these plans.

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 3/6] qapi: add block-dirty-bitmap-enable/disable

[Vladimir Sementsov-Ogievskiy]

22.01.2018 22:56, John Snow wrote:
>
> On 01/22/2018 02:51 PM, Eric Blake wrote:
>> On 01/22/2018 03:09 AM, Vladimir Sementsov-Ogievskiy wrote:
>>
>>>> I have to admit exposing this interface still makes me nervous, but :)
>>>>
>>>> Mechanically correct, and with suggesting phrasing changes:
>>>>
>>>> Reviewed-by: John Snow <jsnow@redhat.com>
>> Should we go with an x- prefix for now, and let things settle for a
>> release before promoting it to fully supported, just in case we find
>> something that justifies your nervousness in accepting the interface?
>>
> I proposed as much in a reply to the cover letter; I'm willing to take
> patch one now for the sake of migration; the rest of this series I want
> a test suite that helps document the anticipated use case, or otherwise
> an x- prefix for the command names.

Give me a try with test, and if it will still be unclear, then let's add 
x- prefix for first steps.
(of course, my aim is to push the api without x- prefixes =)

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 5/6] qapi: add block-dirty-bitmap-merge

[Markus Armbruster]

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

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  qapi/block-core.json         | 38 ++++++++++++++++++++++++++++++++++++++
>  include/block/dirty-bitmap.h |  2 ++
>  block/dirty-bitmap.c         | 18 ++++++++++++++++++
>  blockdev.c                   | 30 ++++++++++++++++++++++++++++++
>  4 files changed, 88 insertions(+)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index b3851ee760..9f9cfa0a44 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1604,6 +1604,20 @@
>              '*persistent': 'bool', '*autoload': 'bool' } }
>  
>  ##
> +# @BlockDirtyBitmapMerge:
> +#
> +# @node: name of device/node which the bitmap is tracking
> +#
> +# @dst_name: name of the destination dirty bitmap
> +#
> +# @src_name: name of the source dirty bitmap
> +#
> +# Since: 2.12
> +##
> +{ 'struct': 'BlockDirtyBitmapMerge',
> +  'data': { 'node': 'str', 'dst_name': 'str', 'src_name': 'str' } }

Please use '-' instead of '_' in QAPI.  Exceptions are possible for
consistency with existing (mal-)practice.

> +
> +##
>  # @block-dirty-bitmap-add:
>  #
>  # Create a dirty bitmap with a name on the node, and start tracking the writes.
> @@ -1714,6 +1728,30 @@
>        'data': 'BlockDirtyBitmap' }
>  
>  ##
> +# @block-dirty-bitmap-merge:
> +#
> +# Merge @src_name dirty bitmap to @dst_name dirty bitmap. @src_name dirty
> +# bitmap is unchanged.
> +#
> +# Returns: nothing on success
> +#          If @node is not a valid block device, DeviceNotFound
> +#          If @dst_name or @src_name is not found, GenericError
> +#          If bitmaps has different sizes or granularities, GenericError

The less error classes other than GenericError are used, the happier I
am...  Do users really need to distinguish between the first error and
other errors?

> +#
> +# Since: 2.12
> +#
> +# Example:
> +#
> +# -> { "execute": "block-dirty-bitmap-merge",
> +#      "arguments": { "node": "drive0", "dst_name": "bitmap0",
> +#                     "src_name": "bitmap1" } }
> +# <- { "return": {} }
> +#
> +##
> +      { 'command': 'block-dirty-bitmap-merge',
> +        'data': 'BlockDirtyBitmapMerge' }
> +
> +##
>  # @BlockDirtyBitmapSha256:
>  #
>  # SHA256 hash of dirty bitmap data
[...]

Re: [Qemu-devel] [PATCH v2 3/6] qapi: add block-dirty-bitmap-enable/disable

[Markus Armbruster]

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

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  qapi/block-core.json | 42 ++++++++++++++++++++++++++++++++++++++++++
>  blockdev.c           | 42 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 84 insertions(+)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 827254db22..b3851ee760 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1672,6 +1672,48 @@
>    'data': 'BlockDirtyBitmap' }
>  
>  ##
> +# @block-dirty-bitmap-enable:
> +#
> +# Enable dirty bitmap, so that it will continue tracking disk changes.
> +#
> +# Returns: nothing on success
> +#          If @node is not a valid block device, DeviceNotFound
> +#          If @name is not found, GenericError with an explanation

What do you mean by "with an explanation"?  If it's the error objects
@desc member: that's trivial, any error carries it.

The less error classes other than GenericError are used, the happier I
am...  Do users really need to distinguish between these two errors?

> +#
> +# Since: 2.12
> +#
> +# Example:
> +#
> +# -> { "execute": "block-dirty-bitmap-enable",
> +#      "arguments": { "node": "drive0", "name": "bitmap0" } }
> +# <- { "return": {} }
> +#
> +##
> +  { 'command': 'block-dirty-bitmap-enable',
> +    'data': 'BlockDirtyBitmap' }
> +
> +##
> +# @block-dirty-bitmap-disable:
> +#
> +# Disable dirty bitmap, so that it will stop tracking disk changes.
> +#
> +# Returns: nothing on success
> +#          If @node is not a valid block device, DeviceNotFound
> +#          If @name is not found, GenericError with an explanation

Likewise.

> +#
> +# Since: 2.12
> +#
> +# Example:
> +#
> +# -> { "execute": "block-dirty-bitmap-disable",
> +#      "arguments": { "node": "drive0", "name": "bitmap0" } }
> +# <- { "return": {} }
> +#
> +##
> +    { 'command': 'block-dirty-bitmap-disable',
> +      'data': 'BlockDirtyBitmap' }
> +
> +##
>  # @BlockDirtyBitmapSha256:
>  #
>  # SHA256 hash of dirty bitmap data
[...]

Re: [Qemu-devel] [PATCH v2 3/6] qapi: add block-dirty-bitmap-enable/disable

[Vladimir Sementsov-Ogievskiy]

03.02.2018 19:09, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   qapi/block-core.json | 42 ++++++++++++++++++++++++++++++++++++++++++
>>   blockdev.c           | 42 ++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 84 insertions(+)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 827254db22..b3851ee760 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -1672,6 +1672,48 @@
>>     'data': 'BlockDirtyBitmap' }
>>   
>>   ##
>> +# @block-dirty-bitmap-enable:
>> +#
>> +# Enable dirty bitmap, so that it will continue tracking disk changes.
>> +#
>> +# Returns: nothing on success
>> +#          If @node is not a valid block device, DeviceNotFound
>> +#          If @name is not found, GenericError with an explanation
> What do you mean by "with an explanation"?  If it's the error objects
> @desc member: that's trivial, any error carries it.
>
> The less error classes other than GenericError are used, the happier I
> am...  Do users really need to distinguish between these two errors?

I've just used same semantics as it is used for 
block-dirty-bitmap-clear.. block-dirty-bitmap-remove and
block-dirty-bitmap-add have the same semantics too.

>
>> +#
>> +# Since: 2.12
>> +#
>> +# Example:
>> +#
>> +# -> { "execute": "block-dirty-bitmap-enable",
>> +#      "arguments": { "node": "drive0", "name": "bitmap0" } }
>> +# <- { "return": {} }
>> +#
>> +##
>> +  { 'command': 'block-dirty-bitmap-enable',
>> +    'data': 'BlockDirtyBitmap' }
>> +
>> +##
>> +# @block-dirty-bitmap-disable:
>> +#
>> +# Disable dirty bitmap, so that it will stop tracking disk changes.
>> +#
>> +# Returns: nothing on success
>> +#          If @node is not a valid block device, DeviceNotFound
>> +#          If @name is not found, GenericError with an explanation
> Likewise.
>
>> +#
>> +# Since: 2.12
>> +#
>> +# Example:
>> +#
>> +# -> { "execute": "block-dirty-bitmap-disable",
>> +#      "arguments": { "node": "drive0", "name": "bitmap0" } }
>> +# <- { "return": {} }
>> +#
>> +##
>> +    { 'command': 'block-dirty-bitmap-disable',
>> +      'data': 'BlockDirtyBitmap' }
>> +
>> +##
>>   # @BlockDirtyBitmapSha256:
>>   #
>>   # SHA256 hash of dirty bitmap data
> [...]


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 5/6] qapi: add block-dirty-bitmap-merge

[Vladimir Sementsov-Ogievskiy]

03.02.2018 19:06, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   qapi/block-core.json         | 38 ++++++++++++++++++++++++++++++++++++++
>>   include/block/dirty-bitmap.h |  2 ++
>>   block/dirty-bitmap.c         | 18 ++++++++++++++++++
>>   blockdev.c                   | 30 ++++++++++++++++++++++++++++++
>>   4 files changed, 88 insertions(+)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index b3851ee760..9f9cfa0a44 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -1604,6 +1604,20 @@
>>               '*persistent': 'bool', '*autoload': 'bool' } }
>>   
>>   ##
>> +# @BlockDirtyBitmapMerge:
>> +#
>> +# @node: name of device/node which the bitmap is tracking
>> +#
>> +# @dst_name: name of the destination dirty bitmap
>> +#
>> +# @src_name: name of the source dirty bitmap
>> +#
>> +# Since: 2.12
>> +##
>> +{ 'struct': 'BlockDirtyBitmapMerge',
>> +  'data': { 'node': 'str', 'dst_name': 'str', 'src_name': 'str' } }
> Please use '-' instead of '_' in QAPI.  Exceptions are possible for
> consistency with existing (mal-)practice.

Ok.

>
>> +
>> +##
>>   # @block-dirty-bitmap-add:
>>   #
>>   # Create a dirty bitmap with a name on the node, and start tracking the writes.
>> @@ -1714,6 +1728,30 @@
>>         'data': 'BlockDirtyBitmap' }
>>   
>>   ##
>> +# @block-dirty-bitmap-merge:
>> +#
>> +# Merge @src_name dirty bitmap to @dst_name dirty bitmap. @src_name dirty
>> +# bitmap is unchanged.
>> +#
>> +# Returns: nothing on success
>> +#          If @node is not a valid block device, DeviceNotFound
>> +#          If @dst_name or @src_name is not found, GenericError
>> +#          If bitmaps has different sizes or granularities, GenericError
> The less error classes other than GenericError are used, the happier I
> am...  Do users really need to distinguish between the first error and
> other errors?

this semantic is already used for other bitmap related commands..

>
>> +#
>> +# Since: 2.12
>> +#
>> +# Example:
>> +#
>> +# -> { "execute": "block-dirty-bitmap-merge",
>> +#      "arguments": { "node": "drive0", "dst_name": "bitmap0",
>> +#                     "src_name": "bitmap1" } }
>> +# <- { "return": {} }
>> +#
>> +##
>> +      { 'command': 'block-dirty-bitmap-merge',
>> +        'data': 'BlockDirtyBitmapMerge' }
>> +
>> +##
>>   # @BlockDirtyBitmapSha256:
>>   #
>>   # SHA256 hash of dirty bitmap data
> [...]


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 5/6] qapi: add block-dirty-bitmap-merge

[Vladimir Sementsov-Ogievskiy]

20.09.2018 22:40, Eric Blake wrote:
> [reviving an old patch]
>
> On 1/16/18 6:54 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   qapi/block-core.json         | 38 
>> ++++++++++++++++++++++++++++++++++++++
>>   include/block/dirty-bitmap.h |  2 ++
>>   block/dirty-bitmap.c         | 18 ++++++++++++++++++
>>   blockdev.c                   | 30 ++++++++++++++++++++++++++++++
>>   4 files changed, 88 insertions(+)
>>
>
> We've already hit a couple issues with this patch mentioned in other 
> threads:
>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index b3851ee760..9f9cfa0a44 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -1604,6 +1604,20 @@
>>               '*persistent': 'bool', '*autoload': 'bool' } }
>>     ##
>> +# @BlockDirtyBitmapMerge:
>> +#
>> +# @node: name of device/node which the bitmap is tracking
>> +#
>> +# @dst_name: name of the destination dirty bitmap
>> +#
>> +# @src_name: name of the source dirty bitmap
>
> Naming choice (prefer - over _):
> https://lists.gnu.org/archive/html/qemu-devel/2018-09/msg02352.html
>
>> +++ b/block/dirty-bitmap.c
>> @@ -699,3 +699,21 @@ char *bdrv_dirty_bitmap_sha256(const 
>> BdrvDirtyBitmap *bitmap, Error **errp)
>>   {
>>       return hbitmap_sha256(bitmap->bitmap, errp);
>>   }
>> +
>> +void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const 
>> BdrvDirtyBitmap *src,
>> +                             Error **errp)
>> +{
>> +    /* only bitmaps from one bds are supported */
>> +    assert(dest->mutex == src->mutex);
>> +
>> +    qemu_mutex_lock(dest->mutex);
>> +
>> +    assert(bdrv_dirty_bitmap_enabled(dest));
>
> Merging to a disabled bitmap is desirable:
> https://lists.gnu.org/archive/html/qemu-devel/2018-09/msg02358.html
>
>> + assert(!bdrv_dirty_bitmap_readonly(dest));
>> +
>> +    if (!hbitmap_merge(dest->bitmap, src->bitmap)) {
>
> And here's another one I ran into. hbitmap_merge() does NOT properly 
> update hbitmap_count(), which means statistics about the number of 
> bits set in the bitmap, as visible through QMP "query-block", are 
> inaccurate after a merge.
>
> Check out hbitmap_set() and hbitmap_reset(), which carefully recompute 
> hb->count using hb_count_between() as part of flipping bits. But since 
> hbitmap_merge() fails to recompute bits, various other aspects of the 
> code go haywire (for example, code that uses hbitmap_empty() or 
> hbitmap_count()==0 to short-circuit operations [and hbitmap_merge() is 
> one such caller] makes the wrong choice on a bitmap that started life 
> empty, and had a non-empty bitmap merged in, because hb->count is 
> still 0).
>
> I think that x-debug-block-dirty-bitmap-sha256 should be enhanced to 
> output count as well as the checksum of the bitmap contents, and then 
> that we need iotests coverage of x-block-dirty-bitmap-merge, complete 
> with a check that the count is properly updated. iotests 165, 169, 
> 176, 199 all perform bitmap checksum validations, but so far none of 
> them perform a bitmap merge.  Be careful in writing any such test that 
> we don't re-introduce any sensitivities on 32- vs. 64-bit or 
> endianness in our checksum computation (see commit 2807746f for 
> comparison).
>

or at least we should check count in tests/test-hbitmap..

About bitmap api: can we now switch naming style from '_' to '-' and 
drop x- prefixes? Or we are waiting for your libvirt API proposal 
acceptance?

-- 
Best regards,
Vladimir