Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] block: let blk_add/remove_aio_context_notifier() tolerate BDS changes

[Stefan Hajnoczi <stefanha@gmail.com>]

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

On Fri, Mar 09, 2018 at 09:56:44AM -0600, Eric Blake wrote:
> On 03/06/2018 02:48 PM, Stefan Hajnoczi wrote:
> > Commit 2019ba0a0197 ("block: Add AioContextNotifier functions to BB")
> > added blk_add/remove_aio_context_notifier() and implemented them by
> > passing through the bdrv_*() equivalent.
> > 
> > This doesn't work across bdrv_append(), which detaches child->bs and
> > re-attaches it to a new BlockDriverState.  When
> > blk_remove_aio_context_notifier() is called we will access the new BDS
> > instead of the one where the notifier was added!
> > 
> > > From the point of view of the blk_*() API user, changes to the root BDS
> > should be transparent.
> > 
> > This patch maintains a list of AioContext notifiers in BlockBackend and
> > adds/removes them from the BlockDriverState as needed.
> > 
> > Reported-by: Stefano Panella <spanella@gmail.com>
> > Cc: Max Reitz <mreitz@redhat.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >   block/block-backend.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >   block/trace-events    |  2 ++
> >   2 files changed, 65 insertions(+)
> > 
> > diff --git a/block/block-backend.c b/block/block-backend.c
> > index 94ffbb6a60..aa27698820 100644
> > --- a/block/block-backend.c
> > +++ b/block/block-backend.c
> > @@ -31,6 +31,13 @@
> >   static AioContext *blk_aiocb_get_aio_context(BlockAIOCB *acb);
> > +typedef struct BlockBackendAioNotifier {
> > +    void (*attached_aio_context)(AioContext *new_context, void *opaque);
> > +    void (*detach_aio_context)(void *opaque);
> 
> Why the difference in tense (past 'attached' vs. present 'detach')?

The naming comes from the bdrv_add_aio_context_notifier() API:

  void bdrv_add_aio_context_notifier(BlockDriverState *bs,
        void (*attached_aio_context)(AioContext *new_context, void *opaque),
        void (*detach_aio_context)(void *opaque), void *opaque)

It's "attached" because bs->aio_context has already been assigned before
the callback is invoked.

It's "detach" because the callback is invoked before bs->aio_context is
cleared.

Not great naming and I found it weird when I looked at the code too, but
at least this patch keeps the BlockBackend naming consistent with the
BlockDriverState naming.

> > @@ -1827,12 +1877,25 @@ void blk_remove_aio_context_notifier(BlockBackend *blk,
> >                                        void (*detach_aio_context)(void *),
> >                                        void *opaque)
> >   {
> > +    BlockBackendAioNotifier *notifier;
> >       BlockDriverState *bs = blk_bs(blk);
> >       if (bs) {
> >           bdrv_remove_aio_context_notifier(bs, attached_aio_context,
> >                                            detach_aio_context, opaque);
> >       }
> > +
> > +    QLIST_FOREACH(notifier, &blk->aio_notifiers, list) {
> > +        if (notifier->attached_aio_context == attached_aio_context &&
> > +            notifier->detach_aio_context == detach_aio_context &&
> > +            notifier->opaque == opaque) {
> > +            QLIST_REMOVE(notifier, list);
> 
> Don't you need to use QLIST_FOREACH_SAFE if you are going to modify the list
> during traversal?

It doesn't matter since we return right away:

  g_free(notifier);
  return;

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]