[Qemu-devel] [PATCH v2 0/4] transaction support for bitmap merge

[Qemu-devel] [PATCH v2 0/4] transaction support for bitmap merge

[Vladimir Sementsov-Ogievskiy]

This is a last brick, necessary to play with nbd bitmap export in
conjunction with image fleecing.

v2: don't compare with v1, it is changed a lot, to do the whole thing
    in .prepare instead of .commit. It is needed to be compatible with
    backup block job transaction actions [John]

Vladimir Sementsov-Ogievskiy (4):
  drity-bitmap: switch assert-fails to errors in bdrv_merge_dirty_bitmap
  dirty-bitmap: restore bitmap after merge
  blockdev: rename block-dirty-bitmap-clear transaction handlers
  qapi: add transaction support for x-block-dirty-bitmap-merge

 qapi/transaction.json        |  2 ++
 include/block/block_int.h    |  2 +-
 include/block/dirty-bitmap.h |  2 +-
 include/qemu/hbitmap.h       | 25 +++++++++++-------
 block/dirty-bitmap.c         | 36 ++++++++++++++++++++------
 blockdev.c                   | 60 +++++++++++++++++++++++++++++++-------------
 util/hbitmap.c               | 11 +++++---
 7 files changed, 100 insertions(+), 38 deletions(-)

-- 
2.11.1

[Qemu-devel] [PATCH v2 1/4] drity-bitmap: switch assert-fails to errors in bdrv_merge_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

Move checks from qmp_x_block_dirty_bitmap_merge() to
bdrv_merge_dirty_bitmap(), to share them with dirty bitmap merge
transaction action in future commit.

Note: for now, only qmp_x_block_dirty_bitmap_merge() calls
bdrv_merge_dirty_bitmap().

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/dirty-bitmap.c | 15 +++++++++++++--
 blockdev.c           | 10 ----------
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index c9b8a6fd52..6c8761e027 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -798,12 +798,23 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
 
     qemu_mutex_lock(dest->mutex);
 
-    assert(bdrv_dirty_bitmap_enabled(dest));
-    assert(!bdrv_dirty_bitmap_readonly(dest));
+    if (bdrv_dirty_bitmap_frozen(dest)) {
+        error_setg(errp, "Bitmap '%s' is frozen and cannot be modified",
+                   dest->name);
+        goto out;
+    }
+
+    if (bdrv_dirty_bitmap_readonly(dest)) {
+        error_setg(errp, "Bitmap '%s' is readonly and cannot be modified",
+                   dest->name);
+        goto out;
+    }
 
     if (!hbitmap_merge(dest->bitmap, src->bitmap)) {
         error_setg(errp, "Bitmaps are incompatible and can't be merged");
+        goto out;
     }
 
+out:
     qemu_mutex_unlock(dest->mutex);
 }
diff --git a/blockdev.c b/blockdev.c
index 72f5347df5..902338e815 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2955,16 +2955,6 @@ void qmp_x_block_dirty_bitmap_merge(const char *node, const char *dst_name,
         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);
-- 
2.11.1

[Qemu-devel] [PATCH v2 2/4] dirty-bitmap: restore bitmap after merge

[Vladimir Sementsov-Ogievskiy]

Add backup parameter to bdrv_merge_dirty_bitmap() and refactor
bdrv_undo_clear_dirty_bitmap() to use it both for undo clean and merge.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/block_int.h    |  2 +-
 include/block/dirty-bitmap.h |  2 +-
 include/qemu/hbitmap.h       | 25 ++++++++++++++++---------
 block/dirty-bitmap.c         | 21 ++++++++++++++++-----
 blockdev.c                   |  4 ++--
 util/hbitmap.c               | 11 ++++++++---
 6 files changed, 44 insertions(+), 21 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index af71b414be..64e38b4fae 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1144,7 +1144,7 @@ bool blk_dev_is_medium_locked(BlockBackend *blk);
 void bdrv_set_dirty(BlockDriverState *bs, int64_t offset, int64_t bytes);
 
 void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out);
-void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in);
+void bdrv_restore_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *backup);
 
 void bdrv_inc_in_flight(BlockDriverState *bs);
 void bdrv_dec_in_flight(BlockDriverState *bs);
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 259bd27c40..201ff7f20b 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -71,7 +71,7 @@ void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
                                        bool persistent);
 void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool qmp_locked);
 void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
-                             Error **errp);
+                             HBitmap **backup, Error **errp);
 
 /* Functions that require manual locking.  */
 void bdrv_dirty_bitmap_lock(BdrvDirtyBitmap *bitmap);
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index ddca52c48e..a7cb780592 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -73,16 +73,23 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size);
 
 /**
  * hbitmap_merge:
- * @a: The bitmap to store the result in.
- * @b: The bitmap to merge into @a.
- * @return true if the merge was successful,
- *         false if it was not attempted.
- *
- * Merge two bitmaps together.
- * A := A (BITOR) B.
- * B is left unmodified.
+ *
+ * Store result of merging @a and @b into @result.
+ * @result is allowed to be equal to @a or @b.
+ *
+ * Return true if the merge was successful,
+ *        false if it was not attempted.
+ */
+bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result);
+
+/**
+ * hbitmap_can_merge:
+ *
+ * hbitmap_can_merge(a, b) && hbitmap_can_merge(a, result) is sufficient and
+ * necessary for hbitmap_merge will not fail.
+ *
  */
-bool hbitmap_merge(HBitmap *a, const HBitmap *b);
+bool hbitmap_can_merge(const HBitmap *a, const HBitmap *b);
 
 /**
  * hbitmap_empty:
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 6c8761e027..8ac933cf1c 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -314,7 +314,7 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
         return NULL;
     }
 
-    if (!hbitmap_merge(parent->bitmap, successor->bitmap)) {
+    if (!hbitmap_merge(parent->bitmap, successor->bitmap, parent->bitmap)) {
         error_setg(errp, "Merging of parent and successor bitmap failed");
         return NULL;
     }
@@ -633,12 +633,12 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
     bdrv_dirty_bitmap_unlock(bitmap);
 }
 
-void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in)
+void bdrv_restore_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *backup)
 {
     HBitmap *tmp = bitmap->bitmap;
     assert(bdrv_dirty_bitmap_enabled(bitmap));
     assert(!bdrv_dirty_bitmap_readonly(bitmap));
-    bitmap->bitmap = in;
+    bitmap->bitmap = backup;
     hbitmap_free(tmp);
 }
 
@@ -791,8 +791,10 @@ int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, uint64_t offset)
 }
 
 void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
-                             Error **errp)
+                             HBitmap **backup, Error **errp)
 {
+    bool ret;
+
     /* only bitmaps from one bds are supported */
     assert(dest->mutex == src->mutex);
 
@@ -810,11 +812,20 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
         goto out;
     }
 
-    if (!hbitmap_merge(dest->bitmap, src->bitmap)) {
+    if (!hbitmap_can_merge(dest->bitmap, src->bitmap)) {
         error_setg(errp, "Bitmaps are incompatible and can't be merged");
         goto out;
     }
 
+    if (backup) {
+        *backup = dest->bitmap;
+        dest->bitmap = hbitmap_alloc(dest->size, hbitmap_granularity(*backup));
+        ret = hbitmap_merge(*backup, src->bitmap, dest->bitmap);
+    } else {
+        ret = hbitmap_merge(dest->bitmap, src->bitmap, dest->bitmap);
+    }
+    assert(ret);
+
 out:
     qemu_mutex_unlock(dest->mutex);
 }
diff --git a/blockdev.c b/blockdev.c
index 902338e815..9cb29ca63e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2032,7 +2032,7 @@ static void block_dirty_bitmap_clear_abort(BlkActionState *common)
                                              common, common);
 
     if (state->backup) {
-        bdrv_undo_clear_dirty_bitmap(state->bitmap, state->backup);
+        bdrv_restore_dirty_bitmap(state->bitmap, state->backup);
     }
 }
 
@@ -2961,7 +2961,7 @@ void qmp_x_block_dirty_bitmap_merge(const char *node, const char *dst_name,
         return;
     }
 
-    bdrv_merge_dirty_bitmap(dst, src, errp);
+    bdrv_merge_dirty_bitmap(dst, src, NULL, errp);
 }
 
 BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
diff --git a/util/hbitmap.c b/util/hbitmap.c
index bcd304041a..d5aca5159f 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -723,6 +723,10 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size)
     }
 }
 
+bool hbitmap_can_merge(const HBitmap *a, const HBitmap *b)
+{
+    return (a->size == b->size) && (a->granularity == b->granularity);
+}
 
 /**
  * Given HBitmaps A and B, let A := A (BITOR) B.
@@ -731,14 +735,15 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size)
  * @return true if the merge was successful,
  *         false if it was not attempted.
  */
-bool hbitmap_merge(HBitmap *a, const HBitmap *b)
+bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result)
 {
     int i;
     uint64_t j;
 
-    if ((a->size != b->size) || (a->granularity != b->granularity)) {
+    if (!hbitmap_can_merge(a, b) || !hbitmap_can_merge(a, result)) {
         return false;
     }
+    assert(hbitmap_can_merge(b, result));
 
     if (hbitmap_count(b) == 0) {
         return true;
@@ -750,7 +755,7 @@ bool hbitmap_merge(HBitmap *a, const HBitmap *b)
      */
     for (i = HBITMAP_LEVELS - 1; i >= 0; i--) {
         for (j = 0; j < a->sizes[i]; j++) {
-            a->levels[i][j] |= b->levels[i][j];
+            result->levels[i][j] = a->levels[i][j] | b->levels[i][j];
         }
     }
 
-- 
2.11.1

[Qemu-devel] [PATCH v2 3/4] blockdev: rename block-dirty-bitmap-clear transaction handlers

[Vladimir Sementsov-Ogievskiy]

Rename block-dirty-bitmap-clear transaction handlers to reuse them for
x-block-dirty-bitmap-merge transaction in the following patch.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 blockdev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 9cb29ca63e..5348e8ba9b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2026,7 +2026,7 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common,
     bdrv_clear_dirty_bitmap(state->bitmap, &state->backup);
 }
 
-static void block_dirty_bitmap_clear_abort(BlkActionState *common)
+static void block_dirty_bitmap_restore(BlkActionState *common)
 {
     BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
                                              common, common);
@@ -2036,7 +2036,7 @@ static void block_dirty_bitmap_clear_abort(BlkActionState *common)
     }
 }
 
-static void block_dirty_bitmap_clear_commit(BlkActionState *common)
+static void block_dirty_bitmap_free_backup(BlkActionState *common)
 {
     BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
                                              common, common);
@@ -2170,8 +2170,8 @@ static const BlkActionOps actions[] = {
     [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_CLEAR] = {
         .instance_size = sizeof(BlockDirtyBitmapState),
         .prepare = block_dirty_bitmap_clear_prepare,
-        .commit = block_dirty_bitmap_clear_commit,
-        .abort = block_dirty_bitmap_clear_abort,
+        .commit = block_dirty_bitmap_free_backup,
+        .abort = block_dirty_bitmap_restore,
     },
     [TRANSACTION_ACTION_KIND_X_BLOCK_DIRTY_BITMAP_ENABLE] = {
         .instance_size = sizeof(BlockDirtyBitmapState),
-- 
2.11.1

[Qemu-devel] [PATCH v2 4/4] qapi: add transaction support for x-block-dirty-bitmap-merge

[Vladimir Sementsov-Ogievskiy]

New action is like clean action: do the whole thing in .prepare and
undo in .abort. This behavior for bitmap-changing actions is needed
because backup job actions use bitmap in .prepare.

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

diff --git a/qapi/transaction.json b/qapi/transaction.json
index d7e4274550..5875cdb16c 100644
--- a/qapi/transaction.json
+++ b/qapi/transaction.json
@@ -48,6 +48,7 @@
 # - @block-dirty-bitmap-clear: since 2.5
 # - @x-block-dirty-bitmap-enable: since 3.0
 # - @x-block-dirty-bitmap-disable: since 3.0
+# - @x-block-dirty-bitmap-merge: since 3.1
 # - @blockdev-backup: since 2.3
 # - @blockdev-snapshot: since 2.5
 # - @blockdev-snapshot-internal-sync: since 1.7
@@ -63,6 +64,7 @@
        'block-dirty-bitmap-clear': 'BlockDirtyBitmap',
        'x-block-dirty-bitmap-enable': 'BlockDirtyBitmap',
        'x-block-dirty-bitmap-disable': 'BlockDirtyBitmap',
+       'x-block-dirty-bitmap-merge': 'BlockDirtyBitmapMerge',
        'blockdev-backup': 'BlockdevBackup',
        'blockdev-snapshot': 'BlockdevSnapshot',
        'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
diff --git a/blockdev.c b/blockdev.c
index 5348e8ba9b..feebbb9a9a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1940,6 +1940,7 @@ static void blockdev_backup_clean(BlkActionState *common)
 typedef struct BlockDirtyBitmapState {
     BlkActionState common;
     BdrvDirtyBitmap *bitmap;
+    BdrvDirtyBitmap *merge_source;
     BlockDriverState *bs;
     HBitmap *backup;
     bool prepared;
@@ -2112,6 +2113,35 @@ static void block_dirty_bitmap_disable_abort(BlkActionState *common)
     }
 }
 
+static void block_dirty_bitmap_merge_prepare(BlkActionState *common,
+                                             Error **errp)
+{
+    BlockDirtyBitmapMerge *action;
+    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+                                             common, common);
+
+    if (action_check_completion_mode(common, errp) < 0) {
+        return;
+    }
+
+    action = common->action->u.x_block_dirty_bitmap_merge.data;
+    state->bitmap = block_dirty_bitmap_lookup(action->node,
+                                              action->dst_name,
+                                              &state->bs,
+                                              errp);
+    if (!state->bitmap) {
+        return;
+    }
+
+    state->merge_source = bdrv_find_dirty_bitmap(state->bs, action->src_name);
+    if (!state->merge_source) {
+        return;
+    }
+
+    bdrv_merge_dirty_bitmap(state->bitmap, state->merge_source, &state->backup,
+                            errp);
+}
+
 static void abort_prepare(BlkActionState *common, Error **errp)
 {
     error_setg(errp, "Transaction aborted using Abort action");
@@ -2182,7 +2212,13 @@ static const BlkActionOps actions[] = {
         .instance_size = sizeof(BlockDirtyBitmapState),
         .prepare = block_dirty_bitmap_disable_prepare,
         .abort = block_dirty_bitmap_disable_abort,
-     }
+    },
+    [TRANSACTION_ACTION_KIND_X_BLOCK_DIRTY_BITMAP_MERGE] = {
+        .instance_size = sizeof(BlockDirtyBitmapState),
+        .prepare = block_dirty_bitmap_merge_prepare,
+        .commit = block_dirty_bitmap_free_backup,
+        .abort = block_dirty_bitmap_restore,
+    }
 };
 
 /**
-- 
2.11.1

Re: [Qemu-devel] [PATCH v2 1/4] drity-bitmap: switch assert-fails to errors in bdrv_merge_dirty_bitmap

[John Snow]

s/drity-bitmap/dirty-bitmap/g

On 07/06/2018 07:36 AM, Vladimir Sementsov-Ogievskiy wrote:
> Move checks from qmp_x_block_dirty_bitmap_merge() to
> bdrv_merge_dirty_bitmap(), to share them with dirty bitmap merge
> transaction action in future commit.
> 
> Note: for now, only qmp_x_block_dirty_bitmap_merge() calls
> bdrv_merge_dirty_bitmap().
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/dirty-bitmap.c | 15 +++++++++++++--
>  blockdev.c           | 10 ----------
>  2 files changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index c9b8a6fd52..6c8761e027 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -798,12 +798,23 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>  
>      qemu_mutex_lock(dest->mutex);
>  
> -    assert(bdrv_dirty_bitmap_enabled(dest));
> -    assert(!bdrv_dirty_bitmap_readonly(dest));
> +    if (bdrv_dirty_bitmap_frozen(dest)) {
> +        error_setg(errp, "Bitmap '%s' is frozen and cannot be modified",
> +                   dest->name);
> +        goto out;
> +    }
> +
> +    if (bdrv_dirty_bitmap_readonly(dest)) {
> +        error_setg(errp, "Bitmap '%s' is readonly and cannot be modified",
> +                   dest->name);
> +        goto out;
> +    }
>  
>      if (!hbitmap_merge(dest->bitmap, src->bitmap)) {
>          error_setg(errp, "Bitmaps are incompatible and can't be merged");
> +        goto out;

This is unusual right before the out: segment, but harmless.

>      }
>  
> +out:
>      qemu_mutex_unlock(dest->mutex);
>  }
> diff --git a/blockdev.c b/blockdev.c
> index 72f5347df5..902338e815 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2955,16 +2955,6 @@ void qmp_x_block_dirty_bitmap_merge(const char *node, const char *dst_name,
>          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);
> 

When fixing the commit title, we can add:

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

Re: [Qemu-devel] [PATCH v2 2/4] dirty-bitmap: restore bitmap after merge

[John Snow]

We actually don't restore anything just yet, so this commit message is a
little off I think.

On 07/06/2018 07:36 AM, Vladimir Sementsov-Ogievskiy wrote:
> Add backup parameter to bdrv_merge_dirty_bitmap() and refactor
> bdrv_undo_clear_dirty_bitmap() to use it both for undo clean and merge.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/block_int.h    |  2 +-
>  include/block/dirty-bitmap.h |  2 +-
>  include/qemu/hbitmap.h       | 25 ++++++++++++++++---------
>  block/dirty-bitmap.c         | 21 ++++++++++++++++-----
>  blockdev.c                   |  4 ++--
>  util/hbitmap.c               | 11 ++++++++---
>  6 files changed, 44 insertions(+), 21 deletions(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index af71b414be..64e38b4fae 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -1144,7 +1144,7 @@ bool blk_dev_is_medium_locked(BlockBackend *blk);
>  void bdrv_set_dirty(BlockDriverState *bs, int64_t offset, int64_t bytes);
>  
>  void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out);
> -void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in);
> +void bdrv_restore_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *backup);
>  
>  void bdrv_inc_in_flight(BlockDriverState *bs);
>  void bdrv_dec_in_flight(BlockDriverState *bs);
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 259bd27c40..201ff7f20b 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -71,7 +71,7 @@ void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
>                                         bool persistent);
>  void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool qmp_locked);
>  void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
> -                             Error **errp);
> +                             HBitmap **backup, Error **errp);
>  
>  /* Functions that require manual locking.  */
>  void bdrv_dirty_bitmap_lock(BdrvDirtyBitmap *bitmap);
> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
> index ddca52c48e..a7cb780592 100644
> --- a/include/qemu/hbitmap.h
> +++ b/include/qemu/hbitmap.h
> @@ -73,16 +73,23 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size);
>  
>  /**
>   * hbitmap_merge:
> - * @a: The bitmap to store the result in.
> - * @b: The bitmap to merge into @a.
> - * @return true if the merge was successful,
> - *         false if it was not attempted.
> - *
> - * Merge two bitmaps together.
> - * A := A (BITOR) B.
> - * B is left unmodified.
> + *
> + * Store result of merging @a and @b into @result.
> + * @result is allowed to be equal to @a or @b.
> + *
> + * Return true if the merge was successful,
> + *        false if it was not attempted.
> + */
> +bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result);

I wonder if this confuses any optimization or analysis tools. I'll
assume it's safe.

> +
> +/**
> + * hbitmap_can_merge:
> + *
> + * hbitmap_can_merge(a, b) && hbitmap_can_merge(a, result) is sufficient and
> + * necessary for hbitmap_merge will not fail.
> + *
>   */
> -bool hbitmap_merge(HBitmap *a, const HBitmap *b);
> +bool hbitmap_can_merge(const HBitmap *a, const HBitmap *b);
>  
>  /**
>   * hbitmap_empty:
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 6c8761e027..8ac933cf1c 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -314,7 +314,7 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
>          return NULL;
>      }
>  
> -    if (!hbitmap_merge(parent->bitmap, successor->bitmap)) {
> +    if (!hbitmap_merge(parent->bitmap, successor->bitmap, parent->bitmap)) {
>          error_setg(errp, "Merging of parent and successor bitmap failed");
>          return NULL;
>      }
> @@ -633,12 +633,12 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
>      bdrv_dirty_bitmap_unlock(bitmap);
>  }
>  
> -void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in)
> +void bdrv_restore_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *backup)
>  {
>      HBitmap *tmp = bitmap->bitmap;
>      assert(bdrv_dirty_bitmap_enabled(bitmap));
>      assert(!bdrv_dirty_bitmap_readonly(bitmap));
> -    bitmap->bitmap = in;
> +    bitmap->bitmap = backup;
>      hbitmap_free(tmp);
>  }
>  
> @@ -791,8 +791,10 @@ int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, uint64_t offset)
>  }
>  
>  void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
> -                             Error **errp)
> +                             HBitmap **backup, Error **errp)
>  {
> +    bool ret;
> +
>      /* only bitmaps from one bds are supported */
>      assert(dest->mutex == src->mutex);
>  
> @@ -810,11 +812,20 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>          goto out;
>      }
>  
> -    if (!hbitmap_merge(dest->bitmap, src->bitmap)) {
> +    if (!hbitmap_can_merge(dest->bitmap, src->bitmap)) {
>          error_setg(errp, "Bitmaps are incompatible and can't be merged");
>          goto out;
>      }
>  
> +    if (backup) {
> +        *backup = dest->bitmap;
> +        dest->bitmap = hbitmap_alloc(dest->size, hbitmap_granularity(*backup));
> +        ret = hbitmap_merge(*backup, src->bitmap, dest->bitmap);
> +    } else {
> +        ret = hbitmap_merge(dest->bitmap, src->bitmap, dest->bitmap);
> +    }
> +    assert(ret);
> +
>  out:
>      qemu_mutex_unlock(dest->mutex);
>  }
> diff --git a/blockdev.c b/blockdev.c
> index 902338e815..9cb29ca63e 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2032,7 +2032,7 @@ static void block_dirty_bitmap_clear_abort(BlkActionState *common)
>                                               common, common);
>  
>      if (state->backup) {
> -        bdrv_undo_clear_dirty_bitmap(state->bitmap, state->backup);
> +        bdrv_restore_dirty_bitmap(state->bitmap, state->backup);
>      }
>  }
>  
> @@ -2961,7 +2961,7 @@ void qmp_x_block_dirty_bitmap_merge(const char *node, const char *dst_name,
>          return;
>      }
>  
> -    bdrv_merge_dirty_bitmap(dst, src, errp);
> +    bdrv_merge_dirty_bitmap(dst, src, NULL, errp);
>  }
>  
>  BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
> diff --git a/util/hbitmap.c b/util/hbitmap.c
> index bcd304041a..d5aca5159f 100644
> --- a/util/hbitmap.c
> +++ b/util/hbitmap.c
> @@ -723,6 +723,10 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size)
>      }
>  }
>  
> +bool hbitmap_can_merge(const HBitmap *a, const HBitmap *b)
> +{
> +    return (a->size == b->size) && (a->granularity == b->granularity);
> +}
>  
>  /**
>   * Given HBitmaps A and B, let A := A (BITOR) B.
> @@ -731,14 +735,15 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size)
>   * @return true if the merge was successful,
>   *         false if it was not attempted.
>   */
> -bool hbitmap_merge(HBitmap *a, const HBitmap *b)
> +bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result)
>  {
>      int i;
>      uint64_t j;
>  
> -    if ((a->size != b->size) || (a->granularity != b->granularity)) {
> +    if (!hbitmap_can_merge(a, b) || !hbitmap_can_merge(a, result)) {
>          return false;
>      }
> +    assert(hbitmap_can_merge(b, result));

This line might really be overkill, but I suppose it doesn't take long
to check, so no harm.

>  
>      if (hbitmap_count(b) == 0) {
>          return true;
> @@ -750,7 +755,7 @@ bool hbitmap_merge(HBitmap *a, const HBitmap *b)
>       */
>      for (i = HBITMAP_LEVELS - 1; i >= 0; i--) {
>          for (j = 0; j < a->sizes[i]; j++) {
> -            a->levels[i][j] |= b->levels[i][j];
> +            result->levels[i][j] = a->levels[i][j] | b->levels[i][j];
>          }
>      }
>  
> 

This patch really does two things; Refactors undo_clear and adds the new
merge functionality, but not worth respinning just to separate two
trivial changes.

Only minor comments, so:

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

Re: [Qemu-devel] [PATCH v2 3/4] blockdev: rename block-dirty-bitmap-clear transaction handlers

[John Snow]

On 07/06/2018 07:36 AM, Vladimir Sementsov-Ogievskiy wrote:
> Rename block-dirty-bitmap-clear transaction handlers to reuse them for
> x-block-dirty-bitmap-merge transaction in the following patch.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  blockdev.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 9cb29ca63e..5348e8ba9b 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2026,7 +2026,7 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common,
>      bdrv_clear_dirty_bitmap(state->bitmap, &state->backup);
>  }
>  
> -static void block_dirty_bitmap_clear_abort(BlkActionState *common)
> +static void block_dirty_bitmap_restore(BlkActionState *common)
>  {
>      BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>                                               common, common);
> @@ -2036,7 +2036,7 @@ static void block_dirty_bitmap_clear_abort(BlkActionState *common)
>      }
>  }
>  
> -static void block_dirty_bitmap_clear_commit(BlkActionState *common)
> +static void block_dirty_bitmap_free_backup(BlkActionState *common)
>  {
>      BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>                                               common, common);
> @@ -2170,8 +2170,8 @@ static const BlkActionOps actions[] = {
>      [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_CLEAR] = {
>          .instance_size = sizeof(BlockDirtyBitmapState),
>          .prepare = block_dirty_bitmap_clear_prepare,
> -        .commit = block_dirty_bitmap_clear_commit,
> -        .abort = block_dirty_bitmap_clear_abort,
> +        .commit = block_dirty_bitmap_free_backup,
> +        .abort = block_dirty_bitmap_restore,
>      },
>      [TRANSACTION_ACTION_KIND_X_BLOCK_DIRTY_BITMAP_ENABLE] = {
>          .instance_size = sizeof(BlockDirtyBitmapState),
> 

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

Re: [Qemu-devel] [PATCH v2 4/4] qapi: add transaction support for x-block-dirty-bitmap-merge

[John Snow]

On 07/06/2018 07:36 AM, Vladimir Sementsov-Ogievskiy wrote:
> New action is like clean action: do the whole thing in .prepare and
> undo in .abort. This behavior for bitmap-changing actions is needed
> because backup job actions use bitmap in .prepare.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  qapi/transaction.json |  2 ++
>  blockdev.c            | 38 +++++++++++++++++++++++++++++++++++++-
>  2 files changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/transaction.json b/qapi/transaction.json
> index d7e4274550..5875cdb16c 100644
> --- a/qapi/transaction.json
> +++ b/qapi/transaction.json
> @@ -48,6 +48,7 @@
>  # - @block-dirty-bitmap-clear: since 2.5
>  # - @x-block-dirty-bitmap-enable: since 3.0
>  # - @x-block-dirty-bitmap-disable: since 3.0
> +# - @x-block-dirty-bitmap-merge: since 3.1
>  # - @blockdev-backup: since 2.3
>  # - @blockdev-snapshot: since 2.5
>  # - @blockdev-snapshot-internal-sync: since 1.7
> @@ -63,6 +64,7 @@
>         'block-dirty-bitmap-clear': 'BlockDirtyBitmap',
>         'x-block-dirty-bitmap-enable': 'BlockDirtyBitmap',
>         'x-block-dirty-bitmap-disable': 'BlockDirtyBitmap',
> +       'x-block-dirty-bitmap-merge': 'BlockDirtyBitmapMerge',
>         'blockdev-backup': 'BlockdevBackup',
>         'blockdev-snapshot': 'BlockdevSnapshot',
>         'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
> diff --git a/blockdev.c b/blockdev.c
> index 5348e8ba9b..feebbb9a9a 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1940,6 +1940,7 @@ static void blockdev_backup_clean(BlkActionState *common)
>  typedef struct BlockDirtyBitmapState {
>      BlkActionState common;
>      BdrvDirtyBitmap *bitmap;
> +    BdrvDirtyBitmap *merge_source;

Is this necessary?

>      BlockDriverState *bs;
>      HBitmap *backup;
>      bool prepared;
> @@ -2112,6 +2113,35 @@ static void block_dirty_bitmap_disable_abort(BlkActionState *common)
>      }
>  }
>  
> +static void block_dirty_bitmap_merge_prepare(BlkActionState *common,
> +                                             Error **errp)
> +{
> +    BlockDirtyBitmapMerge *action;
> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> +                                             common, common);
> +
> +    if (action_check_completion_mode(common, errp) < 0) {
> +        return;
> +    }
> +
> +    action = common->action->u.x_block_dirty_bitmap_merge.data;
> +    state->bitmap = block_dirty_bitmap_lookup(action->node,
> +                                              action->dst_name,
> +                                              &state->bs,
> +                                              errp);
> +    if (!state->bitmap) {
> +        return;
> +    }
> +
> +    state->merge_source = bdrv_find_dirty_bitmap(state->bs, action->src_name);
> +    if (!state->merge_source) {
> +        return;
> +    }
> +
> +    bdrv_merge_dirty_bitmap(state->bitmap, state->merge_source, &state->backup,
> +                            errp);
> +}
> +
>  static void abort_prepare(BlkActionState *common, Error **errp)
>  {
>      error_setg(errp, "Transaction aborted using Abort action");
> @@ -2182,7 +2212,13 @@ static const BlkActionOps actions[] = {
>          .instance_size = sizeof(BlockDirtyBitmapState),
>          .prepare = block_dirty_bitmap_disable_prepare,
>          .abort = block_dirty_bitmap_disable_abort,
> -     }
> +    },
> +    [TRANSACTION_ACTION_KIND_X_BLOCK_DIRTY_BITMAP_MERGE] = {
> +        .instance_size = sizeof(BlockDirtyBitmapState),
> +        .prepare = block_dirty_bitmap_merge_prepare,
> +        .commit = block_dirty_bitmap_free_backup,
> +        .abort = block_dirty_bitmap_restore,
> +    }
>  };
>  
>  /**
> 

If the new state is not necessary and you remove it:

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

Re: [Qemu-devel] [PATCH v2 4/4] qapi: add transaction support for x-block-dirty-bitmap-merge

[Vladimir Sementsov-Ogievskiy]

11.09.2018 22:45, John Snow wrote:
>
> On 07/06/2018 07:36 AM, Vladimir Sementsov-Ogievskiy wrote:
>> New action is like clean action: do the whole thing in .prepare and
>> undo in .abort. This behavior for bitmap-changing actions is needed
>> because backup job actions use bitmap in .prepare.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   qapi/transaction.json |  2 ++
>>   blockdev.c            | 38 +++++++++++++++++++++++++++++++++++++-
>>   2 files changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/qapi/transaction.json b/qapi/transaction.json
>> index d7e4274550..5875cdb16c 100644
>> --- a/qapi/transaction.json
>> +++ b/qapi/transaction.json
>> @@ -48,6 +48,7 @@
>>   # - @block-dirty-bitmap-clear: since 2.5
>>   # - @x-block-dirty-bitmap-enable: since 3.0
>>   # - @x-block-dirty-bitmap-disable: since 3.0
>> +# - @x-block-dirty-bitmap-merge: since 3.1
>>   # - @blockdev-backup: since 2.3
>>   # - @blockdev-snapshot: since 2.5
>>   # - @blockdev-snapshot-internal-sync: since 1.7
>> @@ -63,6 +64,7 @@
>>          'block-dirty-bitmap-clear': 'BlockDirtyBitmap',
>>          'x-block-dirty-bitmap-enable': 'BlockDirtyBitmap',
>>          'x-block-dirty-bitmap-disable': 'BlockDirtyBitmap',
>> +       'x-block-dirty-bitmap-merge': 'BlockDirtyBitmapMerge',
>>          'blockdev-backup': 'BlockdevBackup',
>>          'blockdev-snapshot': 'BlockdevSnapshot',
>>          'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
>> diff --git a/blockdev.c b/blockdev.c
>> index 5348e8ba9b..feebbb9a9a 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -1940,6 +1940,7 @@ static void blockdev_backup_clean(BlkActionState *common)
>>   typedef struct BlockDirtyBitmapState {
>>       BlkActionState common;
>>       BdrvDirtyBitmap *bitmap;
>> +    BdrvDirtyBitmap *merge_source;
> Is this necessary?

looks like it isn't, will drop it.

>
>>       BlockDriverState *bs;
>>       HBitmap *backup;
>>       bool prepared;
>> @@ -2112,6 +2113,35 @@ static void block_dirty_bitmap_disable_abort(BlkActionState *common)
>>       }
>>   }
>>   
>> +static void block_dirty_bitmap_merge_prepare(BlkActionState *common,
>> +                                             Error **errp)
>> +{
>> +    BlockDirtyBitmapMerge *action;
>> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>> +                                             common, common);
>> +
>> +    if (action_check_completion_mode(common, errp) < 0) {
>> +        return;
>> +    }
>> +
>> +    action = common->action->u.x_block_dirty_bitmap_merge.data;
>> +    state->bitmap = block_dirty_bitmap_lookup(action->node,
>> +                                              action->dst_name,
>> +                                              &state->bs,
>> +                                              errp);
>> +    if (!state->bitmap) {
>> +        return;
>> +    }
>> +
>> +    state->merge_source = bdrv_find_dirty_bitmap(state->bs, action->src_name);
>> +    if (!state->merge_source) {
>> +        return;
>> +    }
>> +
>> +    bdrv_merge_dirty_bitmap(state->bitmap, state->merge_source, &state->backup,
>> +                            errp);
>> +}
>> +
>>   static void abort_prepare(BlkActionState *common, Error **errp)
>>   {
>>       error_setg(errp, "Transaction aborted using Abort action");
>> @@ -2182,7 +2212,13 @@ static const BlkActionOps actions[] = {
>>           .instance_size = sizeof(BlockDirtyBitmapState),
>>           .prepare = block_dirty_bitmap_disable_prepare,
>>           .abort = block_dirty_bitmap_disable_abort,
>> -     }
>> +    },
>> +    [TRANSACTION_ACTION_KIND_X_BLOCK_DIRTY_BITMAP_MERGE] = {
>> +        .instance_size = sizeof(BlockDirtyBitmapState),
>> +        .prepare = block_dirty_bitmap_merge_prepare,
>> +        .commit = block_dirty_bitmap_free_backup,
>> +        .abort = block_dirty_bitmap_restore,
>> +    }
>>   };
>>   
>>   /**
>>
> If the new state is not necessary and you remove it:
>
> Reviewed-by: John Snow <jsnow@redhat.com>


-- 
Best regards,
Vladimir