From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42581) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xrwd5-0006qo-3k for qemu-devel@nongnu.org; Fri, 21 Nov 2014 17:24:45 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xrwcw-0004aV-Jq for qemu-devel@nongnu.org; Fri, 21 Nov 2014 17:24:39 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35752) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xrwcw-0004aA-D5 for qemu-devel@nongnu.org; Fri, 21 Nov 2014 17:24:30 -0500 Message-ID: <546FBB93.7000809@redhat.com> Date: Fri, 21 Nov 2014 17:24:19 -0500 From: John Snow MIME-Version: 1.0 References: <1414639364-4499-1-git-send-email-famz@redhat.com> <1414639364-4499-9-git-send-email-famz@redhat.com> <5458B279.8010300@redhat.com> In-Reply-To: <5458B279.8010300@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v6 08/10] qapi: Add transaction support to block-dirty-bitmap-{add, enable, disable} List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , Fam Zheng , qemu-devel@nongnu.org Cc: Kevin Wolf , Benoit Canet , Markus Armbruster , Luiz Capitulino , Stefan Hajnoczi , Jd , Paolo Bonzini , Vladimir Sementsov-Ogievskij On 11/04/2014 06:03 AM, Max Reitz wrote: > 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 >> --- >> 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 *commo= n, >> + Error **errp) >> +{ >> + BlockDirtyBitmapAdd *action; >> + >> + action =3D 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 =3D common->action->block_dirty_bitmap_add; >> + bs =3D bdrv_find(action->device); >> + if (bs) { >> + bm =3D 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 =3D common->action->block_dirty_bitmap_enable; >> + qmp_block_dirty_bitmap_enable(action->device, action->name, errp)= ; >> +} >> + >> +static void block_dirty_bitmap_enable_abort(BlkTransactionState *comm= on) >> +{ >> + BlockDirtyBitmap *action; >> + BdrvDirtyBitmap *bitmap; >> + BlockDriverState *bs; >> + >> + action =3D common->action->block_dirty_bitmap_enable; >> + bs =3D bdrv_find(action->device); >> + if (bs) { >> + bitmap =3D 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? Maybe it's not a problem: The only case in which the enable/disable=20 operation will fail is if it cannot find the device or bitmap -- in=20 which case, the abort operation isn't going to be able to affect=20 anything either. Unless you know something I don't. Still, it does look goofy. Thoughts? >> + } >> + } >> +} >> + >> +static void block_dirty_bitmap_disable_prepare(BlkTransactionState >> *common, >> + Error **errp) >> +{ >> + BlockDirtyBitmap *action; >> + >> + action =3D 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 =3D common->action->block_dirty_bitmap_disable; >> + bs =3D bdrv_find(action->device); >> + if (bs) { >> + bitmap =3D 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[] =3D { >> .prepare =3D internal_snapshot_prepare, >> .abort =3D internal_snapshot_abort, >> }, >> + [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ADD] =3D { >> + .instance_size =3D sizeof(BlkTransactionState), >> + .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(BlkTransactionState), >> + .prepare =3D block_dirty_bitmap_enable_prepare, >> + .abort =3D block_dirty_bitmap_enable_abort, >> + }, >> + [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_DISABLE] =3D { >> + .instance_size =3D sizeof(BlkTransactionState), >> + .prepare =3D block_dirty_bitmap_disable_prepare, >> + .abort =3D 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' >> } } >> ## > > --=20 =E2=80=94js