| Message ID | 20240510011421.55139-1-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | io_uring: support to inject result for NOP | expand |
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.
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
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; }
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(-)