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

[Emanuele Giuseppe Esposito]

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.

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.

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