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 <mre...@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
signature.asc
Description: OpenPGP digital signature
