From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53931) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zvpml-0006vS-4o for qemu-devel@nongnu.org; Mon, 09 Nov 2015 11:59:21 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zvpmk-00057e-0o for qemu-devel@nongnu.org; Mon, 09 Nov 2015 11:59:15 -0500 Date: Mon, 9 Nov 2015 17:59:04 +0100 From: Kevin Wolf Message-ID: <20151109165904.GM3621@noname.redhat.com> References: <1446663467-22485-1-git-send-email-mreitz@redhat.com> <1446663467-22485-8-git-send-email-mreitz@redhat.com> <20151109160443.GG3621@noname.redhat.com> <5640CEEE.80609@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="RnlQjJ0d97Da+TV1" Content-Disposition: inline In-Reply-To: <5640CEEE.80609@redhat.com> Subject: Re: [Qemu-devel] [PATCH v6 07/15] block: Move BDS close notifiers into BB List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: Alberto Garcia , qemu-block@nongnu.org, qemu-devel@nongnu.org, Markus Armbruster , Stefan Hajnoczi , Paolo Bonzini --RnlQjJ0d97Da+TV1 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Am 09.11.2015 um 17:50 hat Max Reitz geschrieben: > On 09.11.2015 17:04, Kevin Wolf wrote: > > Am 04.11.2015 um 19:57 hat Max Reitz geschrieben: > >> The only remaining user of the BDS close notifiers is NBD which uses > >> them to determine when a BDS tree is being ejected. This patch removes > >> the BDS-level close notifiers and adds a notifier list to the > >> BlockBackend structure that is invoked whenever a BDS is removed. > >> > >> Symmetrically to that, another notifier list is added that is invoked > >> whenever a BDS is inserted. The dataplane implementations for virtio-b= lk > >> and virtio-scsi use both notifier types for setting up and removing op > >> blockers. This is not only important for setting up the op blockers on > >> insertion, but also for removing them on ejection since bdrv_delete() > >> asserts that there are no op blockers set up. > >> > >> Signed-off-by: Max Reitz > >=20 > > I think this needs to be split into smaller patches: > >=20 > > 1. Add the new BlockBackend notifiers > > 2. Use them in virtio-blk in order to fix... removable virtio-blk > > devices, or what is it? > > 3. Convert NBD > > 4. Remove old close notifiers >=20 > I'll do my best. >=20 > >> block.c | 7 ---- > >> block/block-backend.c | 19 +++++++--- > >> blockdev-nbd.c | 37 +------------------- > >> hw/block/dataplane/virtio-blk.c | 77 +++++++++++++++++++++++++++++++-= --------- > >> hw/scsi/virtio-scsi.c | 59 +++++++++++++++++++++++++++++++ > >> include/block/block.h | 1 - > >> include/block/block_int.h | 2 -- > >> include/hw/virtio/virtio-scsi.h | 10 ++++++ > >> include/sysemu/block-backend.h | 3 +- > >> nbd.c | 13 +++++++ > >> 10 files changed, 159 insertions(+), 69 deletions(-) > >=20 > >> diff --git a/block/block-backend.c b/block/block-backend.c > >> index 6f9309f..38580f7 100644 > >> --- a/block/block-backend.c > >> +++ b/block/block-backend.c > >> @@ -48,6 +48,8 @@ struct BlockBackend { > >> BlockdevOnError on_read_error, on_write_error; > >> bool iostatus_enabled; > >> BlockDeviceIoStatus iostatus; > >> + > >> + NotifierList remove_bs_notifiers, insert_bs_notifiers; > >> }; > >> =20 > >> typedef struct BlockBackendAIOCB { > >> @@ -98,6 +100,8 @@ BlockBackend *blk_new(const char *name, Error **err= p) > >> blk =3D g_new0(BlockBackend, 1); > >> blk->name =3D g_strdup(name); > >> blk->refcnt =3D 1; > >> + notifier_list_init(&blk->remove_bs_notifiers); > >> + notifier_list_init(&blk->insert_bs_notifiers); > >> QTAILQ_INSERT_TAIL(&blk_backends, blk, link); > >> return blk; > >> } > >> @@ -343,6 +347,8 @@ void blk_hide_on_behalf_of_hmp_drive_del(BlockBack= end *blk) > >> */ > >> void blk_remove_bs(BlockBackend *blk) > >> { > >> + notifier_list_notify(&blk->remove_bs_notifiers, blk); > >> + > >> blk_update_root_state(blk); > >> =20 > >> blk->bs->blk =3D NULL; > >> @@ -359,6 +365,8 @@ void blk_insert_bs(BlockBackend *blk, BlockDriverS= tate *bs) > >> bdrv_ref(bs); > >> blk->bs =3D bs; > >> bs->blk =3D blk; > >> + > >> + notifier_list_notify(&blk->insert_bs_notifiers, blk); > >> } > >=20 > > Do we want to notify on BB deletion, too? It's also some kind of removal > > of a connection between BB and BDS. In other words, should blk_delete() > > call blk_remove_bs() rather than bdrv_unref()? > >=20 > > [ Edit: I see that's what the next patch does. Good. ] > >=20 > > Should blk_unref() also assert that the notifier list is empty? > > Otherwise we would be leaking notifiers. >=20 > You mean blk_delete()? I can do that, yes. Yes, sorry, that's what I meant. Kevin --RnlQjJ0d97Da+TV1 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJWQNDYAAoJEH8JsnLIjy/WDf0P/jGjxLaVlLQBunDjlRXHI5No rqKKYKz7LIZKA6Xwu1m2oDoGFno+OyXpcMmSlxLh4YDaMnlcC/VCN8QTQBJIPKEm pQcPhS7uFXWmtrTh51HaXrnGxgg62fYXBC1EP7TVPVZp8ZRaqOW1exwBe+7DKRPF EuWLxykmKfeq/hpSolW7kqpozz81P6JeUQ4BFTbxxHUGdk9gfgv7FxkLaOxxLWma lcX0Ka8Ff2KB9hduIUooC2OUHQ7IiDJ+UWtkth3LfymY1ysXSKfOdNs0/CyghVbE VTGBSaZI7KSXn4vruWckj0cNG4ukAQhUKGEfVTkfJPUKGE5jMRre2ZYx1xKVtRij Gj9ikdsHjPT7EfKhGDhlNDyB8c+eVET1iOKQJs1SDbCu1B2dwwHvhDaZaOcv3hza Egz2lfmDhKmg80lH2WLLFULGAHzggxOltTHVXSIfZ/MXACpTnXZArUXPlUTHz4ou l4nHMnPZOxT98WiClgfLlZwH1XEf6BIg8a6jGtcA3T5Fc26C+16YFAgQRUYcirP0 fR87INpABR9dxw5mpk5Y3AQ1MMX2eZyXXmRIZyW1wAYURUSXvChXXPZH5zSiyhz2 FB2x02bEFg6MAtv5bGr8Buy5X+78gKVhI6XyL7Mul6Gkew/drff8IlcN2aPqx2HE 34ZjAqV0EK7unciXsrbU =UHte -----END PGP SIGNATURE----- --RnlQjJ0d97Da+TV1--