[3/4] qapi: implement block-dirty-bitmap-remove transaction action

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(-)

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?

>  {
>      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!

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!
>