Re: [Qemu-devel] [PATCH v6 08/10] qapi: Add transaction support to block-dirty-bitmap-{add, enable, disable}

[Max Reitz]

On 2014-10-30 at 04:22, Fam Zheng wrote:
> This adds three qmp commands to transactions.
>
> Users can stop a dirty bitmap, start backup of it, and start another
> dirty bitmap atomically, so that the dirty bitmap is tracked
> incrementally and we don't miss any write.
>
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>   blockdev.c       | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   qapi-schema.json |  5 ++-
>   2 files changed, 96 insertions(+), 1 deletion(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index fca909b..34fa314 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1490,6 +1490,83 @@ static void drive_backup_abort(BlkTransactionState 
> *common)
>       }
>   }
>   
> +static void block_dirty_bitmap_add_prepare(BlkTransactionState *common,
> +                                           Error **errp)
> +{
> +    BlockDirtyBitmapAdd *action;
> +
> +    action = common->action->block_dirty_bitmap_add;
> +    qmp_block_dirty_bitmap_add(action->device, action->name,
> +                               action->has_granularity, action->granularity,
> +                               errp);
> +}
> +
> +static void block_dirty_bitmap_add_abort(BlkTransactionState *common)
> +{
> +    BlockDirtyBitmapAdd *action;
> +    BdrvDirtyBitmap *bm;
> +    BlockDriverState *bs;
> +
> +    action = common->action->block_dirty_bitmap_add;
> +    bs = bdrv_find(action->device);
> +    if (bs) {
> +        bm = bdrv_find_dirty_bitmap(bs, action->name);
> +        if (bm) {
> +            bdrv_release_dirty_bitmap(bs, bm);
> +        }
> +    }
> +}
> +
> +static void block_dirty_bitmap_enable_prepare(BlkTransactionState *common,
> +                                               Error **errp)
> +{
> +    BlockDirtyBitmap *action;
> +
> +    action = common->action->block_dirty_bitmap_enable;
> +    qmp_block_dirty_bitmap_enable(action->device, action->name, errp);
> +}
> +
> +static void block_dirty_bitmap_enable_abort(BlkTransactionState *common)
> +{
> +    BlockDirtyBitmap *action;
> +    BdrvDirtyBitmap *bitmap;
> +    BlockDriverState *bs;
> +
> +    action = common->action->block_dirty_bitmap_enable;
> +    bs = bdrv_find(action->device);
> +    if (bs) {
> +        bitmap = bdrv_find_dirty_bitmap(bs, action->name);
> +        if (bitmap) {
> +            bdrv_disable_dirty_bitmap(bs, bitmap);

But what if the dirty bitmap was enabled before so that this enabling 
transaction was supposed to be a no-op?

> +        }
> +    }
> +}
> +
> +static void block_dirty_bitmap_disable_prepare(BlkTransactionState *common,
> +                                               Error **errp)
> +{
> +    BlockDirtyBitmap *action;
> +
> +    action = common->action->block_dirty_bitmap_disable;
> +    qmp_block_dirty_bitmap_disable(action->device, action->name, errp);
> +}
> +
> +static void block_dirty_bitmap_disable_abort(BlkTransactionState *common)
> +{
> +    BlockDirtyBitmap *action;
> +    BdrvDirtyBitmap *bitmap;
> +    BlockDriverState *bs;
> +
> +    action = common->action->block_dirty_bitmap_disable;
> +    bs = bdrv_find(action->device);
> +    if (bs) {
> +        bitmap = bdrv_find_dirty_bitmap(bs, action->name);
> +        if (bitmap) {
> +            bdrv_enable_dirty_bitmap(bs, bitmap);

Same here, the other way round.

> +        }
> +    }
> +}
> +
>   static void abort_prepare(BlkTransactionState *common, Error **errp)
>   {
>       error_setg(errp, "Transaction aborted using Abort action");
> @@ -1522,6 +1599,21 @@ static const BdrvActionOps actions[] = {
>           .prepare  = internal_snapshot_prepare,
>           .abort = internal_snapshot_abort,
>       },
> +    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ADD] = {
> +        .instance_size = sizeof(BlkTransactionState),
> +        .prepare = block_dirty_bitmap_add_prepare,
> +        .abort = block_dirty_bitmap_add_abort,
> +    },
> +    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ENABLE] = {
> +        .instance_size = sizeof(BlkTransactionState),
> +        .prepare = block_dirty_bitmap_enable_prepare,
> +        .abort = block_dirty_bitmap_enable_abort,
> +    },
> +    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_DISABLE] = {
> +        .instance_size = sizeof(BlkTransactionState),
> +        .prepare = block_dirty_bitmap_disable_prepare,
> +        .abort = block_dirty_bitmap_disable_abort,
> +    },
>   };
>   
>   /*
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 24379ab..ae07677 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1260,7 +1260,10 @@
>          'blockdev-snapshot-sync': 'BlockdevSnapshot',
>          'drive-backup': 'DriveBackup',
>          'abort': 'Abort',
> -       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
> +       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
> +       'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',

Ah, you're reusing that type here. Very well then.

Max

> +       'block-dirty-bitmap-enable': 'BlockDirtyBitmap',
> +       'block-dirty-bitmap-disable': 'BlockDirtyBitmap'
>      } }
>   
>   ##