From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36987) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YNSHt-0005AO-Bx for qemu-devel@nongnu.org; Mon, 16 Feb 2015 15:29:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YNSHp-0002ho-2O for qemu-devel@nongnu.org; Mon, 16 Feb 2015 15:29:01 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36888) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YNSHo-0002hd-P3 for qemu-devel@nongnu.org; Mon, 16 Feb 2015 15:28:57 -0500 Message-ID: <54E25306.80503@redhat.com> Date: Mon, 16 Feb 2015 15:28:54 -0500 From: Max Reitz MIME-Version: 1.0 References: <1423865338-8576-1-git-send-email-jsnow@redhat.com> <1423865338-8576-11-git-send-email-jsnow@redhat.com> In-Reply-To: <1423865338-8576-11-git-send-email-jsnow@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v13 10/17] qapi: Add transaction support to block-dirty-bitmap operations List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow , qemu-devel@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, armbru@redhat.com, vsementsov@parallels.com, stefanha@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 > Signed-off-by: John Snow > --- > 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' > } } > > ##