Doing the opposite can make adding the child node to a non-drained node, as apply_subtree_drain is only done in ->attach() and thus make assert_bdrv_graph_writable fail.
This can happen for example during a transaction rollback (test 245, test_io_with_graph_changes): 1. a node is removed from the graph, thus it is undrained 2. then something happens, and we need to roll back the transactions through tran_abort() 3. at this point, the current code would first attach the undrained node to the graph via QLIST_INSERT_HEAD, and then call ->attach() that will take care of restoring the drain with apply_subtree_drain(), leaving the node undrained between the two operations. Signed-off-by: Emanuele Giuseppe Esposito <eespo...@redhat.com> --- block.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/block.c b/block.c index d870ba5393..c6a550f9c6 100644 --- a/block.c +++ b/block.c @@ -1434,6 +1434,11 @@ static void bdrv_inherited_options(BdrvChildRole role, bool parent_is_format, *child_flags = flags; } +/* + * Add the child node to child->opaque->children list, + * and then apply the drain to the whole child subtree, + * so that the drain count matches with the parent. + */ static void bdrv_child_cb_attach(BdrvChild *child) { BlockDriverState *bs = child->opaque; @@ -2889,8 +2894,6 @@ static void bdrv_replace_child_noperm(BdrvChild **childp, } if (new_bs) { - assert_bdrv_graph_writable(new_bs); - QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent); /* * Detaching the old node may have led to the new node's @@ -2901,12 +2904,19 @@ static void bdrv_replace_child_noperm(BdrvChild **childp, assert(new_bs->quiesce_counter <= new_bs_quiesce_counter); drain_saldo += new_bs->quiesce_counter - new_bs_quiesce_counter; - /* Attach only after starting new drained sections, so that recursive - * drain sections coming from @child don't get an extra .drained_begin - * callback. */ + /* + * First call ->attach() cb. + * In child_of_bds case, add child to the parent + * (child->opaque) ->children list and if + * necessary add missing drains in the child subtree. + */ if (child->klass->attach) { child->klass->attach(child); } + + /* Then add child to new_bs->parents list */ + assert_bdrv_graph_writable(new_bs); + QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent); } /* -- 2.31.1