Re: [PATCH v2 04/10] block.c: bdrv_replace_child_noperm: first remove the child, and then call ->detach()

[Emanuele Giuseppe Esposito]

Unfortunately this patch is not safe: theoretically ->detach can call
bdrv_unapply_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_REMOVE(child, next_parent); and
QLIST_REMOVE(child, next);

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 ->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 <eesposit@redhat.com>
> ---
>  block.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 372f16f4a0..d870ba5393 100644
> --- a/block.c
> +++ b/block.c
> @@ -1448,6 +1448,11 @@ static void bdrv_child_cb_attach(BdrvChild *child)
>      bdrv_apply_subtree_drain(child, bs);
>  }
>  
> +/*
> + * Unapply the drain in the whole child subtree, as
> + * it has been already detached, and then remove from
> + * child->opaque->children.
> + */
>  static void bdrv_child_cb_detach(BdrvChild *child)
>  {
>      BlockDriverState *bs = child->opaque;
> @@ -2864,14 +2869,18 @@ static void bdrv_replace_child_noperm(BdrvChild 
> **childp,
>      }
>  
>      if (old_bs) {
> -        /* 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. */
> +        /* First remove child from child->bs->parents list */
> +        assert_bdrv_graph_writable(old_bs);
> +        QLIST_REMOVE(child, next_parent);
> +        /*
> +         * Then call ->detach() cb.
> +         * In child_of_bds case, update the child parent
> +         * (child->opaque) ->children list and
> +         * remove any drain left in the child subtree.
> +         */
>          if (child->klass->detach) {
>              child->klass->detach(child);
>          }
> -        assert_bdrv_graph_writable(old_bs);
> -        QLIST_REMOVE(child, next_parent);
>      }
>  
>      child->bs = new_bs;
>