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 <spane...@gmail.com> > > Cc: Max Reitz <mre...@redhat.com> > > Signed-off-by: Stefan Hajnoczi <stefa...@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
signature.asc
Description: PGP signature