| Message ID | 20210121170700.59734-2-slp@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nbd/server: Quiesce coroutines on context switch | expand |
On 1/21/21 11:06 AM, Sergio Lopez wrote: > Some graphs may contain an indirect reference to the first BDS in the > chain that can be reached while walking it bottom->up from one its one of its > children. > > Doubling-processing of a BDS is especially problematic for the Double-processing > aio_notifiers, as they might attempt to work on both the old and the > new AIO contexts. > > To avoid this problem, add every child and parent to the ignore list > before actually processing them. > > Suggested-by: Kevin Wolf <kwolf@redhat.com> > Signed-off-by: Sergio Lopez <slp@redhat.com> > --- > block.c | 34 +++++++++++++++++++++++++++------- > 1 file changed, 27 insertions(+), 7 deletions(-) > Reviewed-by: Eric Blake <eblake@redhat.com>
Am 21.01.2021 um 18:06 hat Sergio Lopez geschrieben: > Some graphs may contain an indirect reference to the first BDS in the > chain that can be reached while walking it bottom->up from one its > children. > > Doubling-processing of a BDS is especially problematic for the > aio_notifiers, as they might attempt to work on both the old and the > new AIO contexts. > > To avoid this problem, add every child and parent to the ignore list > before actually processing them. > > Suggested-by: Kevin Wolf <kwolf@redhat.com> > Signed-off-by: Sergio Lopez <slp@redhat.com> > --- > block.c | 34 +++++++++++++++++++++++++++------- > 1 file changed, 27 insertions(+), 7 deletions(-) The patch looks correct to me, I'm just wondering about one thing: > diff --git a/block.c b/block.c > index 8b9d457546..3da99312db 100644 > --- a/block.c > +++ b/block.c > @@ -6414,7 +6414,10 @@ void bdrv_set_aio_context_ignore(BlockDriverState *bs, > AioContext *new_context, GSList **ignore) > { > AioContext *old_context = bdrv_get_aio_context(bs); > - BdrvChild *child; > + GSList *children_to_process = NULL; > + GSList *parents_to_process = NULL; Why do we need these separate lists? Can't we just iterate over bs->parents/children a second time? I don't think the graph can change between the first and the second loop (and if it could, the result would be broken anyway). Kevin
On Mon, Feb 01, 2021 at 01:06:31PM +0100, Kevin Wolf wrote: > Am 21.01.2021 um 18:06 hat Sergio Lopez geschrieben: > > Some graphs may contain an indirect reference to the first BDS in the > > chain that can be reached while walking it bottom->up from one its > > children. > > > > Doubling-processing of a BDS is especially problematic for the > > aio_notifiers, as they might attempt to work on both the old and the > > new AIO contexts. > > > > To avoid this problem, add every child and parent to the ignore list > > before actually processing them. > > > > Suggested-by: Kevin Wolf <kwolf@redhat.com> > > Signed-off-by: Sergio Lopez <slp@redhat.com> > > --- > > block.c | 34 +++++++++++++++++++++++++++------- > > 1 file changed, 27 insertions(+), 7 deletions(-) > > The patch looks correct to me, I'm just wondering about one thing: > > > diff --git a/block.c b/block.c > > index 8b9d457546..3da99312db 100644 > > --- a/block.c > > +++ b/block.c > > @@ -6414,7 +6414,10 @@ void bdrv_set_aio_context_ignore(BlockDriverState *bs, > > AioContext *new_context, GSList **ignore) > > { > > AioContext *old_context = bdrv_get_aio_context(bs); > > - BdrvChild *child; > > + GSList *children_to_process = NULL; > > + GSList *parents_to_process = NULL; > > Why do we need these separate lists? Can't we just iterate over > bs->parents/children a second time? I don't think the graph can change > between the first and the second loop (and if it could, the result would > be broken anyway). It's not strictly needed, but this makes the code more readable by making our intentions clearer. To my eyes, at least. Sergio.
diff --git a/block.c b/block.c index 8b9d457546..3da99312db 100644 --- a/block.c +++ b/block.c @@ -6414,7 +6414,10 @@ void bdrv_set_aio_context_ignore(BlockDriverState *bs, AioContext *new_context, GSList **ignore) { AioContext *old_context = bdrv_get_aio_context(bs); - BdrvChild *child; + GSList *children_to_process = NULL; + GSList *parents_to_process = NULL; + GSList *entry; + BdrvChild *child, *parent; g_assert(qemu_get_current_aio_context() == qemu_get_aio_context()); @@ -6429,16 +6432,33 @@ void bdrv_set_aio_context_ignore(BlockDriverState *bs, continue; } *ignore = g_slist_prepend(*ignore, child); - bdrv_set_aio_context_ignore(child->bs, new_context, ignore); + children_to_process = g_slist_prepend(children_to_process, child); } - QLIST_FOREACH(child, &bs->parents, next_parent) { - if (g_slist_find(*ignore, child)) { + + QLIST_FOREACH(parent, &bs->parents, next_parent) { + if (g_slist_find(*ignore, parent)) { continue; } - assert(child->klass->set_aio_ctx); - *ignore = g_slist_prepend(*ignore, child); - child->klass->set_aio_ctx(child, new_context, ignore); + *ignore = g_slist_prepend(*ignore, parent); + parents_to_process = g_slist_prepend(parents_to_process, parent); + } + + for (entry = children_to_process; + entry != NULL; + entry = g_slist_next(entry)) { + child = entry->data; + bdrv_set_aio_context_ignore(child->bs, new_context, ignore); + } + g_slist_free(children_to_process); + + for (entry = parents_to_process; + entry != NULL; + entry = g_slist_next(entry)) { + parent = entry->data; + assert(parent->klass->set_aio_ctx); + parent->klass->set_aio_ctx(parent, new_context, ignore); } + g_slist_free(parents_to_process); bdrv_detach_aio_context(bs);
Some graphs may contain an indirect reference to the first BDS in the chain that can be reached while walking it bottom->up from one its children. Doubling-processing of a BDS is especially problematic for the aio_notifiers, as they might attempt to work on both the old and the new AIO contexts. To avoid this problem, add every child and parent to the ignore list before actually processing them. Suggested-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Sergio Lopez <slp@redhat.com> --- block.c | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-)