| Message ID | 20180306204819.11266-2-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: fix nbd-server-stop crash after blockdev-snapshot-sync | expand |
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')? > @@ -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? Otherwise makes sense to me.
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
On 03/12/2018 06:27 AM, Stefan Hajnoczi wrote: > 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. >>> >>> +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. Odd, but consistent, so I can live with it. >>> + >>> + 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; Okay, makes sense (a safe iteration is required if you keep iterating after action; but if you quit, the action has no impact on subsequent iteration since there is no further iteration). Reviewed-by: Eric Blake <eblake@redhat.com>
On 2018-03-06 21:48, 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! And nice that we just did not do anything if there was no BDS (in practice that can never happen, but still nice). Also, I like your exclamation mark. It makes this sound so excited! :D > 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(+) Reviewed-by: Max Reitz <mreitz@redhat.com>
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); + void *opaque; + QLIST_ENTRY(BlockBackendAioNotifier) list; +} BlockBackendAioNotifier; + struct BlockBackend { char *name; int refcnt; @@ -69,6 +76,7 @@ struct BlockBackend { bool allow_write_beyond_eof; NotifierList remove_bs_notifiers, insert_bs_notifiers; + QLIST_HEAD(, BlockBackendAioNotifier) aio_notifiers; int quiesce_counter; VMChangeStateEntry *vmsh; @@ -239,6 +247,36 @@ static int blk_root_inactivate(BdrvChild *child) return 0; } +static void blk_root_attach(BdrvChild *child) +{ + BlockBackend *blk = child->opaque; + BlockBackendAioNotifier *notifier; + + trace_blk_root_attach(child, blk, child->bs); + + QLIST_FOREACH(notifier, &blk->aio_notifiers, list) { + bdrv_add_aio_context_notifier(child->bs, + notifier->attached_aio_context, + notifier->detach_aio_context, + notifier->opaque); + } +} + +static void blk_root_detach(BdrvChild *child) +{ + BlockBackend *blk = child->opaque; + BlockBackendAioNotifier *notifier; + + trace_blk_root_detach(child, blk, child->bs); + + QLIST_FOREACH(notifier, &blk->aio_notifiers, list) { + bdrv_remove_aio_context_notifier(child->bs, + notifier->attached_aio_context, + notifier->detach_aio_context, + notifier->opaque); + } +} + static const BdrvChildRole child_root = { .inherit_options = blk_root_inherit_options, @@ -252,6 +290,9 @@ static const BdrvChildRole child_root = { .activate = blk_root_activate, .inactivate = blk_root_inactivate, + + .attach = blk_root_attach, + .detach = blk_root_detach, }; /* @@ -279,6 +320,7 @@ BlockBackend *blk_new(uint64_t perm, uint64_t shared_perm) notifier_list_init(&blk->remove_bs_notifiers); notifier_list_init(&blk->insert_bs_notifiers); + QLIST_INIT(&blk->aio_notifiers); QTAILQ_INSERT_TAIL(&block_backends, blk, link); return blk; @@ -356,6 +398,7 @@ static void blk_delete(BlockBackend *blk) } assert(QLIST_EMPTY(&blk->remove_bs_notifiers.notifiers)); assert(QLIST_EMPTY(&blk->insert_bs_notifiers.notifiers)); + assert(QLIST_EMPTY(&blk->aio_notifiers)); QTAILQ_REMOVE(&block_backends, blk, link); drive_info_del(blk->legacy_dinfo); block_acct_cleanup(&blk->stats); @@ -1813,8 +1856,15 @@ 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) { + BlockBackendAioNotifier *notifier; BlockDriverState *bs = blk_bs(blk); + notifier = g_new(BlockBackendAioNotifier, 1); + notifier->attached_aio_context = attached_aio_context; + notifier->detach_aio_context = detach_aio_context; + notifier->opaque = opaque; + QLIST_INSERT_HEAD(&blk->aio_notifiers, notifier, list); + if (bs) { bdrv_add_aio_context_notifier(bs, attached_aio_context, detach_aio_context, opaque); @@ -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); + g_free(notifier); + return; + } + } + + abort(); } void blk_add_remove_bs_notifier(BlockBackend *blk, Notifier *notify) diff --git a/block/trace-events b/block/trace-events index 02dd80ff0c..7493d521dc 100644 --- a/block/trace-events +++ b/block/trace-events @@ -7,6 +7,8 @@ bdrv_lock_medium(void *bs, bool locked) "bs %p locked %d" # block/block-backend.c blk_co_preadv(void *blk, void *bs, int64_t offset, unsigned int bytes, int flags) "blk %p bs %p offset %"PRId64" bytes %u flags 0x%x" blk_co_pwritev(void *blk, void *bs, int64_t offset, unsigned int bytes, int flags) "blk %p bs %p offset %"PRId64" bytes %u flags 0x%x" +blk_root_attach(void *child, void *blk, void *bs) "child %p blk %p bs %p" +blk_root_detach(void *child, void *blk, void *bs) "child %p blk %p bs %p" # block/io.c bdrv_co_preadv(void *bs, int64_t offset, int64_t nbytes, unsigned int flags) "bs %p offset %"PRId64" nbytes %"PRId64" flags 0x%x"
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(+)