Re: [PATCH v5 10/31] block.c: modify .attach and .detach callbacks of child_of_bds

[Emanuele Giuseppe Esposito]

On 16/12/2021 15:57, Hanna Reitz wrote:
> On 24.11.21 07:43, Emanuele Giuseppe Esposito wrote:
> > According to the assertions put in the previous patch, we should
> > first drain and then modify the ->children list. In this way
> > we prevent other iothreads to read the list while it is being
> > updated.
> >
> > In this case, moving the drain won't cause any harm, because
> > child is a parameter of the drain function so it will still be
> > included in the operation, despite not being in the list.
>
> Sounds good.

Uhm.. I don't think this is useful at all. I was thinking to drop this 
patch in v6.

My plans on subtree drains, ->children and ->parent lists are explained 
in "Removal of Aiocontext lock and usage of subtree drains in aborted 
transactions"

20211213104014.69858-1-eesposit@redhat.com/">https://patchew.org/QEMU/20211213104014.69858-1-eesposit@redhat.com/

And as I say there, commit d736f119da makes sure that it is safe to 
modify the graph even side a bdrv_subtree_drained_begin/end() section.
So this should be unnecessary.

Thank you,
Emanuele
> > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> > ---
> >   block.c | 5 ++---
> >   1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/block.c b/block.c
> > index 522a273140..5516c84ec4 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -1416,6 +1416,7 @@ static void bdrv_child_cb_attach(BdrvChild *child)
> >   {
> >       BlockDriverState *bs = child->opaque;
> > +    bdrv_apply_subtree_drain(child, bs);
> >       assert_bdrv_graph_writable(bs);
> >       QLIST_INSERT_HEAD(&bs->children, child, next);
> > @@ -1423,7 +1424,6 @@ static void bdrv_child_cb_attach(BdrvChild *child)
> >           bdrv_backing_attach(child);
> >       }
> > -    bdrv_apply_subtree_drain(child, bs);
>
> I think we should also remove the empty line above.
>
> Hanna
>
> >   }
> >   static void bdrv_child_cb_detach(BdrvChild *child)
> > @@ -1434,10 +1434,9 @@ static void bdrv_child_cb_detach(BdrvChild *child)
> >           bdrv_backing_detach(child);
> >       }
> > -    bdrv_unapply_subtree_drain(child, bs);
> > -
> >       assert_bdrv_graph_writable(bs);
> >       QLIST_REMOVE(child, next);
> > +    bdrv_unapply_subtree_drain(child, bs);
> >   }
> >   static int bdrv_child_cb_update_filename(BdrvChild *c, 
> > BlockDriverState *base,