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 <vsement...@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