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

[Vladimir Sementsov-Ogievskiy]

On 11/8/22 18:13, Emanuele Giuseppe Esposito wrote:
> Am 08/11/2022 um 15:33 schrieb 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.
>
> I think (and this is a repetition from my previous email) it confuses
> the code. It is clear, but then you don't know if we are in a coroutine
> context or not.

Hmm. But same thing is true for all callers of coroutine wrappers.

I agree that "coroutine wrapper" is a workaround for the design problem. 
Really, the fact that in many places we don't know are we in coroutine or not is very 
uncomfortable.

But still, I'm not sure that's it good to avoid this workaround in one place 
and continue to use it in all other places. I think following common design is 
better. Or rework it deeply by getting rid of the wrappers somehow.

> I am very well aware of what you did with your script, and I am working
> on extending your g_c_w class with g_c_w_simple, where we drop the
> if(qemu_in_coroutine()) case and leave just the coroutine creation.
> Therefore, coroutine functions we use only the _co_ function, otherwise
> we use g_c_w_simple.
> In this way code is clear as you point out, but there is no confusion.

Hmm sounds good, I missed it. Why not use g_c_w_simple here than?

> Thank you,
> Emanuele
> > > 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