Re: [Qemu-devel] [PATCH v6 07/15] block: Move BDS close notifiers into BB

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3797 bytes --]

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-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 <mreitz@redhat.com>
> > 
> > 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
> 
> I'll do my best.
> 
> >>  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.
> 
> You mean blk_delete()? I can do that, yes.

Yes, sorry, that's what I meant.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]