From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52744) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xu2NN-0005Qx-Ll for qemu-devel@nongnu.org; Thu, 27 Nov 2014 11:57:14 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xu2NE-0008Pd-Aj for qemu-devel@nongnu.org; Thu, 27 Nov 2014 11:57:05 -0500 Received: from mail-wi0-x236.google.com ([2a00:1450:400c:c05::236]:37607) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xu2NE-0008PV-4I for qemu-devel@nongnu.org; Thu, 27 Nov 2014 11:56:56 -0500 Received: by mail-wi0-f182.google.com with SMTP id h11so8929892wiw.3 for ; Thu, 27 Nov 2014 08:56:55 -0800 (PST) Date: Thu, 27 Nov 2014 16:56:53 +0000 From: Stefan Hajnoczi Message-ID: <20141127165652.GN15586@stefanha-thinkpad.lan> References: <1416900193-3763-1-git-send-email-ming.lei@canonical.com> <1416900193-3763-2-git-send-email-ming.lei@canonical.com> <20141125130804.GE21126@stefanha-thinkpad.redhat.com> <20141125161856.GA22421@stefanha-thinkpad.redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="0kRkyLZR5zsR9u2P" Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v6 1/3] linux-aio: fix submit aio as a batch List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ming Lei Cc: Kevin Wolf , Paolo Bonzini , qemu-devel , Stefan Hajnoczi --0kRkyLZR5zsR9u2P Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Nov 26, 2014 at 05:15:44PM +0800, Ming Lei wrote: > On Wed, Nov 26, 2014 at 12:18 AM, Stefan Hajnoczi wr= ote: > >> > >> You mean the abort BH may not have chance to run before its deletion > >> in the detach callback? > > > > Exactly. Any time you schedule a BH you need to be aware of things that > > may happen before the BH is invoked. > > > >> If so, bdrv_drain_all() from bdrv_set_aio_context() should have > >> handled the pending BH, right? > > > > I'm not sure if it's good to make subtle assumptions like that. If the > > code changes they will break. >=20 > IMO, that should be the purpose of bdrv_drain_all(), at least from > its comment: >=20 > /* ensure there are no in-flight requests */ >=20 > If it changes in future, the general problem has to be considered. >=20 > > Since it is very easy to protect against this case (the code I posted > > before), it seems worthwhile to be on the safe side. >=20 > Given there hasn't the potential problem in current tree, could you > agree on merging it first? >=20 > BTW, there isn't sort of handling for 'completion_bh' of linux aio too, := -) That's incorrect, completion_bh is protected: void laio_attach_aio_context(void *s_, AioContext *new_context) { struct qemu_laio_state *s =3D s_; s->completion_bh =3D aio_bh_new(new_context, qemu_laio_completion_bh, s= ); aio_set_event_notifier(new_context, &s->e, qemu_laio_completion_cb); ^---- this will reschedule completion_bh if s->e has events pending I am asking for a one-line if statement to avoid introducing a subtle assumption that would be a pain to debug in the future. It's so easy to add that I'm against merging the patch without this protection. --0kRkyLZR5zsR9u2P Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJUd1fUAAoJEJykq7OBq3PIYkMH/Rm3GXjvU5fGrJgX2+xPGHJs MVFv5aXUoB/Po/W9c3ZjTiAAl8AciqEbiLQ3DNcHHl/m1xRDlALfoaMizGuL6v6O xAYiAVAoTes+SkEpIoHBnspizYz+UBZaQvwGEl14DUT38NULtNfuFTqwDHl/Fwzh uXqqfV3CZ0LhiJIGcUshGFPqrp8LkyjcBPz/qdstTYRAKws2qdNq8KFHzjw8x9YG tZuFmD/fAPVLpktPnuiOEHOzbjng65cH0/qLrB2eYR8ZrjLjKllgAsIvjihgoZBO JnxLhsW0zrYqpK1pE778GLJsYhrzO0OA+htm9wdO04GAMu0D3s3tS78jiKqlTl0= =i2rB -----END PGP SIGNATURE----- --0kRkyLZR5zsR9u2P--