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