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