Re: [Qemu-devel] [PATCH v2 4/6] qmp: transaction support for block-dirty-bitmap-enable/disable

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

16.01.2018 15:54, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   qapi/transaction.json |  4 +++
>   blockdev.c            | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 83 insertions(+)
>
> diff --git a/qapi/transaction.json b/qapi/transaction.json
> index bd312792da..b643d848f8 100644
> --- a/qapi/transaction.json
> +++ b/qapi/transaction.json
> @@ -46,6 +46,8 @@
>   # - @abort: since 1.6
>   # - @block-dirty-bitmap-add: since 2.5
>   # - @block-dirty-bitmap-clear: since 2.5
> +# - @block-dirty-bitmap-enable: since 2.12
> +# - @block-dirty-bitmap-disable: since 2.12
>   # - @blockdev-backup: since 2.3
>   # - @blockdev-snapshot: since 2.5
>   # - @blockdev-snapshot-internal-sync: since 1.7
> @@ -59,6 +61,8 @@
>          'abort': 'Abort',
>          'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
>          'block-dirty-bitmap-clear': 'BlockDirtyBitmap',
> +       'block-dirty-bitmap-enable': 'BlockDirtyBitmap',
> +       'block-dirty-bitmap-disable': 'BlockDirtyBitmap',
>          'blockdev-backup': 'BlockdevBackup',
>          'blockdev-snapshot': 'BlockdevSnapshot',
>          'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
> diff --git a/blockdev.c b/blockdev.c
> index 997a6f514c..d338363d78 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1962,6 +1962,7 @@ typedef struct BlockDirtyBitmapState {
>       AioContext *aio_context;
>       HBitmap *backup;
>       bool prepared;
> +    bool was_enabled;
>   } BlockDirtyBitmapState;
>   
>   static void block_dirty_bitmap_add_prepare(BlkActionState *common,
> @@ -2069,6 +2070,74 @@ static void block_dirty_bitmap_clear_clean(BlkActionState *common)
>       }
>   }
>   
> +static void block_dirty_bitmap_enable_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_enable.data;
> +    state->bitmap = block_dirty_bitmap_lookup(action->node,
> +                                              action->name,
> +                                              &state->bs,
> +                                              errp);
> +    if (!state->bitmap) {
> +        return;
> +    }
> +
> +    state->was_enabled = bdrv_dirty_bitmap_enabled(state->bitmap);
> +    bdrv_enable_dirty_bitmap(state->bitmap);
> +}
> +
> +static void block_dirty_bitmap_enable_abort(BlkActionState *common)
> +{
> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> +                                             common, common);
> +
> +    if (!state->was_enabled) {
> +        bdrv_disable_dirty_bitmap(state->bitmap);
> +    }
> +}
> +
> +static void block_dirty_bitmap_disable_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_disable.data;
> +    state->bitmap = block_dirty_bitmap_lookup(action->node,
> +                                              action->name,
> +                                              &state->bs,
> +                                              errp);
> +    if (!state->bitmap) {
> +        return;
> +    }
> +
> +    state->was_enabled = bdrv_dirty_bitmap_enabled(state->bitmap);
> +    bdrv_disable_dirty_bitmap(state->bitmap);

hm. actually, I just make it like _clear is made. But now I doubt,
why do we need disable here? we can disable in commit, and do not need
state->was_enabled..

and for _clear, we not to clear bitmap in commit? and then, we do not need
bdrv_undo_clear_dirty_bitmap at all?

or what do I miss? if nothing, I'll respin the series including _clear 
refactoring.

> +}
> +
> +static void block_dirty_bitmap_disable_abort(BlkActionState *common)
> +{
> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> +                                             common, common);
> +
> +    if (state->was_enabled) {
> +        bdrv_enable_dirty_bitmap(state->bitmap);
> +    }
> +}
> +
>   static void abort_prepare(BlkActionState *common, Error **errp)
>   {
>       error_setg(errp, "Transaction aborted using Abort action");
> @@ -2130,6 +2199,16 @@ static const BlkActionOps actions[] = {
>           .commit = block_dirty_bitmap_clear_commit,
>           .abort = block_dirty_bitmap_clear_abort,
>           .clean = block_dirty_bitmap_clear_clean,
> +    },
> +    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ENABLE] = {
> +        .instance_size = sizeof(BlockDirtyBitmapState),
> +        .prepare = block_dirty_bitmap_enable_prepare,
> +        .abort = block_dirty_bitmap_enable_abort,
> +    },
> +    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_DISABLE] = {
> +        .instance_size = sizeof(BlockDirtyBitmapState),
> +        .prepare = block_dirty_bitmap_disable_prepare,
> +        .abort = block_dirty_bitmap_disable_abort,
>       }
>   };
>   


-- 
Best regards,
Vladimir