On Wed, May 04, 2022 at 02:39:05PM +0100, Stefan Hajnoczi wrote:
> On Tue, Apr 26, 2022 at 04:51:06AM -0400, Emanuele Giuseppe Esposito wrote:
> > This is a new attempt to replace the need to take the AioContext lock to
> > protect graph modifications. In particular, we aim to remove
> > (or better, substitute) the AioContext around bdrv_replace_child_noperm,
> > since this function changes BlockDriverState's ->parents and ->children
> > lists.
> > 
> > In the previous version, we decided to discard using subtree_drains to
> > protect the nodes, for different reasons: for those unfamiliar with it,
> > please see https://patchew.org/QEMU/20220301142113.163174-1-eesposit@redhat.com/
> 
> I reread the thread and it's unclear to me why drain is the wrong
> mechanism for protecting graph modifications. We theorized a lot but
> ultimately is this new mechanism sufficiently different from
> bdrv_drained_begin()/end() to make it worth implementing?
> 
> Instead of invoking .drained_begin() callbacks to stop further I/O,
> we're now queuing coroutines (without backpressure information that
> whoever is spawning I/O needs so they can stop). The writer still waits
> for in-flight I/O to finish, including I/O not associated with the bdrv
> graph we wish to modify (because rdlock is per-AioContext and unrelated
> to a specific graph). Is this really more lightweight than drain?
> 
> If I understand correctly, the original goal was to avoid the need to
> hold the AioContext lock across bdrv_replace_child_noperm(). I would
> focus on that and use drain for now.
> 
> Maybe I've missed an important point about why the new mechanism is
> needed?

Ping?

Stefan