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

[John Snow <jsnow@redhat.com>]

On 02/16/2015 03:28 PM, Max Reitz wrote:
> 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].

I would have liked to have been in the room with myself when I made the 
decision that this patch would correct the behavior you spotted last time.

Surely I could have stopped myself from doing it.

>
>> +}
>> +
>> +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'
>>      } }
>>   ##
>
>

-- 
—js