168 lines
6.3 KiB
Diff
168 lines
6.3 KiB
Diff
From 4ad89aa6a9efcbc0420e332acab2dd06e55be2fa Mon Sep 17 00:00:00 2001
|
|
From: Fiona Ebner <f.ebner@proxmox.com>
|
|
Date: Tue, 25 Nov 2025 14:31:03 +0100
|
|
Subject: [PATCH 3/4] block/io_uring: avoid potentially getting stuck after
|
|
resubmit at the end of ioq_submit()
|
|
|
|
RH-Author: Hanna Czenczek <hreitz@redhat.com>
|
|
RH-MergeRequest: 479: linux-aio/io-uring: Resubmit tails of short requests
|
|
RH-Jira: RHEL-158224
|
|
RH-Acked-by: Kevin Wolf <kwolf@redhat.com>
|
|
RH-Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
|
|
RH-Commit: [3/4] fd599da3ffcd6b37ceff35587ae9dbc1698b0f57 (hreitz/qemu-kvm-c-9-s)
|
|
|
|
Note that this issue seems already fixed as a consequence of the large
|
|
io_uring rework with 047dabef97 ("block/io_uring: use aio_add_sqe()")
|
|
in current master, so this is purely for QEMU stable branches.
|
|
|
|
At the end of ioq_submit(), there is an opportunistic call to
|
|
luring_process_completions(). This is the single caller of
|
|
luring_process_completions() that doesn't use the
|
|
luring_process_completions_and_submit() wrapper.
|
|
|
|
Other callers use the wrapper, because luring_process_completions()
|
|
might require a subsequent call to ioq_submit() after resubmitting a
|
|
request. As noted for luring_resubmit():
|
|
|
|
> Resubmit a request by appending it to submit_queue. The caller must ensure
|
|
> that ioq_submit() is called later so that submit_queue requests are started.
|
|
|
|
So the caller at the end of ioq_submit() violates the contract and can
|
|
in fact be problematic if no other requests come in later. In such a
|
|
case, the request intended to be resubmitted will never be actually be
|
|
submitted via io_uring_submit().
|
|
|
|
A reproducer exposing this issue is [0], which is based on user
|
|
reports from [1]. Another reproducer is iotest 109 with '-i io_uring'.
|
|
|
|
I had the most success to trigger the issue with [0] when using a
|
|
BTRFS RAID 1 storage. With tmpfs, it can take quite a few iterations,
|
|
but also triggers eventually on my machine. With iotest 109 with '-i
|
|
io_uring' the issue triggers reliably on my ext4 file system.
|
|
|
|
Have ioq_submit() submit any resubmitted requests after calling
|
|
luring_process_completions(). The return value from io_uring_submit()
|
|
is checked to be non-negative before the opportunistic processing of
|
|
completions and going for the new resubmit logic, to ensure that a
|
|
failure of io_uring_submit() is not missed. Also note that the return
|
|
value already was not necessarily the total number of submissions,
|
|
since the loop might've been iterated more than once even before the
|
|
current change.
|
|
|
|
Only trigger the resubmission logic if it is actually necessary to
|
|
avoid changing behavior more than necessary. For example iotest 109
|
|
would produce more 'mirror ready' events if always resubmitting after
|
|
luring_process_completions() at the end of ioq_submit().
|
|
|
|
Note iotest 109 still does not pass as is when run with '-i io_uring',
|
|
because of two offset values for BLOCK_JOB_COMPLETED events being zero
|
|
instead of non-zero as in the expected output. Note that the two
|
|
affected test cases are expected failures and still fail, so they just
|
|
fail "faster". The test cases are actually not triggering the resubmit
|
|
logic, so the reason seems to be different ordering of requests and
|
|
completions of the current aio=io_uring implementation versus
|
|
aio=threads.
|
|
|
|
[0]:
|
|
|
|
> #!/bin/bash -e
|
|
> #file=/mnt/btrfs/disk.raw
|
|
> file=/tmp/disk.raw
|
|
> filesize=256
|
|
> readsize=512
|
|
> rm -f $file
|
|
> truncate -s $filesize $file
|
|
> ./qemu-system-x86_64 --trace '*uring*' --qmp stdio \
|
|
> --blockdev raw,node-name=node0,file.driver=file,file.cache.direct=off,file.filename=$file,file.aio=io_uring \
|
|
> <<EOF
|
|
> {"execute": "qmp_capabilities"}
|
|
> {"execute": "human-monitor-command", "arguments": { "command-line": "qemu-io node0 \"read 0 $readsize \"" }}
|
|
> {"execute": "quit"}
|
|
> EOF
|
|
|
|
[1]: https://forum.proxmox.com/threads/170045/
|
|
|
|
Cc: qemu-stable@nongnu.org
|
|
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
|
|
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
|
|
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
|
|
(cherry picked from commit 2bb0153cd806b8f6b4f82b353bd0113cd1c488a5)
|
|
Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
|
|
---
|
|
block/io_uring.c | 16 +++++++++++++---
|
|
1 file changed, 13 insertions(+), 3 deletions(-)
|
|
|
|
diff --git a/block/io_uring.c b/block/io_uring.c
|
|
index dd4f304910..5dbafc8f7b 100644
|
|
--- a/block/io_uring.c
|
|
+++ b/block/io_uring.c
|
|
@@ -120,11 +120,14 @@ static void luring_resubmit_short_read(LuringState *s, LuringAIOCB *luringcb,
|
|
* event loop. When there are no events left to complete the BH is being
|
|
* canceled.
|
|
*
|
|
+ * Returns whether ioq_submit() must be called again afterwards since requests
|
|
+ * were resubmitted via luring_resubmit().
|
|
*/
|
|
-static void luring_process_completions(LuringState *s)
|
|
+static bool luring_process_completions(LuringState *s)
|
|
{
|
|
struct io_uring_cqe *cqes;
|
|
int total_bytes;
|
|
+ bool resubmit = false;
|
|
|
|
defer_call_begin();
|
|
|
|
@@ -182,6 +185,7 @@ static void luring_process_completions(LuringState *s)
|
|
*/
|
|
if (ret == -EINTR || ret == -EAGAIN) {
|
|
luring_resubmit(s, luringcb);
|
|
+ resubmit = true;
|
|
continue;
|
|
}
|
|
} else if (!luringcb->qiov) {
|
|
@@ -194,6 +198,7 @@ static void luring_process_completions(LuringState *s)
|
|
if (luringcb->is_read) {
|
|
if (ret > 0) {
|
|
luring_resubmit_short_read(s, luringcb, ret);
|
|
+ resubmit = true;
|
|
continue;
|
|
} else {
|
|
/* Pad with zeroes */
|
|
@@ -224,6 +229,8 @@ end:
|
|
qemu_bh_cancel(s->completion_bh);
|
|
|
|
defer_call_end();
|
|
+
|
|
+ return resubmit;
|
|
}
|
|
|
|
static int ioq_submit(LuringState *s)
|
|
@@ -231,6 +238,7 @@ static int ioq_submit(LuringState *s)
|
|
int ret = 0;
|
|
LuringAIOCB *luringcb, *luringcb_next;
|
|
|
|
+resubmit:
|
|
while (s->io_q.in_queue > 0) {
|
|
/*
|
|
* Try to fetch sqes from the ring for requests waiting in
|
|
@@ -260,12 +268,14 @@ static int ioq_submit(LuringState *s)
|
|
}
|
|
s->io_q.blocked = (s->io_q.in_queue > 0);
|
|
|
|
- if (s->io_q.in_flight) {
|
|
+ if (ret >= 0 && s->io_q.in_flight) {
|
|
/*
|
|
* We can try to complete something just right away if there are
|
|
* still requests in-flight.
|
|
*/
|
|
- luring_process_completions(s);
|
|
+ if (luring_process_completions(s)) {
|
|
+ goto resubmit;
|
|
+ }
|
|
}
|
|
return ret;
|
|
}
|
|
--
|
|
2.47.3
|
|
|