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

[Paolo Bonzini]

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:

* 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)

* 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)

* 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)

> We might suppose that many callbacks are called under drain and in
> GLOBAL_STATE, which should be enough, but from our experimentation in
> the previous series we saw that currently not everything is under drain,
> leaving some operations unprotected (remember assert_graph_writable
> temporarily disabled, since drain coverage for bdrv_replace_child_noperm
> was not 100%?).
> Therefore we need to add more drains. But isn't drain what we decided to
> drop at the beginning? Why isn't drain good?

To sum up the patch ordering deadlock that we have right now:

* in some cases, graph manipulations are protected by the AioContext lock

* eliminating the AioContext lock is needed to move callbacks to 
coroutine contexts (see above for the deadlock scenario)

* moving callbacks to coroutine context is needed by the graph rwlock 
implementation

On one hand, we cannot protect the graph across manipulations with a 
graph rwlock without removing the AioContext lock; on the other hand, 
the AioContext lock is what _right now_ protects the graph.

So I'd rather go back to Emanuele's draining approach.  It may not be 
beautiful, but it allows progress.  Once that is in place, we can remove 
the AioContext lock (which mostly protects virtio-blk/virtio-scsi code 
right now) and reevaluate our next steps.

Paolo