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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1572 bytes --]

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?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]