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

[Kevin Wolf <kwolf@redhat.com>]

Am 09.12.2022 um 13:20 hat Emanuele Giuseppe Esposito geschrieben:
> Am 09/12/2022 um 13:18 schrieb Emanuele Giuseppe Esposito:
> > Am 05/12/2022 um 14:01 schrieb Kevin Wolf:
> >> 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.

That's not what I had in mind. I really meant a no_co_wrapper, not a
no_co_wrapper_mixed.

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

I'm not suggesting not fixing the root cause (which is calling functions
in coroutines that aren't supposed to be called in coroutines), but just
wondering if generated functions to drop out of coroutine would be a
nice tool to keep the fixes simple.

We already have bdrv_co_drained_begin/end that use an open-coded version
of this. We would want a bdrv_co_open() that can be used by the
.bdrv_co_create implementations and a bdrv_co_new_open_driver() and
blk_co_insert_bs() that could be used by this test case. All of these
are functions that are normally forbidden to be called from a coroutine,
but when you first drop out of the coroutine, they are fine.

The alternative solution is not just merging this test case patch, but
we still need to fix all of the .bdrv_co_create implementations, even if
we decide to write the code manually instead of generating it.

> At least the above is valid if the change you are proposing is the
> following (tested already, works)

bdrv_replace_child_noperm() is the wrong place to do things like this,
it's way too deep down the call chain. Callers really want things to be
atomic there, and involving a BH would make that impossible.

Kevin