From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36286) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yqi72-0006oO-Oi for qemu-devel@nongnu.org; Fri, 08 May 2015 09:14:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yqi71-0002Qm-NK for qemu-devel@nongnu.org; Fri, 08 May 2015 09:14:44 -0400 Date: Fri, 8 May 2015 14:14:32 +0100 From: Stefan Hajnoczi Message-ID: <20150508131432.GO13985@stefanha-thinkpad.redhat.com> References: <1429747493-24397-1-git-send-email-jsnow@redhat.com> <1429747493-24397-2-git-send-email-jsnow@redhat.com> <20150507145440.GN13985@stefanha-thinkpad.redhat.com> <554B9F52.9020003@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="aTCJOP0qgkSGqHWA" Content-Disposition: inline In-Reply-To: <554B9F52.9020003@redhat.com> Subject: Re: [Qemu-devel] [Qemu-block] [PATCH v3 01/10] qapi: Add transaction support to block-dirty-bitmap operations List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow Cc: famz@redhat.com, qemu-block@nongnu.org, qemu-devel@nongnu.org, mreitz@redhat.com, vsementsov@parallels.com, stefanha@redhat.com --aTCJOP0qgkSGqHWA Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, May 07, 2015 at 01:22:26PM -0400, John Snow wrote: >=20 >=20 > On 05/07/2015 10:54 AM, Stefan Hajnoczi wrote: > > On Wed, Apr 22, 2015 at 08:04:44PM -0400, John Snow wrote: > >> +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, + > >> &state->aio_context, + > >> 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() */=20 > >> +} + +static void > >> block_dirty_bitmap_clear_commit(BlkTransactionState *common) +{ + > >> BlockDirtyBitmapState *state =3D DO_UPCAST(BlockDirtyBitmapState, + > >> common, common); + bdrv_clear_dirty_bitmap(state->bitmap); +} > >=20 > > These semantics don't work in this example: > >=20 > > [block-dirty-bitmap-clear, drive-backup] > >=20 > > Since drive-backup starts the blockjob in .prepare() but=20 > > block-dirty-bitmap-clear only clears the bitmap in .commit() the > > order is wrong. > >=20 > > .prepare() has to do something non-destructive, like stashing away > > the HBitmap and replacing it with an empty one. Then .commit() can > > discard the old bitmap while .abort() can move the old bitmap back > > to undo the operation. > >=20 > > Stefan > >=20 >=20 > Hmm, that's sort of gross. That means that any transactional command > *ever* destined to be used with drive-backup in any conceivable way > needs to move a lot more of its action forward to .prepare(). >=20 > That sort of defeats the premise of .prepare() and .commit(), no? And > all because drive-backup jumped the gun. No it doesn't. Actions have to appear atomic to the qmp_transaction caller. Both approaches achieve that so they are both correct in isolation. The ambiguity is whether "commit the changes" for .commit() means "changes take effect" or "discard stashed state, making undo impossible". I think the "discard stashed state, making undo impossible" interpretation is good because .commit() is not allowed to fail. That function should only do things that never fail. > That's going to get hard to maintain as we add more transactions. Yes, we need to be consistent and stick to one of the interpretations in order to guarantee ordering. Unfortunately, there is already an inconsistency: 1. internal_snapshot - snapshot taken in .prepare() 2. external_snapshot - BDS node appended in .commit() 3. drive_backup - block job started in .prepare() 4. blockdev_backup - block job started in .prepare() external_snapshot followed by internal_snapshot acts like the reverse ordering! Stefan --aTCJOP0qgkSGqHWA Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJVTLa4AAoJEJykq7OBq3PIYmQH/2/98AjqMywz961Qs6/AYJY1 WqX8aAXEe9eW2Q03pzUchzYU7gS/4wvApR7tTpz+eGX1JJCtP046LdX3P1CAxrT9 inCREgYw6Mm+YHEGfxdq4wEwyAHPLOjAHr9r6bsCLW0aOaa/d1umPzexV2XUMf4e NWUKTK9vIzmTQ2rOBCsOUjFKk2etCBId8RO3zEwMEIKbFmaxHyPdpVLSBiHyP3Sa dP2mI5CUPB+nMgGBeLuHDIwy1a3z7lja3YH0uMgA38x7YE9/qlCxyyzwNg/21IV9 sJxVflk7iBsqgwMSbdlMr6PseZudlcbtoqKJaXDWgRH64wKKbmjywWTasNq2oVY= =uh0S -----END PGP SIGNATURE----- --aTCJOP0qgkSGqHWA--