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 <eespo...@redhat.com>
> ---
> 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
