From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48182) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xsu8u-0007ej-S0 for qemu-devel@nongnu.org; Mon, 24 Nov 2014 08:57:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xsu8o-0006To-MF for qemu-devel@nongnu.org; Mon, 24 Nov 2014 08:57:28 -0500 Received: from mx1.redhat.com ([209.132.183.28]:40676) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xsu8o-0006Sz-Er for qemu-devel@nongnu.org; Mon, 24 Nov 2014 08:57:22 -0500 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sAODvLDJ019126 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Mon, 24 Nov 2014 08:57:21 -0500 Date: Mon, 24 Nov 2014 13:57:19 +0000 From: Stefan Hajnoczi Message-ID: <20141124135719.GI4596@stefanha-thinkpad.redhat.com> References: <1416566940-4430-1-git-send-email-stefanha@redhat.com> <1416566940-4430-4-git-send-email-stefanha@redhat.com> <546F4362.3030608@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="o7gdRJTuwFmWapyH" Content-Disposition: inline In-Reply-To: <546F4362.3030608@redhat.com> Subject: Re: [Qemu-devel] [PATCH 3/4] blockdev: acquire AioContext in QMP 'transaction' actions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: Kevin Wolf , Paolo Bonzini , qemu-devel@nongnu.org --o7gdRJTuwFmWapyH Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Nov 21, 2014 at 02:51:30PM +0100, Max Reitz wrote: > On 2014-11-21 at 11:48, Stefan Hajnoczi wrote: > >The transaction QMP command performs operations atomically on a group of > >drives. This command needs to acquire AioContext in order to work > >safely when virtio-blk dataplane IOThreads are accessing drives. > > > >The transactional nature of the command means that actions are split > >into prepare, commit, abort, and clean functions. Acquire the > >AioContext in prepare and don't release it until one of the other > >functions is called. This prevents the IOThread from running the > >AioContext before the transaction has completed. > > > >Signed-off-by: Stefan Hajnoczi > >--- > > blockdev.c | 52 +++++++++++++++++++++++++++++++++= +++++++- > > hw/block/dataplane/virtio-blk.c | 2 ++ > > 2 files changed, 53 insertions(+), 1 deletion(-) > > > >diff --git a/blockdev.c b/blockdev.c > >index 0d06983..90cb33d 100644 > >--- a/blockdev.c > >+++ b/blockdev.c > >@@ -1193,6 +1193,7 @@ struct BlkTransactionState { > > typedef struct InternalSnapshotState { > > BlkTransactionState common; > > BlockDriverState *bs; > >+ AioContext *aio_context; > > QEMUSnapshotInfo sn; > > } InternalSnapshotState; > >@@ -1226,6 +1227,10 @@ static void internal_snapshot_prepare(BlkTransact= ionState *common, > > return; > > } > >+ /* AioContext is released in .clean() */ > >+ state->aio_context =3D bdrv_get_aio_context(bs); > >+ aio_context_acquire(state->aio_context); > >+ > > if (!bdrv_is_inserted(bs)) { > > error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device); > > return; > >@@ -1303,11 +1308,22 @@ static void internal_snapshot_abort(BlkTransacti= onState *common) > > } > > } > >+static void internal_snapshot_clean(BlkTransactionState *common) > >+{ > >+ InternalSnapshotState *state =3D DO_UPCAST(InternalSnapshotState, > >+ common, common); > >+ > >+ if (state->aio_context) { > >+ aio_context_release(state->aio_context); > >+ } > >+} > >+ > > /* external snapshot private data */ > > typedef struct ExternalSnapshotState { > > BlkTransactionState common; > > BlockDriverState *old_bs; > > BlockDriverState *new_bs; > >+ AioContext *aio_context; > > } ExternalSnapshotState; > > static void external_snapshot_prepare(BlkTransactionState *common, > >@@ -1374,6 +1390,10 @@ static void external_snapshot_prepare(BlkTransact= ionState *common, > > return; > > } > >+ /* Acquire AioContext now so any threads operating on old_bs stop */ >=20 > Any reason why this comment differs so much from the one for internal > snapshots? >=20 > >+ state->aio_context =3D bdrv_get_aio_context(state->old_bs); > >+ aio_context_acquire(state->aio_context); > >+ > > if (!bdrv_is_inserted(state->old_bs)) { > > error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device); > > return; > >@@ -1432,6 +1452,8 @@ static void external_snapshot_commit(BlkTransactio= nState *common) > > ExternalSnapshotState *state =3D > > DO_UPCAST(ExternalSnapshotState, common, = common); > >+ bdrv_set_aio_context(state->new_bs, state->aio_context); > >+ > > /* This removes our old bs and adds the new bs */ > > bdrv_append(state->new_bs, state->old_bs); > > /* We don't need (or want) to use the transactional > >@@ -1439,6 +1461,8 @@ static void external_snapshot_commit(BlkTransactio= nState *common) > > * don't want to abort all of them if one of them fails the reopen= */ > > bdrv_reopen(state->new_bs, state->new_bs->open_flags & ~BDRV_O_RDW= R, > > NULL); > >+ > >+ aio_context_release(state->aio_context); > > } > > static void external_snapshot_abort(BlkTransactionState *common) > >@@ -1448,23 +1472,38 @@ static void external_snapshot_abort(BlkTransacti= onState *common) > > if (state->new_bs) { > > bdrv_unref(state->new_bs); > > } > >+ if (state->aio_context) { > >+ aio_context_release(state->aio_context); > >+ } > > } >=20 > It does work this way, but I would have gone for adding > external_snapshot_clean() here, too. This is why the comment differs :). Since we already have these two functions I didn't add a separate .clean(). It could be done either way but unless anyone feels strongly about it, I'd like to do it like this. --o7gdRJTuwFmWapyH Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJUczk/AAoJEJykq7OBq3PIbuUIAJCe2CU1WrfY2rXSun8590Vl JXZv+d/d9r3otRllP9chcfm1+awWBhn9CIBF1tlDX7EEgKr0rmGTl0S/tvjmI/bU HXyCDYu/TIebTOJZBBnxh1r/rEGQOcaHlFOseHm9F5MVn1dgnfFGCqcg6H7LZAyf b+bvQQZiQnM8csmAWll6Lf+iwiAmohbWtWbFs0S0AelXuZofgsWiACHXr4MZkcoB piWSJLlbHRyf/WWc4dZPp/efjGlFgZsXFKi0hUcaYS8aFX8qujjF9djRJSjXgcK9 L7HS67NbGIkYW9ezySvm0wpn5HIPII/vjpF3yiHz7wlNmjaZGnGuqU59S/RK4BU= =g1Ur -----END PGP SIGNATURE----- --o7gdRJTuwFmWapyH--