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

[Vladimir Sementsov-Ogievskiy <v.sementsov-og@mail.ru>]

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