On 12/6/23 01:41, Jens Axboe wrote:
On 12/5/23 6:26 PM, Pavel Begunkov wrote:
On 12/5/23 22:00, Jens Axboe wrote:
On 12/5/23 2:55 PM, Keith Busch wrote:
From: Keith Busch <kbusch@xxxxxxxxxx>
No need to rebuild the issue_flags on every IO: they're always the same.
Suggested-by: Jens Axboe <axboe@xxxxxxxxx>
Signed-off-by: Keith Busch <kbusch@xxxxxxxxxx>
---
[...]
diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
index 8a38b9f75d841..dbc0bfbfd0f05 100644
--- a/io_uring/uring_cmd.c
+++ b/io_uring/uring_cmd.c
@@ -158,19 +158,13 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
if (ret)
return ret;
- if (ctx->flags & IORING_SETUP_SQE128)
- issue_flags |= IO_URING_F_SQE128;
- if (ctx->flags & IORING_SETUP_CQE32)
- issue_flags |= IO_URING_F_CQE32;
- if (ctx->compat)
- issue_flags |= IO_URING_F_COMPAT;
if (ctx->flags & IORING_SETUP_IOPOLL) {
if (!file->f_op->uring_cmd_iopoll)
return -EOPNOTSUPP;
- issue_flags |= IO_URING_F_IOPOLL;
req->iopoll_completed = 0;
}
+ issue_flags |= ctx->issue_flags;
ret = file->f_op->uring_cmd(ioucmd, issue_flags);
if (ret == -EAGAIN) {
if (!req_has_async_data(req)) {
I obviously like this idea, but it should be accompanied by getting rid
of ->compat and ->syscall_iopoll in the ctx as well?
This all piggy backing cmd specific bits onto core io_uring issue_flags
business is pretty nasty. Apart from that, it mixes constant io_uring
flags and "execution context" issue_flags. And we're dancing around it
not really addressing the problem.
IMHO, cmds should be testing for IORING_SETUP flags directly via
helpers, not renaming them and abusing core io_uring flags. E.g. I had
a patch like below but didn't care enough to send:
diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 909377068a87..1a82a0633f16 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -2874,7 +2874,7 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
ublk_ctrl_cmd_dump(cmd);
- if (!(issue_flags & IO_URING_F_SQE128))
+ if (!(io_uring_cmd_get_ctx_flags(cmd) & IORING_SETUP_SQE128))
goto out;
ret = ublk_check_cmd_op(cmd_op);
diff --git a/include/linux/io_uring/cmd.h b/include/linux/io_uring/cmd.h
index d69b4038aa3e..8a18a705ff31 100644
--- a/include/linux/io_uring/cmd.h
+++ b/include/linux/io_uring/cmd.h
@@ -79,4 +79,11 @@ static inline struct task_struct *io_uring_cmd_get_task(struct io_uring_cmd *cmd
return cmd_to_io_kiocb(cmd)->task;
}
+static inline unsigned io_uring_cmd_get_ctx_flags(struct io_uring_cmd *cmd)
+{
+ struct io_ring_ctx *ctx = cmd_to_io_kiocb(cmd)->ctx;
+
+ return ctx->flags;
+}
+
#endif /* _LINUX_IO_URING_CMD_H */
Yeah this is fine too, I just don't like our current scheme of having to
mirror state in issue flags. Consolidating one way or another would be
really nice.
Just hiding ->compat into the cache won't help it, most of the cmd
flags mirror IORING_SETUP_*, so unless it checks IORING_SETUP_* directly
there will be this duplication.
--
Pavel Begunkov
