| Message ID | 20240509023413.4124075-1-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | io_uring: add IORING_OP_NOP_FAIL | expand |
On 5/8/24 8:34 PM, Ming Lei wrote: > Add IORING_OP_NOP_FAIL so that it is easy to inject failure from > userspace. > > Like IORING_OP_NOP, the main use case is test, and it is very helpful > for covering failure handling code in io_uring core change. Rather than use a new opcode for this, why don't we just add it to the existing NOP? I know we don't check for flags in currently, so you would not know if it worked, but we could add that and just backport that one-liner as well. And if we had such a flag, the fail res could be passed in as well.
On Wed, May 08, 2024 at 08:55:09PM -0600, Jens Axboe wrote: > On 5/8/24 8:34 PM, Ming Lei wrote: > > Add IORING_OP_NOP_FAIL so that it is easy to inject failure from > > userspace. > > > > Like IORING_OP_NOP, the main use case is test, and it is very helpful > > for covering failure handling code in io_uring core change. > > Rather than use a new opcode for this, why don't we just add it to > the existing NOP? I know we don't check for flags in currently, so > you would not know if it worked, but we could add that and just > backport that one-liner as well. Yeah, it is just for avoiding to break existed tests which may not build over liburing. I will switch to this way, looks one-line backporting can solve it. > And if we had such a flag, the fail res could be passed in as well. We can just pass the 'injected_fail_res' via sqe->len, meantime keep 'nop_flags' for error injection and future extension. thanks, Ming
On Thu, May 9, 2024 at 11:05 AM Ming Lei <ming.lei@redhat.com> wrote: > > On Wed, May 08, 2024 at 08:55:09PM -0600, Jens Axboe wrote: > > On 5/8/24 8:34 PM, Ming Lei wrote: > > > Add IORING_OP_NOP_FAIL so that it is easy to inject failure from > > > userspace. > > > > > > Like IORING_OP_NOP, the main use case is test, and it is very helpful > > > for covering failure handling code in io_uring core change. > > > > Rather than use a new opcode for this, why don't we just add it to > > the existing NOP? I know we don't check for flags in currently, so > > you would not know if it worked, but we could add that and just > > backport that one-liner as well. > > Yeah, it is just for avoiding to break existed tests which may not build > over liburing. > > I will switch to this way, looks one-line backporting can solve it. I guess backporting can't work, because application code expects NOP to complete successfully with and w/o non-zero sqe->rw_flags. However, the backport has to fail NOP in case of non-zero sqe->rw_flags. Thanks.
On 5/8/24 9:05 PM, Ming Lei wrote: > On Wed, May 08, 2024 at 08:55:09PM -0600, Jens Axboe wrote: >> On 5/8/24 8:34 PM, Ming Lei wrote: >>> Add IORING_OP_NOP_FAIL so that it is easy to inject failure from >>> userspace. >>> >>> Like IORING_OP_NOP, the main use case is test, and it is very helpful >>> for covering failure handling code in io_uring core change. >> >> Rather than use a new opcode for this, why don't we just add it to >> the existing NOP? I know we don't check for flags in currently, so >> you would not know if it worked, but we could add that and just >> backport that one-liner as well. > > Yeah, it is just for avoiding to break existed tests which may not build > over liburing. Don't think that's a huge risk, if someone is using the raw interface and just not clearing, they would run into issues with mixed command usage anyway. And a pure nop test would be fine anyway, as everything starts out cleared. > I will switch to this way, looks one-line backporting can solve it. Exactly, thanks! >> And if we had such a flag, the fail res could be passed in as well. > > We can just pass the 'injected_fail_res' via sqe->len, meantime keep > 'nop_flags' for error injection and future extension. Precisely.
diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h index 922f29b07ccc..18e58477e0f0 100644 --- a/include/uapi/linux/io_uring.h +++ b/include/uapi/linux/io_uring.h @@ -72,6 +72,7 @@ struct io_uring_sqe { __u32 waitid_flags; __u32 futex_flags; __u32 install_fd_flags; + __s32 nop_fail_res; }; __u64 user_data; /* data to be passed back at completion time */ /* pack this to avoid bogus arm OABI complaints */ @@ -259,6 +260,7 @@ enum io_uring_op { IORING_OP_FUTEX_WAITV, IORING_OP_FIXED_FD_INSTALL, IORING_OP_FTRUNCATE, + IORING_OP_NOP_FAIL, /* this goes last, obviously */ IORING_OP_LAST, diff --git a/io_uring/nop.c b/io_uring/nop.c index d956599a3c1b..c30547e53b5c 100644 --- a/io_uring/nop.c +++ b/io_uring/nop.c @@ -10,6 +10,12 @@ #include "io_uring.h" #include "nop.h" +struct io_nop_fail { + /* NOTE: kiocb has the file as the first member, so don't do it here */ + struct file *file; + int res; +}; + int io_nop_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) { return 0; @@ -23,3 +29,24 @@ int io_nop(struct io_kiocb *req, unsigned int issue_flags) io_req_set_res(req, 0, 0); return IOU_OK; } + +int io_nop_fail_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) +{ + struct io_nop_fail *nf = io_kiocb_to_cmd(req, struct io_nop_fail); + + nf->res = READ_ONCE(sqe->nop_fail_res); + return 0; +} + +/* + * IORING_OP_NOP just posts a completion event, nothing else. + */ +int io_nop_fail(struct io_kiocb *req, unsigned int issue_flags) +{ + struct io_nop_fail *nf = io_kiocb_to_cmd(req, struct io_nop_fail); + + if (nf->res < 0) + req_set_fail(req); + io_req_set_res(req, nf->res, 0); + return IOU_OK; +} diff --git a/io_uring/nop.h b/io_uring/nop.h index 97f1535c9dec..ef40d3b15899 100644 --- a/io_uring/nop.h +++ b/io_uring/nop.h @@ -2,3 +2,6 @@ int io_nop_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe); int io_nop(struct io_kiocb *req, unsigned int issue_flags); + +int io_nop_fail_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe); +int io_nop_fail(struct io_kiocb *req, unsigned int issue_flags); diff --git a/io_uring/opdef.c b/io_uring/opdef.c index 92b657a063a0..eadc5a12ee06 100644 --- a/io_uring/opdef.c +++ b/io_uring/opdef.c @@ -56,6 +56,12 @@ const struct io_issue_def io_issue_defs[] = { .prep = io_nop_prep, .issue = io_nop, }, + [IORING_OP_NOP_FAIL] = { + .audit_skip = 1, + .iopoll = 1, + .prep = io_nop_fail_prep, + .issue = io_nop_fail, + }, [IORING_OP_READV] = { .needs_file = 1, .unbound_nonreg_file = 1, @@ -506,6 +512,9 @@ const struct io_cold_def io_cold_defs[] = { [IORING_OP_NOP] = { .name = "NOP", }, + [IORING_OP_NOP_FAIL] = { + .name = "NOP_FAIL", + }, [IORING_OP_READV] = { .name = "READV", .cleanup = io_readv_writev_cleanup,
Add IORING_OP_NOP_FAIL so that it is easy to inject failure from userspace. Like IORING_OP_NOP, the main use case is test, and it is very helpful for covering failure handling code in io_uring core change. Signed-off-by: Ming Lei <ming.lei@redhat.com> --- include/uapi/linux/io_uring.h | 2 ++ io_uring/nop.c | 27 +++++++++++++++++++++++++++ io_uring/nop.h | 3 +++ io_uring/opdef.c | 9 +++++++++ 4 files changed, 41 insertions(+)