From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49797) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bvoTx-000797-7B for qemu-devel@nongnu.org; Sun, 16 Oct 2016 12:40:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bvoTu-0006hH-4I for qemu-devel@nongnu.org; Sun, 16 Oct 2016 12:40:17 -0400 Date: Sun, 16 Oct 2016 17:40:10 +0100 From: Stefan Hajnoczi Message-ID: <20161016164010.GF2128@stefanha-x1.localdomain> References: <1476380062-18001-1-git-send-email-pbonzini@redhat.com> <1476380062-18001-16-git-send-email-pbonzini@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="MZf7D3rAEoQgPanC" Content-Disposition: inline In-Reply-To: <1476380062-18001-16-git-send-email-pbonzini@redhat.com> Subject: Re: [Qemu-devel] [Qemu-block] [PATCH 15/18] block: only call aio_poll on the current thread's AioContext List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org, kwolf@redhat.com, famz@redhat.com, stefanha@redhat.com, qemu-block@nongnu.org --MZf7D3rAEoQgPanC Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Oct 13, 2016 at 07:34:19PM +0200, Paolo Bonzini wrote: > aio_poll is not thread safe; for example bdrv_drain can hang if > the last in-flight I/O operation is completed in the I/O thread after > the main thread has checked bs->in_flight. >=20 > The bug remains latent as long as all of it is called within > aio_context_acquire/aio_context_release, but this will change soon. >=20 > To fix this, if bdrv_drain is called from outside the I/O thread, > signal the main AioContext through a dummy bottom half. The event > loop then only runs in the I/O thread. >=20 > Reviewed-by: Fam Zheng > Signed-off-by: Paolo Bonzini > --- > async.c | 1 + > block.c | 2 ++ > block/io.c | 7 +++++++ > include/block/block.h | 24 +++++++++++++++++++++--- > include/block/block_int.h | 1 + > 5 files changed, 32 insertions(+), 3 deletions(-) >=20 > diff --git a/async.c b/async.c > index f30d011..fb37b03 100644 > --- a/async.c > +++ b/async.c > @@ -61,6 +61,7 @@ void aio_bh_schedule_oneshot(AioContext *ctx, QEMUBHFun= c *cb, void *opaque) > smp_wmb(); > ctx->first_bh =3D bh; > qemu_mutex_unlock(&ctx->bh_lock); > + aio_notify(ctx); > } > =20 > QEMUBH *aio_bh_new(AioContext *ctx, QEMUBHFunc *cb, void *opaque) > diff --git a/block.c b/block.c > index fbe485c..a17baab 100644 > --- a/block.c > +++ b/block.c > @@ -2090,7 +2090,9 @@ int bdrv_reopen_multiple(AioContext *ctx, BlockReop= enQueue *bs_queue, Error **er > =20 > assert(bs_queue !=3D NULL); > =20 > + aio_context_release(ctx); > bdrv_drain_all(); > + aio_context_acquire(ctx); > =20 > QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) { > if (bdrv_reopen_prepare(&bs_entry->state, bs_queue, &local_err))= { > diff --git a/block/io.c b/block/io.c > index 7d3dcfc..cd28909 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -474,8 +474,15 @@ void bdrv_inc_in_flight(BlockDriverState *bs) > atomic_inc(&bs->in_flight); > } > =20 > +static void dummy_bh_cb(void *opaque) > +{ > +} > + > void bdrv_wakeup(BlockDriverState *bs) > { > + if (bs->wakeup) { > + aio_bh_schedule_oneshot(qemu_get_aio_context(), dummy_bh_cb, NUL= L); > + } > } Why use a dummy BH instead of aio_notify()? > =20 > void bdrv_dec_in_flight(BlockDriverState *bs) > diff --git a/include/block/block.h b/include/block/block.h > index ba4318b..72d5d8e 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -343,9 +343,27 @@ void bdrv_drain_all(void); > #define bdrv_poll_while(bs, cond) ({ \ > bool waited_ =3D false; \ > BlockDriverState *bs_ =3D (bs); \ > - while ((cond)) { \ > - aio_poll(bdrv_get_aio_context(bs_), true); \ > - waited_ =3D true; \ > + AioContext *ctx_ =3D bdrv_get_aio_context(bs_); \ > + if (aio_context_in_iothread(ctx_)) { \ > + while ((cond)) { \ > + aio_poll(ctx_, true); \ > + waited_ =3D true; \ > + } \ > + } else { \ > + assert(qemu_get_current_aio_context() =3D=3D \ > + qemu_get_aio_context()); \ The assumption is that IOThread #1 will never call bdrv_poll_while() on IOThread #2's AioContext. I believe that is true today. Is this what you had in mind? Please add a comment since it's not completely obvious from the assert expression. > + /* Ask bdrv_dec_in_flight to wake up the main \ > + * QEMU AioContext. \ > + */ \ > + assert(!bs_->wakeup); \ > + bs_->wakeup =3D true; \ > + while ((cond)) { \ > + aio_context_release(ctx_); \ > + aio_poll(qemu_get_aio_context(), true); \ > + aio_context_acquire(ctx_); \ > + waited_ =3D true; \ > + } \ > + bs_->wakeup =3D false; \ > } \ > waited_; }) > =20 > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 11f877b..0516f62 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -470,6 +470,7 @@ struct BlockDriverState { > NotifierWithReturnList before_write_notifiers; > =20 > /* number of in-flight requests; overall and serialising */ > + bool wakeup; > unsigned int in_flight; > unsigned int serialising_in_flight; > =20 > --=20 > 2.7.4 >=20 >=20 >=20 --MZf7D3rAEoQgPanC Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEcBAEBAgAGBQJYA61qAAoJEJykq7OBq3PIM84IAJtswavgv5o0bm0cDtzaPvBS F/NQhWq9+wJcWfJgX0PquIZ8y4VG/T8lBc39kVHTBGcZ9XqXT9Sjf66eGob9G7eY GnRFHz8SyLS5NJPo0gkkYgnvqkwLSjapBdp1qLloBoELeg+/cbJ8skfnYFIWP6K8 Y5c0DL4UiVMTyP4vVtnVstpshCLDTJZUI/sx7yzmjWvjCaJfN4msMjXmJP6GWhaL P21+sS8vynOGevSbw9HjzhresXxwPqdfSQQWM27IuWGUSVRe/B8pXN9JQ6LLxcQS atRiNbugDWOd7UgTErrxp75J5K/5RNrJuZO2CMKEfJ0Gqf/92q2PWIgTXchNk9I= =H0e3 -----END PGP SIGNATURE----- --MZf7D3rAEoQgPanC--