[PATCH 0/4] block/dirty-bitmaps: fix and improve bitmap merge

[PATCH 0/4] block/dirty-bitmaps: fix and improve bitmap merge

[Vladimir Sementsov-Ogievskiy]

Hi all!

Here are some good refactoring and fix of bitmap merge paths.

Vladimir Sementsov-Ogievskiy (4):
  block: bdrv_merge_dirty_bitmap: add return value
  block: block_dirty_bitmap_merge(): fix error path
  block: improve block_dirty_bitmap_merge(): don't allocate extra bitmap
  block: simplify handling of try to merge different sized bitmaps

 include/block/block_int.h       |  2 +-
 include/block/dirty-bitmap.h    |  2 +-
 include/qemu/hbitmap.h          | 15 ++-------------
 block/backup.c                  |  6 ++----
 block/dirty-bitmap.c            | 31 ++++++++++++++-----------------
 block/monitor/bitmap-qmp-cmds.c | 31 +++++++++----------------------
 util/hbitmap.c                  | 25 +++++++------------------
 7 files changed, 36 insertions(+), 76 deletions(-)

-- 
2.31.1

[PATCH 1/4] block: bdrv_merge_dirty_bitmap: add return value

[Vladimir Sementsov-Ogievskiy]

Add return value to bdrv_merge_dirty_bitmap() and use it to reduce
error propagation.

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

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 40950ae3d5..f95d350b70 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -77,7 +77,7 @@ void bdrv_dirty_bitmap_set_persistence(BdrvDirtyBitmap *bitmap,
                                        bool persistent);
 void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_bitmap_set_busy(BdrvDirtyBitmap *bitmap, bool busy);
-void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
+bool bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
                              HBitmap **backup, Error **errp);
 void bdrv_dirty_bitmap_skip_store(BdrvDirtyBitmap *bitmap, bool skip);
 bool bdrv_dirty_bitmap_get(BdrvDirtyBitmap *bitmap, int64_t offset);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 0ef46163e3..d16b96ee62 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -881,10 +881,10 @@ bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap,
  *
  * @backup: If provided, make a copy of dest here prior to merge.
  */
-void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
+bool bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
                              HBitmap **backup, Error **errp)
 {
-    bool ret;
+    bool ret = false;
 
     bdrv_dirty_bitmaps_lock(dest->bs);
     if (src->bs != dest->bs) {
@@ -912,6 +912,8 @@ out:
     if (src->bs != dest->bs) {
         bdrv_dirty_bitmaps_unlock(src->bs);
     }
+
+    return ret;
 }
 
 /**
diff --git a/block/monitor/bitmap-qmp-cmds.c b/block/monitor/bitmap-qmp-cmds.c
index 9f11deec64..83970b22fa 100644
--- a/block/monitor/bitmap-qmp-cmds.c
+++ b/block/monitor/bitmap-qmp-cmds.c
@@ -259,7 +259,6 @@ BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
     BlockDriverState *bs;
     BdrvDirtyBitmap *dst, *src, *anon;
     BlockDirtyBitmapMergeSourceList *lst;
-    Error *local_err = NULL;
 
     dst = block_dirty_bitmap_lookup(node, target, &bs, errp);
     if (!dst) {
@@ -297,9 +296,7 @@ BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
             abort();
         }
 
-        bdrv_merge_dirty_bitmap(anon, src, NULL, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
+        if (!bdrv_merge_dirty_bitmap(anon, src, NULL, errp)) {
             dst = NULL;
             goto out;
         }
-- 
2.31.1

Re: [PATCH 1/4] block: bdrv_merge_dirty_bitmap: add return value

[Vladimir Sementsov-Ogievskiy]

I forget that I already sent it in other series: [PATCH v3 02/19] block/dirty-bitmap: bdrv_merge_dirty_bitmap(): add return value

"[PATCH v3 02/19] block/dirty-bitmap: bdrv_merge_dirty_bitmap(): add return value" is a bit better as it adds a comment. And has Hanna's r-b

15.02.2022 20:53, Vladimir Sementsov-Ogievskiy wrote:
> Add return value to bdrv_merge_dirty_bitmap() and use it to reduce
> error propagation.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   include/block/dirty-bitmap.h    | 2 +-
>   block/dirty-bitmap.c            | 6 ++++--
>   block/monitor/bitmap-qmp-cmds.c | 5 +----
>   3 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 40950ae3d5..f95d350b70 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -77,7 +77,7 @@ void bdrv_dirty_bitmap_set_persistence(BdrvDirtyBitmap *bitmap,
>                                          bool persistent);
>   void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap);
>   void bdrv_dirty_bitmap_set_busy(BdrvDirtyBitmap *bitmap, bool busy);
> -void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
> +bool bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>                                HBitmap **backup, Error **errp);
>   void bdrv_dirty_bitmap_skip_store(BdrvDirtyBitmap *bitmap, bool skip);
>   bool bdrv_dirty_bitmap_get(BdrvDirtyBitmap *bitmap, int64_t offset);
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 0ef46163e3..d16b96ee62 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -881,10 +881,10 @@ bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap,
>    *
>    * @backup: If provided, make a copy of dest here prior to merge.
>    */
> -void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
> +bool bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>                                HBitmap **backup, Error **errp)
>   {
> -    bool ret;
> +    bool ret = false;
>   
>       bdrv_dirty_bitmaps_lock(dest->bs);
>       if (src->bs != dest->bs) {
> @@ -912,6 +912,8 @@ out:
>       if (src->bs != dest->bs) {
>           bdrv_dirty_bitmaps_unlock(src->bs);
>       }
> +
> +    return ret;
>   }
>   
>   /**
> diff --git a/block/monitor/bitmap-qmp-cmds.c b/block/monitor/bitmap-qmp-cmds.c
> index 9f11deec64..83970b22fa 100644
> --- a/block/monitor/bitmap-qmp-cmds.c
> +++ b/block/monitor/bitmap-qmp-cmds.c
> @@ -259,7 +259,6 @@ BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
>       BlockDriverState *bs;
>       BdrvDirtyBitmap *dst, *src, *anon;
>       BlockDirtyBitmapMergeSourceList *lst;
> -    Error *local_err = NULL;
>   
>       dst = block_dirty_bitmap_lookup(node, target, &bs, errp);
>       if (!dst) {
> @@ -297,9 +296,7 @@ BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
>               abort();
>           }
>   
> -        bdrv_merge_dirty_bitmap(anon, src, NULL, &local_err);
> -        if (local_err) {
> -            error_propagate(errp, local_err);
> +        if (!bdrv_merge_dirty_bitmap(anon, src, NULL, errp)) {
>               dst = NULL;
>               goto out;
>           }
> 


-- 
Best regards,
Vladimir

[PATCH 2/4] block: block_dirty_bitmap_merge(): fix error path

[Vladimir Sementsov-Ogievskiy]

At the end we ignore failure of bdrv_merge_dirty_bitmap() and report
success. And still set errp. That's wrong.

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

diff --git a/block/monitor/bitmap-qmp-cmds.c b/block/monitor/bitmap-qmp-cmds.c
index 83970b22fa..a94aaa9fb3 100644
--- a/block/monitor/bitmap-qmp-cmds.c
+++ b/block/monitor/bitmap-qmp-cmds.c
@@ -303,7 +303,10 @@ BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
     }
 
     /* Merge into dst; dst is unchanged on failure. */
-    bdrv_merge_dirty_bitmap(dst, anon, backup, errp);
+    if (!bdrv_merge_dirty_bitmap(dst, anon, backup, errp)) {
+        dst = NULL;
+        goto out;
+    }
 
  out:
     bdrv_release_dirty_bitmap(anon);
-- 
2.31.1

Re: [PATCH 2/4] block: block_dirty_bitmap_merge(): fix error path

[Nikta Lapshin]

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


On 2/15/22 20:53, Vladimir Sementsov-Ogievskiy wrote:

Reviewed-by: Nikita Lapshin<nikita.lapshin@virtuozzo.com>

> At the end we ignore failure of bdrv_merge_dirty_bitmap() and report
> success. And still set errp. That's wrong.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
> ---
>   block/monitor/bitmap-qmp-cmds.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/block/monitor/bitmap-qmp-cmds.c b/block/monitor/bitmap-qmp-cmds.c
> index 83970b22fa..a94aaa9fb3 100644
> --- a/block/monitor/bitmap-qmp-cmds.c
> +++ b/block/monitor/bitmap-qmp-cmds.c
> @@ -303,7 +303,10 @@ BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
>       }
>   
>       /* Merge into dst; dst is unchanged on failure. */
> -    bdrv_merge_dirty_bitmap(dst, anon, backup, errp);
> +    if (!bdrv_merge_dirty_bitmap(dst, anon, backup, errp)) {
> +        dst = NULL;
> +        goto out;
> +    }
>   
>    out:
>       bdrv_release_dirty_bitmap(anon);

[-- Attachment #2: Type: text/html, Size: 1472 bytes --]

[PATCH 3/4] block: improve block_dirty_bitmap_merge(): don't allocate extra bitmap

[Vladimir Sementsov-Ogievskiy]

We don't need extra bitmap. All we need is to backup the original
bitmap when we do first merge. So, drop extra temporary bitmap and work
directly with target and backup.

Note that block_dirty_bitmap_merge() semantics changed: on failure
target may be modified now, and caller should call
block_dirty_bitmap_restore() if needed. The only caller is
qmp_transaction() and ->abort() is called for failed action. So, we are
OK.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/monitor/bitmap-qmp-cmds.c | 31 +++++++++----------------------
 1 file changed, 9 insertions(+), 22 deletions(-)

diff --git a/block/monitor/bitmap-qmp-cmds.c b/block/monitor/bitmap-qmp-cmds.c
index a94aaa9fb3..2ce1b4e455 100644
--- a/block/monitor/bitmap-qmp-cmds.c
+++ b/block/monitor/bitmap-qmp-cmds.c
@@ -252,12 +252,15 @@ void qmp_block_dirty_bitmap_disable(const char *node, const char *name,
     bdrv_disable_dirty_bitmap(bitmap);
 }
 
+/*
+ * On failure target may be modified. In this case @backup is set.
+ */
 BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
                                           BlockDirtyBitmapMergeSourceList *bms,
                                           HBitmap **backup, Error **errp)
 {
     BlockDriverState *bs;
-    BdrvDirtyBitmap *dst, *src, *anon;
+    BdrvDirtyBitmap *dst, *src;
     BlockDirtyBitmapMergeSourceList *lst;
 
     dst = block_dirty_bitmap_lookup(node, target, &bs, errp);
@@ -265,12 +268,6 @@ BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
         return NULL;
     }
 
-    anon = bdrv_create_dirty_bitmap(bs, bdrv_dirty_bitmap_granularity(dst),
-                                    NULL, errp);
-    if (!anon) {
-        return NULL;
-    }
-
     for (lst = bms; lst; lst = lst->next) {
         switch (lst->value->type) {
             const char *name, *node;
@@ -279,8 +276,7 @@ BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
             src = bdrv_find_dirty_bitmap(bs, name);
             if (!src) {
                 error_setg(errp, "Dirty bitmap '%s' not found", name);
-                dst = NULL;
-                goto out;
+                return NULL;
             }
             break;
         case QTYPE_QDICT:
@@ -288,28 +284,19 @@ BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
             name = lst->value->u.external.name;
             src = block_dirty_bitmap_lookup(node, name, NULL, errp);
             if (!src) {
-                dst = NULL;
-                goto out;
+                return NULL;
             }
             break;
         default:
             abort();
         }
 
-        if (!bdrv_merge_dirty_bitmap(anon, src, NULL, errp)) {
-            dst = NULL;
-            goto out;
+        if (!bdrv_merge_dirty_bitmap(dst, src, backup, errp)) {
+            return NULL;
         }
+        backup = NULL; /* Set once by first bdrv_merge_dirty_bitmap() call */
     }
 
-    /* Merge into dst; dst is unchanged on failure. */
-    if (!bdrv_merge_dirty_bitmap(dst, anon, backup, errp)) {
-        dst = NULL;
-        goto out;
-    }
-
- out:
-    bdrv_release_dirty_bitmap(anon);
     return dst;
 }
 
-- 
2.31.1

Re: [PATCH 3/4] block: improve block_dirty_bitmap_merge(): don't allocate extra bitmap

[Nikta Lapshin]

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


On 2/15/22 20:53, Vladimir Sementsov-Ogievskiy wrote:

> We don't need extra bitmap. All we need is to backup the original
> bitmap when we do first merge. So, drop extra temporary bitmap and work
> directly with target and backup.
>
> Note that block_dirty_bitmap_merge() semantics changed: on failure
> target may be modified now, and caller should call
> block_dirty_bitmap_restore() if needed. The only caller is
> qmp_transaction() and ->abort() is called for failed action. So, we are
> OK.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
> ---
>   block/monitor/bitmap-qmp-cmds.c | 31 +++++++++----------------------
>   1 file changed, 9 insertions(+), 22 deletions(-)
>
Reviewed-by: Nikita Lapshin<nikita.lapshin@virtuozzo.com>

[-- Attachment #2: Type: text/html, Size: 1275 bytes --]

Re: [PATCH 3/4] block: improve block_dirty_bitmap_merge(): don't allocate extra bitmap

[Vladimir Sementsov-Ogievskiy]

15.02.2022 20:53, Vladimir Sementsov-Ogievskiy wrote:
> We don't need extra bitmap. All we need is to backup the original
> bitmap when we do first merge. So, drop extra temporary bitmap and work
> directly with target and backup.
> 
> Note that block_dirty_bitmap_merge() semantics changed: on failure
> target may be modified now, and caller should call
> block_dirty_bitmap_restore() if needed. The only caller is
> qmp_transaction() and ->abort() is called for failed action. So, we are
> OK.

But actually caller rely on the fact that target bitmap is returned together with set @backup and this leads to bug. I have to make a v2.

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block/monitor/bitmap-qmp-cmds.c | 31 +++++++++----------------------
>   1 file changed, 9 insertions(+), 22 deletions(-)
> 
> diff --git a/block/monitor/bitmap-qmp-cmds.c b/block/monitor/bitmap-qmp-cmds.c
> index a94aaa9fb3..2ce1b4e455 100644
> --- a/block/monitor/bitmap-qmp-cmds.c
> +++ b/block/monitor/bitmap-qmp-cmds.c
> @@ -252,12 +252,15 @@ void qmp_block_dirty_bitmap_disable(const char *node, const char *name,
>       bdrv_disable_dirty_bitmap(bitmap);
>   }
>   
> +/*
> + * On failure target may be modified. In this case @backup is set.
> + */
>   BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
>                                             BlockDirtyBitmapMergeSourceList *bms,
>                                             HBitmap **backup, Error **errp)
>   {
>       BlockDriverState *bs;
> -    BdrvDirtyBitmap *dst, *src, *anon;
> +    BdrvDirtyBitmap *dst, *src;
>       BlockDirtyBitmapMergeSourceList *lst;
>   
>       dst = block_dirty_bitmap_lookup(node, target, &bs, errp);
> @@ -265,12 +268,6 @@ BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
>           return NULL;
>       }
>   
> -    anon = bdrv_create_dirty_bitmap(bs, bdrv_dirty_bitmap_granularity(dst),
> -                                    NULL, errp);
> -    if (!anon) {
> -        return NULL;
> -    }
> -
>       for (lst = bms; lst; lst = lst->next) {
>           switch (lst->value->type) {
>               const char *name, *node;
> @@ -279,8 +276,7 @@ BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
>               src = bdrv_find_dirty_bitmap(bs, name);
>               if (!src) {
>                   error_setg(errp, "Dirty bitmap '%s' not found", name);
> -                dst = NULL;
> -                goto out;
> +                return NULL;
>               }
>               break;
>           case QTYPE_QDICT:
> @@ -288,28 +284,19 @@ BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
>               name = lst->value->u.external.name;
>               src = block_dirty_bitmap_lookup(node, name, NULL, errp);
>               if (!src) {
> -                dst = NULL;
> -                goto out;
> +                return NULL;
>               }
>               break;
>           default:
>               abort();
>           }
>   
> -        if (!bdrv_merge_dirty_bitmap(anon, src, NULL, errp)) {
> -            dst = NULL;
> -            goto out;
> +        if (!bdrv_merge_dirty_bitmap(dst, src, backup, errp)) {
> +            return NULL;
>           }
> +        backup = NULL; /* Set once by first bdrv_merge_dirty_bitmap() call */
>       }
>   
> -    /* Merge into dst; dst is unchanged on failure. */
> -    if (!bdrv_merge_dirty_bitmap(dst, anon, backup, errp)) {
> -        dst = NULL;
> -        goto out;
> -    }
> -
> - out:
> -    bdrv_release_dirty_bitmap(anon);
>       return dst;
>   }
>   


-- 
Best regards,
Vladimir

[PATCH 4/4] block: simplify handling of try to merge different sized bitmaps

[Vladimir Sementsov-Ogievskiy]

We have too much logic to simply check that bitmaps are of the same
size. Let's just define that hbitmap_merge() and
bdrv_dirty_bitmap_merge_internal() require their argument bitmaps be of
same size, this simplifies things.

Let's look through the callers:

For backup_init_bcs_bitmap() we already assert that merge can't fail.

In bdrv_reclaim_dirty_bitmap_locked() we gracefully handle the error
that can't happen: successor always has same size as its parent, drop
this logic.

In bdrv_merge_dirty_bitmap() we already has assertion and separate
check. Make the check explicit and improve error message.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/block_int.h |  2 +-
 include/qemu/hbitmap.h    | 15 ++-------------
 block/backup.c            |  6 ++----
 block/dirty-bitmap.c      | 25 ++++++++++---------------
 util/hbitmap.c            | 25 +++++++------------------
 5 files changed, 22 insertions(+), 51 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 27008cfb22..cc40b6363d 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1382,7 +1382,7 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t offset, int64_t bytes);
 
 void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out);
 void bdrv_restore_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *backup);
-bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
+void bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
                                       const BdrvDirtyBitmap *src,
                                       HBitmap **backup, bool lock);
 
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index 5e71b6d6f7..4dc1c6ad14 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -76,20 +76,9 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size);
  *
  * 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.
+ * All bitmaps must have same size.
  */
-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_can_merge(const HBitmap *a, const HBitmap *b);
+void hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result);
 
 /**
  * hbitmap_empty:
diff --git a/block/backup.c b/block/backup.c
index 21d5983779..fb3d4b0e13 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -228,15 +228,13 @@ out:
 
 static void backup_init_bcs_bitmap(BackupBlockJob *job)
 {
-    bool ret;
     uint64_t estimate;
     BdrvDirtyBitmap *bcs_bitmap = block_copy_dirty_bitmap(job->bcs);
 
     if (job->sync_mode == MIRROR_SYNC_MODE_BITMAP) {
         bdrv_clear_dirty_bitmap(bcs_bitmap, NULL);
-        ret = bdrv_dirty_bitmap_merge_internal(bcs_bitmap, job->sync_bitmap,
-                                               NULL, true);
-        assert(ret);
+        bdrv_dirty_bitmap_merge_internal(bcs_bitmap, job->sync_bitmap, NULL,
+                                         true);
     } else if (job->sync_mode == MIRROR_SYNC_MODE_TOP) {
         /*
          * We can't hog the coroutine to initialize this thoroughly.
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index d16b96ee62..9d803fcda3 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -309,10 +309,7 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BdrvDirtyBitmap *parent,
         return NULL;
     }
 
-    if (!hbitmap_merge(parent->bitmap, successor->bitmap, parent->bitmap)) {
-        error_setg(errp, "Merging of parent and successor bitmap failed");
-        return NULL;
-    }
+    hbitmap_merge(parent->bitmap, successor->bitmap, parent->bitmap);
 
     parent->disabled = successor->disabled;
     parent->busy = false;
@@ -899,13 +896,14 @@ bool bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
         goto out;
     }
 
-    if (!hbitmap_can_merge(dest->bitmap, src->bitmap)) {
-        error_setg(errp, "Bitmaps are incompatible and can't be merged");
+    if (bdrv_dirty_bitmap_size(src) != bdrv_dirty_bitmap_size(dest)) {
+        error_setg(errp, "Bitmaps are of different sizes (destination size is %"
+                   PRId64 ", source size is %" PRId64 ") and can't be merged",
+                   bdrv_dirty_bitmap_size(dest), bdrv_dirty_bitmap_size(src));
         goto out;
     }
 
-    ret = bdrv_dirty_bitmap_merge_internal(dest, src, backup, false);
-    assert(ret);
+    bdrv_dirty_bitmap_merge_internal(dest, src, backup, false);
 
 out:
     bdrv_dirty_bitmaps_unlock(dest->bs);
@@ -919,18 +917,17 @@ out:
 /**
  * bdrv_dirty_bitmap_merge_internal: merge src into dest.
  * Does NOT check bitmap permissions; not suitable for use as public API.
+ * @dest, @src and @backup (if not NULL) must have same size.
  *
  * @backup: If provided, make a copy of dest here prior to merge.
  * @lock: If true, lock and unlock bitmaps on the way in/out.
  * returns true if the merge succeeded; false if unattempted.
  */
-bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
+void bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
                                       const BdrvDirtyBitmap *src,
                                       HBitmap **backup,
                                       bool lock)
 {
-    bool ret;
-
     assert(!bdrv_dirty_bitmap_readonly(dest));
     assert(!bdrv_dirty_bitmap_inconsistent(dest));
     assert(!bdrv_dirty_bitmap_inconsistent(src));
@@ -945,9 +942,9 @@ bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
     if (backup) {
         *backup = dest->bitmap;
         dest->bitmap = hbitmap_alloc(dest->size, hbitmap_granularity(*backup));
-        ret = hbitmap_merge(*backup, src->bitmap, dest->bitmap);
+        hbitmap_merge(*backup, src->bitmap, dest->bitmap);
     } else {
-        ret = hbitmap_merge(dest->bitmap, src->bitmap, dest->bitmap);
+        hbitmap_merge(dest->bitmap, src->bitmap, dest->bitmap);
     }
 
     if (lock) {
@@ -956,6 +953,4 @@ bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
             bdrv_dirty_bitmaps_unlock(src->bs);
         }
     }
-
-    return ret;
 }
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 305b894a63..d0aaf205ed 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -840,11 +840,6 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size)
     }
 }
 
-bool hbitmap_can_merge(const HBitmap *a, const HBitmap *b)
-{
-    return (a->orig_size == b->orig_size);
-}
-
 /**
  * hbitmap_sparse_merge: performs dst = dst | src
  * works with differing granularities.
@@ -868,28 +863,24 @@ static void hbitmap_sparse_merge(HBitmap *dst, const HBitmap *src)
  * Given HBitmaps A and B, let R := A (BITOR) B.
  * Bitmaps A and B will not be modified,
  *     except when bitmap R is an alias of A or B.
- *
- * @return true if the merge was successful,
- *         false if it was not attempted.
+ * Bitmaps must have same size.
  */
-bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result)
+void hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result)
 {
     int i;
     uint64_t j;
 
-    if (!hbitmap_can_merge(a, b) || !hbitmap_can_merge(a, result)) {
-        return false;
-    }
-    assert(hbitmap_can_merge(b, result));
+    assert(a->orig_size == result->orig_size);
+    assert(b->orig_size == result->orig_size);
 
     if ((!hbitmap_count(a) && result == b) ||
         (!hbitmap_count(b) && result == a)) {
-        return true;
+        return;
     }
 
     if (!hbitmap_count(a) && !hbitmap_count(b)) {
         hbitmap_reset_all(result);
-        return true;
+        return;
     }
 
     if (a->granularity != b->granularity) {
@@ -902,7 +893,7 @@ bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result)
         if (b != result) {
             hbitmap_sparse_merge(result, b);
         }
-        return true;
+        return;
     }
 
     /* This merge is O(size), as BITS_PER_LONG and HBITMAP_LEVELS are constant.
@@ -918,8 +909,6 @@ bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result)
 
     /* Recompute the dirty count */
     result->count = hb_count_between(result, 0, result->size - 1);
-
-    return true;
 }
 
 char *hbitmap_sha256(const HBitmap *bitmap, Error **errp)
-- 
2.31.1

Re: [PATCH 4/4] block: simplify handling of try to merge different sized bitmaps

[Nikta Lapshin]

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

On 2/15/22 20:53, Vladimir Sementsov-Ogievskiy wrote:

> We have too much logic to simply check that bitmaps are of the same
> size. Let's just define that hbitmap_merge() and
> bdrv_dirty_bitmap_merge_internal() require their argument bitmaps be of
> same size, this simplifies things.
>
> Let's look through the callers:
>
> For backup_init_bcs_bitmap() we already assert that merge can't fail.
>
> In bdrv_reclaim_dirty_bitmap_locked() we gracefully handle the error
> that can't happen: successor always has same size as its parent, drop
> this logic.
>
> In bdrv_merge_dirty_bitmap() we already has assertion and separate
> check. Make the check explicit and improve error message.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
> ---
>   include/block/block_int.h |  2 +-
>   include/qemu/hbitmap.h    | 15 ++-------------
>   block/backup.c            |  6 ++----
>   block/dirty-bitmap.c      | 25 ++++++++++---------------
>   util/hbitmap.c            | 25 +++++++------------------
>   5 files changed, 22 insertions(+), 51 deletions(-)
>
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 27008cfb22..cc40b6363d 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -1382,7 +1382,7 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t offset, int64_t bytes);
>   
>   void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out);
>   void bdrv_restore_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *backup);
> -bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
> +void bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
>                                         const BdrvDirtyBitmap *src,
>                                         HBitmap **backup, bool lock);
>   
> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
> index 5e71b6d6f7..4dc1c6ad14 100644
> --- a/include/qemu/hbitmap.h
> +++ b/include/qemu/hbitmap.h
> @@ -76,20 +76,9 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size);
>    *
>    * 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.
> + * All bitmaps must have same size.
>    */
> -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_can_merge(const HBitmap *a, const HBitmap *b);
> +void hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result);
>   
>   /**
>    * hbitmap_empty:
> diff --git a/block/backup.c b/block/backup.c
> index 21d5983779..fb3d4b0e13 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -228,15 +228,13 @@ out:
>   
>   static void backup_init_bcs_bitmap(BackupBlockJob *job)
>   {
> -    bool ret;
>       uint64_t estimate;
>       BdrvDirtyBitmap *bcs_bitmap = block_copy_dirty_bitmap(job->bcs);
>   
>       if (job->sync_mode == MIRROR_SYNC_MODE_BITMAP) {
>           bdrv_clear_dirty_bitmap(bcs_bitmap, NULL);
> -        ret = bdrv_dirty_bitmap_merge_internal(bcs_bitmap, job->sync_bitmap,
> -                                               NULL, true);
> -        assert(ret);
> +        bdrv_dirty_bitmap_merge_internal(bcs_bitmap, job->sync_bitmap, NULL,
> +                                         true);
>       } else if (job->sync_mode == MIRROR_SYNC_MODE_TOP) {
>           /*
>            * We can't hog the coroutine to initialize this thoroughly.
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index d16b96ee62..9d803fcda3 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -309,10 +309,7 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BdrvDirtyBitmap *parent,
>           return NULL;
>       }
>   
> -    if (!hbitmap_merge(parent->bitmap, successor->bitmap, parent->bitmap)) {
> -        error_setg(errp, "Merging of parent and successor bitmap failed");
> -        return NULL;
> -    }
> +    hbitmap_merge(parent->bitmap, successor->bitmap, parent->bitmap);
>   
>       parent->disabled = successor->disabled;
>       parent->busy = false;
> @@ -899,13 +896,14 @@ bool bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>           goto out;
>       }
>   
> -    if (!hbitmap_can_merge(dest->bitmap, src->bitmap)) {
> -        error_setg(errp, "Bitmaps are incompatible and can't be merged");
> +    if (bdrv_dirty_bitmap_size(src) != bdrv_dirty_bitmap_size(dest)) {
> +        error_setg(errp, "Bitmaps are of different sizes (destination size is %"
> +                   PRId64 ", source size is %" PRId64 ") and can't be merged",
> +                   bdrv_dirty_bitmap_size(dest), bdrv_dirty_bitmap_size(src));
>           goto out;
>       }
>   
> -    ret = bdrv_dirty_bitmap_merge_internal(dest, src, backup, false);
> -    assert(ret);
> +    bdrv_dirty_bitmap_merge_internal(dest, src, backup, false);
>   
>   out:
>       bdrv_dirty_bitmaps_unlock(dest->bs);
> @@ -919,18 +917,17 @@ out:
>   /**
>    * bdrv_dirty_bitmap_merge_internal: merge src into dest.
>    * Does NOT check bitmap permissions; not suitable for use as public API.
> + * @dest, @src and @backup (if not NULL) must have same size.
>    *
>    * @backup: If provided, make a copy of dest here prior to merge.
>    * @lock: If true, lock and unlock bitmaps on the way in/out.
>    * returns true if the merge succeeded; false if unattempted.
>    */

There is no return value after this change.

> -bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
> +void bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
>                                         const BdrvDirtyBitmap *src,
>                                         HBitmap **backup,
>                                         bool lock)
>   {
> -    bool ret;
> -
>       assert(!bdrv_dirty_bitmap_readonly(dest));
>       assert(!bdrv_dirty_bitmap_inconsistent(dest));
>       assert(!bdrv_dirty_bitmap_inconsistent(src));
> @@ -945,9 +942,9 @@ bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
>       if (backup) {
>           *backup = dest->bitmap;
>           dest->bitmap = hbitmap_alloc(dest->size, hbitmap_granularity(*backup));
> -        ret = hbitmap_merge(*backup, src->bitmap, dest->bitmap);
> +        hbitmap_merge(*backup, src->bitmap, dest->bitmap);
>       } else {
> -        ret = hbitmap_merge(dest->bitmap, src->bitmap, dest->bitmap);
> +        hbitmap_merge(dest->bitmap, src->bitmap, dest->bitmap);
>       }
>   
>       if (lock) {
> @@ -956,6 +953,4 @@ bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
>               bdrv_dirty_bitmaps_unlock(src->bs);
>           }
>       }
> -
> -    return ret;
>   }
> diff --git a/util/hbitmap.c b/util/hbitmap.c
> index 305b894a63..d0aaf205ed 100644
> --- a/util/hbitmap.c
> +++ b/util/hbitmap.c
> @@ -840,11 +840,6 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size)
>       }
>   }
>   
> -bool hbitmap_can_merge(const HBitmap *a, const HBitmap *b)
> -{
> -    return (a->orig_size == b->orig_size);
> -}
> -
>   /**
>    * hbitmap_sparse_merge: performs dst = dst | src
>    * works with differing granularities.
> @@ -868,28 +863,24 @@ static void hbitmap_sparse_merge(HBitmap *dst, const HBitmap *src)
>    * Given HBitmaps A and B, let R := A (BITOR) B.
>    * Bitmaps A and B will not be modified,
>    *     except when bitmap R is an alias of A or B.
> - *
> - * @return true if the merge was successful,
> - *         false if it was not attempted.
> + * Bitmaps must have same size.
>    */
> -bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result)
> +void hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result)
>   {
>       int i;
>       uint64_t j;
>   
> -    if (!hbitmap_can_merge(a, b) || !hbitmap_can_merge(a, result)) {
> -        return false;
> -    }
> -    assert(hbitmap_can_merge(b, result));
> +    assert(a->orig_size == result->orig_size);
> +    assert(b->orig_size == result->orig_size);
>   

When hbitmap_merge() is called it is impossible to know if 'a' and 'b' have
the same orig_size. Of course there is workaround as it is shown using
bdrv_dirty_bitmap_size. However one function for checking looks to me better
than workaround. May be I missed the idea so I don't insist on it moreover.

>       if ((!hbitmap_count(a) && result == b) ||
>           (!hbitmap_count(b) && result == a)) {
> -        return true;
> +        return;
>       }
>   
>       if (!hbitmap_count(a) && !hbitmap_count(b)) {
>           hbitmap_reset_all(result);
> -        return true;
> +        return;
>       }
>   
>       if (a->granularity != b->granularity) {
> @@ -902,7 +893,7 @@ bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result)
>           if (b != result) {
>               hbitmap_sparse_merge(result, b);
>           }
> -        return true;
> +        return;
>       }
>   
>       /* This merge is O(size), as BITS_PER_LONG and HBITMAP_LEVELS are constant.
> @@ -918,8 +909,6 @@ bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result)
>   
>       /* Recompute the dirty count */
>       result->count = hb_count_between(result, 0, result->size - 1);
> -
> -    return true;
>   }
>   
>   char *hbitmap_sha256(const HBitmap *bitmap, Error **errp)

With changed description:
Reviewed-by: Nikita Lapshin<nikita.lapshin@virtuozzo.com>

[-- Attachment #2: Type: text/html, Size: 10043 bytes --]

Re: [PATCH 4/4] block: simplify handling of try to merge different sized bitmaps

[Vladimir Sementsov-Ogievskiy via]

19.02.2022 06:04, Nikta Lapshin wrote:
> On 2/15/22 20:53, Vladimir Sementsov-Ogievskiy wrote:
> 
>> We have too much logic to simply check that bitmaps are of the same
>> size. Let's just define that hbitmap_merge() and
>> bdrv_dirty_bitmap_merge_internal() require their argument bitmaps be of
>> same size, this simplifies things.
>>
>> Let's look through the callers:
>>
>> For backup_init_bcs_bitmap() we already assert that merge can't fail.
>>
>> In bdrv_reclaim_dirty_bitmap_locked() we gracefully handle the error
>> that can't happen: successor always has same size as its parent, drop
>> this logic.
>>
>> In bdrv_merge_dirty_bitmap() we already has assertion and separate
>> check. Make the check explicit and improve error message.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
>> ---
>>   include/block/block_int.h |  2 +-
>>   include/qemu/hbitmap.h    | 15 ++-------------
>>   block/backup.c            |  6 ++----
>>   block/dirty-bitmap.c      | 25 ++++++++++---------------
>>   util/hbitmap.c            | 25 +++++++------------------
>>   5 files changed, 22 insertions(+), 51 deletions(-)
>>
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index 27008cfb22..cc40b6363d 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -1382,7 +1382,7 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t offset, int64_t bytes);
>>   
>>   void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out);
>>   void bdrv_restore_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *backup);
>> -bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
>> +void bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
>>                                         const BdrvDirtyBitmap *src,
>>                                         HBitmap **backup, bool lock);
>>   
>> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
>> index 5e71b6d6f7..4dc1c6ad14 100644
>> --- a/include/qemu/hbitmap.h
>> +++ b/include/qemu/hbitmap.h
>> @@ -76,20 +76,9 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size);
>>    *
>>    * 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.
>> + * All bitmaps must have same size.
>>    */
>> -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_can_merge(const HBitmap *a, const HBitmap *b);
>> +void hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result);
>>   
>>   /**
>>    * hbitmap_empty:
>> diff --git a/block/backup.c b/block/backup.c
>> index 21d5983779..fb3d4b0e13 100644
>> --- a/block/backup.c
>> +++ b/block/backup.c
>> @@ -228,15 +228,13 @@ out:
>>   
>>   static void backup_init_bcs_bitmap(BackupBlockJob *job)
>>   {
>> -    bool ret;
>>       uint64_t estimate;
>>       BdrvDirtyBitmap *bcs_bitmap = block_copy_dirty_bitmap(job->bcs);
>>   
>>       if (job->sync_mode == MIRROR_SYNC_MODE_BITMAP) {
>>           bdrv_clear_dirty_bitmap(bcs_bitmap, NULL);
>> -        ret = bdrv_dirty_bitmap_merge_internal(bcs_bitmap, job->sync_bitmap,
>> -                                               NULL, true);
>> -        assert(ret);
>> +        bdrv_dirty_bitmap_merge_internal(bcs_bitmap, job->sync_bitmap, NULL,
>> +                                         true);
>>       } else if (job->sync_mode == MIRROR_SYNC_MODE_TOP) {
>>           /*
>>            * We can't hog the coroutine to initialize this thoroughly.
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index d16b96ee62..9d803fcda3 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -309,10 +309,7 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BdrvDirtyBitmap *parent,
>>           return NULL;
>>       }
>>   
>> -    if (!hbitmap_merge(parent->bitmap, successor->bitmap, parent->bitmap)) {
>> -        error_setg(errp, "Merging of parent and successor bitmap failed");
>> -        return NULL;
>> -    }
>> +    hbitmap_merge(parent->bitmap, successor->bitmap, parent->bitmap);
>>   
>>       parent->disabled = successor->disabled;
>>       parent->busy = false;
>> @@ -899,13 +896,14 @@ bool bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>>           goto out;
>>       }
>>   
>> -    if (!hbitmap_can_merge(dest->bitmap, src->bitmap)) {
>> -        error_setg(errp, "Bitmaps are incompatible and can't be merged");
>> +    if (bdrv_dirty_bitmap_size(src) != bdrv_dirty_bitmap_size(dest)) {
>> +        error_setg(errp, "Bitmaps are of different sizes (destination size is %"
>> +                   PRId64 ", source size is %" PRId64 ") and can't be merged",
>> +                   bdrv_dirty_bitmap_size(dest), bdrv_dirty_bitmap_size(src));
>>           goto out;
>>       }
>>   
>> -    ret = bdrv_dirty_bitmap_merge_internal(dest, src, backup, false);
>> -    assert(ret);
>> +    bdrv_dirty_bitmap_merge_internal(dest, src, backup, false);
>>   
>>   out:
>>       bdrv_dirty_bitmaps_unlock(dest->bs);
>> @@ -919,18 +917,17 @@ out:
>>   /**
>>    * bdrv_dirty_bitmap_merge_internal: merge src into dest.
>>    * Does NOT check bitmap permissions; not suitable for use as public API.
>> + * @dest, @src and @backup (if not NULL) must have same size.
>>    *
>>    * @backup: If provided, make a copy of dest here prior to merge.
>>    * @lock: If true, lock and unlock bitmaps on the way in/out.
>>    * returns true if the merge succeeded; false if unattempted.
>>    */
> 
> There is no return value after this change.

Right, thanks!

> 
>> -bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
>> +void bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
>>                                         const BdrvDirtyBitmap *src,
>>                                         HBitmap **backup,
>>                                         bool lock)
>>   {
>> -    bool ret;
>> -
>>       assert(!bdrv_dirty_bitmap_readonly(dest));
>>       assert(!bdrv_dirty_bitmap_inconsistent(dest));
>>       assert(!bdrv_dirty_bitmap_inconsistent(src));
>> @@ -945,9 +942,9 @@ bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
>>       if (backup) {
>>           *backup = dest->bitmap;
>>           dest->bitmap = hbitmap_alloc(dest->size, hbitmap_granularity(*backup));
>> -        ret = hbitmap_merge(*backup, src->bitmap, dest->bitmap);
>> +        hbitmap_merge(*backup, src->bitmap, dest->bitmap);
>>       } else {
>> -        ret = hbitmap_merge(dest->bitmap, src->bitmap, dest->bitmap);
>> +        hbitmap_merge(dest->bitmap, src->bitmap, dest->bitmap);
>>       }
>>   
>>       if (lock) {
>> @@ -956,6 +953,4 @@ bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
>>               bdrv_dirty_bitmaps_unlock(src->bs);
>>           }
>>       }
>> -
>> -    return ret;
>>   }
>> diff --git a/util/hbitmap.c b/util/hbitmap.c
>> index 305b894a63..d0aaf205ed 100644
>> --- a/util/hbitmap.c
>> +++ b/util/hbitmap.c
>> @@ -840,11 +840,6 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size)
>>       }
>>   }
>>   
>> -bool hbitmap_can_merge(const HBitmap *a, const HBitmap *b)
>> -{
>> -    return (a->orig_size == b->orig_size);
>> -}
>> -
>>   /**
>>    * hbitmap_sparse_merge: performs dst = dst | src
>>    * works with differing granularities.
>> @@ -868,28 +863,24 @@ static void hbitmap_sparse_merge(HBitmap *dst, const HBitmap *src)
>>    * Given HBitmaps A and B, let R := A (BITOR) B.
>>    * Bitmaps A and B will not be modified,
>>    *     except when bitmap R is an alias of A or B.
>> - *
>> - * @return true if the merge was successful,
>> - *         false if it was not attempted.
>> + * Bitmaps must have same size.
>>    */
>> -bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result)
>> +void hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result)
>>   {
>>       int i;
>>       uint64_t j;
>>   
>> -    if (!hbitmap_can_merge(a, b) || !hbitmap_can_merge(a, result)) {
>> -        return false;
>> -    }
>> -    assert(hbitmap_can_merge(b, result));
>> +    assert(a->orig_size == result->orig_size);
>> +    assert(b->orig_size == result->orig_size);
>>   
> 
> When hbitmap_merge() is called it is impossible to know if 'a' and 'b' have
> the same orig_size.

orig_size is the same as size of corresponding BdrvDirtyBitmap.

So, for QAPI, we can simply check sizes of BdrvDirtyBitmap objects. And for all other uses we always sure that bitmaps are of the same size.

> Of course there is workaround as it is shown using
> bdrv_dirty_bitmap_size. However one function for checking looks to me better
> than workaround. May be I missed the idea so I don't insist on it moreover.
> 
>>       if ((!hbitmap_count(a) && result == b) ||
>>           (!hbitmap_count(b) && result == a)) {
>> -        return true;
>> +        return;
>>       }
>>   
>>       if (!hbitmap_count(a) && !hbitmap_count(b)) {
>>           hbitmap_reset_all(result);
>> -        return true;
>> +        return;
>>       }
>>   
>>       if (a->granularity != b->granularity) {
>> @@ -902,7 +893,7 @@ bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result)
>>           if (b != result) {
>>               hbitmap_sparse_merge(result, b);
>>           }
>> -        return true;
>> +        return;
>>       }
>>   
>>       /* This merge is O(size), as BITS_PER_LONG and HBITMAP_LEVELS are constant.
>> @@ -918,8 +909,6 @@ bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result)
>>   
>>       /* Recompute the dirty count */
>>       result->count = hb_count_between(result, 0, result->size - 1);
>> -
>> -    return true;
>>   }
>>   
>>   char *hbitmap_sha256(const HBitmap *bitmap, Error **errp)
> 
> With changed description:
> Reviewed-by: Nikita Lapshin<nikita.lapshin@virtuozzo.com>
> 

Thanks a lot for reviewing!

-- 
Best regards,
Vladimir

Re: [PATCH 4/4] block: simplify handling of try to merge different sized bitmaps

[Vladimir Sementsov-Ogievskiy]

15.02.2022 20:53, Vladimir Sementsov-Ogievskiy wrote:
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index d16b96ee62..9d803fcda3 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -309,10 +309,7 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BdrvDirtyBitmap *parent,
>           return NULL;
>       }
>   
> -    if (!hbitmap_merge(parent->bitmap, successor->bitmap, parent->bitmap)) {
> -        error_setg(errp, "Merging of parent and successor bitmap failed");
> -        return NULL;
> -    }
> +    hbitmap_merge(parent->bitmap, successor->bitmap, parent->bitmap);
>   
>       parent->disabled = successor->disabled;
>       parent->busy = false;
> @@ -899,13 +896,14 @@ bool bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>           goto out;
>       }
>   
> -    if (!hbitmap_can_merge(dest->bitmap, src->bitmap)) {
> -        error_setg(errp, "Bitmaps are incompatible and can't be merged");
> +    if (bdrv_dirty_bitmap_size(src) != bdrv_dirty_bitmap_size(dest)) {
> +        error_setg(errp, "Bitmaps are of different sizes (destination size is %"
> +                   PRId64 ", source size is %" PRId64 ") and can't be merged",
> +                   bdrv_dirty_bitmap_size(dest), bdrv_dirty_bitmap_size(src));
>           goto out;
>       }
>   
> -    ret = bdrv_dirty_bitmap_merge_internal(dest, src, backup, false);
> -    assert(ret);
> +    bdrv_dirty_bitmap_merge_internal(dest, src, backup, false);

bug here: actually we still should set ret to true: it's a return status which we are going to return to the caller!

>   
>   out:
>       bdrv_dirty_bitmaps_unlock(dest->bs);



-- 
Best regards,
Vladimir