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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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