On Mon, Apr 4, 2022 at 11:25 AM Stefan Hajnoczi <stefa...@redhat.com> wrote:
> - The new API doesn't stop more I/O requests from being submitted, it > just blocks the current coroutine so request processing is deferred. > New I/O requests would not complete until the write-side critical section ends. However they would still be accepted: from the point of view of the guest, the "consumed" index of the virtio ring would move forward, unlike bdrv_drained_begin/end(). - In other words, is_external is a flow control API whereas the new API > queues up request coroutines without notifying the caller. > Yes, I think this is the same I wrote above. > - The new API still needs to be combined with bdrv_drained_begin/end() > to ensure in-flight requests are done. > I don't think so, because in-flight requests would take the lock for reading. The write side would not start until those in-flight requests release the lock. - It's not obvious to me whether the new API obsoletes is_external. I think > it probably doesn't. > I agree that it doesn't. This new lock is only protecting ->parents and ->children. bdrv_drained_begin()/end() remains necessary, for example, when you need to send a request during the drained section. An example is block_resize. In addition, bdrv_drained_begin()/end() ensures that the callback of blk_aio_*() functions has been invoked (see commit 46aaf2a566, "block-backend: Decrease in_flight only after callback", 2018-09-25). This new lock would not ensure that. As an aside, instead of is_external, QEMU could remove/add the ioeventfd handler in the blk->dev_ops->drained_begin and blk->dev_ops->drained_end callbacks respectively. But that's just a code cleanup. Paolo
