Re: [PATCH v5 10/31] block.c: modify .attach and .detach callbacks of child_of_bds

[Hanna Reitz]

On 24.11.21 07:43, Emanuele Giuseppe Esposito wrote:
> According to the assertions put in the previous patch, we should
> first drain and then modify the ->children list. In this way
> we prevent other iothreads to read the list while it is being
> updated.
>
> In this case, moving the drain won't cause any harm, because
> child is a parameter of the drain function so it will still be
> included in the operation, despite not being in the list.

Sounds good.

> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   block.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/block.c b/block.c
> index 522a273140..5516c84ec4 100644
> --- a/block.c
> +++ b/block.c
> @@ -1416,6 +1416,7 @@ static void bdrv_child_cb_attach(BdrvChild *child)
>   {
>       BlockDriverState *bs = child->opaque;
>   
> +    bdrv_apply_subtree_drain(child, bs);
>       assert_bdrv_graph_writable(bs);
>       QLIST_INSERT_HEAD(&bs->children, child, next);
>   
> @@ -1423,7 +1424,6 @@ static void bdrv_child_cb_attach(BdrvChild *child)
>           bdrv_backing_attach(child);
>       }
>   
> -    bdrv_apply_subtree_drain(child, bs);

I think we should also remove the empty line above.

Hanna

>   }
>   
>   static void bdrv_child_cb_detach(BdrvChild *child)
> @@ -1434,10 +1434,9 @@ static void bdrv_child_cb_detach(BdrvChild *child)
>           bdrv_backing_detach(child);
>       }
>   
> -    bdrv_unapply_subtree_drain(child, bs);
> -
>       assert_bdrv_graph_writable(bs);
>       QLIST_REMOVE(child, next);
> +    bdrv_unapply_subtree_drain(child, bs);
>   }
>   
>   static int bdrv_child_cb_update_filename(BdrvChild *c, BlockDriverState *base,