From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48635) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YNTIL-00035r-LD for qemu-devel@nongnu.org; Mon, 16 Feb 2015 16:33:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YNTII-000454-57 for qemu-devel@nongnu.org; Mon, 16 Feb 2015 16:33:33 -0500 Received: from mx1.redhat.com ([209.132.183.28]:50417) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YNTIH-00044x-SF for qemu-devel@nongnu.org; Mon, 16 Feb 2015 16:33:30 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t1GLXTTC012736 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Mon, 16 Feb 2015 16:33:29 -0500 Message-ID: <54E26228.6000907@redhat.com> Date: Mon, 16 Feb 2015 16:33:28 -0500 From: John Snow MIME-Version: 1.0 References: <1423865338-8576-1-git-send-email-jsnow@redhat.com> <1423865338-8576-11-git-send-email-jsnow@redhat.com> <54E25306.80503@redhat.com> In-Reply-To: <54E25306.80503@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable 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: Max Reitz , qemu-devel@nongnu.org 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 >> 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 *commo= n, >> + Error **errp) >> +{ >> + BlockDirtyBitmapAdd *action; >> + BlockDirtyBitmapState *state =3D DO_UPCAST(BlockDirtyBitmapState, >> + common, common); >> + >> + action =3D 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 =3D 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=20 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 =3D DO_UPCAST(BlockDirtyBitmapState, > > Indentation is one space off. > >> + common, common); >> + >> + action =3D 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, NUL= L); > > [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=3Dnone,id=3Ddrv,file=3Dtest.qcow2 -qmp stdio -machine accel=3Dqtest = -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-b= itmap-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-b= itmap-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, remembe= r > 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 =3D 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 =3D common->action->block_dirty_bitmap_enable; >> + assert(action =3D=3D common->action->block_dirty_bitmap_disable); >> + >> + state->bitmap =3D 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 =3D bdrv_get_aio_context(bs); >> + aio_context_acquire(state->aio_context); >> +} >> + >> +static void block_dirty_bitmap_enable_commit(BlkTransactionState >> *common) >> +{ >> + BlockDirtyBitmapState *state =3D DO_UPCAST(BlockDirtyBitmapState, >> + common, common); >> + bdrv_enable_dirty_bitmap(state->bitmap); >> +} >> + >> +static void block_dirty_bitmap_disable_commit(BlkTransactionState >> *common) >> +{ >> + BlockDirtyBitmapState *state =3D DO_UPCAST(BlockDirtyBitmapState, >> + common, common); >> + bdrv_disable_dirty_bitmap(state->bitmap); >> +} >> + >> +static void block_dirty_bitmap_toggle_clean(BlkTransactionState *comm= on) >> +{ >> + BlockDirtyBitmapState *state =3D 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 =3D DO_UPCAST(BlockDirtyBitmapState, >> + common, common); >> + BlockDirtyBitmap *action; >> + >> + action =3D common->action->block_dirty_bitmap_clear; >> + state->bitmap =3D 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 =3D bdrv_get_aio_context(state->bs); >> + aio_context_acquire(state->aio_context); >> +} >> + >> +static void block_dirty_bitmap_clear_commit(BlkTransactionState *comm= on) >> +{ >> + BlockDirtyBitmapState *state =3D DO_UPCAST(BlockDirtyBitmapState, >> + common, common); >> + bdrv_clear_dirty_bitmap(state->bitmap); >> +} >> + >> +static void block_dirty_bitmap_clear_clean(BlkTransactionState *commo= n) >> +{ >> + BlockDirtyBitmapState *state =3D 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[] =3D { >> .abort =3D internal_snapshot_abort, >> .clean =3D internal_snapshot_clean, >> }, >> + [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ADD] =3D { >> + .instance_size =3D sizeof(BlkTransactionState), > > [2] This should be sizeof(BlockDirtyBitmapState). > >> + .prepare =3D block_dirty_bitmap_add_prepare, >> + .abort =3D block_dirty_bitmap_add_abort, >> + }, >> + [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ENABLE] =3D { >> + .instance_size =3D sizeof(BlockDirtyBitmapState), >> + .prepare =3D block_dirty_bitmap_toggle_prepare, >> + .commit =3D block_dirty_bitmap_enable_commit, >> + .clean =3D block_dirty_bitmap_toggle_clean, >> + }, >> + [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_DISABLE] =3D { >> + .instance_size =3D sizeof(BlockDirtyBitmapState), >> + .prepare =3D block_dirty_bitmap_toggle_prepare, >> + .commit =3D block_dirty_bitmap_disable_commit, >> + .clean =3D block_dirty_bitmap_toggle_clean, >> + }, >> + [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_CLEAR] =3D { >> + .instance_size =3D sizeof(BlockDirtyBitmapState), >> + .prepare =3D block_dirty_bitmap_clear_prepare, >> + .commit =3D block_dirty_bitmap_clear_commit, >> + .clean =3D 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' >> } } >> ## > > --=20 =E2=80=94js