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