Re: [Qemu-devel] [PATCH 01/11] qapi: Add transaction support to block-dirty-bitmap operations

[Max Reitz]

On 2015-03-04 at 23:15, John Snow wrote:
> This adds two qmp commands to transactions.
>
> block-dirty-bitmap-add allows you to create a bitmap simultaneously
> alongside a new full backup to accomplish a clean synchronization
> point.
>
> block-dirty-bitmap-clear allows you to reset a bitmap back to as-if
> it were new, which can also be used alongside a full backup to
> accomplish a clean synchronization point.
>
> Signed-off-by: Fam Zheng <address@hidden>
> Signed-off-by: John Snow <address@hidden>
> ---
>   blockdev.c       | 101 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   qapi-schema.json |   6 +++-
>   2 files changed, 106 insertions(+), 1 deletion(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index e0671be..5120af1 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1664,6 +1664,96 @@ static void blockdev_backup_clean(BlkTransactionState 
> *common)
>       }
>   }
>   
> +typedef struct BlockDirtyBitmapState {
> +    BlkTransactionState common;
> +    BdrvDirtyBitmap *bitmap;
> +    BlockDriverState *bs;
> +    AioContext *aio_context;
> +    bool prepared;
> +} BlockDirtyBitmapState;
> +
> +static void block_dirty_bitmap_add_prepare(BlkTransactionState *common,
> +                                           Error **errp)
> +{
> +    Error *local_err = NULL;
> +    BlockDirtyBitmapAdd *action;
> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> +                                             common, common);
> +
> +    action = common->action->block_dirty_bitmap_add;
> +    /* AIO context taken within qmp_block_dirty_bitmap_add */
> +    qmp_block_dirty_bitmap_add(action->node, action->name,
> +                               action->has_granularity, action->granularity,
> +                               &local_err);
> +
> +    if (!local_err) {
> +        state->prepared = true;
> +    } else {
> +        error_propagate(errp, local_err);
> +    }
> +}
> +
> +static void block_dirty_bitmap_add_abort(BlkTransactionState *common)
> +{
> +    BlockDirtyBitmapAdd *action;
> +   BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,

Indentation is still off.

> +                                             common, common);
> +
> +    action = common->action->block_dirty_bitmap_add;
> +    /* Should not be able to fail: IF the bitmap was added via .prepare(),
> +     * then the node reference and bitmap name must have been valid.
> +     */
> +    if (state->prepared) {
> +        qmp_block_dirty_bitmap_remove(action->node, action->name, 
> &error_abort);
> +    }
> +}
> +
> +static void block_dirty_bitmap_clear_prepare(BlkTransactionState *common,
> +                                             Error **errp)
> +{
> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> +                                             common, common);
> +    BlockDirtyBitmap *action;
> +
> +    action = common->action->block_dirty_bitmap_clear;
> +    state->bitmap = block_dirty_bitmap_lookup(action->node,
> +                                              action->name,
> +                                              &state->bs,
> +                                              errp);

This will need fix-up for v3 (and following, probably) of your 
transactionless series (&state->aio_context as the fourth parameter).

> +    if (!state->bitmap) {
> +        return;
> +    }
> +
> +    if (bdrv_dirty_bitmap_frozen(state->bitmap)) {
> +        error_setg(errp, "Cannot modify a frozen bitmap");
> +        return;
> +    } else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) {
> +        error_setg(errp, "Cannot clear a disabled bitmap");
> +        return;
> +    }
> +
> +    /* AioContext is released in .clean() */
> +    state->aio_context = bdrv_get_aio_context(state->bs);
> +    aio_context_acquire(state->aio_context);

And this will probably need to be dropped for v4 of said series.

Other than that, looks good.

Max

> +}
> +
> +static void block_dirty_bitmap_clear_commit(BlkTransactionState *common)
> +{
> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> +                                             common, common);
> +    bdrv_clear_dirty_bitmap(state->bitmap);
> +}
> +
> +static void block_dirty_bitmap_clear_clean(BlkTransactionState *common)
> +{
> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> +                                             common, common);
> +
> +    if (state->aio_context) {
> +        aio_context_release(state->aio_context);
> +    }
> +}
> +
>   static void abort_prepare(BlkTransactionState *common, Error **errp)
>   {
>       error_setg(errp, "Transaction aborted using Abort action");
> @@ -1704,6 +1794,17 @@ static const BdrvActionOps actions[] = {
>           .abort = internal_snapshot_abort,
>           .clean = internal_snapshot_clean,
>       },
> +    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ADD] = {
> +        .instance_size = sizeof(BlockDirtyBitmapState),
> +        .prepare = block_dirty_bitmap_add_prepare,
> +        .abort = block_dirty_bitmap_add_abort,
> +    },
> +    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_CLEAR] = {
> +        .instance_size = sizeof(BlockDirtyBitmapState),
> +        .prepare = block_dirty_bitmap_clear_prepare,
> +        .commit = block_dirty_bitmap_clear_commit,
> +        .clean = block_dirty_bitmap_clear_clean,
> +    }
>   };
>   
>   /*
> diff --git a/qapi-schema.json b/qapi-schema.json
> index e16f8eb..12c61f3 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1332,6 +1332,8 @@
>   # abort since 1.6
>   # blockdev-snapshot-internal-sync since 1.7
>   # blockdev-backup since 2.3
> +# block-dirty-bitmap-add since 2.3
> +# block-dirty-bitmap-clear since 2.3
>   ##
>   { 'union': 'TransactionAction',
>     'data': {
> @@ -1339,7 +1341,9 @@
>          'drive-backup': 'DriveBackup',
>          'blockdev-backup': 'BlockdevBackup',
>          'abort': 'Abort',
> -       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
> +       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
> +       'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
> +       'block-dirty-bitmap-clear': 'BlockDirtyBitmap'
>      } }
>   
>   ##