Re: [RFC PATCH v2 0/8] Removal of AioContext lock, bs->parents and ->children: new rwlock

[Kevin Wolf <kwolf@redhat.com>]

Am 18.05.2022 um 14:43 hat Paolo Bonzini geschrieben:
> On 5/18/22 14:28, Emanuele Giuseppe Esposito wrote:
> > For example, all callers of bdrv_open() always take the AioContext lock.
> > Often it is taken very high in the call stack, but it's always taken.
> 
> I think it's actually not a problem of who takes the AioContext lock or
> where; the requirements are contradictory:

Okay, now that I have explained which challenges I see with the drain
solution, I'd also like to understand the problem that even motivates
you to go back to drains.

> * IO_OR_GS_CODE() functions, when called from coroutine context, expect to
> be called with the AioContext lock taken (example: bdrv_co_yield_to_drain)

Am I right to say this is not inherently part of the definition of
IO_OR_GS_CODE(), but just a property that these functions have in
practice?

In practice, the functions that are IO_OR_GS_CODE() are those that call
AIO_WAIT_WHILE() - which drops the lock, so it must have been taken
first. Of course, when calling from coroutine context, AIO_WAIT_WHILE()
is wrong, so these functions all have a different code path for
coroutines (or they aren't suitable to be called in coroutines at all).

Using a different code path means that the restrictions from
AIO_WAIT_WHILE() don't really apply any more and these functions become
effectively IO_CODE() when called in a coroutine. (At least I'm not
aware of any other piece of code apart from AIO_WAIT_WHILE() that makes
a function IO_OR_GS_CODE().)

Surrounding IO_CODE() with aio_context_acquire/release() is in the
definition of IO_CODE(), so your assumption seems right. (Not sure if
it's actually necessary in all cases and whether all callers do this
correctly, but with this definition we have declared that expections to
this are in fact bugs.)

(You mention bdrv_co_yield_to_drain() as an example. I don't think it's
a good example. The function isn't annotated, but it seems to me that
the correct annotation would be IO_CODE() anyway, i.e. safe to call from
any thread, not just IO_OR_GS_CODE().)

> * to call these functions with the lock taken, the code has to run in the
> BDS's home iothread.  Attempts to do otherwise results in deadlocks (the
> main loop's AIO_WAIT_WHILEs expect progress from the iothread, that cannot
> happen without releasing the aiocontext lock)

This problem can't happen in the main thread itself, AIO_WAIT_WHILE() is
safe both in the home thread and the main thread (at least as long as
you lock only once) because it temporarily drops the lock. It has also
become the definition of IO_OR_GS_CODE(): This code has to run in the
home thread or the main thread.


Of course, above I just concluded that when called from coroutines, in
practice IO_OR_GS_CODE() essentially turns into IO_CODE(). This is
supposed to allow much more:

 * I/O API functions. These functions are thread-safe, and therefore
 * can run in any thread as long as the thread has called
 * aio_context_acquire/release().

Come to think of it, I believe that many of the functions we declared
IO_CODE() are actually just IO_OR_GS_CODE() (at best; for iothreads,
they certainly require running in the home thread, but the main thread
allowed by IO_OR_GS_CODE() might not work). We have all the coroutine
machinery so that the AioContext lock of the current thread is
automatically released and reacquired across yield. However, this is the
wrong AioContext when called from a different thread, so we need an
additional lock - which should be dropped during yield, too, but it
doesn't happen.

Maybe this is really the scenario you mean with this point?

Switching to drain for locking doesn't solve the problem, but only
possibly defer it. In order to complete the multiqueue work, we need
to make IO_CODE() functions actually conform to the definition of
IO_CODE().

Do we have a plan what this should look like in the final state when all
the multiqueue work is completed? Will it require replacing the more or
less automatic AioContext lock handling that we currently have in
coroutines with explicit unlock/lock around all yield points? I assume
that some kind of lock will still have to be held and it wouldn't just
disappear with the removal of the AioContext lock? Or will we only have
coroutine locks which don't have this problem?

> * running the code in the BDS's home iothread is not possible for
> GLOBAL_STATE_CODE() functions (unless the BDS home iothread is the main
> thread, but that cannot be guaranteed in general)

There is nothing that stops GLOBAL_STATE_CODE() from scheduling work in
the home iothread of a BDS and then waiting for it - or if it is a
coroutine, even to reschedule itself into the BDS home thread
temporarily.

This is essentially what all of the generated_co_wrapper functions do,
and the coroutine case is what bdrv_co_enter() does.

So I'm not sure what you mean by this?

Kevin