Re: [RFC PATCH] test-bdrv-drain: keep graph manipulations out of coroutines

[Emanuele Giuseppe Esposito <eesposit@redhat.com>]

Am 05/12/2022 um 14:01 schrieb Kevin Wolf:
> Am 02.12.2022 um 18:22 hat Paolo Bonzini geschrieben:
>> On 12/2/22 14:42, Emanuele Giuseppe Esposito wrote:
>>>
>>>
>>> Am 02/12/2022 um 14:27 schrieb Paolo Bonzini:
>>>> Changes to the BlockDriverState graph will have to take the
>>>> corresponding lock for writing, and therefore cannot be done
>>>> inside a coroutine.  Move them outside the test body.
>>>>
>>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>>> ---
>>>>   tests/unit/test-bdrv-drain.c | 63 ++++++++++++++++++++++++++----------
>>>>   1 file changed, 46 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
>>>> index 6ae44116fe79..d85083dd4f9e 100644
>>>> --- a/tests/unit/test-bdrv-drain.c
>>>> +++ b/tests/unit/test-bdrv-drain.c
>>>> @@ -199,25 +199,40 @@ static void do_drain_end_unlocked(enum drain_type drain_type, BlockDriverState *
>>>>       }
>>>>   }
>>>> +static BlockBackend *blk;
>>>> +static BlockDriverState *bs, *backing;
>>>> +
>>>> +static void test_drv_cb_init(void)
>>>> +{
>>>> +    blk = blk_new(qemu_get_aio_context(), BLK_PERM_ALL, BLK_PERM_ALL);
>>>> +    bs = bdrv_new_open_driver(&bdrv_test, "test-node", BDRV_O_RDWR,
>>>> +                              &error_abort);
>>>> +    blk_insert_bs(blk, bs, &error_abort);
>>>> +
>>>> +    backing = bdrv_new_open_driver(&bdrv_test, "backing", 0, &error_abort);
>>>> +    bdrv_set_backing_hd(bs, backing, &error_abort);
>>>> +}
>>>> +
>>>> +static void test_drv_cb_fini(void)
>>>
>>> fini stands for "finito"? :)
>>
>> No, for finish :)
>> http://ftp.math.utah.edu/u/ma/hohn/linux/misc/elf/node3.html
>>
>>> Anyways, an alternative solution for this is also here (probably coming
>>> from you too):
>>> https://lists.nongnu.org/archive/html/qemu-devel/2022-03/msg03517.html
>>
>> Much better.  At least patches 7-8 from that series have to be salvaged,
>> possibly 10 as well.
> 
> I wonder if we need a more general solution for this because this test
> is not the only place that calls this kind of functions in a coroutine.
> The one I'm aware of in particular is all the .bdrv_co_create
> implementations, but I'm almost sure there are more.
> 
> Can we use a yield_to_drain()-like mechanism for these functions? Maybe
> even something like the opposite of co_wrapper, a no_co_wrapper that
> generates a foo_co() variant that drops out of coroutine context before
> calling foo()?
> 

I implemented something like yield_to_drain as you suggested, but when
thinking about it aren't we making a fix that will cost us even more
work in the future? If we use a yield_to_drain-like function, we are
doing something similar to g_c_w, and losing track of whether the caller
is a coroutine or not. And the function could then be used potentially
everywhere. Then we will realize "oh we need to get rid of this and
split the functions differentiating the coroutine context" and
eventually go through ALL the callers again to figure what is doing
what, and implement the same fix of this patch or my series once again.

Instead, even though this is just a test, we have a clear separation and
one less case to worry about in the future.

Thank you,
Emanuele