Re: [PATCH 02/12] block/io.c: make bdrv_do_drained_begin_quiesce static and introduce bdrv_drained_begin_no_poll

[Paolo Bonzini]

On 1/18/22 17:27, Emanuele Giuseppe Esposito wrote:
> Using bdrv_do_drained_begin_quiesce() in bdrv_child_cb_drained_end()

bdrv_child_cb_drained_begin()

> is not a good idea: the callback might be called when running
> a drain in a coroutine, and bdrv_drained_begin_poll() does not
> handle that case, resulting in assertion failure.
>
> Instead, bdrv_do_drained_begin with no recursion and poll
> will accomplish the same thing (invoking bdrv_do_drained_begin_quiesce)
> but will firstly check if we are already in a coroutine, and exit
> from that via bdrv_co_yield_to_drain().

A better subject would be "fix bdrv_child_cb_drained_begin invocations 
from a coroutine".

Paolo

> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   block.c                  | 2 +-
>   block/io.c               | 7 ++++++-
>   include/block/block-io.h | 8 +++++---
>   3 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/block.c b/block.c
> index 668986c879..08fde585f4 100644
> --- a/block.c
> +++ b/block.c
> @@ -1206,7 +1206,7 @@ static char *bdrv_child_get_parent_desc(BdrvChild *c)
>   static void bdrv_child_cb_drained_begin(BdrvChild *child)
>   {
>       BlockDriverState *bs = child->opaque;
> -    bdrv_do_drained_begin_quiesce(bs, NULL, false);
> +    bdrv_drained_begin_no_poll(bs);
>   }
>   
>   static bool bdrv_child_cb_drained_poll(BdrvChild *child)
> diff --git a/block/io.c b/block/io.c
> index 3be08cad29..5123b7b713 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -425,7 +425,7 @@ static void coroutine_fn 
> bdrv_co_yield_to_drain(BlockDriverState *bs,
>       }
>   }
>   
> -void bdrv_do_drained_begin_quiesce(BlockDriverState *bs,
> +static void bdrv_do_drained_begin_quiesce(BlockDriverState *bs,
>                                      BdrvChild *parent, bool ignore_bds_parents)
>   {
>       assert(!qemu_in_coroutine());
> @@ -487,6 +487,11 @@ void bdrv_subtree_drained_begin(BlockDriverState *bs)
>       bdrv_do_drained_begin(bs, true, NULL, false, true);
>   }
>   
> +void bdrv_drained_begin_no_poll(BlockDriverState *bs)
> +{
> +    bdrv_do_drained_begin(bs, false, NULL, false, false);
> +}
> +
>   /**
>    * This function does not poll, nor must any of its recursively called
>    * functions.  The *drained_end_counter pointee will be incremented
> diff --git a/include/block/block-io.h b/include/block/block-io.h
> index a69bc5bd36..34a991649c 100644
> --- a/include/block/block-io.h
> +++ b/include/block/block-io.h
> @@ -238,13 +238,15 @@ bool bdrv_drain_poll(BlockDriverState *bs, bool recursive,
>   void bdrv_drained_begin(BlockDriverState *bs);
>   
>   /**
> - * bdrv_do_drained_begin_quiesce:
> + * bdrv_drained_begin_no_poll:
>    *
>    * Quiesces a BDS like bdrv_drained_begin(), but does not wait for already
>    * running requests to complete.
> + * Same as bdrv_drained_begin(), but do not poll for the subgraph to
> + * actually become unquiesced. Therefore, no graph changes will occur
> + * with this function.
>    */
> -void bdrv_do_drained_begin_quiesce(BlockDriverState *bs,
> -                                   BdrvChild *parent, bool ignore_bds_parents);
> +void bdrv_drained_begin_no_poll(BlockDriverState *bs);
>   
>   /**
>    * Like bdrv_drained_begin, but recursively begins a quiesced section for