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

[Emanuele Giuseppe Esposito]

Am 08/11/2022 um 16:52 schrieb 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.

And the only way to figure if it's a coroutine or not is by adding
assertions and pray that the iotests don't fail *and* cover all cases.

> 
> 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.

Well, one step at the time :) it's already difficult to verify that such
replacement cover and is correct for all cases :)

> 
>>
>> 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?

Because I came up with it this morning.

Thank you,
Emanuele

> 
>>
>> 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;
>>>
>>
>