On Wed, Nov 27, 2024 at 5:41 AM Bernd Schubert <bschubert@xxxxxxx> wrote: > > This adds basic support for ring SQEs (with opcode=IORING_OP_URING_CMD). > For now only FUSE_URING_REQ_FETCH is handled to register queue entries. > > Signed-off-by: Bernd Schubert <bschubert@xxxxxxx> > --- > fs/fuse/Kconfig | 12 ++ > fs/fuse/Makefile | 1 + > fs/fuse/dev.c | 4 + > fs/fuse/dev_uring.c | 318 ++++++++++++++++++++++++++++++++++++++++++++++ > fs/fuse/dev_uring_i.h | 115 +++++++++++++++++ > fs/fuse/fuse_i.h | 5 + > fs/fuse/inode.c | 10 ++ > include/uapi/linux/fuse.h | 67 ++++++++++ > 8 files changed, 532 insertions(+) > > diff --git a/fs/fuse/Kconfig b/fs/fuse/Kconfig > index 8674dbfbe59dbf79c304c587b08ebba3cfe405be..11f37cefc94b2af5a675c238801560c822b95f1a 100644 > --- a/fs/fuse/Kconfig > +++ b/fs/fuse/Kconfig > @@ -63,3 +63,15 @@ config FUSE_PASSTHROUGH > to be performed directly on a backing file. > > If you want to allow passthrough operations, answer Y. > + > +config FUSE_IO_URING > + bool "FUSE communication over io-uring" > + default y > + depends on FUSE_FS > + depends on IO_URING > + help > + This allows sending FUSE requests over the IO uring interface and > + also adds request core affinity. > + > + If you want to allow fuse server/client communication through io-uring, > + answer Y > diff --git a/fs/fuse/Makefile b/fs/fuse/Makefile > index ce0ff7a9007b94b4ab246b5271f227d126c768e8..fcf16b1c391a9bf11ca9f3a25b137acdb203ac47 100644 > --- a/fs/fuse/Makefile > +++ b/fs/fuse/Makefile > @@ -14,5 +14,6 @@ fuse-y := dev.o dir.o file.o inode.o control.o xattr.o acl.o readdir.o ioctl.o > fuse-y += iomode.o > fuse-$(CONFIG_FUSE_DAX) += dax.o > fuse-$(CONFIG_FUSE_PASSTHROUGH) += passthrough.o > +fuse-$(CONFIG_FUSE_IO_URING) += dev_uring.o > > virtiofs-y := virtio_fs.o > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c > index 63c3865aebb7811fdf4a5729b2181ee8321421dc..0770373492ae9ee83c4154fede9dcfd7be9fb33d 100644 > --- a/fs/fuse/dev.c > +++ b/fs/fuse/dev.c > @@ -6,6 +6,7 @@ > See the file COPYING. > */ > > +#include "dev_uring_i.h" > #include "fuse_i.h" > #include "fuse_dev_i.h" > > @@ -2452,6 +2453,9 @@ const struct file_operations fuse_dev_operations = { > .fasync = fuse_dev_fasync, > .unlocked_ioctl = fuse_dev_ioctl, > .compat_ioctl = compat_ptr_ioctl, > +#ifdef CONFIG_FUSE_IO_URING > + .uring_cmd = fuse_uring_cmd, > +#endif > }; > EXPORT_SYMBOL_GPL(fuse_dev_operations); > > diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c > new file mode 100644 > index 0000000000000000000000000000000000000000..af9c5f116ba1dcf6c01d0359d1a06491c92c32f9 > --- /dev/null > +++ b/fs/fuse/dev_uring.c > @@ -0,0 +1,318 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * FUSE: Filesystem in Userspace > + * Copyright (c) 2023-2024 DataDirect Networks. > + */ > + > +#include <linux/fs.h> nit: for consistency, should this line be placed directly above the other "#include <linux..." line? > + > +#include "fuse_i.h" > +#include "dev_uring_i.h" > +#include "fuse_dev_i.h" > + > +#include <linux/io_uring/cmd.h> > + > +#ifdef CONFIG_FUSE_IO_URING > +static bool __read_mostly enable_uring; > +module_param(enable_uring, bool, 0644); > +MODULE_PARM_DESC(enable_uring, > + "Enable uring userspace communication through uring."); nit: The double uring seems a bit repetitive to me. Maybe just "enable uring userspace communication" or "enable userspace communication through uring"? Also, super nit but I noticed the other MODULE_PARM_DESCs don't have trailing periods. > +#endif > + > +bool fuse_uring_enabled(void) > +{ > + return enable_uring; > +} > + > +static int fuse_ring_ent_unset_userspace(struct fuse_ring_ent *ent) > +{ > + struct fuse_ring_queue *queue = ent->queue; > + > + lockdep_assert_held(&queue->lock); > + > + if (WARN_ON_ONCE(ent->state != FRRS_USERSPACE)) > + return -EIO; > + > + ent->state = FRRS_COMMIT; > + list_move(&ent->list, &queue->ent_commit_queue); > + > + return 0; > +} > + > +void fuse_uring_destruct(struct fuse_conn *fc) > +{ > + struct fuse_ring *ring = fc->ring; > + int qid; > + > + if (!ring) > + return; > + > + for (qid = 0; qid < ring->nr_queues; qid++) { > + struct fuse_ring_queue *queue = ring->queues[qid]; > + > + if (!queue) > + continue; > + > + WARN_ON(!list_empty(&queue->ent_avail_queue)); > + WARN_ON(!list_empty(&queue->ent_commit_queue)); > + > + kfree(queue); > + ring->queues[qid] = NULL; > + } > + > + kfree(ring->queues); > + kfree(ring); > + fc->ring = NULL; > +} > + > +#define FUSE_URING_IOV_SEGS 2 /* header and payload */ nit: to make the code flow easier, might be better to move #defines to the top of the file after the includes. > + > +/* > + * Basic ring setup for this connection based on the provided configuration > + */ > +static struct fuse_ring *fuse_uring_create(struct fuse_conn *fc) > +{ > + struct fuse_ring *ring = NULL; > + size_t nr_queues = num_possible_cpus(); > + struct fuse_ring *res = NULL; > + > + ring = kzalloc(sizeof(*fc->ring) + > + nr_queues * sizeof(struct fuse_ring_queue), I think you just need kzalloc(sizeof(*fc->ring)); here since you're allocating ring->queues later below > + GFP_KERNEL_ACCOUNT); > + if (!ring) > + return NULL; > + > + ring->queues = kcalloc(nr_queues, sizeof(struct fuse_ring_queue *), > + GFP_KERNEL_ACCOUNT); > + if (!ring->queues) > + goto out_err; > + > + spin_lock(&fc->lock); > + if (fc->ring) { > + /* race, another thread created the ring in the mean time */ nit: s/mean time/meantime > + spin_unlock(&fc->lock); > + res = fc->ring; > + goto out_err; > + } > + > + fc->ring = ring; > + ring->nr_queues = nr_queues; > + ring->fc = fc; > + > + spin_unlock(&fc->lock); > + return ring; > + > +out_err: > + if (ring) I think you meant if (ring->queues) > + kfree(ring->queues); > + kfree(ring); > + return res; > +} > + > +static struct fuse_ring_queue *fuse_uring_create_queue(struct fuse_ring *ring, > + int qid) > +{ > + struct fuse_conn *fc = ring->fc; > + struct fuse_ring_queue *queue; > + > + queue = kzalloc(sizeof(*queue), GFP_KERNEL_ACCOUNT); > + if (!queue) > + return ERR_PTR(-ENOMEM); > + spin_lock(&fc->lock); This probably doesn't make much of a difference but might be better to minimize logic inside the lock, eg do the queue initialization stuff outside the lock > + if (ring->queues[qid]) { > + spin_unlock(&fc->lock); > + kfree(queue); > + return ring->queues[qid]; > + } > + > + queue->qid = qid; > + queue->ring = ring; > + spin_lock_init(&queue->lock); > + > + INIT_LIST_HEAD(&queue->ent_avail_queue); > + INIT_LIST_HEAD(&queue->ent_commit_queue); > + > + WRITE_ONCE(ring->queues[qid], queue); Just curious, why do we need WRITE_ONCE here if it's already protected by the fc->lock? > + spin_unlock(&fc->lock); > + > + return queue; > +} > + > +/* > + * Make a ring entry available for fuse_req assignment > + */ > +static void fuse_uring_ent_avail(struct fuse_ring_ent *ring_ent, > + struct fuse_ring_queue *queue) > +{ > + list_move(&ring_ent->list, &queue->ent_avail_queue); > + ring_ent->state = FRRS_WAIT; > +} > + > +/* > + * fuse_uring_req_fetch command handling > + */ > +static void _fuse_uring_fetch(struct fuse_ring_ent *ring_ent, > + struct io_uring_cmd *cmd, > + unsigned int issue_flags) > +{ > + struct fuse_ring_queue *queue = ring_ent->queue; > + > + spin_lock(&queue->lock); > + fuse_uring_ent_avail(ring_ent, queue); > + ring_ent->cmd = cmd; > + spin_unlock(&queue->lock); > +} > + > +/* > + * sqe->addr is a ptr to an iovec array, iov[0] has the headers, iov[1] > + * the payload > + */ > +static int fuse_uring_get_iovec_from_sqe(const struct io_uring_sqe *sqe, > + struct iovec iov[FUSE_URING_IOV_SEGS]) > +{ > + struct iovec __user *uiov = u64_to_user_ptr(READ_ONCE(sqe->addr)); > + struct iov_iter iter; > + ssize_t ret; > + > + if (sqe->len != FUSE_URING_IOV_SEGS) > + return -EINVAL; > + > + /* > + * Direction for buffer access will actually be READ and WRITE, > + * using write for the import should include READ access as well. > + */ > + ret = import_iovec(WRITE, uiov, FUSE_URING_IOV_SEGS, > + FUSE_URING_IOV_SEGS, &iov, &iter); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +static int fuse_uring_fetch(struct io_uring_cmd *cmd, unsigned int issue_flags, > + struct fuse_conn *fc) > +{ > + const struct fuse_uring_cmd_req *cmd_req = io_uring_sqe_cmd(cmd->sqe); > + struct fuse_ring *ring = fc->ring; > + struct fuse_ring_queue *queue; > + struct fuse_ring_ent *ring_ent; > + int err; > + struct iovec iov[FUSE_URING_IOV_SEGS]; > + > + err = fuse_uring_get_iovec_from_sqe(cmd->sqe, iov); > + if (err) { > + pr_info_ratelimited("Failed to get iovec from sqe, err=%d\n", > + err); > + return err; > + } > + > + err = -ENOMEM; > + if (!ring) { > + ring = fuse_uring_create(fc); > + if (!ring) > + return err; > + } > + > + queue = ring->queues[cmd_req->qid]; > + if (!queue) { > + queue = fuse_uring_create_queue(ring, cmd_req->qid); > + if (!queue) > + return err; > + } > + > + /* > + * The created queue above does not need to be destructed in > + * case of entry errors below, will be done at ring destruction time. > + */ > + > + ring_ent = kzalloc(sizeof(*ring_ent), GFP_KERNEL_ACCOUNT); > + if (ring_ent == NULL) nit: !ring_ent > + return err; > + > + INIT_LIST_HEAD(&ring_ent->list); > + > + ring_ent->queue = queue; > + ring_ent->cmd = cmd; > + > + err = -EINVAL; > + if (iov[0].iov_len < sizeof(struct fuse_uring_req_header)) { > + pr_info_ratelimited("Invalid header len %zu\n", iov[0].iov_len); > + goto err; > + } > + > + ring_ent->headers = iov[0].iov_base; > + ring_ent->payload = iov[1].iov_base; > + ring_ent->max_arg_len = iov[1].iov_len; > + > + if (ring_ent->max_arg_len < > + max_t(size_t, FUSE_MIN_READ_BUFFER, fc->max_write)) { If I'm understanding this correctly, iov[0] is the header and iov[1] is the payload. Is this right that the payload len must always be equal to fc->max_write? Also, do we need to take into account fc->max_pages too? > + pr_info_ratelimited("Invalid req payload len %zu\n", > + ring_ent->max_arg_len); > + goto err; > + } > + > + spin_lock(&queue->lock); > + > + /* > + * FUSE_URING_REQ_FETCH is an initialization exception, needs > + * state override > + */ > + ring_ent->state = FRRS_USERSPACE; > + err = fuse_ring_ent_unset_userspace(ring_ent); > + spin_unlock(&queue->lock); > + if (WARN_ON_ONCE(err != 0)) nit: WARN_ON_ONCE(err) > + goto err; > + > + _fuse_uring_fetch(ring_ent, cmd, issue_flags); > + > + return 0; > +err: > + list_del_init(&ring_ent->list); > + kfree(ring_ent); > + return err; > +} > + > +/* > + * Entry function from io_uring to handle the given passthrough command > + * (op cocde IORING_OP_URING_CMD) > + */ > +int fuse_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags) > +{ > + struct fuse_dev *fud; > + struct fuse_conn *fc; > + u32 cmd_op = cmd->cmd_op; > + int err; > + > + /* Disabled for now, especially as teardown is not implemented yet */ > + pr_info_ratelimited("fuse-io-uring is not enabled yet\n"); > + return -EOPNOTSUPP; > + > + if (!enable_uring) { > + pr_info_ratelimited("fuse-io-uring is disabled\n"); > + return -EOPNOTSUPP; > + } > + > + fud = fuse_get_dev(cmd->file); > + if (!fud) { > + pr_info_ratelimited("No fuse device found\n"); > + return -ENOTCONN; > + } > + fc = fud->fc; > + > + if (!fc->connected || fc->aborted) > + return fc->aborted ? -ECONNABORTED : -ENOTCONN; I find if (fc->aborted) return -ECONNABORTED; if (!fc->connected) return -ENOTCONN; easier to read > + > + switch (cmd_op) { > + case FUSE_URING_REQ_FETCH: FUSE_URING_REQ_FETCH is only used for initialization, would it be clearer if this was named FUSE_URING_INIT or something like that? > + err = fuse_uring_fetch(cmd, issue_flags, fc); > + if (err) { > + pr_info_once("fuse_uring_fetch failed err=%d\n", err); > + return err; > + } > + break; > + default: > + return -EINVAL; > + } > + > + return -EIOCBQUEUED; > +} > diff --git a/fs/fuse/dev_uring_i.h b/fs/fuse/dev_uring_i.h > new file mode 100644 > index 0000000000000000000000000000000000000000..75c644cc0b2bb3721b08f8695964815d53f46e92 > --- /dev/null > +++ b/fs/fuse/dev_uring_i.h > @@ -0,0 +1,115 @@ > +/* SPDX-License-Identifier: GPL-2.0 > + * > + * FUSE: Filesystem in Userspace > + * Copyright (c) 2023-2024 DataDirect Networks. > + */ > + > +#ifndef _FS_FUSE_DEV_URING_I_H > +#define _FS_FUSE_DEV_URING_I_H > + > +#include "fuse_i.h" > + > +#ifdef CONFIG_FUSE_IO_URING > + > +enum fuse_ring_req_state { > + FRRS_INVALID = 0, > + > + /* The ring entry received from userspace and it being processed */ nit: "it is being processed" > + FRRS_COMMIT, > + > + /* The ring entry is waiting for new fuse requests */ > + FRRS_WAIT, > + > + /* The ring entry is in or on the way to user space */ > + FRRS_USERSPACE, > +}; > + > +/** A fuse ring entry, part of the ring queue */ > +struct fuse_ring_ent { > + /* userspace buffer */ > + struct fuse_uring_req_header __user *headers; > + void *__user *payload; > + > + /* the ring queue that owns the request */ > + struct fuse_ring_queue *queue; > + > + struct io_uring_cmd *cmd; > + > + struct list_head list; > + > + /* size of payload buffer */ > + size_t max_arg_len; > + > + /* > + * state the request is currently in > + * (enum fuse_ring_req_state) > + */ > + unsigned int state; > + > + struct fuse_req *fuse_req; > +}; > + > +struct fuse_ring_queue { > + /* > + * back pointer to the main fuse uring structure that holds this > + * queue > + */ > + struct fuse_ring *ring; > + > + /* queue id, typically also corresponds to the cpu core */ > + unsigned int qid; > + > + /* > + * queue lock, taken when any value in the queue changes _and_ also > + * a ring entry state changes. > + */ > + spinlock_t lock; > + > + /* available ring entries (struct fuse_ring_ent) */ > + struct list_head ent_avail_queue; > + > + /* > + * entries in the process of being committed or in the process > + * to be send to userspace > + */ > + struct list_head ent_commit_queue; > +}; > + > +/** > + * Describes if uring is for communication and holds alls the data needed > + * for uring communication > + */ > +struct fuse_ring { > + /* back pointer */ > + struct fuse_conn *fc; > + > + /* number of ring queues */ > + size_t nr_queues; > + > + struct fuse_ring_queue **queues; > +}; > + > +bool fuse_uring_enabled(void); > +void fuse_uring_destruct(struct fuse_conn *fc); > +int fuse_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags); > + > +#else /* CONFIG_FUSE_IO_URING */ > + > +struct fuse_ring; > + > +static inline void fuse_uring_create(struct fuse_conn *fc) > +{ > +} > + > +static inline void fuse_uring_destruct(struct fuse_conn *fc) > +{ > +} > + > +static inline bool fuse_uring_enabled(void) > +{ > + return false; > +} > + > +#endif /* CONFIG_FUSE_IO_URING */ > + > +#endif /* _FS_FUSE_DEV_URING_I_H */ > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h > index e3748751e231d0991c050b31bdd84db0b8016f9f..a21256ec4c3b4bd7c67eae2d03b68d87dcc8234b 100644 > --- a/fs/fuse/fuse_i.h > +++ b/fs/fuse/fuse_i.h > @@ -914,6 +914,11 @@ struct fuse_conn { > /** IDR for backing files ids */ > struct idr backing_files_map; > #endif > + > +#ifdef CONFIG_FUSE_IO_URING > + /** uring connection information*/ > + struct fuse_ring *ring; > +#endif > }; > > /* > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c > index fd3321e29a3e569bf06be22a5383cf34fd42c051..76267c79e920204175e5713853de8214c5555d46 100644 > --- a/fs/fuse/inode.c > +++ b/fs/fuse/inode.c > @@ -7,6 +7,7 @@ > */ > > #include "fuse_i.h" > +#include "dev_uring_i.h" > > #include <linux/pagemap.h> > #include <linux/slab.h> > @@ -959,6 +960,8 @@ static void delayed_release(struct rcu_head *p) > { > struct fuse_conn *fc = container_of(p, struct fuse_conn, rcu); > > + fuse_uring_destruct(fc); > + > put_user_ns(fc->user_ns); > fc->release(fc); > } > @@ -1413,6 +1416,13 @@ void fuse_send_init(struct fuse_mount *fm) > if (IS_ENABLED(CONFIG_FUSE_PASSTHROUGH)) > flags |= FUSE_PASSTHROUGH; > > + /* > + * This is just an information flag for fuse server. No need to check > + * the reply - server is either sending IORING_OP_URING_CMD or not. > + */ > + if (fuse_uring_enabled()) > + flags |= FUSE_OVER_IO_URING; > + > ia->in.flags = flags; > ia->in.flags2 = flags >> 32; > > diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h > index f1e99458e29e4fdce5273bc3def242342f207ebd..6d39077edf8cde4fa77130efcec16323839a676c 100644 > --- a/include/uapi/linux/fuse.h > +++ b/include/uapi/linux/fuse.h > @@ -220,6 +220,14 @@ > * > * 7.41 > * - add FUSE_ALLOW_IDMAP > + * 7.42 > + * - Add FUSE_OVER_IO_URING and all other io-uring related flags and data > + * structures: > + * - fuse_uring_ent_in_out > + * - fuse_uring_req_header > + * - fuse_uring_cmd_req > + * - FUSE_URING_IN_OUT_HEADER_SZ > + * - FUSE_URING_OP_IN_OUT_SZ > */ > > #ifndef _LINUX_FUSE_H > @@ -425,6 +433,7 @@ struct fuse_file_lock { > * FUSE_HAS_RESEND: kernel supports resending pending requests, and the high bit > * of the request ID indicates resend requests > * FUSE_ALLOW_IDMAP: allow creation of idmapped mounts > + * FUSE_OVER_IO_URING: Indicate that Client supports io-uring > */ > #define FUSE_ASYNC_READ (1 << 0) > #define FUSE_POSIX_LOCKS (1 << 1) > @@ -471,6 +480,7 @@ struct fuse_file_lock { > /* Obsolete alias for FUSE_DIRECT_IO_ALLOW_MMAP */ > #define FUSE_DIRECT_IO_RELAX FUSE_DIRECT_IO_ALLOW_MMAP > #define FUSE_ALLOW_IDMAP (1ULL << 40) > +#define FUSE_OVER_IO_URING (1ULL << 41) > > /** > * CUSE INIT request/reply flags > @@ -1206,4 +1216,61 @@ struct fuse_supp_groups { > uint32_t groups[]; > }; > > +/** > + * Size of the ring buffer header > + */ > +#define FUSE_URING_IN_OUT_HEADER_SZ 128 > +#define FUSE_URING_OP_IN_OUT_SZ 128 > + > +struct fuse_uring_ent_in_out { > + uint64_t flags; > + > + /* size of use payload buffer */ > + uint32_t payload_sz; > + uint32_t padding; > + > + uint8_t reserved[30]; out of curiosity, how was 30 chosen here? I think this makes the struct 46 bytes? > +}; > + > +/** > + * This structure mapped onto the > + */ > +struct fuse_uring_req_header { > + /* struct fuse_in / struct fuse_out */ > + char in_out[FUSE_URING_IN_OUT_HEADER_SZ]; > + > + /* per op code structs */ > + char op_in[FUSE_URING_OP_IN_OUT_SZ]; > + > + /* struct fuse_ring_in_out */ > + char ring_ent_in_out[sizeof(struct fuse_uring_ent_in_out)]; > +}; > + > +/** > + * sqe commands to the kernel > + */ > +enum fuse_uring_cmd { > + FUSE_URING_REQ_INVALID = 0, > + > + /* submit sqe to kernel to get a request */ > + FUSE_URING_REQ_FETCH = 1, > + > + /* commit result and fetch next request */ > + FUSE_URING_REQ_COMMIT_AND_FETCH = 2, > +}; > + > +/** > + * In the 80B command area of the SQE. > + */ > +struct fuse_uring_cmd_req { > + uint64_t flags; > + > + /* entry identifier */ > + uint64_t commit_id; > + > + /* queue the command is for (queue index) */ > + uint16_t qid; > + uint8_t padding[6]; > +}; > + > #endif /* _LINUX_FUSE_H */ > I'll be out the rest of this week for an American holiday (Thanksgiving) and next week for a work trip but will review the rest of this patchset after that when i get back. Thanks, Joanne > -- > 2.43.0 >