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

[Max Reitz <mreitz@redhat.com>]

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

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.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]