From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35488) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZvowJ-00051v-2v for qemu-devel@nongnu.org; Mon, 09 Nov 2015 11:05:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZvowE-0006jS-43 for qemu-devel@nongnu.org; Mon, 09 Nov 2015 11:05:03 -0500 Date: Mon, 9 Nov 2015 17:04:43 +0100 From: Kevin Wolf Message-ID: <20151109160443.GG3621@noname.redhat.com> References: <1446663467-22485-1-git-send-email-mreitz@redhat.com> <1446663467-22485-8-git-send-email-mreitz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1446663467-22485-8-git-send-email-mreitz@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 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-blk > 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 I think this needs to be split into smaller patches: 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 > 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(-) > 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; > }; > > typedef struct BlockBackendAIOCB { > @@ -98,6 +100,8 @@ BlockBackend *blk_new(const char *name, Error **errp) > blk = g_new0(BlockBackend, 1); > blk->name = g_strdup(name); > blk->refcnt = 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(BlockBackend *blk) > */ > void blk_remove_bs(BlockBackend *blk) > { > + notifier_list_notify(&blk->remove_bs_notifiers, blk); > + > blk_update_root_state(blk); > > blk->bs->blk = NULL; > @@ -359,6 +365,8 @@ void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs) > bdrv_ref(bs); > blk->bs = bs; > bs->blk = blk; > + > + notifier_list_notify(&blk->insert_bs_notifiers, blk); > } 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()? [ Edit: I see that's what the next patch does. Good. ] Should blk_unref() also assert that the notifier list is empty? Otherwise we would be leaking notifiers. Kevin