* [PATCH v4 0/1] io_uring: fix skipping of old timeout events
@ 2021-01-15 16:54 Marcelo Diop-Gonzalez
2021-01-15 16:54 ` [PATCH v4 1/1] io_uring: flush timeouts that should already have expired Marcelo Diop-Gonzalez
2021-01-15 17:02 ` [PATCH v4 0/1] io_uring: fix skipping of old timeout events Jens Axboe
0 siblings, 2 replies; 7+ messages in thread
From: Marcelo Diop-Gonzalez @ 2021-01-15 16:54 UTC (permalink / raw)
To: axboe, asml.silence; +Cc: io-uring, Marcelo Diop-Gonzalez
This patch tries to fix a problem with IORING_OP_TIMEOUT events
not being flushed if they should already have expired. The test below
hangs before this change (unless you run with $ ./a.out ~/somefile 1):
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <liburing.h>
int main(int argc, char **argv) {
if (argc < 2)
return 1;
int fd = open(argv[1], O_RDONLY);
if (fd < 0) {
perror("open");
return 1;
}
struct io_uring ring;
io_uring_queue_init(4, &ring, 0);
struct io_uring_sqe *sqe = io_uring_get_sqe(&ring);
struct __kernel_timespec ts = { .tv_sec = 9999999 };
io_uring_prep_timeout(sqe, &ts, 1, 0);
sqe->user_data = 123;
int ret = io_uring_submit(&ring);
if (ret < 0) {
fprintf(stderr, "submit(timeout_sqe): %d\n", ret);
return 1;
}
int n = 2;
if (argc > 2)
n = atoi(argv[2]);
char buf;
for (int i = 0; i < n; i++) {
sqe = io_uring_get_sqe(&ring);
if (!sqe) {
fprintf(stderr, "too many\n");
exit(1);
}
io_uring_prep_read(sqe, fd, &buf, 1, 0);
}
ret = io_uring_submit(&ring);
if (ret < 0) {
fprintf(stderr, "submit(read_sqe): %d\n", ret);
exit(1);
}
struct io_uring_cqe *cqe;
for (int i = 0; i < n+1; i++) {
struct io_uring_cqe *cqe;
int ret = io_uring_wait_cqe(&ring, &cqe);
if (ret < 0) {
fprintf(stderr, "wait_cqe(): %d\n", ret);
return 1;
}
if (cqe->user_data == 123)
printf("timeout found\n");
io_uring_cqe_seen(&ring, cqe);
}
}
v4: Reorder ->cq_timeouts read, and add some comments
v3: Add ->last_flush member to handle more overflow issues
v2: Properly handle u32 overflow issues
Marcelo Diop-Gonzalez (1):
io_uring: flush timeouts that should already have expired
fs/io_uring.c | 34 ++++++++++++++++++++++++++++++----
1 file changed, 30 insertions(+), 4 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH v4 1/1] io_uring: flush timeouts that should already have expired
2021-01-15 16:54 [PATCH v4 0/1] io_uring: fix skipping of old timeout events Marcelo Diop-Gonzalez
@ 2021-01-15 16:54 ` Marcelo Diop-Gonzalez
2021-01-15 17:02 ` [PATCH v4 0/1] io_uring: fix skipping of old timeout events Jens Axboe
1 sibling, 0 replies; 7+ messages in thread
From: Marcelo Diop-Gonzalez @ 2021-01-15 16:54 UTC (permalink / raw)
To: axboe, asml.silence; +Cc: io-uring, Marcelo Diop-Gonzalez
Right now io_flush_timeouts() checks if the current number of events
is equal to ->timeout.target_seq, but this will miss some timeouts if
there have been more than 1 event added since the last time they were
flushed (possible in io_submit_flush_completions(), for example). Fix
it by recording the last sequence at which timeouts were flushed so
that the number of events seen can be compared to the number of events
needed without overflow.
Signed-off-by: Marcelo Diop-Gonzalez <[email protected]>
Reviewed-by: Pavel Begunkov <[email protected]>
---
fs/io_uring.c | 34 ++++++++++++++++++++++++++++++----
1 file changed, 30 insertions(+), 4 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 372be9caf340..06cc79d39586 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -354,6 +354,7 @@ struct io_ring_ctx {
unsigned cq_entries;
unsigned cq_mask;
atomic_t cq_timeouts;
+ unsigned cq_last_tm_flush;
unsigned long cq_check_overflow;
struct wait_queue_head cq_wait;
struct fasync_struct *cq_fasync;
@@ -1639,19 +1640,38 @@ static void __io_queue_deferred(struct io_ring_ctx *ctx)
static void io_flush_timeouts(struct io_ring_ctx *ctx)
{
- while (!list_empty(&ctx->timeout_list)) {
+ u32 seq;
+
+ if (list_empty(&ctx->timeout_list))
+ return;
+
+ seq = ctx->cached_cq_tail - atomic_read(&ctx->cq_timeouts);
+
+ do {
+ u32 events_needed, events_got;
struct io_kiocb *req = list_first_entry(&ctx->timeout_list,
struct io_kiocb, timeout.list);
if (io_is_timeout_noseq(req))
break;
- if (req->timeout.target_seq != ctx->cached_cq_tail
- - atomic_read(&ctx->cq_timeouts))
+
+ /*
+ * Since seq can easily wrap around over time, subtract
+ * the last seq at which timeouts were flushed before comparing.
+ * Assuming not more than 2^31-1 events have happened since,
+ * these subtractions won't have wrapped, so we can check if
+ * target is in [last_seq, current_seq] by comparing the two.
+ */
+ events_needed = req->timeout.target_seq - ctx->cq_last_tm_flush;
+ events_got = seq - ctx->cq_last_tm_flush;
+ if (events_got < events_needed)
break;
list_del_init(&req->timeout.list);
io_kill_timeout(req);
- }
+ } while (!list_empty(&ctx->timeout_list));
+
+ ctx->cq_last_tm_flush = seq;
}
static void io_commit_cqring(struct io_ring_ctx *ctx)
@@ -5837,6 +5857,12 @@ static int io_timeout(struct io_kiocb *req)
tail = ctx->cached_cq_tail - atomic_read(&ctx->cq_timeouts);
req->timeout.target_seq = tail + off;
+ /* Update the last seq here in case io_flush_timeouts() hasn't.
+ * This is safe because ->completion_lock is held, and submissions
+ * and completions are never mixed in the same ->completion_lock section.
+ */
+ ctx->cq_last_tm_flush = tail;
+
/*
* Insertion sort, ensuring the first entry in the list is always
* the one we need first.
--
2.20.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v4 0/1] io_uring: fix skipping of old timeout events
2021-01-15 16:54 [PATCH v4 0/1] io_uring: fix skipping of old timeout events Marcelo Diop-Gonzalez
2021-01-15 16:54 ` [PATCH v4 1/1] io_uring: flush timeouts that should already have expired Marcelo Diop-Gonzalez
@ 2021-01-15 17:02 ` Jens Axboe
2021-01-15 18:31 ` Marcelo Diop-Gonzalez
1 sibling, 1 reply; 7+ messages in thread
From: Jens Axboe @ 2021-01-15 17:02 UTC (permalink / raw)
To: Marcelo Diop-Gonzalez, asml.silence; +Cc: io-uring
On 1/15/21 9:54 AM, Marcelo Diop-Gonzalez wrote:
> This patch tries to fix a problem with IORING_OP_TIMEOUT events
> not being flushed if they should already have expired. The test below
> hangs before this change (unless you run with $ ./a.out ~/somefile 1):
>
> #include <fcntl.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <unistd.h>
>
> #include <liburing.h>
>
> int main(int argc, char **argv) {
> if (argc < 2)
> return 1;
>
> int fd = open(argv[1], O_RDONLY);
> if (fd < 0) {
> perror("open");
> return 1;
> }
>
> struct io_uring ring;
> io_uring_queue_init(4, &ring, 0);
>
> struct io_uring_sqe *sqe = io_uring_get_sqe(&ring);
>
> struct __kernel_timespec ts = { .tv_sec = 9999999 };
> io_uring_prep_timeout(sqe, &ts, 1, 0);
> sqe->user_data = 123;
> int ret = io_uring_submit(&ring);
> if (ret < 0) {
> fprintf(stderr, "submit(timeout_sqe): %d\n", ret);
> return 1;
> }
>
> int n = 2;
> if (argc > 2)
> n = atoi(argv[2]);
>
> char buf;
> for (int i = 0; i < n; i++) {
> sqe = io_uring_get_sqe(&ring);
> if (!sqe) {
> fprintf(stderr, "too many\n");
> exit(1);
> }
> io_uring_prep_read(sqe, fd, &buf, 1, 0);
> }
> ret = io_uring_submit(&ring);
> if (ret < 0) {
> fprintf(stderr, "submit(read_sqe): %d\n", ret);
> exit(1);
> }
>
> struct io_uring_cqe *cqe;
> for (int i = 0; i < n+1; i++) {
> struct io_uring_cqe *cqe;
> int ret = io_uring_wait_cqe(&ring, &cqe);
> if (ret < 0) {
> fprintf(stderr, "wait_cqe(): %d\n", ret);
> return 1;
> }
> if (cqe->user_data == 123)
> printf("timeout found\n");
> io_uring_cqe_seen(&ring, cqe);
> }
> }
Can you turn this into a test case for liburing? I'll apply the
associated patch, thanks (and to Pavel for review as well).
--
Jens Axboe
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v4 0/1] io_uring: fix skipping of old timeout events
2021-01-15 17:02 ` [PATCH v4 0/1] io_uring: fix skipping of old timeout events Jens Axboe
@ 2021-01-15 18:31 ` Marcelo Diop-Gonzalez
2021-01-15 18:38 ` Jens Axboe
0 siblings, 1 reply; 7+ messages in thread
From: Marcelo Diop-Gonzalez @ 2021-01-15 18:31 UTC (permalink / raw)
To: Jens Axboe; +Cc: asml.silence, io-uring
On Fri, Jan 15, 2021 at 10:02:12AM -0700, Jens Axboe wrote:
> On 1/15/21 9:54 AM, Marcelo Diop-Gonzalez wrote:
> > This patch tries to fix a problem with IORING_OP_TIMEOUT events
> > not being flushed if they should already have expired. The test below
> > hangs before this change (unless you run with $ ./a.out ~/somefile 1):
> >
> > #include <fcntl.h>
> > #include <stdio.h>
> > #include <stdlib.h>
> > #include <string.h>
> > #include <unistd.h>
> >
> > #include <liburing.h>
> >
> > int main(int argc, char **argv) {
> > if (argc < 2)
> > return 1;
> >
> > int fd = open(argv[1], O_RDONLY);
> > if (fd < 0) {
> > perror("open");
> > return 1;
> > }
> >
> > struct io_uring ring;
> > io_uring_queue_init(4, &ring, 0);
> >
> > struct io_uring_sqe *sqe = io_uring_get_sqe(&ring);
> >
> > struct __kernel_timespec ts = { .tv_sec = 9999999 };
> > io_uring_prep_timeout(sqe, &ts, 1, 0);
> > sqe->user_data = 123;
> > int ret = io_uring_submit(&ring);
> > if (ret < 0) {
> > fprintf(stderr, "submit(timeout_sqe): %d\n", ret);
> > return 1;
> > }
> >
> > int n = 2;
> > if (argc > 2)
> > n = atoi(argv[2]);
> >
> > char buf;
> > for (int i = 0; i < n; i++) {
> > sqe = io_uring_get_sqe(&ring);
> > if (!sqe) {
> > fprintf(stderr, "too many\n");
> > exit(1);
> > }
> > io_uring_prep_read(sqe, fd, &buf, 1, 0);
> > }
> > ret = io_uring_submit(&ring);
> > if (ret < 0) {
> > fprintf(stderr, "submit(read_sqe): %d\n", ret);
> > exit(1);
> > }
> >
> > struct io_uring_cqe *cqe;
> > for (int i = 0; i < n+1; i++) {
> > struct io_uring_cqe *cqe;
> > int ret = io_uring_wait_cqe(&ring, &cqe);
> > if (ret < 0) {
> > fprintf(stderr, "wait_cqe(): %d\n", ret);
> > return 1;
> > }
> > if (cqe->user_data == 123)
> > printf("timeout found\n");
> > io_uring_cqe_seen(&ring, cqe);
> > }
> > }
>
> Can you turn this into a test case for liburing? I'll apply the
> associated patch, thanks (and to Pavel for review as well).
Yup, can do. I'll try to clean it up some first (especially so it
doesn't just hang when it fails :/)
>
> --
> Jens Axboe
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v4 0/1] io_uring: fix skipping of old timeout events
2021-01-15 18:31 ` Marcelo Diop-Gonzalez
@ 2021-01-15 18:38 ` Jens Axboe
2021-01-15 18:48 ` Pavel Begunkov
0 siblings, 1 reply; 7+ messages in thread
From: Jens Axboe @ 2021-01-15 18:38 UTC (permalink / raw)
To: Marcelo Diop-Gonzalez; +Cc: asml.silence, io-uring
On 1/15/21 11:31 AM, Marcelo Diop-Gonzalez wrote:
> On Fri, Jan 15, 2021 at 10:02:12AM -0700, Jens Axboe wrote:
>> On 1/15/21 9:54 AM, Marcelo Diop-Gonzalez wrote:
>>> This patch tries to fix a problem with IORING_OP_TIMEOUT events
>>> not being flushed if they should already have expired. The test below
>>> hangs before this change (unless you run with $ ./a.out ~/somefile 1):
>>>
>>> #include <fcntl.h>
>>> #include <stdio.h>
>>> #include <stdlib.h>
>>> #include <string.h>
>>> #include <unistd.h>
>>>
>>> #include <liburing.h>
>>>
>>> int main(int argc, char **argv) {
>>> if (argc < 2)
>>> return 1;
>>>
>>> int fd = open(argv[1], O_RDONLY);
>>> if (fd < 0) {
>>> perror("open");
>>> return 1;
>>> }
>>>
>>> struct io_uring ring;
>>> io_uring_queue_init(4, &ring, 0);
>>>
>>> struct io_uring_sqe *sqe = io_uring_get_sqe(&ring);
>>>
>>> struct __kernel_timespec ts = { .tv_sec = 9999999 };
>>> io_uring_prep_timeout(sqe, &ts, 1, 0);
>>> sqe->user_data = 123;
>>> int ret = io_uring_submit(&ring);
>>> if (ret < 0) {
>>> fprintf(stderr, "submit(timeout_sqe): %d\n", ret);
>>> return 1;
>>> }
>>>
>>> int n = 2;
>>> if (argc > 2)
>>> n = atoi(argv[2]);
>>>
>>> char buf;
>>> for (int i = 0; i < n; i++) {
>>> sqe = io_uring_get_sqe(&ring);
>>> if (!sqe) {
>>> fprintf(stderr, "too many\n");
>>> exit(1);
>>> }
>>> io_uring_prep_read(sqe, fd, &buf, 1, 0);
>>> }
>>> ret = io_uring_submit(&ring);
>>> if (ret < 0) {
>>> fprintf(stderr, "submit(read_sqe): %d\n", ret);
>>> exit(1);
>>> }
>>>
>>> struct io_uring_cqe *cqe;
>>> for (int i = 0; i < n+1; i++) {
>>> struct io_uring_cqe *cqe;
>>> int ret = io_uring_wait_cqe(&ring, &cqe);
>>> if (ret < 0) {
>>> fprintf(stderr, "wait_cqe(): %d\n", ret);
>>> return 1;
>>> }
>>> if (cqe->user_data == 123)
>>> printf("timeout found\n");
>>> io_uring_cqe_seen(&ring, cqe);
>>> }
>>> }
>>
>> Can you turn this into a test case for liburing? I'll apply the
>> associated patch, thanks (and to Pavel for review as well).
>
> Yup, can do. I'll try to clean it up some first (especially so it
> doesn't just hang when it fails :/)
That'd of course be nice, but not a hard requirement. A lot of the
regressions tests will crash a broken kernel, so...
--
Jens Axboe
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v4 0/1] io_uring: fix skipping of old timeout events
2021-01-15 18:38 ` Jens Axboe
@ 2021-01-15 18:48 ` Pavel Begunkov
2021-01-15 22:15 ` Marcelo Diop-Gonzalez
0 siblings, 1 reply; 7+ messages in thread
From: Pavel Begunkov @ 2021-01-15 18:48 UTC (permalink / raw)
To: Jens Axboe, Marcelo Diop-Gonzalez; +Cc: io-uring
On 15/01/2021 18:38, Jens Axboe wrote:
> On 1/15/21 11:31 AM, Marcelo Diop-Gonzalez wrote:
>> On Fri, Jan 15, 2021 at 10:02:12AM -0700, Jens Axboe wrote:
>>> On 1/15/21 9:54 AM, Marcelo Diop-Gonzalez wrote:
>>>> This patch tries to fix a problem with IORING_OP_TIMEOUT events
>>>> not being flushed if they should already have expired. The test below
>>>> hangs before this change (unless you run with $ ./a.out ~/somefile 1):
>>>
>>> Can you turn this into a test case for liburing? I'll apply the
>>> associated patch, thanks (and to Pavel for review as well).
>>
>> Yup, can do. I'll try to clean it up some first (especially so it
>> doesn't just hang when it fails :/)
>
> That'd of course be nice, but not a hard requirement. A lot of the
> regressions tests will crash a broken kernel, so...
Ha, they definitely will.
Marcelo, replacing reads with nop requests should trigger it as well,
it's probably easier and even more reliable as we always complete
them inline (if not linked or IOSQE_ASYNC).
--
Pavel Begunkov
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 0/1] io_uring: fix skipping of old timeout events
2021-01-15 18:48 ` Pavel Begunkov
@ 2021-01-15 22:15 ` Marcelo Diop-Gonzalez
0 siblings, 0 replies; 7+ messages in thread
From: Marcelo Diop-Gonzalez @ 2021-01-15 22:15 UTC (permalink / raw)
To: Pavel Begunkov; +Cc: Jens Axboe, io-uring
On Fri, Jan 15, 2021 at 06:48:58PM +0000, Pavel Begunkov wrote:
> On 15/01/2021 18:38, Jens Axboe wrote:
> > On 1/15/21 11:31 AM, Marcelo Diop-Gonzalez wrote:
> >> On Fri, Jan 15, 2021 at 10:02:12AM -0700, Jens Axboe wrote:
> >>> On 1/15/21 9:54 AM, Marcelo Diop-Gonzalez wrote:
> >>>> This patch tries to fix a problem with IORING_OP_TIMEOUT events
> >>>> not being flushed if they should already have expired. The test below
> >>>> hangs before this change (unless you run with $ ./a.out ~/somefile 1):
> >>>
> >>> Can you turn this into a test case for liburing? I'll apply the
> >>> associated patch, thanks (and to Pavel for review as well).
> >>
> >> Yup, can do. I'll try to clean it up some first (especially so it
> >> doesn't just hang when it fails :/)
> >
> > That'd of course be nice, but not a hard requirement. A lot of the
> > regressions tests will crash a broken kernel, so...
>
> Ha, they definitely will.
>
> Marcelo, replacing reads with nop requests should trigger it as well,
> it's probably easier and even more reliable as we always complete
> them inline (if not linked or IOSQE_ASYNC).
Oh good idea, yeah that's better for sure. Didn't even know that existed :D
>
> --
> Pavel Begunkov
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-01-15 22:16 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-01-15 16:54 [PATCH v4 0/1] io_uring: fix skipping of old timeout events Marcelo Diop-Gonzalez
2021-01-15 16:54 ` [PATCH v4 1/1] io_uring: flush timeouts that should already have expired Marcelo Diop-Gonzalez
2021-01-15 17:02 ` [PATCH v4 0/1] io_uring: fix skipping of old timeout events Jens Axboe
2021-01-15 18:31 ` Marcelo Diop-Gonzalez
2021-01-15 18:38 ` Jens Axboe
2021-01-15 18:48 ` Pavel Begunkov
2021-01-15 22:15 ` Marcelo Diop-Gonzalez
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox