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

[John Snow]

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 <address@hidden>
> ---
>  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!