Re: [PATCH v3 1/2] block: Avoid processing BDS twice in bdrv_set_aio_context_ignore()

[Sergio Lopez]

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.

signature.asc
Description: PGP signature