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 <eespo...@redhat.com>
>>>> Reviewed-by: Paolo Bonzini <pbonz...@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;
>>>
>>
>
