Message ID | 20220118162738.1366281-1-eesposit@redhat.com (mailing list archive) |
---|---|
Headers | show |
Series | Removal of Aiocontext lock through drains: protect bdrv_replace_child_noperm. | expand |
On 1/18/22 17:27, Emanuele Giuseppe Esposito wrote: > * Finally fully enable assert_bdrv_graph_writable(), checking also for the > drains, in patch 11. Wow, this is definitely not just moving code around! It's great that you could pull this off. It gives me a lot more confidence that the locking model and the graph/IO split are correct, and also a lot more confidence about the logic to move the drains back and forth. It does add a lot more complication to the API with all the _unlocked variants, but I think we're getting closer and closer to the endgame for the AioContext lock. Paolo
If I understand correctly aio_context_acquire() calls are not explicitly removed in this patch series but switching to BDRV_POLL_WHILE_UNLOCKED() means that we no longer need to run under the AioContext lock? Still, I'm a bit surprised I didn't notice any aio_context_acquire/release() removals in this patch series because I thought callers need that before they switch to BDRV_POLL_WHILE_UNLOCKED()?
On 1/26/22 12:29, Stefan Hajnoczi wrote: > Still, I'm a bit surprised I didn't notice any > aio_context_acquire/release() removals in this patch series because I > thought callers need that before they switch to > BDRV_POLL_WHILE_UNLOCKED()? I think the callers are new and were not calling bdrv_subtree_drained_begin() (and thus BDRV_POLL_WHILE) at all? Emanuele, enlighten us. :) Paolo
On 27/01/2022 14:46, Paolo Bonzini wrote: > On 1/26/22 12:29, Stefan Hajnoczi wrote: >> Still, I'm a bit surprised I didn't notice any >> aio_context_acquire/release() removals in this patch series because I >> thought callers need that before they switch to >> BDRV_POLL_WHILE_UNLOCKED()? > > I think the callers are new and were not calling > bdrv_subtree_drained_begin() (and thus BDRV_POLL_WHILE) at all? > > Emanuele, enlighten us. :) Yes, the callers were not calling any kind of drains (or at least most of them) *and* no context lock was acquired before calling them. The current logic (or at least how I see it) when draining is: "ok, we need to use bdrv_drain or bdrv_subtree_drain, but these function call BDRV_POLL_WHILE(), which in turns calls AIO_WAIT_WHILE and thus performs aio_context_release(lock); [...] aio_context_acquire(lock); *Therefore* we need to acquire the lock". The lock is taken as a consequence of the drain implementation. This makes the lock usage useless, because we are just blindly acquiring it for the purpose of making BDRV_POLL_WHILE happy. On the other side, here no lock was acquired before, and BDRV_POLL_WHILE_UNLOCKED is not releasing anything, thus no lock is needed. This seems to hold and kinda proves my logic above. Thank you, Emanuele