[PATCH] io_uring: support to inject result for NOP

[PATCH] io_uring: support to inject result for NOP

[Ming Lei]

Support to inject result for NOP so that we can inject failure from
userspace. It is very helpful for covering failure handling code in
io_uring core change.

With nop flags, it could be possible to add more test feature for NOP in
future, but the NOP behavior of direct completion has to be kept.

Cleared NOP SQE is required, look both liburing and Rust io-uring crate
clears SQE, and it shouldn't be one big deal for raw, especially it is
just NOP.

Suggested-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 include/uapi/linux/io_uring.h | 13 ++++++++++++-
 io_uring/nop.c                | 28 ++++++++++++++++++++++++----
 2 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index 922f29b07ccc..5db3a209b302 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -48,7 +48,10 @@ struct io_uring_sqe {
 			__u32	optname;
 		};
 	};
-	__u32	len;		/* buffer size or number of iovecs */
+	union {
+		__u32	len;		/* buffer size or number of iovecs */
+		__s32	result;		/* for NOP to inject result only */
+	};
 	union {
 		__kernel_rwf_t	rw_flags;
 		__u32		fsync_flags;
@@ -72,6 +75,7 @@ struct io_uring_sqe {
 		__u32		waitid_flags;
 		__u32		futex_flags;
 		__u32		install_fd_flags;
+		__u32		nop_flags;
 	};
 	__u64	user_data;	/* data to be passed back at completion time */
 	/* pack this to avoid bogus arm OABI complaints */
@@ -413,6 +417,13 @@ enum io_uring_msg_ring_flags {
  */
 #define IORING_FIXED_FD_NO_CLOEXEC	(1U << 0)
 
+/*
+ * IORING_OP_NOP flags (sqe->nop_flags)
+ *
+ * IORING_NOP_INJECT_RESULT	Inject result from sqe->result
+ */
+#define IORING_NOP_INJECT_RESULT	(1U << 0)
+
 /*
  * IO completion data structure (Completion Queue Entry)
  */
diff --git a/io_uring/nop.c b/io_uring/nop.c
index d956599a3c1b..c002b8ef2d5f 100644
--- a/io_uring/nop.c
+++ b/io_uring/nop.c
@@ -10,16 +10,36 @@
 #include "io_uring.h"
 #include "nop.h"
 
+struct io_nop {
+	/* NOTE: kiocb has the file as the first member, so don't do it here */
+	struct file     *file;
+	unsigned int	flags;
+	int             result;
+};
+
 int io_nop_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 {
+	struct io_nop *nop = io_kiocb_to_cmd(req, struct io_nop);
+	unsigned int flags;
+
+	flags = READ_ONCE(sqe->nop_flags);
+	if (flags & ~IORING_NOP_INJECT_RESULT)
+		return -EINVAL;
+
+	nop->flags = flags;
+	if (nop->flags & IORING_NOP_INJECT_RESULT)
+		nop->result = READ_ONCE(sqe->result);
+	else
+		nop->result = 0;
 	return 0;
 }
 
-/*
- * IORING_OP_NOP just posts a completion event, nothing else.
- */
 int io_nop(struct io_kiocb *req, unsigned int issue_flags)
 {
-	io_req_set_res(req, 0, 0);
+	struct io_nop *nop = io_kiocb_to_cmd(req, struct io_nop);
+
+	if (nop->result < 0)
+		req_set_fail(req);
+	io_req_set_res(req, nop->result, 0);
 	return IOU_OK;
 }
-- 
2.44.0

Re: [PATCH] io_uring: support to inject result for NOP

[Jens Axboe]

On 5/9/24 7:14 PM, Ming Lei wrote:
> Support to inject result for NOP so that we can inject failure from
> userspace. It is very helpful for covering failure handling code in
> io_uring core change.
> 
> With nop flags, it could be possible to add more test feature for NOP in
> future, but the NOP behavior of direct completion has to be kept.
> 
> Cleared NOP SQE is required, look both liburing and Rust io-uring crate
> clears SQE, and it shouldn't be one big deal for raw, especially it is
> just NOP.

I think this implementation looks fine, but probably would be best if
you first had a prep patch that adds nop_flags to io_uring_sqe, and
checks it in io_nop_prep() and fails if it's non-zero. Then we can mark
that for stable, rather than need to do the whole thing.

Then patch 2 adds the actual meat of this patch, and now adds the proper
check in io_nop_prep() for whether any unknown flags are set.

> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
> index 922f29b07ccc..5db3a209b302 100644
> --- a/include/uapi/linux/io_uring.h
> +++ b/include/uapi/linux/io_uring.h
> @@ -48,7 +48,10 @@ struct io_uring_sqe {
>  			__u32	optname;
>  		};
>  	};
> -	__u32	len;		/* buffer size or number of iovecs */
> +	union {
> +		__u32	len;		/* buffer size or number of iovecs */
> +		__s32	result;		/* for NOP to inject result only */
> +	};
>  	union {

And I'd drop that, just use 'len' throughout.

Rest of the patch looks fine as-is.

-- 
Jens Axboe

Re: [PATCH] io_uring: support to inject result for NOP

[Ming Lei]

On Thu, May 09, 2024 at 08:19:40PM -0600, Jens Axboe wrote:
> On 5/9/24 7:14 PM, Ming Lei wrote:
> > Support to inject result for NOP so that we can inject failure from
> > userspace. It is very helpful for covering failure handling code in
> > io_uring core change.
> > 
> > With nop flags, it could be possible to add more test feature for NOP in
> > future, but the NOP behavior of direct completion has to be kept.
> > 
> > Cleared NOP SQE is required, look both liburing and Rust io-uring crate
> > clears SQE, and it shouldn't be one big deal for raw, especially it is
> > just NOP.
> 
> I think this implementation looks fine, but probably would be best if
> you first had a prep patch that adds nop_flags to io_uring_sqe, and
> checks it in io_nop_prep() and fails if it's non-zero. Then we can mark
> that for stable, rather than need to do the whole thing.
> 
> Then patch 2 adds the actual meat of this patch, and now adds the proper
> check in io_nop_prep() for whether any unknown flags are set.
> 
> > diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
> > index 922f29b07ccc..5db3a209b302 100644
> > --- a/include/uapi/linux/io_uring.h
> > +++ b/include/uapi/linux/io_uring.h
> > @@ -48,7 +48,10 @@ struct io_uring_sqe {
> >  			__u32	optname;
> >  		};
> >  	};
> > -	__u32	len;		/* buffer size or number of iovecs */
> > +	union {
> > +		__u32	len;		/* buffer size or number of iovecs */
> > +		__s32	result;		/* for NOP to inject result only */
> > +	};
> >  	union {
> 
> And I'd drop that, just use 'len' throughout.
> 
> Rest of the patch looks fine as-is.

OK, will do both two in V2.

Thanks,
Ming