On 8/12/24 12:26, Christoph Hellwig wrote:
On Fri, Aug 09, 2024 at 06:35:27PM +0100, Mark Harmstone wrote:
Mark, please CC io_uring for next versions, I've only found out
about the patch because Christoph added us.
Adds an io_uring interface for asynchronous encoded reads, using the
same interface as for the ioctl. To use this you would use an SQE opcode
of IORING_OP_URING_CMD, the cmd_op would be BTRFS_IOC_ENCODED_READ, and
addr would point to the userspace address of the
btrfs_ioctl_encoded_io_args struct. As with the ioctl, you need to have
CAP_SYS_ADMIN for this to work.
What is the point if this doesn't actually do anything but returning
-EIOCBQUEUED?
Note that that the internals of the btrfs encoded read is built
around kiocbs anyway, so you might as well turn things upside down,
implement a real async io_uring cmd and just wait for it to complete
to implement the existing synchronous ioctl.
Signed-off-by: Mark Harmstone <maharmstone@xxxxxx>
---
...
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 0493272a7668..8f5cc7d1429c 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
...
+static void btrfs_uring_encoded_read_compat_cb(struct io_uring_cmd *cmd,
+ unsigned int issue_flags)
+{
+ int ret;
+
+ ret = btrfs_ioctl_encoded_read(cmd->file, (void __user *)cmd->sqe->addr,
+ true);
+
+ io_uring_cmd_done(cmd, ret, 0, issue_flags);
+}
+
+static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd,
+ unsigned int issue_flags)
+{
+ if (issue_flags & IO_URING_F_COMPAT)
Instead of two different callbacks we can add a helper
# include/linux/io_uring/cmd.h
static inline bool io_uring_cmd_is_compat(struct io_uring_cmd *cmd)
{
#ifdef COMPAT
struct io_kiocb *req = cmd_to_io_kiocb(ioucmd);
return req->ctx->compat;
#else
return false;
#endif
}
But then we pass the flag in btrfs_ioctl_encoded_read() and use to
interpret struct btrfs_ioctl_encoded_io_args, but next just call
import_iovec(), which derives compat differently. Since io_uring worker
threads are now forks of the original thread, ctx->compat vs
in_compat_syscall() are only marginally different, e.g. when we pass a
ring to another process with a different compat'ness, but I don't think
that something we care about much. Let's just make it consistent.
And the last point, I'm surprised there are two versions of
btrfs_ioctl_encoded_io_args. Maybe, it's a good moment to fix it if
we're creating a new interface.
E.g. by adding a new structure defined right with u64 and such, use it
in io_uring, and cast to it in the ioctl code when it's x64 (with
a good set of BUILD_BUG_ON sprinkled) and convert structures otherwise?
+ io_uring_cmd_complete_in_task(cmd, btrfs_uring_encoded_read_compat_cb);
+ else
+ io_uring_cmd_complete_in_task(cmd, btrfs_uring_encoded_read_cb);
As mentioned, the callback will be executed by the submitter task.
You mentioned offloading to a thread/iowq, that would look like:
btrfs_uring_encoded_read() {
if (issue_flags & IO_URING_F_NONBLOCK)
return -EAGAIN;
// it's a worker thread, block is allowed
}
It's a bad pattern though for anything requiring good performance.
At minimum it should try to execute with a nowait flag set first
nowait = issue_flags & IO_URING_F_NONBLOCK;
btrfs_ioctl_encoded_read(..., nowait);
If needs to block it would return -EAGAIN, so that the core
io_uring would spin up a worker thread for it. Even better
if it does it asynchronously as Christoph mentioned.
+
+ return -EIOCBQUEUED;
+}
+
+int btrfs_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags)
+{
+ switch (cmd->cmd_op) {
+ case BTRFS_IOC_ENCODED_READ:
+#if defined(CONFIG_64BIT) && defined(CONFIG_COMPAT)
+ case BTRFS_IOC_ENCODED_READ_32:
+#endif
+ return btrfs_uring_encoded_read(cmd, issue_flags);
+ }
+
+ io_uring_cmd_done(cmd, -EINVAL, 0, issue_flags);
+ return -EIOCBQUEUED;
+}
+
long btrfs_ioctl(struct file *file, unsigned int
--
Pavel Begunkov
