Re: [Qemu-devel] [PATCH 3/4] qapi: implement block-dirty-bitmap-remove transaction action

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

08.06.2019 1:57, John Snow wrote:
> 
> 
> On 6/3/19 8:00 AM, Vladimir Sementsov-Ogievskiy wrote:
>> It is used to do transactional movement of the bitmap (which is
>> possible in conjunction with merge command). Transactional bitmap
>> movement is needed in scenarios with external snapshot, when we don't
>> want to leave copy of the bitmap in the base image.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   qapi/transaction.json |  2 ++
>>   blockdev.c            | 74 +++++++++++++++++++++++++++++++++++++++----
>>   2 files changed, 70 insertions(+), 6 deletions(-)
>>
>> diff --git a/qapi/transaction.json b/qapi/transaction.json
>> index 95edb78227..da95b804aa 100644
>> --- a/qapi/transaction.json
>> +++ b/qapi/transaction.json
>> @@ -45,6 +45,7 @@
>>   #
>>   # - @abort: since 1.6
>>   # - @block-dirty-bitmap-add: since 2.5
>> +# - @block-dirty-bitmap-remove: since 4.1
>>   # - @block-dirty-bitmap-clear: since 2.5
>>   # - @block-dirty-bitmap-enable: since 4.0
>>   # - @block-dirty-bitmap-disable: since 4.0
>> @@ -61,6 +62,7 @@
>>     'data': {
>>          'abort': 'Abort',
>>          'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
>> +       'block-dirty-bitmap-remove': 'BlockDirtyBitmap',
>>          'block-dirty-bitmap-clear': 'BlockDirtyBitmap',
>>          'block-dirty-bitmap-enable': 'BlockDirtyBitmap',
>>          'block-dirty-bitmap-disable': 'BlockDirtyBitmap',
>> diff --git a/blockdev.c b/blockdev.c
>> index 5b3eef0d3e..0d9aa7f0a1 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -2135,6 +2135,46 @@ static void block_dirty_bitmap_merge_prepare(BlkActionState *common,
>>                                                   errp);
>>   }
>>   
>> +static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
>> +        const char *node, const char *name, bool release,
>> +        BlockDriverState **bitmap_bs, Error **errp);
>> +
>> +static void block_dirty_bitmap_remove_prepare(BlkActionState *common,
>> +                                              Error **errp)
>> +{
>> +    BlockDirtyBitmap *action;
>> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>> +                                             common, common);
>> +
>> +    if (action_check_completion_mode(common, errp) < 0) {
>> +        return;
>> +    }
>> +
>> +    action = common->action->u.block_dirty_bitmap_remove.data;
>> +
>> +    state->bitmap = do_block_dirty_bitmap_remove(action->node, action->name,
>> +                                                 false, &state->bs, errp);
>> +    if (state->bitmap) {
>> +        bdrv_dirty_bitmap_hide(state->bitmap);
>> +    }
>> +}
>> +
>> +static void block_dirty_bitmap_remove_abort(BlkActionState *common)
>> +{
>> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>> +                                             common, common);
>> +
>> +    bdrv_dirty_bitmap_unhide(state->bitmap);
>> +}
>> +
>> +static void block_dirty_bitmap_remove_commit(BlkActionState *common)
>> +{
>> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>> +                                             common, common);
>> +
>> +    bdrv_release_dirty_bitmap(state->bs, state->bitmap);
>> +}
>> +
>>   static void abort_prepare(BlkActionState *common, Error **errp)
>>   {
>>       error_setg(errp, "Transaction aborted using Abort action");
>> @@ -2212,6 +2252,12 @@ static const BlkActionOps actions[] = {
>>           .commit = block_dirty_bitmap_free_backup,
>>           .abort = block_dirty_bitmap_restore,
>>       },
>> +    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_REMOVE] = {
>> +        .instance_size = sizeof(BlockDirtyBitmapState),
>> +        .prepare = block_dirty_bitmap_remove_prepare,
>> +        .commit = block_dirty_bitmap_remove_commit,
>> +        .abort = block_dirty_bitmap_remove_abort,
>> +    },
>>       /* Where are transactions for MIRROR, COMMIT and STREAM?
>>        * Although these blockjobs use transaction callbacks like the backup job,
>>        * these jobs do not necessarily adhere to transaction semantics.
>> @@ -2870,20 +2916,21 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
>>       bdrv_dirty_bitmap_set_persistence(bitmap, persistent);
>>   }
>>   
>> -void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
>> -                                   Error **errp)
>> +static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
>> +        const char *node, const char *name, bool release,
>> +        BlockDriverState **bitmap_bs, Error **errp)
> 
> Hm, why does the hide feature need to copy the persistent bit when we're
> removing it here anyway?
> 
> If release is false, we still call bdrv_remove_persistent_dirty_bitmap,
> yeah?
> 
> And when we go to undo it, we won't have undone the persistent removal,
> right?

Hm, good question. I remember there was bad thing on storing persistent bitmap,
as this code is not prepared for unnamed persistent bitmaps. Aha, I was wrong in
my previous answer, it is valid to go to storing during transaction, and this is
exactly reopen to RO. So we must drop persistence.

> 
>>   {
>>       BlockDriverState *bs;
>>       BdrvDirtyBitmap *bitmap;
>>   
>>       bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
>>       if (!bitmap || !bs) {
>> -        return;
>> +        return NULL;
>>       }
>>   
>>       if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_BUSY | BDRV_BITMAP_RO,
>>                                   errp)) {
>> -        return;
>> +        return NULL;
>>       }
>>   
>>       if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
>> @@ -2893,13 +2940,28 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
>>           aio_context_acquire(aio_context);
>>           bdrv_remove_persistent_dirty_bitmap(bs, name, &local_err);
>>           aio_context_release(aio_context);
>> +
>>           if (local_err != NULL) {
>>               error_propagate(errp, local_err);
>> -            return;
>> +            return NULL;
>>           }
>>       }
>>   
>> -    bdrv_release_dirty_bitmap(bs, bitmap);
>> +    if (release) {
>> +        bdrv_release_dirty_bitmap(bs, bitmap);
>> +    }
>> +
>> +    if (bitmap_bs) {
>> +        *bitmap_bs = bs;
>> +    }
>> +
>> +    return bitmap;
>> +}
>> +
>> +void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
>> +                                   Error **errp)
>> +{
>> +    do_block_dirty_bitmap_remove(node, name, true, NULL, errp);
>>   }
>>   
>>   /**
>>
> 
> Seems about right otherwise, though!
> 


-- 
Best regards,
Vladimir