|
From: | Hanna Reitz |
Subject: | Re: [PATCH v4 05/25] block/block-backend.c: assertions for block-backend |
Date: | Tue, 16 Nov 2021 13:29:21 +0100 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.3.0 |
On 16.11.21 11:15, Emanuele Giuseppe Esposito wrote:
On 12/11/2021 12:01, Hanna Reitz wrote:On 25.10.21 12:17, Emanuele Giuseppe Esposito wrote:All the global state (GS) API functions will check that qemu_in_main_thread() returns true. If not, it means that the safety of BQL cannot be guaranteed, and they need to be moved to I/O. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> ---block/block-backend.c | 90 +++++++++++++++++++++++++++++++++++++++++-softmmu/qdev-monitor.c | 2 + 2 files changed, 91 insertions(+), 1 deletion(-) diff --git a/block/block-backend.c b/block/block-backend.c index 0afc03fd66..ed45576007 100644 --- a/block/block-backend.c +++ b/block/block-backend.c[...]@@ -1550,6 +1596,7 @@ BlockAIOCB *blk_aio_pwritev(BlockBackend *blk, int64_t offset,void blk_aio_cancel(BlockAIOCB *acb) { + assert(qemu_in_main_thread()); bdrv_aio_cancel(acb); }This function is in block-backend-io.h, though.I am confused a little on the {blk/bdrv}_aio functions, namely blk_aio_cancel bdrv_aio_cancel blk_aio_cancel_async bdrv_aio_cancel_async Do you think they should be I/O? The assertion seems to hold though.
Hm, semantically I would have classified them as I/O because they don’t modify state. I don’t have a strong opinion, though, because they don’t actually do I/O. They just cancel other I/O requests.
Most importantly though now I see there’s a comment in bdrv_aio_cancel() that states that “thread-safe code should use bdrv_aio_cancel_async exclusively”, which to me implies that bdrv_aio_cancel() (and blk_aio_cancel()) must be classified as GS, and it sounds like bdrv_aio_cancel_async() (and blk_aio_cancel_async()) should be classified as I/O. Looking at the AIOCBInfo.cancel_async implementations (called by bdrv_aio_cancel_async()) I’m not sure they’re all really thread-safe, though...? But at least bdrv_aio_cancel() claims they should be, so...
It seems to me like the intended separation is that bdrv_aio_cancel() should be GS and bdrv_aio_cancel_async() should be I/O. I can’t verify that the .cancel_async implementations are really thread-safe, but neither can I verify that blk_aio_cancel_async() is only called by BQL callers. That the assertions hold during testing isn’t too convincing for me, because we never wrote tests specifically to exercise these paths.
Hanna
[Prev in Thread] | Current Thread | [Next in Thread] |