Re: [PATCH v2 1/9] block: call bdrv_co_drain_begin in a coroutine

[Vladimir Sementsov-Ogievskiy]

On 11/4/22 12:56, Emanuele Giuseppe Esposito wrote:
> It seems that bdrv_open_driver() forgot to create a coroutine
> where to call bs->drv->bdrv_co_drain_begin(), a callback
> marked as coroutine_fn.
>
> Because there is no active I/O at this point, the coroutine
> should end right after entering, so the caller does not need
> to poll until it is finished.

Hmm. I see your point. But isn't it better to go the generic way and use a 
generated coroutine wrapper? Nothing guarantees that .bdrv_co_drain_begin() 
handlers will never do any yield point even on driver open...

Look for example at bdrv_co_check(). It has a generated wrapper bdrv_check(), 
declared in include/block/block-io.h

So you just need to declare the wrapper, and use it in bdrv_open_driver(), the 
code would be clearer too.

> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   block.c | 36 +++++++++++++++++++++++++++++++-----
>   1 file changed, 31 insertions(+), 5 deletions(-)
>
> diff --git a/block.c b/block.c
> index 5311b21f8e..d2b2800039 100644
> --- a/block.c
> +++ b/block.c
> @@ -1637,12 +1637,34 @@ out:
>       g_free(gen_node_name);
>   }
>   
> +typedef struct DrainCo {
> +    BlockDriverState *bs;
> +    int ret;
> +} DrainCo;
> +
> +static void coroutine_fn bdrv_co_drain_begin(void *opaque)
> +{
> +    int i;
> +    DrainCo *co = opaque;
> +    BlockDriverState *bs = co->bs;
> +
> +    for (i = 0; i < bs->quiesce_counter; i++) {
> +        bs->drv->bdrv_co_drain_begin(bs);
> +    }
> +    co->ret = 0;
> +}
> +
>   static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv,
>                               const char *node_name, QDict *options,
>                               int open_flags, Error **errp)
>   {
>       Error *local_err = NULL;
> -    int i, ret;
> +    int ret;
> +    Coroutine *co;
> +    DrainCo dco = {
> +        .bs = bs,
> +        .ret = NOT_DONE,
> +    };
>       GLOBAL_STATE_CODE();
>   
>       bdrv_assign_node_name(bs, node_name, &local_err);
> @@ -1690,10 +1712,14 @@ static int bdrv_open_driver(BlockDriverState *bs, 
> BlockDriver *drv,
>       assert(bdrv_min_mem_align(bs) != 0);
>       assert(is_power_of_2(bs->bl.request_alignment));
>   
> -    for (i = 0; i < bs->quiesce_counter; i++) {
> -        if (drv->bdrv_co_drain_begin) {
> -            drv->bdrv_co_drain_begin(bs);
> -        }
> +    if (drv->bdrv_co_drain_begin) {
> +        co = qemu_coroutine_create(bdrv_co_drain_begin, &dco);
> +        qemu_coroutine_enter(co);
> +        /*
> +         * There should be no reason for drv->bdrv_co_drain_begin to wait at
> +         * this point, because the device does not have any active I/O.
> +         */
> +        assert(dco.ret != NOT_DONE);
>       }
>   
>       return 0;

--
Best regards,
Vladimir