From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58912) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z1t68-000877-Vk for qemu-devel@nongnu.org; Mon, 08 Jun 2015 05:12:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z1t65-00009k-PQ for qemu-devel@nongnu.org; Mon, 08 Jun 2015 05:12:00 -0400 Date: Mon, 8 Jun 2015 10:11:53 +0100 From: Stefan Hajnoczi Message-ID: <20150608091153.GC30104@stefanha-thinkpad.redhat.com> References: <1433339175-12300-1-git-send-email-yarygin@linux.vnet.ibm.com> <1433339175-12300-2-git-send-email-yarygin@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="ctP54qlpMx3WjD+/" Content-Disposition: inline In-Reply-To: <1433339175-12300-2-git-send-email-yarygin@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [PATCH 1/2] block-backend: Introduce blk_drain() and replace blk_drain_all() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Yarygin Cc: Kevin Wolf , qemu-block@nongnu.org, Ekaterina Tumanova , qemu-devel@nongnu.org, Christian Borntraeger , Stefan Hajnoczi , Cornelia Huck , Paolo Bonzini --ctP54qlpMx3WjD+/ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Jun 03, 2015 at 04:46:14PM +0300, Alexander Yarygin wrote: > Each call of the virtio_blk_reset() function calls blk_drain_all(), > which works for all existing BlockDriverStates, while only one > BlockDriverState needs to be drained. >=20 > This patch introduces the blk_drain() function and replaces > blk_drain_all() on it in virtio_blk_reset(). >=20 > Cc: Christian Borntraeger > Cc: Cornelia Huck > Cc: Kevin Wolf > Cc: Paolo Bonzini > Cc: Stefan Hajnoczi > Signed-off-by: Alexander Yarygin > --- > block/block-backend.c | 5 +++++ > hw/block/virtio-blk.c | 2 +- > include/sysemu/block-backend.h | 1 + > 3 files changed, 7 insertions(+), 1 deletion(-) >=20 > diff --git a/block/block-backend.c b/block/block-backend.c > index 93e46f3..aee8a12 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c > @@ -700,6 +700,11 @@ int blk_flush_all(void) > return bdrv_flush_all(); > } > =20 > +void blk_drain(BlockBackend *blk) > +{ > + bdrv_drain(blk->bs); > +} > + > void blk_drain_all(void) > { > bdrv_drain_all(); > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > index e6afe97..abaca58 100644 > --- a/hw/block/virtio-blk.c > +++ b/hw/block/virtio-blk.c > @@ -660,7 +660,7 @@ static void virtio_blk_reset(VirtIODevice *vdev) > * This should cancel pending requests, but can't do nicely until th= ere > * are per-device request lists. > */ > - blk_drain_all(); > + blk_drain(s->blk); > blk_set_enable_write_cache(s->blk, s->original_wce); > } I noticed a bug in the current code when reviewing this: If the 'backup' block job is active when the virtio-blk device is reset, bs is moved to the global AioContext while the target stays in the iothread AioContext. Accesses will be made to target without acquiring its AioContext. This is not your fault but calling blk_drain() increases the chance that this will deadlock. The bs request cannot make progress until target completes the I/O request. Since target doesn't pump events when we call aio_poll() on bs, draining bs will result in a hang if there are write requests pending on bs. I'll write a patch to address this case. Stefan --ctP54qlpMx3WjD+/ Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJVdVxZAAoJEJykq7OBq3PIjIwIAJMVKbvr/jNHYjR7rwRkQJDC mOpTCXcp7Z5oN6DDy9/FMyElu4d0fFRh2kLQX04pU5M8XSS4VydRCdxFd6QVqFX/ /U9A7raonRXTeM4XXuv4pT6n3N43jiQuICJSreDSz2mS7pNtDaIxEliZhFg6e9/9 qn4G3Q5uadYZEkRu6McZmvG+XYXeSdmZMuIDtLyW2gbBjnFBeOtBOM70PfN5HKfM Zog1Si+Bbjtx/rudHKW+mSxhr3mbriLZFgHB1EOgIPnyIP1KGKxRTmsOoe1bxtix OVhrx7uubsboc8+9PV/lRE+NT3tKx3NM6zdmQBIHdE9MtT0FY3d9n0Tmz/S0u/k= =aMj6 -----END PGP SIGNATURE----- --ctP54qlpMx3WjD+/--