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

[Kevin Wolf <kwolf@redhat.com>]

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

>  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