Re: [PATCH v2 05/10] block.c: bdrv_replace_child_noperm: first call ->attach(), and then add child

[Emanuele Giuseppe Esposito]

Unfortunately this patch is not safe: theoretically ->attach can call
bdrv_apply_subtree_drain, and if it polls, will can call a bh that
for example reads the graph, finding it in an inconsistent state, since
it is between the two writes QLIST_INSERT_HEAD(&bs->children, child,
next); and QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent);

Please ignore it.
This patch could eventually go in the subtree_drain serie, if we decide
to go in that direction.

Emanuele


Am 14/03/2022 um 14:18 schrieb Emanuele Giuseppe Esposito:
> 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 <eesposit@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);
>      }
>  
>      /*
>