Re: [Qemu-devel] [PATCH v13 10/17] qapi: Add transaction support to block-dirty-bitmap operations

[Max Reitz <mreitz@redhat.com>]

On 2015-02-13 at 17:08, John Snow wrote:
> This adds four 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.
>
> For starting a new incremental backup chain, users can also chain
> together a bitmap clear and a full block backup.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   blockdev.c       | 170 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   qapi-schema.json |  10 +++-
>   2 files changed, 179 insertions(+), 1 deletion(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 8422e94..20fea62 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1675,6 +1675,153 @@ static void blockdev_backup_clean(BlkTransactionState *common)
>       }
>   }
>   
> +typedef struct BlockDirtyBitmapState {
> +    BlkTransactionState common;
> +    BdrvDirtyBitmap *bitmap;
> +    BlockDriverState *bs;
> +    AioContext *aio_context;
> +} BlockDirtyBitmapState;
> +
> +static void block_dirty_bitmap_add_prepare(BlkTransactionState *common,
> +                                           Error **errp)
> +{
> +    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,
> +                               errp);
> +
> +    state->bitmap = block_dirty_bitmap_lookup(action->node, action->name,
> +                                              NULL, NULL);

If the bitmap has been added successfully, this will succeed. If the 
bitmap has not been added successfully because a bitmap with the same 
name already exists, this will succeed, too. Therefore, this fails in 
one case of failure. See [1].

> +}
> +
> +static void block_dirty_bitmap_add_abort(BlkTransactionState *common)
> +{
> +    BlockDirtyBitmapAdd *action;
> +   BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,

Indentation is one space off.

> +                                             common, common);
> +
> +    action = common->action->block_dirty_bitmap_add;
> +    /* Should not fail meaningfully: IF the bitmap was added via .prepare(),
> +     * then the node reference and bitmap name must have been valid.
> +     * THUS: any failure here could only indicate the lack of a bitmap at all.
> +     * We will use the presence of the bitmap field to check that we succeeded
> +     * in the first place, though.
> +     */
> +    if (state->bitmap) {
> +        qmp_block_dirty_bitmap_remove(action->node, action->name, NULL);

[1] As said above, state->bitmap will be non-NULL if the bitmap existed 
before. Thus, this may remove a pre-existing bitmap which has not been 
added by this transaction. Case in point:

$ x86_64-softmmu/qemu-system-x86_64 -drive 
if=none,id=drv,file=test.qcow2 -qmp stdio -machine accel=qtest -display 
none -nodefaults
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 2}, 
"package": ""}, "capabilities": []}}
{'execute':'qmp_capabilities'}
{"return": {}}
{'execute':'query-block'}
{"return": [{"device": "drv", "locked": false, "removable": true, 
"inserted": {"iops_rd": 0, "detect_zeroes": "off", "image": 
{"virtual-size": 67108864, "filename": "test.qcow2", "cluster-size": 
65536, "format": "qcow2", "actual-size": 200704, "format-specific": 
{"type": "qcow2", "data": {"compat": "1.1", "lazy-refcounts": false, 
"corrupt": false}}, "dirty-flag": false}, "iops_wr": 0, "ro": false, 
"backing_file_depth": 0, "drv": "qcow2", "iops": 0, "bps_wr": 0, 
"write_threshold": 0, "encrypted": false, "bps": 0, "bps_rd": 0, 
"cache": {"no-flush": false, "direct": false, "writeback": true}, 
"file": "test.qcow2", "encryption_key_missing": false}, "tray_open": 
false, "type": "unknown"}]}
{'execute':'transaction','arguments':{'actions':[{'type':'block-dirty-bitmap-add','data':{'node':'drv','name':'foo'}}]}}
{"return": {}}
{'execute':'query-block'}
{"return": [{"device": "drv", "locked": false, "removable": true, 
"inserted": {"iops_rd": 0, "detect_zeroes": "off", "image": 
{"virtual-size": 67108864, "filename": "test.qcow2", "cluster-size": 
65536, "format": "qcow2", "actual-size": 200704, "format-specific": 
{"type": "qcow2", "data": {"compat": "1.1", "lazy-refcounts": false, 
"corrupt": false}}, "dirty-flag": false}, "iops_wr": 0, "ro": false, 
"backing_file_depth": 0, "drv": "qcow2", "iops": 0, "bps_wr": 0, 
"write_threshold": 0, "encrypted": false, "bps": 0, "bps_rd": 0, 
"cache": {"no-flush": false, "direct": false, "writeback": true}, 
"file": "test.qcow2", "encryption_key_missing": false}, "tray_open": 
false, "dirty-bitmaps": [{"name": "foo", "granularity": 65536, "count": 
0}], "type": "unknown"}]}
{'execute':'transaction','arguments':{'actions':[{'type':'block-dirty-bitmap-add','data':{'node':'drv','name':'foo'}}]}}
{"error": {"class": "GenericError", "desc": "Bitmap already exists: foo"}}
{'execute':'query-block'}
{"return": [{"device": "drv", "locked": false, "removable": true, 
"inserted": {"iops_rd": 0, "detect_zeroes": "off", "image": 
{"virtual-size": 67108864, "filename": "test.qcow2", "cluster-size": 
65536, "format": "qcow2", "actual-size": 200704, "format-specific": 
{"type": "qcow2", "data": {"compat": "1.1", "lazy-refcounts": false, 
"corrupt": false}}, "dirty-flag": false}, "iops_wr": 0, "ro": false, 
"backing_file_depth": 0, "drv": "qcow2", "iops": 0, "bps_wr": 0, 
"write_threshold": 0, "encrypted": false, "bps": 0, "bps_rd": 0, 
"cache": {"no-flush": false, "direct": false, "writeback": true}, 
"file": "test.qcow2", "encryption_key_missing": false}, "tray_open": 
false, "type": "unknown"}]}

No dirty-bitmaps any more for drv.

(You'll want to heed [2] first before trying this, or you'll get a 
segfault in the very first transaction)

I see two ways to solve this: As I said, you are free to call 
qmp_block_dirty_bitmap_remove() even if creating the bitmap failed; 
there is only one case where you must not call that function, and that 
is if the bitmap existed before the transaction. In that case, you 
should be calling block_dirty_bitmap_lookup() before 
qmp_block_dirty_bitmap_add(), and if it succeeds you should not be 
calling qmp_block_dirty_bitmap_remove(); if it fails you can call 
qmp_block_dirty_bitmap_remove() (although I personally find that ugly 
because you may be calling that function knowing that it will fail).

The other way is to evaluate the error returned by 
qmp_block_dirty_bitmap_add() (pass it to a local Error pointer, remember 
whether that was non-NULL, propagate it to errp). If there was an error, 
do not call qmp_block_dirty_bitmap_remove() in 
block_dirtybitmap_add_abort().

Max

> +    }
> +}
> +
> +/**
> + * Enable and Disable re-use the same preparation.
> + */
> +static void block_dirty_bitmap_toggle_prepare(BlkTransactionState *common,
> +                                              Error **errp)
> +{
> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> +                                             common, common);
> +    BlockDirtyBitmap *action;
> +    BlockDriverState *bs;
> +
> +    /* We may be used by either enable or disable;
> +     * We use the "enable" member of the union here,
> +     * but "disable" should be functionally equivalent: */
> +    action = common->action->block_dirty_bitmap_enable;
> +    assert(action == common->action->block_dirty_bitmap_disable);
> +
> +    state->bitmap = block_dirty_bitmap_lookup(action->node,
> +                                              action->name,
> +                                              &bs,
> +                                              errp);
> +    if (!state->bitmap) {
> +        return;
> +    }
> +
> +    if (bdrv_dirty_bitmap_frozen(state->bitmap)) {
> +        error_setg(errp, "Cannot modify a frozen bitmap");
> +        return;
> +    }
> +
> +    /* AioContext is released in .clean() */
> +    state->aio_context = bdrv_get_aio_context(bs);
> +    aio_context_acquire(state->aio_context);
> +}
> +
> +static void block_dirty_bitmap_enable_commit(BlkTransactionState *common)
> +{
> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> +                                             common, common);
> +    bdrv_enable_dirty_bitmap(state->bitmap);
> +}
> +
> +static void block_dirty_bitmap_disable_commit(BlkTransactionState *common)
> +{
> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> +                                             common, common);
> +    bdrv_disable_dirty_bitmap(state->bitmap);
> +}
> +
> +static void block_dirty_bitmap_toggle_clean(BlkTransactionState *common)
> +{
> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> +                                             common, common);
> +
> +    if (state->aio_context) {
> +        aio_context_release(state->aio_context);
> +    }
> +}
> +
> +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);
> +    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);
> +}
> +
> +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");
> @@ -1715,6 +1862,29 @@ static const BdrvActionOps actions[] = {
>           .abort = internal_snapshot_abort,
>           .clean = internal_snapshot_clean,
>       },
> +    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ADD] = {
> +        .instance_size = sizeof(BlkTransactionState),

[2] This should be sizeof(BlockDirtyBitmapState).

> +        .prepare = block_dirty_bitmap_add_prepare,
> +        .abort = block_dirty_bitmap_add_abort,
> +    },
> +    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ENABLE] = {
> +        .instance_size = sizeof(BlockDirtyBitmapState),
> +        .prepare = block_dirty_bitmap_toggle_prepare,
> +        .commit = block_dirty_bitmap_enable_commit,
> +        .clean = block_dirty_bitmap_toggle_clean,
> +    },
> +    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_DISABLE] = {
> +        .instance_size = sizeof(BlockDirtyBitmapState),
> +        .prepare = block_dirty_bitmap_toggle_prepare,
> +        .commit = block_dirty_bitmap_disable_commit,
> +        .clean = block_dirty_bitmap_toggle_clean,
> +    },
> +    [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..0c31203 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1332,6 +1332,10 @@
>   # 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-enable since 2.3
> +# block-dirty-bitmap-disable since 2.3
> +# block-dirty-bitmap-clear since 2.3
>   ##
>   { 'union': 'TransactionAction',
>     'data': {
> @@ -1339,7 +1343,11 @@
>          '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-enable': 'BlockDirtyBitmap',
> +       'block-dirty-bitmap-disable': 'BlockDirtyBitmap',
> +       'block-dirty-bitmap-clear': 'BlockDirtyBitmap'
>      } }
>   
>   ##