[Qemu-devel] [PATCH v2 for-2.13 0/7] Dirty bitmaps fixing and refactoring

[Qemu-devel] [PATCH v2 for-2.13 0/7] Dirty bitmaps fixing and refactoring

[Vladimir Sementsov-Ogievskiy]

v2:
 
05: drop bdrv_undo_clear_dirty_bitmap(), which becomes unused.

Vladimir Sementsov-Ogievskiy (7):
  block/dirty-bitmap: add lock to bdrv_enable/disable_dirty_bitmap
  dirty-bitmaps: fix comment about dirty_bitmap_mutex
  dirty-bitmap: remove missed bdrv_dirty_bitmap_get_autoload header
  dirty-bitmap: separate unused meta-bitmap related functions
  blockdev: refactor block-dirty-bitmap-clear transaction
  block/dirty-bitmap: bdrv_clear_dirty_bitmap: drop unused parameter
  blockdev: unify block-dirty-bitmap-clear command and transaction
    action

 include/block/block_int.h    | 15 ++++++----
 include/block/dirty-bitmap.h | 15 ++++++----
 block/dirty-bitmap.c         | 32 +++++++++-----------
 blockdev.c                   | 69 ++++++++++++++------------------------------
 4 files changed, 54 insertions(+), 77 deletions(-)

-- 
2.11.1

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

[Vladimir Sementsov-Ogievskiy]

Functions write to BdrvDirtyBitmap field, so the should take the lock.

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

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 967159479d..6c00288fd7 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -445,15 +445,19 @@ 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 2/7] dirty-bitmaps: fix comment about dirty_bitmap_mutex

[Vladimir Sementsov-Ogievskiy]

Clarify first two cases and fix Modify -> Any access in third case.
Also, drop 'only' from third case, as it a bit confuses, when thinking
about case where we modify BdrvDirtyBitmap and access HBitmap.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/block_int.h | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index c4dd1d4bb8..189666efa5 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -709,10 +709,14 @@ struct BlockDriverState {
     uint64_t write_threshold_offset;
     NotifierWithReturn write_threshold_notifier;
 
-    /* Writing to the list requires the BQL _and_ the dirty_bitmap_mutex.
-     * Reading from the list can be done with either the BQL or the
-     * dirty_bitmap_mutex.  Modifying a bitmap only requires
-     * dirty_bitmap_mutex.  */
+    /* Writing to the list (i.e. to any field of BdrvDirtyBitmap or to the
+     * list-head) requires both the BQL _and_ the dirty_bitmap_mutex.
+     *
+     * Reading from the list (from any field of BdrvDirtyBitmap or from the
+     * list-head) can be done with either the BQL or the dirty_bitmap_mutex.
+     *
+     * Any access to underlying HBitmap requires dirty_bitmap_mutex.
+     */
     QemuMutex dirty_bitmap_mutex;
     QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps;
 
-- 
2.11.1

[Qemu-devel] [PATCH v2 3/7] dirty-bitmap: remove missed bdrv_dirty_bitmap_get_autoload header

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/dirty-bitmap.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 1ff8949b1b..c7e910016d 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -88,7 +88,6 @@ int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes);
 bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
 bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
-bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_qmp_locked(BdrvDirtyBitmap *bitmap);
 bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
-- 
2.11.1

[Qemu-devel] [PATCH v2 4/7] dirty-bitmap: separate unused meta-bitmap related functions

[Vladimir Sementsov-Ogievskiy]

Separate them in the header and clarify needed locking in comments.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/dirty-bitmap.h | 14 +++++++++-----
 block/dirty-bitmap.c         |  5 +++++
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index c7e910016d..b7ccfd1363 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -9,9 +9,6 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
                                           uint32_t granularity,
                                           const char *name,
                                           Error **errp);
-void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,
-                                   int chunk_size);
-void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
                                        BdrvDirtyBitmap *bitmap,
                                        Error **errp);
@@ -45,7 +42,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
                            int64_t offset, int64_t bytes);
 void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
                              int64_t offset, int64_t bytes);
-BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap);
 BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter);
 
@@ -84,7 +80,6 @@ void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
 int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter);
 void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t offset);
 int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
-int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes);
 bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
 bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
@@ -99,4 +94,13 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
                                                   BdrvDirtyBitmap *bitmap,
                                                   Error **errp);
 
+/*
+ * Unused for now meta-bitmaps related functions
+ */
+
+void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap, int chunk_size);
+void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap);
+BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap);
+int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);
+
 #endif
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 6c00288fd7..1812e17549 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -149,6 +149,8 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
  * @bitmap: the block dirty bitmap for which to create a meta dirty bitmap.
  * @chunk_size: how many bytes of bitmap data does each bit in the meta bitmap
  * track.
+ *
+ * Called with BQL taken.
  */
 void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,
                                    int chunk_size)
@@ -160,6 +162,7 @@ void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,
     qemu_mutex_unlock(bitmap->mutex);
 }
 
+/* Called with BQL taken. */
 void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap)
 {
     assert(bitmap->meta);
@@ -529,6 +532,7 @@ BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap)
     return iter;
 }
 
+/* Called with BQL and dirty_bitmap_mutex locked. */
 BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap)
 {
     BdrvDirtyBitmapIter *iter = g_new(BdrvDirtyBitmapIter, 1);
@@ -688,6 +692,7 @@ int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
     return hbitmap_count(bitmap->bitmap);
 }
 
+/* Called with BQL or dirty_bitmap_mutex locked */
 int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
 {
     return hbitmap_count(bitmap->meta);
-- 
2.11.1

[Qemu-devel] [PATCH v2 5/7] blockdev: refactor block-dirty-bitmap-clear transaction

[Vladimir Sementsov-Ogievskiy]

bdrv_clear_dirty_bitmap do not fail, so we can call it in transaction
commit, avoiding any rollback.

After this, bdrv_undo_clear_dirty_bitmap() becomes unused, so, drop it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/block_int.h |  1 -
 block/dirty-bitmap.c      |  9 ---------
 blockdev.c                | 16 +---------------
 3 files changed, 1 insertion(+), 25 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 189666efa5..22059c8119 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1087,7 +1087,6 @@ 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_inc_in_flight(BlockDriverState *bs);
 void bdrv_dec_in_flight(BlockDriverState *bs);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 1812e17549..3c69a8eed9 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -607,15 +607,6 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
     bdrv_dirty_bitmap_unlock(bitmap);
 }
 
-void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in)
-{
-    HBitmap *tmp = bitmap->bitmap;
-    assert(bdrv_dirty_bitmap_enabled(bitmap));
-    assert(!bdrv_dirty_bitmap_readonly(bitmap));
-    bitmap->bitmap = in;
-    hbitmap_free(tmp);
-}
-
 uint64_t bdrv_dirty_bitmap_serialization_size(const BdrvDirtyBitmap *bitmap,
                                               uint64_t offset, uint64_t bytes)
 {
diff --git a/blockdev.c b/blockdev.c
index c31bf3d98d..88eae60c1c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2050,7 +2050,6 @@ typedef struct BlockDirtyBitmapState {
     BlkActionState common;
     BdrvDirtyBitmap *bitmap;
     BlockDriverState *bs;
-    HBitmap *backup;
     bool prepared;
 } BlockDirtyBitmapState;
 
@@ -2129,18 +2128,6 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common,
         error_setg(errp, "Cannot clear a readonly bitmap");
         return;
     }
-
-    bdrv_clear_dirty_bitmap(state->bitmap, &state->backup);
-}
-
-static void block_dirty_bitmap_clear_abort(BlkActionState *common)
-{
-    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
-                                             common, common);
-
-    if (state->backup) {
-        bdrv_undo_clear_dirty_bitmap(state->bitmap, state->backup);
-    }
 }
 
 static void block_dirty_bitmap_clear_commit(BlkActionState *common)
@@ -2148,7 +2135,7 @@ static void block_dirty_bitmap_clear_commit(BlkActionState *common)
     BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
                                              common, common);
 
-    hbitmap_free(state->backup);
+    bdrv_clear_dirty_bitmap(state->bitmap, NULL);
 }
 
 static void abort_prepare(BlkActionState *common, Error **errp)
@@ -2210,7 +2197,6 @@ static const BlkActionOps actions[] = {
         .instance_size = sizeof(BlockDirtyBitmapState),
         .prepare = block_dirty_bitmap_clear_prepare,
         .commit = block_dirty_bitmap_clear_commit,
-        .abort = block_dirty_bitmap_clear_abort,
     }
 };
 
-- 
2.11.1

[Qemu-devel] [PATCH v2 6/7] block/dirty-bitmap: bdrv_clear_dirty_bitmap: drop unused parameter

[Vladimir Sementsov-Ogievskiy]

Drop parameter "HBitmap **out" which is unused now, all callers set
it to NULL.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/block_int.h |  2 +-
 block/dirty-bitmap.c      | 14 +++++---------
 blockdev.c                |  4 ++--
 3 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 22059c8119..61fac62c06 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1086,7 +1086,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_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 
 void bdrv_inc_in_flight(BlockDriverState *bs);
 void bdrv_dec_in_flight(BlockDriverState *bs);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 3c69a8eed9..35600922f5 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -591,19 +591,15 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
     bdrv_dirty_bitmap_unlock(bitmap);
 }
 
-void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
+void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap)
 {
     assert(bdrv_dirty_bitmap_enabled(bitmap));
     assert(!bdrv_dirty_bitmap_readonly(bitmap));
+
     bdrv_dirty_bitmap_lock(bitmap);
-    if (!out) {
-        hbitmap_reset_all(bitmap->bitmap);
-    } else {
-        HBitmap *backup = bitmap->bitmap;
-        bitmap->bitmap = hbitmap_alloc(bitmap->size,
-                                       hbitmap_granularity(backup));
-        *out = backup;
-    }
+
+    hbitmap_reset_all(bitmap->bitmap);
+
     bdrv_dirty_bitmap_unlock(bitmap);
 }
 
diff --git a/blockdev.c b/blockdev.c
index 88eae60c1c..54e1ba8f44 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2135,7 +2135,7 @@ static void block_dirty_bitmap_clear_commit(BlkActionState *common)
     BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
                                              common, common);
 
-    bdrv_clear_dirty_bitmap(state->bitmap, NULL);
+    bdrv_clear_dirty_bitmap(state->bitmap);
 }
 
 static void abort_prepare(BlkActionState *common, Error **errp)
@@ -2906,7 +2906,7 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
         return;
     }
 
-    bdrv_clear_dirty_bitmap(bitmap, NULL);
+    bdrv_clear_dirty_bitmap(bitmap);
 }
 
 BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
-- 
2.11.1

[Qemu-devel] [PATCH v2 7/7] blockdev: unify block-dirty-bitmap-clear command and transaction action

[Vladimir Sementsov-Ogievskiy]

 - use error messages from qmp command, as they are more descriptive
 - we need not check bs, as block_dirty_bitmap_lookup never returns
   bs = NULL on success (and if user set bs to be not NULL pointer),
   so, let's just assert it.

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

diff --git a/blockdev.c b/blockdev.c
index 54e1ba8f44..3f0979298e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2114,18 +2114,26 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common,
     if (!state->bitmap) {
         return;
     }
+    assert(state->bs != NULL);
 
     if (bdrv_dirty_bitmap_frozen(state->bitmap)) {
-        error_setg(errp, "Cannot modify a frozen bitmap");
+        error_setg(errp,
+                   "Bitmap '%s' is currently frozen and cannot be modified",
+                   action->name);
         return;
     } else if (bdrv_dirty_bitmap_qmp_locked(state->bitmap)) {
-        error_setg(errp, "Cannot modify a locked bitmap");
+        error_setg(errp,
+                   "Bitmap '%s' is currently locked and cannot be modified",
+                   action->name);
         return;
     } else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) {
-        error_setg(errp, "Cannot clear a disabled bitmap");
+        error_setg(errp,
+                   "Bitmap '%s' is currently disabled and cannot be cleared",
+                   action->name);
         return;
     } else if (bdrv_dirty_bitmap_readonly(state->bitmap)) {
-        error_setg(errp, "Cannot clear a readonly bitmap");
+        error_setg(errp, "Bitmap '%s' is readonly and cannot be cleared",
+                   action->name);
         return;
     }
 }
@@ -2878,35 +2886,16 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
 void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
                                   Error **errp)
 {
-    BdrvDirtyBitmap *bitmap;
-    BlockDriverState *bs;
-
-    bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
-    if (!bitmap || !bs) {
-        return;
-    }
-
-    if (bdrv_dirty_bitmap_frozen(bitmap)) {
-        error_setg(errp,
-                   "Bitmap '%s' is currently frozen and cannot be modified",
-                   name);
-        return;
-    } else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) {
-        error_setg(errp,
-                   "Bitmap '%s' is currently locked and cannot be modified",
-                   name);
-        return;
-    } else if (!bdrv_dirty_bitmap_enabled(bitmap)) {
-        error_setg(errp,
-                   "Bitmap '%s' is currently disabled and cannot be cleared",
-                   name);
-        return;
-    } else if (bdrv_dirty_bitmap_readonly(bitmap)) {
-        error_setg(errp, "Bitmap '%s' is readonly and cannot be cleared", name);
-        return;
-    }
+    BlockDirtyBitmap data = {
+        .node = (char *)node,
+        .name = (char *)name
+    };
+    TransactionAction action = {
+        .type = TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_CLEAR,
+        .u.block_dirty_bitmap_clear.data = &data,
+    };
 
-    bdrv_clear_dirty_bitmap(bitmap);
+    blockdev_do_action(&action, errp);
 }
 
 BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
-- 
2.11.1

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

[John Snow]

On 04/16/2018 07:44 AM, Vladimir Sementsov-Ogievskiy wrote:
> Functions write to BdrvDirtyBitmap field, so the should take the lock.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Looks like this won't introduce any recursive locking that I can spot,
so this looks correct.

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

Re: [Qemu-devel] [PATCH v2 2/7] dirty-bitmaps: fix comment about dirty_bitmap_mutex

[John Snow]

On 04/16/2018 07:44 AM, Vladimir Sementsov-Ogievskiy wrote:
> Clarify first two cases and fix Modify -> Any access in third case.
> Also, drop 'only' from third case, as it a bit confuses, when thinking
> about case where we modify BdrvDirtyBitmap and access HBitmap.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/block_int.h | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index c4dd1d4bb8..189666efa5 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -709,10 +709,14 @@ struct BlockDriverState {
>      uint64_t write_threshold_offset;
>      NotifierWithReturn write_threshold_notifier;
>  
> -    /* Writing to the list requires the BQL _and_ the dirty_bitmap_mutex.
> -     * Reading from the list can be done with either the BQL or the
> -     * dirty_bitmap_mutex.  Modifying a bitmap only requires
> -     * dirty_bitmap_mutex.  */
> +    /* Writing to the list (i.e. to any field of BdrvDirtyBitmap or to the
> +     * list-head) requires both the BQL _and_ the dirty_bitmap_mutex.
> +     *
> +     * Reading from the list (from any field of BdrvDirtyBitmap or from the
> +     * list-head) can be done with either the BQL or the dirty_bitmap_mutex.
> +     *
> +     * Any access to underlying HBitmap requires dirty_bitmap_mutex.

"to the underlying HBitmap," probably.

> +     */
>      QemuMutex dirty_bitmap_mutex;
>      QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps;
>  
> 

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

Re: [Qemu-devel] [PATCH v2 3/7] dirty-bitmap: remove missed bdrv_dirty_bitmap_get_autoload header

[John Snow]

On 04/16/2018 07:44 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/dirty-bitmap.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 1ff8949b1b..c7e910016d 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -88,7 +88,6 @@ int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);
>  void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes);
>  bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
>  bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
> -bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
>  bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
>  bool bdrv_dirty_bitmap_qmp_locked(BdrvDirtyBitmap *bitmap);
>  bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
> 

Assuming you checked for others as well.

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

Re: [Qemu-devel] [PATCH v2 4/7] dirty-bitmap: separate unused meta-bitmap related functions

[John Snow]

On 04/16/2018 07:44 AM, Vladimir Sementsov-Ogievskiy wrote:
> Separate them in the header and clarify needed locking in comments.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/dirty-bitmap.h | 14 +++++++++-----
>  block/dirty-bitmap.c         |  5 +++++
>  2 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index c7e910016d..b7ccfd1363 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -9,9 +9,6 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>                                            uint32_t granularity,
>                                            const char *name,
>                                            Error **errp);
> -void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> -                                   int chunk_size);
> -void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap);
>  int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
>                                         BdrvDirtyBitmap *bitmap,
>                                         Error **errp);
> @@ -45,7 +42,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>                             int64_t offset, int64_t bytes);
>  void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>                               int64_t offset, int64_t bytes);
> -BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap);
>  BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap);
>  void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter);
>  
> @@ -84,7 +80,6 @@ void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
>  int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter);
>  void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t offset);
>  int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
> -int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);
>  void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes);
>  bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
>  bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
> @@ -99,4 +94,13 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
>                                                    BdrvDirtyBitmap *bitmap,
>                                                    Error **errp);
>  
> +/*
> + * Unused for now meta-bitmaps related functions
> + */
> +

I assume you have plans to use them still? I thought these were useful
for storage migration -- but I guess we don't use these for the postcopy
mechanism?

> +void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap, int chunk_size);
> +void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap);
> +BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap);
> +int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);
> +
>  #endif
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 6c00288fd7..1812e17549 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -149,6 +149,8 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>   * @bitmap: the block dirty bitmap for which to create a meta dirty bitmap.
>   * @chunk_size: how many bytes of bitmap data does each bit in the meta bitmap
>   * track.
> + *
> + * Called with BQL taken.
>   */
>  void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>                                     int chunk_size)
> @@ -160,6 +162,7 @@ void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>      qemu_mutex_unlock(bitmap->mutex);
>  }
>  
> +/* Called with BQL taken. */
>  void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>  {
>      assert(bitmap->meta);
> @@ -529,6 +532,7 @@ BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap)
>      return iter;
>  }
>  
> +/* Called with BQL and dirty_bitmap_mutex locked. */
>  BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap)
>  {
>      BdrvDirtyBitmapIter *iter = g_new(BdrvDirtyBitmapIter, 1);
> @@ -688,6 +692,7 @@ int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
>      return hbitmap_count(bitmap->bitmap);
>  }
>  
> +/* Called with BQL or dirty_bitmap_mutex locked */
>  int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
>  {
>      return hbitmap_count(bitmap->meta);
> 

Re: [Qemu-devel] [PATCH v2 5/7] blockdev: refactor block-dirty-bitmap-clear transaction

[John Snow]

On 04/16/2018 07:44 AM, Vladimir Sementsov-Ogievskiy wrote:
> bdrv_clear_dirty_bitmap do not fail, so we can call it in transaction
> commit, avoiding any rollback.
> 
> After this, bdrv_undo_clear_dirty_bitmap() becomes unused, so, drop it.
> 

I'm trying to remember why we ever bothered doing it the other way. Is
it because of timings with respect to other operations and we wanted the
"clear" to take effect sooner rather than later to co-operate with other
commands?

(You and Nikolay hinted about similar problems in the Libvirt PULL api
thread, too. It continues to be an issue.)

Hopping in my time machine:

https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg01229.html

(Oh, I wrote it...)

Looks like it was a conscious decision to avoid ordering conflicts with
other block jobs.

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/block_int.h |  1 -
>  block/dirty-bitmap.c      |  9 ---------
>  blockdev.c                | 16 +---------------
>  3 files changed, 1 insertion(+), 25 deletions(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 189666efa5..22059c8119 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -1087,7 +1087,6 @@ 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_inc_in_flight(BlockDriverState *bs);
>  void bdrv_dec_in_flight(BlockDriverState *bs);
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 1812e17549..3c69a8eed9 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -607,15 +607,6 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
>      bdrv_dirty_bitmap_unlock(bitmap);
>  }
>  
> -void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in)
> -{
> -    HBitmap *tmp = bitmap->bitmap;
> -    assert(bdrv_dirty_bitmap_enabled(bitmap));
> -    assert(!bdrv_dirty_bitmap_readonly(bitmap));
> -    bitmap->bitmap = in;
> -    hbitmap_free(tmp);
> -}
> -
>  uint64_t bdrv_dirty_bitmap_serialization_size(const BdrvDirtyBitmap *bitmap,
>                                                uint64_t offset, uint64_t bytes)
>  {
> diff --git a/blockdev.c b/blockdev.c
> index c31bf3d98d..88eae60c1c 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2050,7 +2050,6 @@ typedef struct BlockDirtyBitmapState {
>      BlkActionState common;
>      BdrvDirtyBitmap *bitmap;
>      BlockDriverState *bs;
> -    HBitmap *backup;
>      bool prepared;
>  } BlockDirtyBitmapState;
>  
> @@ -2129,18 +2128,6 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common,
>          error_setg(errp, "Cannot clear a readonly bitmap");
>          return;
>      }
> -
> -    bdrv_clear_dirty_bitmap(state->bitmap, &state->backup);
> -}
> -
> -static void block_dirty_bitmap_clear_abort(BlkActionState *common)
> -{
> -    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> -                                             common, common);
> -
> -    if (state->backup) {
> -        bdrv_undo_clear_dirty_bitmap(state->bitmap, state->backup);
> -    }
>  }
>  
>  static void block_dirty_bitmap_clear_commit(BlkActionState *common)
> @@ -2148,7 +2135,7 @@ static void block_dirty_bitmap_clear_commit(BlkActionState *common)
>      BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>                                               common, common);
>  
> -    hbitmap_free(state->backup);
> +    bdrv_clear_dirty_bitmap(state->bitmap, NULL);
>  }
>  
>  static void abort_prepare(BlkActionState *common, Error **errp)
> @@ -2210,7 +2197,6 @@ static const BlkActionOps actions[] = {
>          .instance_size = sizeof(BlockDirtyBitmapState),
>          .prepare = block_dirty_bitmap_clear_prepare,
>          .commit = block_dirty_bitmap_clear_commit,
> -        .abort = block_dirty_bitmap_clear_abort,
>      }
>  };
>  
> 

Re: [Qemu-devel] [PATCH v2 6/7] block/dirty-bitmap: bdrv_clear_dirty_bitmap: drop unused parameter

[John Snow]

On 04/16/2018 07:44 AM, Vladimir Sementsov-Ogievskiy wrote:
> Drop parameter "HBitmap **out" which is unused now, all callers set
> it to NULL.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

And now this patch is in question too.

Re: [Qemu-devel] [PATCH v2 5/7] blockdev: refactor block-dirty-bitmap-clear transaction

[John Snow]

On 04/16/2018 07:44 AM, Vladimir Sementsov-Ogievskiy wrote:
> bdrv_clear_dirty_bitmap do not fail, so we can call it in transaction
> commit, avoiding any rollback.
> 
> After this, bdrv_undo_clear_dirty_bitmap() becomes unused, so, drop it.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---

Sorry if it was my idea entirely that led you on this wild goose chase,
I had forgotten :(

Re: [Qemu-devel] [PATCH v2 4/7] dirty-bitmap: separate unused meta-bitmap related functions

[Vladimir Sementsov-Ogievskiy]

20.04.2018 00:54, John Snow wrote:
> On 04/16/2018 07:44 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Separate them in the header and clarify needed locking in comments.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   include/block/dirty-bitmap.h | 14 +++++++++-----
>>   block/dirty-bitmap.c         |  5 +++++
>>   2 files changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>> index c7e910016d..b7ccfd1363 100644
>> --- a/include/block/dirty-bitmap.h
>> +++ b/include/block/dirty-bitmap.h
>> @@ -9,9 +9,6 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>>                                             uint32_t granularity,
>>                                             const char *name,
>>                                             Error **errp);
>> -void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>> -                                   int chunk_size);
>> -void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap);
>>   int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
>>                                          BdrvDirtyBitmap *bitmap,
>>                                          Error **errp);
>> @@ -45,7 +42,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>>                              int64_t offset, int64_t bytes);
>>   void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>>                                int64_t offset, int64_t bytes);
>> -BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap);
>>   BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap);
>>   void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter);
>>   
>> @@ -84,7 +80,6 @@ void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
>>   int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter);
>>   void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t offset);
>>   int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
>> -int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);
>>   void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes);
>>   bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
>>   bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
>> @@ -99,4 +94,13 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
>>                                                     BdrvDirtyBitmap *bitmap,
>>                                                     Error **errp);
>>   
>> +/*
>> + * Unused for now meta-bitmaps related functions
>> + */
>> +
> I assume you have plans to use them still? I thought these were useful
> for storage migration -- but I guess we don't use these for the postcopy
> mechanism?

I don't have plans for near future, so for me it's ok to drop them.

Theoretical usage is partial storing (if we know, which portions of 
bitmaps are changed, we can save to file only these portions, not the 
whole bitmaps), but it don't look like near future. May be, Fam has some 
plans?

>
>> +void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap, int chunk_size);
>> +void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap);
>> +BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap);
>> +int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);
>> +
>>   #endif
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 6c00288fd7..1812e17549 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -149,6 +149,8 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>>    * @bitmap: the block dirty bitmap for which to create a meta dirty bitmap.
>>    * @chunk_size: how many bytes of bitmap data does each bit in the meta bitmap
>>    * track.
>> + *
>> + * Called with BQL taken.
>>    */
>>   void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>>                                      int chunk_size)
>> @@ -160,6 +162,7 @@ void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>>       qemu_mutex_unlock(bitmap->mutex);
>>   }
>>   
>> +/* Called with BQL taken. */
>>   void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>>   {
>>       assert(bitmap->meta);
>> @@ -529,6 +532,7 @@ BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap)
>>       return iter;
>>   }
>>   
>> +/* Called with BQL and dirty_bitmap_mutex locked. */
>>   BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap)
>>   {
>>       BdrvDirtyBitmapIter *iter = g_new(BdrvDirtyBitmapIter, 1);
>> @@ -688,6 +692,7 @@ int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
>>       return hbitmap_count(bitmap->bitmap);
>>   }
>>   
>> +/* Called with BQL or dirty_bitmap_mutex locked */
>>   int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
>>   {
>>       return hbitmap_count(bitmap->meta);
>>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 5/7] blockdev: refactor block-dirty-bitmap-clear transaction

[Vladimir Sementsov-Ogievskiy]

20.04.2018 01:47, John Snow wrote:
>
> On 04/16/2018 07:44 AM, Vladimir Sementsov-Ogievskiy wrote:
>> bdrv_clear_dirty_bitmap do not fail, so we can call it in transaction
>> commit, avoiding any rollback.
>>
>> After this, bdrv_undo_clear_dirty_bitmap() becomes unused, so, drop it.
>>
> I'm trying to remember why we ever bothered doing it the other way. Is
> it because of timings with respect to other operations and we wanted the
> "clear" to take effect sooner rather than later to co-operate with other
> commands?
>
> (You and Nikolay hinted about similar problems in the Libvirt PULL api
> thread, too. It continues to be an issue.)
>
> Hopping in my time machine:
>
> https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg01229.html
>
> (Oh, I wrote it...)
>
> Looks like it was a conscious decision to avoid ordering conflicts with
> other block jobs.

ohh, interesting.. so, transactions are tricky.

Don't you think, that this letter shows, that backup transaction is wrong,
rather than clear?

I think, when we write a transaction, we should understand, that full 
effect of the
operation will be achieved only after corresponding .commit. So, backup 
should not do in
.prepare something dependent on other operations.

>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   include/block/block_int.h |  1 -
>>   block/dirty-bitmap.c      |  9 ---------
>>   blockdev.c                | 16 +---------------
>>   3 files changed, 1 insertion(+), 25 deletions(-)
>>
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index 189666efa5..22059c8119 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -1087,7 +1087,6 @@ 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_inc_in_flight(BlockDriverState *bs);
>>   void bdrv_dec_in_flight(BlockDriverState *bs);
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 1812e17549..3c69a8eed9 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -607,15 +607,6 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
>>       bdrv_dirty_bitmap_unlock(bitmap);
>>   }
>>   
>> -void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in)
>> -{
>> -    HBitmap *tmp = bitmap->bitmap;
>> -    assert(bdrv_dirty_bitmap_enabled(bitmap));
>> -    assert(!bdrv_dirty_bitmap_readonly(bitmap));
>> -    bitmap->bitmap = in;
>> -    hbitmap_free(tmp);
>> -}
>> -
>>   uint64_t bdrv_dirty_bitmap_serialization_size(const BdrvDirtyBitmap *bitmap,
>>                                                 uint64_t offset, uint64_t bytes)
>>   {
>> diff --git a/blockdev.c b/blockdev.c
>> index c31bf3d98d..88eae60c1c 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -2050,7 +2050,6 @@ typedef struct BlockDirtyBitmapState {
>>       BlkActionState common;
>>       BdrvDirtyBitmap *bitmap;
>>       BlockDriverState *bs;
>> -    HBitmap *backup;
>>       bool prepared;
>>   } BlockDirtyBitmapState;
>>   
>> @@ -2129,18 +2128,6 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common,
>>           error_setg(errp, "Cannot clear a readonly bitmap");
>>           return;
>>       }
>> -
>> -    bdrv_clear_dirty_bitmap(state->bitmap, &state->backup);
>> -}
>> -
>> -static void block_dirty_bitmap_clear_abort(BlkActionState *common)
>> -{
>> -    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>> -                                             common, common);
>> -
>> -    if (state->backup) {
>> -        bdrv_undo_clear_dirty_bitmap(state->bitmap, state->backup);
>> -    }
>>   }
>>   
>>   static void block_dirty_bitmap_clear_commit(BlkActionState *common)
>> @@ -2148,7 +2135,7 @@ static void block_dirty_bitmap_clear_commit(BlkActionState *common)
>>       BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>>                                                common, common);
>>   
>> -    hbitmap_free(state->backup);
>> +    bdrv_clear_dirty_bitmap(state->bitmap, NULL);
>>   }
>>   
>>   static void abort_prepare(BlkActionState *common, Error **errp)
>> @@ -2210,7 +2197,6 @@ static const BlkActionOps actions[] = {
>>           .instance_size = sizeof(BlockDirtyBitmapState),
>>           .prepare = block_dirty_bitmap_clear_prepare,
>>           .commit = block_dirty_bitmap_clear_commit,
>> -        .abort = block_dirty_bitmap_clear_abort,
>>       }
>>   };
>>   
>>


-- 
Best regards,
Vladimir