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

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

[Vladimir Sementsov-Ogievskiy]

v3: rebase on master, one patch is already merged.

Vladimir Sementsov-Ogievskiy (3):
  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-io.h    |  2 +-
 include/qemu/hbitmap.h          | 15 ++-----------
 block/backup.c                  |  6 ++----
 block/dirty-bitmap.c            | 26 ++++++++++------------
 block/monitor/bitmap-qmp-cmds.c | 38 +++++++++++++++++----------------
 util/hbitmap.c                  | 25 ++++++----------------
 6 files changed, 43 insertions(+), 69 deletions(-)

-- 
2.35.1

[PATCH v3 1/3] 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@openvz.org>
Reviewed-by: Nikita Lapshin <nikita.lapshin@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 8e35616c2e..4db704c015 100644
--- a/block/monitor/bitmap-qmp-cmds.c
+++ b/block/monitor/bitmap-qmp-cmds.c
@@ -309,7 +309,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.35.1

[PATCH v3 2/3] 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.

Still to keep old semantics, that on failure target is unchanged and
user don't need to restore, we need a local_backup variable and do
restore ourselves on failure path.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
---
 block/monitor/bitmap-qmp-cmds.c | 39 ++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/block/monitor/bitmap-qmp-cmds.c b/block/monitor/bitmap-qmp-cmds.c
index 4db704c015..07d0da323b 100644
--- a/block/monitor/bitmap-qmp-cmds.c
+++ b/block/monitor/bitmap-qmp-cmds.c
@@ -261,8 +261,9 @@ BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
                                           HBitmap **backup, Error **errp)
 {
     BlockDriverState *bs;
-    BdrvDirtyBitmap *dst, *src, *anon;
+    BdrvDirtyBitmap *dst, *src;
     BlockDirtyBitmapMergeSourceList *lst;
+    HBitmap *local_backup = NULL;
 
     GLOBAL_STATE_CODE();
 
@@ -271,12 +272,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;
@@ -285,8 +280,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;
+                goto fail;
             }
             break;
         case QTYPE_QDICT:
@@ -294,29 +288,34 @@ 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;
+                goto fail;
             }
             break;
         default:
             abort();
         }
 
-        if (!bdrv_merge_dirty_bitmap(anon, src, NULL, errp)) {
-            dst = NULL;
-            goto out;
+        /* We do backup only for first merge operation */
+        if (!bdrv_merge_dirty_bitmap(dst, src,
+                                     local_backup ? NULL : &local_backup,
+                                     errp))
+        {
+            goto fail;
         }
     }
 
-    /* Merge into dst; dst is unchanged on failure. */
-    if (!bdrv_merge_dirty_bitmap(dst, anon, backup, errp)) {
-        dst = NULL;
-        goto out;
+    if (backup) {
+        *backup = local_backup;
     }
 
- out:
-    bdrv_release_dirty_bitmap(anon);
     return dst;
+
+fail:
+    if (local_backup) {
+        bdrv_restore_dirty_bitmap(dst, local_backup);
+    }
+
+    return NULL;
 }
 
 void qmp_block_dirty_bitmap_merge(const char *node, const char *target,
-- 
2.35.1

[PATCH v3 3/3] 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@openvz.org>
Reviewed-by: Nikita Lapshin <nikita.lapshin@virtuozzo.com>
---
 include/block/block_int-io.h |  2 +-
 include/qemu/hbitmap.h       | 15 ++-------------
 block/backup.c               |  6 ++----
 block/dirty-bitmap.c         | 26 +++++++++++---------------
 util/hbitmap.c               | 25 +++++++------------------
 5 files changed, 23 insertions(+), 51 deletions(-)

diff --git a/include/block/block_int-io.h b/include/block/block_int-io.h
index bb454200e5..ded29e7494 100644
--- a/include/block/block_int-io.h
+++ b/include/block/block_int-io.h
@@ -102,7 +102,7 @@ bool blk_dev_is_tray_open(BlockBackend *blk);
 void bdrv_set_dirty(BlockDriverState *bs, int64_t offset, int64_t bytes);
 
 void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out);
-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 5bd986aa44..af4e4ab746 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.
- */
-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.
- *
+ * All bitmaps must have same size.
  */
-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 5cfd0b999c..b2b649e305 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 da1b91166f..bf3dc0512a 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;
@@ -912,13 +909,15 @@ 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);
+    ret = true;
 
 out:
     bdrv_dirty_bitmaps_unlock(dest->bs);
@@ -932,17 +931,16 @@ 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;
     IO_CODE();
 
     assert(!bdrv_dirty_bitmap_readonly(dest));
@@ -959,9 +957,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) {
@@ -970,6 +968,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 ea989e1f0e..297db35fb1 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -873,11 +873,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.
@@ -901,28 +896,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) {
@@ -935,7 +926,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.
@@ -951,8 +942,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.35.1

Re: [PATCH v3 0/3] block/dirty-bitmaps: fix and improve bitmap merge

[Hanna Reitz]

On 01.04.22 12:08, Vladimir Sementsov-Ogievskiy wrote:
> v3: rebase on master, one patch is already merged.
>
> Vladimir Sementsov-Ogievskiy (3):
>    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-io.h    |  2 +-
>   include/qemu/hbitmap.h          | 15 ++-----------
>   block/backup.c                  |  6 ++----
>   block/dirty-bitmap.c            | 26 ++++++++++------------
>   block/monitor/bitmap-qmp-cmds.c | 38 +++++++++++++++++----------------
>   util/hbitmap.c                  | 25 ++++++----------------
>   6 files changed, 43 insertions(+), 69 deletions(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH v3 0/3] block/dirty-bitmaps: fix and improve bitmap merge

[Eric Blake]

On Fri, Apr 01, 2022 at 01:08:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> v3: rebase on master, one patch is already merged.
> 
> Vladimir Sementsov-Ogievskiy (3):
>   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

Is any of this series worth getting into 7.0, or are we safe letting
it slide to 7.1?

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

Re: [PATCH v3 0/3] block/dirty-bitmaps: fix and improve bitmap merge

[Vladimir Sementsov-Ogievskiy]

01.04.2022 23:17, Eric Blake wrote:
> On Fri, Apr 01, 2022 at 01:08:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> v3: rebase on master, one patch is already merged.
>>
>> Vladimir Sementsov-Ogievskiy (3):
>>    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
> 
> Is any of this series worth getting into 7.0, or are we safe letting
> it slide to 7.1?
> 

Let's check.

Only first patch is a fix.

anon bitmap is created on same bs where dst is found, so they should be compatible in size.

But bdrv_merge_dirty_bitmap also do some checks on dst status, which may actually fail..

So, in bad case we set errp, but return non-NULL dst bitmap.

Look at callers of block_dirty_bitmap_merge:

1. qmp_block_dirty_bitmap_merge() is OK, it ignores return value.

2. qmp_transaction use local_err to detect error, so we'll go through error path, that's good. state->bitmap is set, but it's not really matter. What makes sense is state->backup set or not?

state->backup is initialized with zero, as qmp_transaction() use g_malloc0 to allocate state buffer.

And bdrv_merge_dirty_bitmap() will do all checks prior to call bdrv_dirty_bitmap_merge_internal(), which actually can set @backup. So, state->backup is not set in our bad case.

So that all should be OK to postpone for 7.1.

-- 
Best regards,
Vladimir

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

[Kevin Wolf]

Am 01.04.2022 um 12:08 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 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.
> 
> Still to keep old semantics, that on failure target is unchanged and
> user don't need to restore, we need a local_backup variable and do
> restore ourselves on failure path.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
> ---
>  block/monitor/bitmap-qmp-cmds.c | 39 ++++++++++++++++-----------------
>  1 file changed, 19 insertions(+), 20 deletions(-)
> 
> diff --git a/block/monitor/bitmap-qmp-cmds.c b/block/monitor/bitmap-qmp-cmds.c
> index 4db704c015..07d0da323b 100644
> --- a/block/monitor/bitmap-qmp-cmds.c
> +++ b/block/monitor/bitmap-qmp-cmds.c
> @@ -261,8 +261,9 @@ BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
>                                            HBitmap **backup, Error **errp)
>  {
>      BlockDriverState *bs;
> -    BdrvDirtyBitmap *dst, *src, *anon;
> +    BdrvDirtyBitmap *dst, *src;
>      BlockDirtyBitmapMergeSourceList *lst;
> +    HBitmap *local_backup = NULL;
>  
>      GLOBAL_STATE_CODE();
>  
> @@ -271,12 +272,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;
> @@ -285,8 +280,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;
> +                goto fail;
>              }
>              break;
>          case QTYPE_QDICT:
> @@ -294,29 +288,34 @@ 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;
> +                goto fail;
>              }
>              break;
>          default:
>              abort();
>          }
>  
> -        if (!bdrv_merge_dirty_bitmap(anon, src, NULL, errp)) {
> -            dst = NULL;
> -            goto out;
> +        /* We do backup only for first merge operation */
> +        if (!bdrv_merge_dirty_bitmap(dst, src,
> +                                     local_backup ? NULL : &local_backup,
> +                                     errp))
> +        {
> +            goto fail;
>          }
>      }
>  
> -    /* Merge into dst; dst is unchanged on failure. */
> -    if (!bdrv_merge_dirty_bitmap(dst, anon, backup, errp)) {
> -        dst = NULL;
> -        goto out;
> +    if (backup) {
> +        *backup = local_backup;
>      }

Don't we need something like '... else { hbitmap_free(local_backup); }'
in order to avoid leaking the memory?

>  
> - out:
> -    bdrv_release_dirty_bitmap(anon);
>      return dst;
> +
> +fail:
> +    if (local_backup) {
> +        bdrv_restore_dirty_bitmap(dst, local_backup);
> +    }
> +
> +    return NULL;
>  }

Kevin

Re: [PATCH v3 1/3] block: block_dirty_bitmap_merge(): fix error path

[Kevin Wolf]

Am 01.04.2022 um 12:08 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 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@openvz.org>
> Reviewed-by: Nikita Lapshin <nikita.lapshin@virtuozzo.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

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

[Kevin Wolf]

Am 01.04.2022 um 12:08 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 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@openvz.org>
> Reviewed-by: Nikita Lapshin <nikita.lapshin@virtuozzo.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

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

[Vladimir Sementsov-Ogievskiy]

03.05.2022 12:24, Kevin Wolf wrote:
> Am 01.04.2022 um 12:08 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 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.
>>
>> Still to keep old semantics, that on failure target is unchanged and
>> user don't need to restore, we need a local_backup variable and do
>> restore ourselves on failure path.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
>> ---
>>   block/monitor/bitmap-qmp-cmds.c | 39 ++++++++++++++++-----------------
>>   1 file changed, 19 insertions(+), 20 deletions(-)
>>
>> diff --git a/block/monitor/bitmap-qmp-cmds.c b/block/monitor/bitmap-qmp-cmds.c
>> index 4db704c015..07d0da323b 100644
>> --- a/block/monitor/bitmap-qmp-cmds.c
>> +++ b/block/monitor/bitmap-qmp-cmds.c
>> @@ -261,8 +261,9 @@ BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
>>                                             HBitmap **backup, Error **errp)
>>   {
>>       BlockDriverState *bs;
>> -    BdrvDirtyBitmap *dst, *src, *anon;
>> +    BdrvDirtyBitmap *dst, *src;
>>       BlockDirtyBitmapMergeSourceList *lst;
>> +    HBitmap *local_backup = NULL;
>>   
>>       GLOBAL_STATE_CODE();
>>   
>> @@ -271,12 +272,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;
>> @@ -285,8 +280,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;
>> +                goto fail;
>>               }
>>               break;
>>           case QTYPE_QDICT:
>> @@ -294,29 +288,34 @@ 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;
>> +                goto fail;
>>               }
>>               break;
>>           default:
>>               abort();
>>           }
>>   
>> -        if (!bdrv_merge_dirty_bitmap(anon, src, NULL, errp)) {
>> -            dst = NULL;
>> -            goto out;
>> +        /* We do backup only for first merge operation */
>> +        if (!bdrv_merge_dirty_bitmap(dst, src,
>> +                                     local_backup ? NULL : &local_backup,
>> +                                     errp))
>> +        {
>> +            goto fail;
>>           }
>>       }
>>   
>> -    /* Merge into dst; dst is unchanged on failure. */
>> -    if (!bdrv_merge_dirty_bitmap(dst, anon, backup, errp)) {
>> -        dst = NULL;
>> -        goto out;
>> +    if (backup) {
>> +        *backup = local_backup;
>>       }
> 
> Don't we need something like '... else { hbitmap_free(local_backup); }'
> in order to avoid leaking the memory?

Right! Will resend.

> 
>>   
>> - out:
>> -    bdrv_release_dirty_bitmap(anon);
>>       return dst;
>> +
>> +fail:
>> +    if (local_backup) {
>> +        bdrv_restore_dirty_bitmap(dst, local_backup);
>> +    }
>> +
>> +    return NULL;
>>   }
> 
> Kevin
> 


-- 
Best regards,
Vladimir