Am 08.02.2022 um 16:36 hat Emanuele Giuseppe Esposito geschrieben:
> 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 | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/block.c b/block.c
> index ec346a7e2e..08a6e3a4ef 100644
> --- a/block.c
> +++ b/block.c
> @@ -2872,8 +2872,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
> @@ -2890,6 +2888,10 @@ static void bdrv_replace_child_noperm(BdrvChild
> **childp,
> if (child->klass->attach) {
> child->klass->attach(child);
> }
> +
> + assert_bdrv_graph_writable(new_bs);
> + QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent);
> +
> }
Extra empty line. Looks good otherwise.
Does this also mean that the order in bdrv_child_cb_attach/detach() is
wrong? Or maybe adding a new node to bs->children is okay even when the
child node isn't drained.
Kevin
