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 <vsement...@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!