diff mbox series

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

Message ID 20220215175310.68058-4-vsementsov@virtuozzo.com
State New
Headers show
Series block/dirty-bitmaps: fix and improve bitmap merge | expand

Commit Message

Vladimir Sementsov-Ogievskiy Feb. 15, 2022, 5:53 p.m. UTC 
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(-)

Comments

Nikta Lapshin Feb. 19, 2022, 10:29 a.m. UTC | #1 
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>
Vladimir Sementsov-Ogievskiy Feb. 21, 2022, 3:59 p.m. UTC | #2 
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;
>   }
>
diff mbox series

Patch

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;
 }