[PATCH 0/6] return an error when cqe is dropped

[PATCH 0/6] return an error when cqe is dropped

[Dylan Yudaken]

This series addresses a rare but real error condition when a CQE is
dropped. Many applications rely on 1 SQE resulting in 1 CQE and may even
block waiting for the CQE. In overflow conditions if the GFP_ATOMIC
allocation fails, the CQE is dropped and a counter is incremented. However
the application is not actively signalled that something bad has
happened. We would like to indicate this error condition to the
application but in a way that does not rely on the application doing
invasive changes such as checking a flag before each wait.

This series returns an error code to the application when the error hits,
and then resets the error condition. If the application is ok with this
error it can continue as is, or more likely it can clean up sanely.

Patches 1&2 add tracing for overflows
Patches 3&4 prep for adding this error
Patch 5 is the main one returning an error
Patch 6 allows liburing to test these conditions more easily with IOPOLL

Dylan Yudaken (6):
  io_uring: add trace support for CQE overflow
  io_uring: trace cqe overflows
  io_uring: rework io_uring_enter to simplify return value
  io_uring: use constants for cq_overflow bitfield
  io_uring: return an error when cqe is dropped
  io_uring: allow NOP opcode in IOPOLL mode

 fs/io_uring.c                   | 89 ++++++++++++++++++++++-----------
 include/trace/events/io_uring.h | 42 +++++++++++++++-
 2 files changed, 102 insertions(+), 29 deletions(-)


base-commit: 7c648b7d6186c59ed3a0e0ae4b774aaf4b415ef2
-- 
2.30.2

[PATCH 1/6] io_uring: add trace support for CQE overflow

[Dylan Yudaken]

Add trace function for overflowing CQ ring.

Signed-off-by: Dylan Yudaken <dylany@fb.com>
---
 include/trace/events/io_uring.h | 42 ++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/include/trace/events/io_uring.h b/include/trace/events/io_uring.h
index cddf5b6fbeb4..42534ec2ab9d 100644
--- a/include/trace/events/io_uring.h
+++ b/include/trace/events/io_uring.h
@@ -530,7 +530,7 @@ TRACE_EVENT(io_uring_req_failed,
 	),
 
 	TP_printk("ring %p, req %p, user_data 0x%llx, "
-		"op %d, flags 0x%x, prio=%d, off=%llu, addr=%llu, "
+		  "op %d, flags 0x%x, prio=%d, off=%llu, addr=%llu, "
 		  "len=%u, rw_flags=0x%x, buf_index=%d, "
 		  "personality=%d, file_index=%d, pad=0x%llx/%llx, error=%d",
 		  __entry->ctx, __entry->req, __entry->user_data,
@@ -543,6 +543,46 @@ TRACE_EVENT(io_uring_req_failed,
 		  (unsigned long long) __entry->pad2, __entry->error)
 );
 
+
+/*
+ * io_uring_cqe_overflow - a CQE overflowed
+ *
+ * @ctx:		pointer to a ring context structure
+ * @user_data:		user data associated with the request
+ * @res:		CQE result
+ * @cflags:		CQE flags
+ * @ocqe:		pointer to the overflow cqe (if available)
+ *
+ */
+TRACE_EVENT(io_uring_cqe_overflow,
+
+	TP_PROTO(void *ctx, unsigned long long user_data, s32 res, u32 cflags,
+		 void *ocqe),
+
+	TP_ARGS(ctx, user_data, res, cflags, ocqe),
+
+	TP_STRUCT__entry (
+		__field(  void *,		ctx		)
+		__field(  unsigned long long,	user_data	)
+		__field(  s32,			res		)
+		__field(  u32,			cflags		)
+		__field(  void *,		ocqe		)
+	),
+
+	TP_fast_assign(
+		__entry->ctx		= ctx;
+		__entry->user_data	= user_data;
+		__entry->res		= res;
+		__entry->cflags		= cflags;
+		__entry->ocqe		= ocqe;
+	),
+
+	TP_printk("ring %p, user_data 0x%llx, res %d, flags %x, "
+		  "overflow_cqe %p",
+		  __entry->ctx, __entry->user_data, __entry->res,
+		  __entry->cflags, __entry->ocqe)
+);
+
 #endif /* _TRACE_IO_URING_H */
 
 /* This part must be outside protection */
-- 
2.30.2

[PATCH 2/6] io_uring: trace cqe overflows

[Dylan Yudaken]

Trace cqe overflows in io_uring. Print ocqe before the check, so if it is
NULL it indicates that it has been dropped.

Signed-off-by: Dylan Yudaken <dylany@fb.com>
---
 fs/io_uring.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 7e1d5243bbbc..d654faffa486 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2107,6 +2107,7 @@ static bool io_cqring_event_overflow(struct io_ring_ctx *ctx, u64 user_data,
 	struct io_overflow_cqe *ocqe;
 
 	ocqe = kmalloc(sizeof(*ocqe), GFP_ATOMIC | __GFP_ACCOUNT);
+	trace_io_uring_cqe_overflow(ctx, user_data, res, cflags, ocqe);
 	if (!ocqe) {
 		/*
 		 * If we're in ring overflow flush mode, or in task cancel mode,
-- 
2.30.2

[PATCH 3/6] io_uring: rework io_uring_enter to simplify return value

[Dylan Yudaken]

io_uring_enter returns the count submitted preferrably over an error
code. In some code paths this check is not required, so reorganise the
code so that the check is only done as needed.
This is also a prep for returning error codes only in waiting scenarios.

Signed-off-by: Dylan Yudaken <dylany@fb.com>
---
 fs/io_uring.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index d654faffa486..1837b3afa47f 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -10843,7 +10843,6 @@ SYSCALL_DEFINE6(io_uring_enter, unsigned int, fd, u32, to_submit,
 		size_t, argsz)
 {
 	struct io_ring_ctx *ctx;
-	int submitted = 0;
 	struct fd f;
 	long ret;
 
@@ -10906,15 +10905,15 @@ SYSCALL_DEFINE6(io_uring_enter, unsigned int, fd, u32, to_submit,
 			if (ret)
 				goto out;
 		}
-		submitted = to_submit;
+		ret = to_submit;
 	} else if (to_submit) {
 		ret = io_uring_add_tctx_node(ctx);
 		if (unlikely(ret))
 			goto out;
 
 		mutex_lock(&ctx->uring_lock);
-		submitted = io_submit_sqes(ctx, to_submit);
-		if (submitted != to_submit) {
+		ret = io_submit_sqes(ctx, to_submit);
+		if (ret != to_submit) {
 			mutex_unlock(&ctx->uring_lock);
 			goto out;
 		}
@@ -10923,6 +10922,7 @@ SYSCALL_DEFINE6(io_uring_enter, unsigned int, fd, u32, to_submit,
 		mutex_unlock(&ctx->uring_lock);
 	}
 	if (flags & IORING_ENTER_GETEVENTS) {
+		int ret2;
 		if (ctx->syscall_iopoll) {
 			/*
 			 * We disallow the app entering submit/complete with
@@ -10932,22 +10932,29 @@ SYSCALL_DEFINE6(io_uring_enter, unsigned int, fd, u32, to_submit,
 			 */
 			mutex_lock(&ctx->uring_lock);
 iopoll_locked:
-			ret = io_validate_ext_arg(flags, argp, argsz);
-			if (likely(!ret)) {
-				min_complete = min(min_complete, ctx->cq_entries);
-				ret = io_iopoll_check(ctx, min_complete);
+			ret2 = io_validate_ext_arg(flags, argp, argsz);
+			if (likely(!ret2)) {
+				min_complete = min(min_complete,
+						   ctx->cq_entries);
+				ret2 = io_iopoll_check(ctx, min_complete);
 			}
 			mutex_unlock(&ctx->uring_lock);
 		} else {
 			const sigset_t __user *sig;
 			struct __kernel_timespec __user *ts;
 
-			ret = io_get_ext_arg(flags, argp, &argsz, &ts, &sig);
-			if (unlikely(ret))
-				goto out;
-			min_complete = min(min_complete, ctx->cq_entries);
-			ret = io_cqring_wait(ctx, min_complete, sig, argsz, ts);
+			ret2 = io_get_ext_arg(flags, argp, &argsz, &ts, &sig);
+			if (likely(!ret2)) {
+				min_complete = min(min_complete,
+						   ctx->cq_entries);
+				ret2 = io_cqring_wait(ctx, min_complete, sig,
+						      argsz, ts);
+			}
 		}
+
+		if (!ret)
+			ret = ret2;
+
 	}
 
 out:
@@ -10955,7 +10962,7 @@ SYSCALL_DEFINE6(io_uring_enter, unsigned int, fd, u32, to_submit,
 out_fput:
 	if (!(flags & IORING_ENTER_REGISTERED_RING))
 		fdput(f);
-	return submitted ? submitted : ret;
+	return ret;
 }
 
 #ifdef CONFIG_PROC_FS
-- 
2.30.2

[PATCH 4/6] io_uring: use constants for cq_overflow bitfield

[Dylan Yudaken]

Prepare to use this bitfield for more flags by using constants instead of
magic value 0

Signed-off-by: Dylan Yudaken <dylany@fb.com>
---
 fs/io_uring.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 1837b3afa47f..db878c114e16 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -431,7 +431,7 @@ struct io_ring_ctx {
 	struct wait_queue_head	sqo_sq_wait;
 	struct list_head	sqd_list;
 
-	unsigned long		check_cq_overflow;
+	unsigned long		check_cq;
 
 	struct {
 		/*
@@ -903,6 +903,10 @@ struct io_cqe {
 	};
 };
 
+enum {
+	IO_CHECK_CQ_OVERFLOW_BIT,
+};
+
 /*
  * NOTE! Each of the iocb union members has the file pointer
  * as the first entry in their struct definition. So you can
@@ -2024,7 +2028,7 @@ static bool __io_cqring_overflow_flush(struct io_ring_ctx *ctx, bool force)
 
 	all_flushed = list_empty(&ctx->cq_overflow_list);
 	if (all_flushed) {
-		clear_bit(0, &ctx->check_cq_overflow);
+		clear_bit(IO_CHECK_CQ_OVERFLOW_BIT, &ctx->check_cq);
 		WRITE_ONCE(ctx->rings->sq_flags,
 			   ctx->rings->sq_flags & ~IORING_SQ_CQ_OVERFLOW);
 	}
@@ -2040,7 +2044,7 @@ static bool io_cqring_overflow_flush(struct io_ring_ctx *ctx)
 {
 	bool ret = true;
 
-	if (test_bit(0, &ctx->check_cq_overflow)) {
+	if (test_bit(IO_CHECK_CQ_OVERFLOW_BIT, &ctx->check_cq)) {
 		/* iopoll syncs against uring_lock, not completion_lock */
 		if (ctx->flags & IORING_SETUP_IOPOLL)
 			mutex_lock(&ctx->uring_lock);
@@ -2118,7 +2122,7 @@ static bool io_cqring_event_overflow(struct io_ring_ctx *ctx, u64 user_data,
 		return false;
 	}
 	if (list_empty(&ctx->cq_overflow_list)) {
-		set_bit(0, &ctx->check_cq_overflow);
+		set_bit(IO_CHECK_CQ_OVERFLOW_BIT, &ctx->check_cq);
 		WRITE_ONCE(ctx->rings->sq_flags,
 			   ctx->rings->sq_flags | IORING_SQ_CQ_OVERFLOW);
 
@@ -2961,7 +2965,7 @@ static int io_iopoll_check(struct io_ring_ctx *ctx, long min)
 	 * If we do, we can potentially be spinning for commands that
 	 * already triggered a CQE (eg in error).
 	 */
-	if (test_bit(0, &ctx->check_cq_overflow))
+	if (test_bit(IO_CHECK_CQ_OVERFLOW_BIT, &ctx->check_cq))
 		__io_cqring_overflow_flush(ctx, false);
 	if (io_cqring_events(ctx))
 		return 0;
@@ -8271,7 +8275,8 @@ static int io_wake_function(struct wait_queue_entry *curr, unsigned int mode,
 	 * Cannot safely flush overflowed CQEs from here, ensure we wake up
 	 * the task, and the next invocation will do it.
 	 */
-	if (io_should_wake(iowq) || test_bit(0, &iowq->ctx->check_cq_overflow))
+	if (io_should_wake(iowq) ||
+	    test_bit(IO_CHECK_CQ_OVERFLOW_BIT, &iowq->ctx->check_cq))
 		return autoremove_wake_function(curr, mode, wake_flags, key);
 	return -1;
 }
@@ -8299,7 +8304,7 @@ static inline int io_cqring_wait_schedule(struct io_ring_ctx *ctx,
 	if (ret || io_should_wake(iowq))
 		return ret;
 	/* let the caller flush overflows, retry */
-	if (test_bit(0, &ctx->check_cq_overflow))
+	if (test_bit(IO_CHECK_CQ_OVERFLOW_BIT, &ctx->check_cq))
 		return 1;
 
 	if (!schedule_hrtimeout(&timeout, HRTIMER_MODE_ABS))
@@ -10094,7 +10099,8 @@ static __poll_t io_uring_poll(struct file *file, poll_table *wait)
 	 * Users may get EPOLLIN meanwhile seeing nothing in cqring, this
 	 * pushs them to do the flush.
 	 */
-	if (io_cqring_events(ctx) || test_bit(0, &ctx->check_cq_overflow))
+	if (io_cqring_events(ctx) ||
+	    test_bit(IO_CHECK_CQ_OVERFLOW_BIT, &ctx->check_cq))
 		mask |= EPOLLIN | EPOLLRDNORM;
 
 	return mask;
-- 
2.30.2

[PATCH 5/6] io_uring: return an error when cqe is dropped

[Dylan Yudaken]

Right now io_uring will not actively inform userspace if a CQE is
dropped. This is extremely rare, requiring a CQ ring overflow, as well as
a GFP_ATOMIC kmalloc failure. However the consequences could cause for
example applications to go into an undefined state, possibly waiting for a
CQE that never arrives.

Return an error code (EBADR) in these cases. Since this is expected to be
incredibly rare, try and avoid as much as possible affecting the hot code
paths, and so it only is returned lazily and when there is no other
available CQEs.

Once the error is returned, reset the error condition assuming the user is
either ok with it or will clean up appropriately.

Signed-off-by: Dylan Yudaken <dylany@fb.com>
---
 fs/io_uring.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index db878c114e16..e46dc67c917c 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -905,6 +905,7 @@ struct io_cqe {
 
 enum {
 	IO_CHECK_CQ_OVERFLOW_BIT,
+	IO_CHECK_CQ_DROPPED_BIT,
 };
 
 /*
@@ -2119,6 +2120,7 @@ static bool io_cqring_event_overflow(struct io_ring_ctx *ctx, u64 user_data,
 		 * on the floor.
 		 */
 		io_account_cq_overflow(ctx);
+		set_bit(IO_CHECK_CQ_DROPPED_BIT, &ctx->check_cq);
 		return false;
 	}
 	if (list_empty(&ctx->cq_overflow_list)) {
@@ -2959,16 +2961,26 @@ static int io_iopoll_check(struct io_ring_ctx *ctx, long min)
 {
 	unsigned int nr_events = 0;
 	int ret = 0;
+	unsigned long check_cq;
 
 	/*
 	 * Don't enter poll loop if we already have events pending.
 	 * If we do, we can potentially be spinning for commands that
 	 * already triggered a CQE (eg in error).
 	 */
-	if (test_bit(IO_CHECK_CQ_OVERFLOW_BIT, &ctx->check_cq))
+	check_cq = READ_ONCE(ctx->check_cq);
+	if (check_cq & BIT(IO_CHECK_CQ_OVERFLOW_BIT))
 		__io_cqring_overflow_flush(ctx, false);
 	if (io_cqring_events(ctx))
 		return 0;
+
+	/*
+	 * Similarly do not spin if we have not informed the user of any
+	 * dropped CQE.
+	 */
+	if (unlikely(check_cq & BIT(IO_CHECK_CQ_DROPPED_BIT)))
+		return -EBADR;
+
 	do {
 		/*
 		 * If a submit got punted to a workqueue, we can have the
@@ -8298,15 +8310,18 @@ static inline int io_cqring_wait_schedule(struct io_ring_ctx *ctx,
 					  ktime_t timeout)
 {
 	int ret;
+	unsigned long check_cq;
 
 	/* make sure we run task_work before checking for signals */
 	ret = io_run_task_work_sig();
 	if (ret || io_should_wake(iowq))
 		return ret;
+	check_cq = READ_ONCE(ctx->check_cq);
 	/* let the caller flush overflows, retry */
-	if (test_bit(IO_CHECK_CQ_OVERFLOW_BIT, &ctx->check_cq))
+	if (check_cq & BIT(IO_CHECK_CQ_OVERFLOW_BIT))
 		return 1;
-
+	if (unlikely(check_cq & BIT(IO_CHECK_CQ_DROPPED_BIT)))
+		return -EBADR;
 	if (!schedule_hrtimeout(&timeout, HRTIMER_MODE_ABS))
 		return -ETIME;
 	return 1;
@@ -10958,9 +10973,18 @@ SYSCALL_DEFINE6(io_uring_enter, unsigned int, fd, u32, to_submit,
 			}
 		}
 
-		if (!ret)
+		if (!ret) {
 			ret = ret2;
 
+			/*
+			 * EBADR indicates that one or more CQE were dropped.
+			 * Once the user has been informed we can clear the bit
+			 * as they are obviously ok with those drops.
+			 */
+			if (unlikely(ret2 == -EBADR))
+				clear_bit(IO_CHECK_CQ_DROPPED_BIT,
+					  &ctx->check_cq);
+		}
 	}
 
 out:
-- 
2.30.2

[PATCH 6/6] io_uring: allow NOP opcode in IOPOLL mode

[Dylan Yudaken]

This is useful for tests so that IOPOLL can be tested without requiring
files. NOP is acceptable in IOPOLL as it always completes immediately.

Signed-off-by: Dylan Yudaken <dylany@fb.com>
---
 fs/io_uring.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index e46dc67c917c..a4e42ba708b4 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -4526,11 +4526,6 @@ static int io_splice(struct io_kiocb *req, unsigned int issue_flags)
  */
 static int io_nop(struct io_kiocb *req, unsigned int issue_flags)
 {
-	struct io_ring_ctx *ctx = req->ctx;
-
-	if (unlikely(ctx->flags & IORING_SETUP_IOPOLL))
-		return -EINVAL;
-
 	__io_req_complete(req, issue_flags, 0, 0);
 	return 0;
 }
-- 
2.30.2

Re: [PATCH 0/6] return an error when cqe is dropped

[Jens Axboe]

On Thu, 21 Apr 2022 02:13:39 -0700, Dylan Yudaken wrote:
> This series addresses a rare but real error condition when a CQE is
> dropped. Many applications rely on 1 SQE resulting in 1 CQE and may even
> block waiting for the CQE. In overflow conditions if the GFP_ATOMIC
> allocation fails, the CQE is dropped and a counter is incremented. However
> the application is not actively signalled that something bad has
> happened. We would like to indicate this error condition to the
> application but in a way that does not rely on the application doing
> invasive changes such as checking a flag before each wait.
> 
> [...]

Applied, thanks!

[1/6] io_uring: add trace support for CQE overflow
      commit: f457ab8deb017140aef05be3027a00a18a7d16b7
[2/6] io_uring: trace cqe overflows
      commit: 2a847e6faf76810ae68a6e81bd9ac3a7c81534d0
[3/6] io_uring: rework io_uring_enter to simplify return value
      commit: db9bb58b391c9e62da68bc139598e8470d892c77
[4/6] io_uring: use constants for cq_overflow bitfield
      commit: b293240e2634b2100196d7314aeeb84299ce6d5b
[5/6] io_uring: return an error when cqe is dropped
      commit: 34a7ee8a42c8496632465f3f0b444b3a7b908c46
[6/6] io_uring: allow NOP opcode in IOPOLL mode
      commit: ebbe59f49556822b9bcc7b0d4d96bae31f522905

Best regards,
-- 
Jens Axboe

Re: [PATCH 6/6] io_uring: allow NOP opcode in IOPOLL mode

[Jens Axboe]

On Thu, Apr 21, 2022 at 3:17 AM Dylan Yudaken <dylany@fb.com> wrote:
>
> This is useful for tests so that IOPOLL can be tested without requiring
> files. NOP is acceptable in IOPOLL as it always completes immediately.

This one actually breaks two liburing test cases (link and defer) that
assume NOP on IOPOLL will return -EINVAL. Not a huge deal, but we do
need to figure out how to make them reliably -EINVAL in a different
way then.

Maybe add a nop_flags to the usual flags spot in the sqe, and define
a flag that says NOP_IOPOLL or something. Require this flag set for
allowing NOP on iopoll. That'd allow testing, but still retain the
-EINVAL behavior if not set.

Alternatively, modify test cases...

I'll drop this one for now, just because it fails the regression
tests.

-- 
Jens Axboe

Re: [PATCH 6/6] io_uring: allow NOP opcode in IOPOLL mode

[Dylan Yudaken]

On Thu, 2022-04-21 at 17:33 -0600, Jens Axboe wrote:
> On Thu, Apr 21, 2022 at 3:17 AM Dylan Yudaken <dylany@fb.com> wrote:
> > 
> > This is useful for tests so that IOPOLL can be tested without
> > requiring
> > files. NOP is acceptable in IOPOLL as it always completes
> > immediately.
> 
> This one actually breaks two liburing test cases (link and defer)
> that
> assume NOP on IOPOLL will return -EINVAL. Not a huge deal, but we do
> need to figure out how to make them reliably -EINVAL in a different
> way then.
> 
> Maybe add a nop_flags to the usual flags spot in the sqe, and define
> a flag that says NOP_IOPOLL or something. Require this flag set for
> allowing NOP on iopoll. That'd allow testing, but still retain the
> -EINVAL behavior if not set.
> 
> Alternatively, modify test cases...
> 
> I'll drop this one for now, just because it fails the regression
> tests.
> 

That's fine - sorry I didn't notice that. I think fixing the tests is
the better approach here. It should be easy to get an -EINVAL from it.