[2/5] block: Add AioContextNotifier functions to BB

Because all BlockDriverStates behind a single BlockBackend reside in a
single AioContext, it is fine to just pass these functions
(blk_add_aio_context_notifier() and blk_remove_aio_context_notifier())
through to the root BlockDriverState.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/block-backend.c          | 18 ++++++++++++++++++
 include/sysemu/block-backend.h |  8 ++++++++
 2 files changed, 26 insertions(+)

On 17/11/2014 16:30, Max Reitz wrote:
> Because all BlockDriverStates behind a single BlockBackend reside in a
> single AioContext, it is fine to just pass these functions
> (blk_add_aio_context_notifier() and blk_remove_aio_context_notifier())
> through to the root BlockDriverState.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

The logical question then is: are there any function in BlockDriverState
that do not make sense as a BlockBackend API?

Paolo


> ---
>  block/block-backend.c          | 18 ++++++++++++++++++
>  include/sysemu/block-backend.h |  8 ++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 89f69b7..7a7f690 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -624,6 +624,24 @@ void blk_set_aio_context(BlockBackend *blk, AioContext *new_context)
>      bdrv_set_aio_context(blk->bs, new_context);
>  }
>  
> +void blk_add_aio_context_notifier(BlockBackend *blk,
> +        void (*attached_aio_context)(AioContext *new_context, void *opaque),
> +        void (*detach_aio_context)(void *opaque), void *opaque)
> +{
> +    bdrv_add_aio_context_notifier(blk->bs, attached_aio_context,
> +                                  detach_aio_context, opaque);
> +}
> +
> +void blk_remove_aio_context_notifier(BlockBackend *blk,
> +                                     void (*attached_aio_context)(AioContext *,
> +                                                                  void *),
> +                                     void (*detach_aio_context)(void *),
> +                                     void *opaque)
> +{
> +    bdrv_remove_aio_context_notifier(blk->bs, attached_aio_context,
> +                                     detach_aio_context, opaque);
> +}
> +
>  void blk_io_plug(BlockBackend *blk)
>  {
>      bdrv_io_plug(blk->bs);
> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
> index 0c46b82..d9c1337 100644
> --- a/include/sysemu/block-backend.h
> +++ b/include/sysemu/block-backend.h
> @@ -135,6 +135,14 @@ void blk_op_block_all(BlockBackend *blk, Error *reason);
>  void blk_op_unblock_all(BlockBackend *blk, Error *reason);
>  AioContext *blk_get_aio_context(BlockBackend *blk);
>  void blk_set_aio_context(BlockBackend *blk, AioContext *new_context);
> +void blk_add_aio_context_notifier(BlockBackend *blk,
> +        void (*attached_aio_context)(AioContext *new_context, void *opaque),
> +        void (*detach_aio_context)(void *opaque), void *opaque);
> +void blk_remove_aio_context_notifier(BlockBackend *blk,
> +                                     void (*attached_aio_context)(AioContext *,
> +                                                                  void *),
> +                                     void (*detach_aio_context)(void *),
> +                                     void *opaque);
>  void blk_io_plug(BlockBackend *blk);
>  void blk_io_unplug(BlockBackend *blk);
>  BlockAcctStats *blk_get_stats(BlockBackend *blk);
>

On 2014-11-17 at 18:26, Paolo Bonzini wrote:
> On 17/11/2014 16:30, Max Reitz wrote:
>> Because all BlockDriverStates behind a single BlockBackend reside in a
>> single AioContext, it is fine to just pass these functions
>> (blk_add_aio_context_notifier() and blk_remove_aio_context_notifier())
>> through to the root BlockDriverState.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> The logical question then is: are there any function in BlockDriverState
> that do not make sense as a BlockBackend API?

Well, surely bdrv_swap(), or bdrv_drop_intermediate(). These are 
functions which work on multiple BDSs (in the same tree, that is, behind 
the same BB) so they don't make sense on the BB.

Others could simply be passed through to the root BDS but it somehow 
doesn't make sense to execute them on the BB. For instance, 
bdrv_set_key(); this is something for an individual BDS, in contrast to 
other operations like reading and writing which will probably be passed 
through the BDS tree; or bdrv_get_info().

The functions added in this patch do make sense on a BB level: Since all 
BDSs behind one BB are always in the same AioContext, it makes sense to 
consider that the BB's AioContext.

The next patch is more difficult to justify. Closing a BDS is somehow 
passed through, but at first glance it doesn't make a whole lot of sense 
on the BB level. However, when you consider (as far as I looked into it) 
that a BDS is only closed when there are either no references to it 
(which will not happen as long as it has a BB) or when it is ejected, it 
suddenly does make sense: "Ejecting" really is something for the BB, so 
it makes sense to wait for that event (even though the name "close 
notifier" doesn't sound much like it...). Maybe I should sometimes take 
a deeper look into when a BDS belonging to a BB may be closed and if 
it's really only due to ejection, rename the "close notifiers" to 
something like "eject notifiers" (on the BB level).

Max

On 18/11/2014 10:26, Max Reitz wrote:
> However, when you consider (as far as I looked into it) that a BDS is
> only closed when there are either no references to it (which will not
> happen as long as it has a BB) or when it is ejected, it suddenly does
> make sense: "Ejecting" really is something for the BB, so it makes sense
> to wait for that event (even though the name "close notifier" doesn't
> sound much like it...).

I agree, and indeed the notifier was added to deal with ejection.

Thanks for clarifying.  I guess if QEMU were written in C++ we would
have a hierarchy like this:

    BlockDevice (abstract)
        BlockBackend
        BlockDriverState

and there would be no duplication of function names; the BlockBackend
implementation would still have to forward to the top BlockDriverState.

Paolo