Re: [PATCH 2/6] block.c: bdrv_replace_child_noperm: first remove the child, and then call ->detach()
Am 08.02.2022 um 16:36 hat Emanuele Giuseppe Esposito geschrieben: > Doing the opposite can make ->detach() (more precisely > bdrv_unapply_subtree_drain() in bdrv_child_cb_detach) undo the subtree_drain > just performed to protect the removal of the child from the graph, > thus making the fully-enabled assert_bdrv_graph_writable fail. > > Note that assert_bdrv_graph_writable is not yet fully enabled. > > Signed-off-by: Emanuele Giuseppe Esposito > --- > block.c | 10 ++ > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/block.c b/block.c > index 4551eba2aa..ec346a7e2e 100644 > --- a/block.c > +++ b/block.c > @@ -2854,14 +2854,16 @@ static void bdrv_replace_child_noperm(BdrvChild > **childp, > } > > if (old_bs) { > -/* Detach first so that the recursive drain sections coming from > @child > +assert_bdrv_graph_writable(old_bs); > +QLIST_REMOVE(child, next_parent); > +/* > + * Detach first so that the recursive drain sections coming from > @child > * are already gone and we only end the drain sections that came from > - * elsewhere. */ > + * elsewhere. > + */ This comment is confusing, but that's not your fault. It was originally added in commit d736f119dae and referred to calling .detach() before calling .drained_end(), which was the very next thing it would do. Commit debc2927671 moved the .drained_end() code to the end of the whole operation, but left this comment (and a similar one for .attach() and .drained_begin()) around even though it doesn't explain the new code very well any more. > if (child->klass->detach) { > child->klass->detach(child); > } > -assert_bdrv_graph_writable(old_bs); > -QLIST_REMOVE(child, next_parent); > } > > child->bs = new_bs; After digging into what the comment really meant, I think it doesn't refer to the order of QLIST_REMOVE() and .detach(). The change looks safe to me. I would just suggest updating the comment to explain the order you're fixing here instead of the now irrelevant one. Kevin
Re: [PATCH 2/6] block.c: bdrv_replace_child_noperm: first remove the child, and then call ->detach()
On Tue, Feb 08, 2022 at 10:36:51AM -0500, Emanuele Giuseppe Esposito wrote: > Doing the opposite can make ->detach() (more precisely > bdrv_unapply_subtree_drain() in bdrv_child_cb_detach) undo the subtree_drain > just performed to protect the removal of the child from the graph, > thus making the fully-enabled assert_bdrv_graph_writable fail. > > Note that assert_bdrv_graph_writable is not yet fully enabled. > > Signed-off-by: Emanuele Giuseppe Esposito > --- > block.c | 10 ++ > 1 file changed, 6 insertions(+), 4 deletions(-) Reviewed-by: Stefan Hajnoczi signature.asc Description: PGP signature
[PATCH 2/6] block.c: bdrv_replace_child_noperm: first remove the child, and then call ->detach()
Doing the opposite can make ->detach() (more precisely bdrv_unapply_subtree_drain() in bdrv_child_cb_detach) undo the subtree_drain just performed to protect the removal of the child from the graph, thus making the fully-enabled assert_bdrv_graph_writable fail. Note that assert_bdrv_graph_writable is not yet fully enabled. Signed-off-by: Emanuele Giuseppe Esposito --- block.c | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/block.c b/block.c index 4551eba2aa..ec346a7e2e 100644 --- a/block.c +++ b/block.c @@ -2854,14 +2854,16 @@ static void bdrv_replace_child_noperm(BdrvChild **childp, } if (old_bs) { -/* Detach first so that the recursive drain sections coming from @child +assert_bdrv_graph_writable(old_bs); +QLIST_REMOVE(child, next_parent); +/* + * Detach first so that the recursive drain sections coming from @child * are already gone and we only end the drain sections that came from - * elsewhere. */ + * elsewhere. + */ if (child->klass->detach) { child->klass->detach(child); } -assert_bdrv_graph_writable(old_bs); -QLIST_REMOVE(child, next_parent); } child->bs = new_bs; -- 2.31.1