Re: [PATCH RFC v3 05/17] fuse: Add a uring config ioctl

[Bernd Schubert <bernd.schubert@fastmail.fm>]

On 9/4/24 02:43, Joanne Koong wrote:
> On Sun, Sep 1, 2024 at 6:37 AM Bernd Schubert <bschubert@ddn.com> wrote:
>>
>> This only adds the initial ioctl for basic fuse-uring initialization.
>> More ioctl types will be added later to initialize queues.
>>
>> This also adds data structures needed or initialized by the ioctl
>> command and that will be used later.
>>
>> Signed-off-by: Bernd Schubert <bschubert@ddn.com>
> 
> Exciting to read through the work in this patchset!
> 
> I left some comments, lots of which are more granular / implementation
> details than high-level design, in case that would be helpful to you
> in reducing the turnaround time for this patchset. Let me know if
> you'd prefer a hold-off on that though, if your intention with the RFC
> is more to get high-level feedback.

Thanks Joanne! I'm going to address your comments later this week.
> 
> 
> Thanks,
> Joanne
> 
>> ---
>>  fs/fuse/Kconfig           |  12 ++++
>>  fs/fuse/Makefile          |   1 +
>>  fs/fuse/dev.c             |  33 ++++++++---
>>  fs/fuse/dev_uring.c       | 141 ++++++++++++++++++++++++++++++++++++++++++++++
>>  fs/fuse/dev_uring_i.h     | 113 +++++++++++++++++++++++++++++++++++++
>>  fs/fuse/fuse_dev_i.h      |   1 +
>>  fs/fuse/fuse_i.h          |   5 ++
>>  fs/fuse/inode.c           |   3 +
>>  include/uapi/linux/fuse.h |  47 ++++++++++++++++
>>  9 files changed, 349 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/fuse/Kconfig b/fs/fuse/Kconfig
>> index 8674dbfbe59d..11f37cefc94b 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.
> 
> nit: this wording is a little bit awkward imo. Maybe something like
> "... over the IO uring interface and enables core affinity for each
> request" or "... over the IO uring interface and pins each request to
> a specific core"?
> I think there's an extra whitespace here in front of "also".
> 
>> +
>> +         If you want to allow fuse server/client communication through io-uring,
>> +         answer Y
> 
> super nit: missing a period at the end of Y.
> 
>> diff --git a/fs/fuse/Makefile b/fs/fuse/Makefile
>> index 6e0228c6d0cb..7193a14374fd 100644
>> --- a/fs/fuse/Makefile
>> +++ b/fs/fuse/Makefile
>> @@ -11,5 +11,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 dbc222f9b0f0..6489179e7260 100644
>> --- a/fs/fuse/dev.c
>> +++ b/fs/fuse/dev.c
>> @@ -8,6 +8,7 @@
>>
>>  #include "fuse_i.h"
>>  #include "fuse_dev_i.h"
>> +#include "dev_uring_i.h"
>>
>>  #include <linux/init.h>
>>  #include <linux/module.h>
>> @@ -26,6 +27,13 @@
>>  MODULE_ALIAS_MISCDEV(FUSE_MINOR);
>>  MODULE_ALIAS("devname:fuse");
>>
>> +#ifdef CONFIG_FUSE_IO_URING
>> +static bool __read_mostly enable_uring;
> 
> I don't see where enable_uring gets used in this patchset, are you
> planning to use this in a separate future patchset?

Ouch, thanks, I broke it from the previous patch, when I refactored ioctls. As I wrote in the introduction, this patch set is not completely tested - had missed it. Thanks again for spotting.

> 
>> +module_param(enable_uring, bool, 0644);
>> +MODULE_PARM_DESC(enable_uring,
>> +                "Enable uring userspace communication through uring.");
>                                      ^^^ extra "uring" here?
> 
>> +#endif
>> +
>>  static struct kmem_cache *fuse_req_cachep;
>>
>>  static void fuse_request_init(struct fuse_mount *fm, struct fuse_req *req)
>> @@ -2298,16 +2306,12 @@ static int fuse_device_clone(struct fuse_conn *fc, struct file *new)
>>         return 0;
>>  }
>>
>> -static long fuse_dev_ioctl_clone(struct file *file, __u32 __user *argp)
>> +static long _fuse_dev_ioctl_clone(struct file *file, int oldfd)
> 
> I think it'd be a bit clearer if this change was moved to patch 06/17
> "Add the queue configuration ioctl" where it gets used

Oh, yeah, accidentally in here.

> 
>>  {
>>         int res;
>> -       int oldfd;
>>         struct fuse_dev *fud = NULL;
>>         struct fd f;
>>
>> -       if (get_user(oldfd, argp))
>> -               return -EFAULT;
>> -
>>         f = fdget(oldfd);
>>         if (!f.file)
>>                 return -EINVAL;
>> @@ -2330,6 +2334,16 @@ static long fuse_dev_ioctl_clone(struct file *file, __u32 __user *argp)
>>         return res;
>>  }
>>
>> +static long fuse_dev_ioctl_clone(struct file *file, __u32 __user *argp)
>> +{
>> +       int oldfd;
>> +
>> +       if (get_user(oldfd, argp))
>> +               return -EFAULT;
>> +
>> +       return _fuse_dev_ioctl_clone(file, oldfd);
>> +}
>> +
>>  static long fuse_dev_ioctl_backing_open(struct file *file,
>>                                         struct fuse_backing_map __user *argp)
>>  {
>> @@ -2365,8 +2379,9 @@ static long fuse_dev_ioctl_backing_close(struct file *file, __u32 __user *argp)
>>         return fuse_backing_close(fud->fc, backing_id);
>>  }
>>
>> -static long fuse_dev_ioctl(struct file *file, unsigned int cmd,
>> -                          unsigned long arg)
>> +static long
>> +fuse_dev_ioctl(struct file *file, unsigned int cmd,
>> +              unsigned long arg)
> 
> I think you accidentally added this line break here?

Yeah :(

> 
>>  {
>>         void __user *argp = (void __user *)arg;
>>
>> @@ -2380,6 +2395,10 @@ static long fuse_dev_ioctl(struct file *file, unsigned int cmd,
>>         case FUSE_DEV_IOC_BACKING_CLOSE:
>>                 return fuse_dev_ioctl_backing_close(file, argp);
>>
>> +#ifdef CONFIG_FUSE_IO_URING
>> +       case FUSE_DEV_IOC_URING_CFG:
>> +               return fuse_uring_conn_cfg(file, argp);
>> +#endif
>>         default:
>>                 return -ENOTTY;
>>         }
>> diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
>> new file mode 100644
>> index 000000000000..4e7518ef6527
>> --- /dev/null
>> +++ b/fs/fuse/dev_uring.c
>> @@ -0,0 +1,141 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * FUSE: Filesystem in Userspace
>> + * Copyright (c) 2023-2024 DataDirect Networks.
>> + */
>> +
>> +#include "fuse_dev_i.h"
>> +#include "fuse_i.h"
>> +#include "dev_uring_i.h"
>> +
>> +#include <linux/init.h>
>> +#include <linux/module.h>
>> +#include <linux/poll.h>
>> +#include <linux/sched/signal.h>
>> +#include <linux/uio.h>
>> +#include <linux/miscdevice.h>
>> +#include <linux/pagemap.h>
>> +#include <linux/file.h>
>> +#include <linux/slab.h>
>> +#include <linux/pipe_fs_i.h>
>> +#include <linux/swap.h>
>> +#include <linux/splice.h>
>> +#include <linux/sched.h>
>> +#include <linux/io_uring.h>
>> +#include <linux/mm.h>
>> +#include <linux/io.h>
>> +#include <linux/io_uring.h>
>> +#include <linux/io_uring/cmd.h>
>> +#include <linux/topology.h>
>> +#include <linux/io_uring/cmd.h>
> 
> Are all of these headers (eg miscdevice.h, pipe_fs_i.h, topology.h) needed?

Actually already removed in my local v4 branch. I noticed myself on Monday.

> 
>> +
>> +static void fuse_uring_queue_cfg(struct fuse_ring_queue *queue, int qid,
>> +                                struct fuse_ring *ring)
>> +{
>> +       int tag;
>> +
>> +       queue->qid = qid;
>> +       queue->ring = ring;
>> +
>> +       for (tag = 0; tag < ring->queue_depth; tag++) {
>> +               struct fuse_ring_ent *ent = &queue->ring_ent[tag];
>> +
>> +               ent->queue = queue;
>> +               ent->tag = tag;
>> +       }
>> +}
>> +
>> +static int _fuse_uring_conn_cfg(struct fuse_ring_config *rcfg,
>> +                               struct fuse_conn *fc, struct fuse_ring *ring,
>> +                               size_t queue_sz)
> 
> Should this function just be marked "void" as the return type?

Yeah, I had missed it.

> 
>> +{
>> +       ring->numa_aware = rcfg->numa_aware;
>> +       ring->nr_queues = rcfg->nr_queues;
>> +       ring->per_core_queue = rcfg->nr_queues > 1;
>> +
>> +       ring->max_nr_sync = rcfg->sync_queue_depth;
>> +       ring->max_nr_async = rcfg->async_queue_depth;
>> +       ring->queue_depth = ring->max_nr_sync + ring->max_nr_async;
>> +
>> +       ring->req_buf_sz = rcfg->user_req_buf_sz;
>> +
>> +       ring->queue_size = queue_sz;
>> +
>> +       fc->ring = ring;
>> +       ring->fc = fc;
>> +
>> +       return 0;
>> +}
>> +
>> +static int fuse_uring_cfg_sanity(struct fuse_ring_config *rcfg)
>> +{
>> +       if (rcfg->nr_queues == 0) {
>> +               pr_info("zero number of queues is invalid.\n");
> 
> I think this might get misinterpreted as "zero queues are invalid" -
> maybe something like: "fuse_ring_config nr_queues=0 is invalid arg"
> might be clearer?
> 
>> +               return -EINVAL;
>> +       }
>> +
>> +       if (rcfg->nr_queues > 1 && rcfg->nr_queues != num_present_cpus()) {
> 
> Will it always be that nr_queues must be the number of CPUs on the
> system or will that constraint be relaxed in the future?

In all my testing performance rather suffered when any kind of cpu switching was involved. I guess we should first find a good reason to relax it and then need to think about which queue to use, when a request comes on a different core. Do you have a use case?

> 
>> +               pr_info("nr-queues (%d) does not match nr-cores (%d).\n",
> 
> nit: %u for nr_queues,  s/nr-queues/nr_queues
> It might be useful here to specify "uring nr_queues" as well to make
> it more obvious
> 
>> +                       rcfg->nr_queues, num_present_cpus());
>> +               return -EINVAL;
>> +       }
>> +
> 
> Should this function also sanity check that the queue depth is <=
> FUSE_URING_MAX_QUEUE_DEPTH?

Right.

> 
>> +       return 0;
>> +}
>> +
>> +/*
>> + * Basic ring setup for this connection based on the provided configuration
>> + */
>> +int fuse_uring_conn_cfg(struct file *file, void __user *argp)
> 
> Is there a reason we pass in "void __user *argp" instead of "struct
> fuse_ring_config __user *argp"?

Will fix it.

> 
>> +{
>> +       struct fuse_ring_config rcfg;
>> +       int res;
>> +       struct fuse_dev *fud;
>> +       struct fuse_conn *fc;
>> +       struct fuse_ring *ring = NULL;
>> +       struct fuse_ring_queue *queue;
>> +       int qid;
>> +
>> +       res = copy_from_user(&rcfg, (void *)argp, sizeof(rcfg));
> 
> I don't think we need this "(void *)" cast here
> 
>> +       if (res != 0)
>> +               return -EFAULT;
>> +       res = fuse_uring_cfg_sanity(&rcfg);
>> +       if (res != 0)
>> +               return res;
>> +
>> +       fud = fuse_get_dev(file);
>> +       if (fud == NULL)
> 
> nit: if (!fud)
> 
>> +               return -ENODEV;
> 
> Should this be -ENODEV or -EPERM? -ENODEV makes sense to me but I'm
> seeing other callers of fuse_get_dev() in fuse/dev.c return -EPERM
> when fud is NULL.
> 
>> +       fc = fud->fc;
>> +
> 
> Should we add a check
> if (fc->ring)
>    return -EINVAL (or -EALREADY);
> 
> if not, then i think we need to move the "for (qid = 0; ...)" logic
> below to be within the "if (fc->ring == NULL)" check
> 
>> +       if (fc->ring == NULL) {
> 
> nit: if (!fc->ring)
> 
>> +               size_t queue_depth = rcfg.async_queue_depth +
>> +                                    rcfg.sync_queue_depth;
>> +               size_t queue_sz = sizeof(struct fuse_ring_queue) +
>> +                                 sizeof(struct fuse_ring_ent) * queue_depth;
>> +
>> +               ring = kvzalloc(sizeof(*fc->ring) + queue_sz * rcfg.nr_queues,
>> +                               GFP_KERNEL_ACCOUNT);
>> +               if (ring == NULL)
> 
> nit: if (!ring)
> 
>> +                       return -ENOMEM;
>> +
>> +               spin_lock(&fc->lock);
>> +               if (fc->ring == NULL)
> 
> if (!fc->ring)
> 
>> +                       res = _fuse_uring_conn_cfg(&rcfg, fc, ring, queue_sz);
>> +               else
>> +                       res = -EALREADY;
>> +               spin_unlock(&fc->lock);
>> +               if (res != 0)
> 
> nit: if (res)
> 
>> +                       goto err;
>> +       }
>> +
>> +       for (qid = 0; qid < ring->nr_queues; qid++) {
>> +               queue = fuse_uring_get_queue(ring, qid);
>> +               fuse_uring_queue_cfg(queue, qid, ring);
>> +       }
>> +
>> +       return 0;
>> +err:
>> +       kvfree(ring);
>> +       return res;
>> +}
>> diff --git a/fs/fuse/dev_uring_i.h b/fs/fuse/dev_uring_i.h
>> new file mode 100644
>> index 000000000000..d4eff87bcd1f
>> --- /dev/null
>> +++ b/fs/fuse/dev_uring_i.h
>> @@ -0,0 +1,113 @@
>> +/* 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
>> +
>> +/* IORING_MAX_ENTRIES */
> 
> nit: I'm not sure this comment is that helpful. The
> "FUSE_URING_MAX_QUEUE_DEPTH" name is clear enough, I think.
> 
>> +#define FUSE_URING_MAX_QUEUE_DEPTH 32768
>> +
>> +/* A fuse ring entry, part of the ring queue */
>> +struct fuse_ring_ent {
>> +       /* back pointer */
>> +       struct fuse_ring_queue *queue;
> 
> Do you think it's worth using the tag to find the queue (i think we
> can just use some containerof magic to get the queue backpointer here
> since ring_ent is embedded within struct fuse_ring_queue?) instead of
> having this be an explicit 8 byte pointer? I'm thinking about the case
> where the user sets a queue depth of 32k (eg
> FUSE_URING_MAX_QUEUE_DEPTH) and is on an 8-core system where they set
> nr_queues to 8. This would end up in 8 * 32k * 8 = 2 MiB extra memory
> allocated which seems non-trivial (but I guess this is also an extreme
> case). Curious what your thoughts on this are.
> 
>> +
>> +       /* array index in the ring-queue */
>> +       unsigned int tag;
> 
> Just wondering, is this called "tag" instead of "index" to be
> consistent with an io-ring naming convention?
> 
>> +};
>> +
>> +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;
>> +
>> +       /* size depends on queue depth */
>> +       struct fuse_ring_ent ring_ent[] ____cacheline_aligned_in_smp;
>> +};
>> +
>> +/**
>> + * Describes if uring is for communication and holds alls the data needed
> 
> nit: maybe this should just be "Holds all the data needed for uring
> communication"?
> 
> nit: s/alls/all
> 
>> + * for uring communication
>> + */
>> +struct fuse_ring {
>> +       /* back pointer */
>> +       struct fuse_conn *fc;
>> +
>> +       /* number of ring queues */
> 
> I think it's worth calling out here too that this must be the number
> of CPUs on the system and that each CPU operates its own ring queue.
> 
>> +       size_t nr_queues;
>> +
>> +       /* number of entries per queue */
>> +       size_t queue_depth;
>> +
>> +       /* req_arg_len + sizeof(struct fuse_req) */
> 
> What is req_arg_len? In _fuse_uring_conn_cfg(), it looks like this
> gets set to rcfg->user_req_buf_sz which is passed in from userspace,
> but from what I understand, "struct fuse_req" is a kernel-defined
> struct? I'm a bit confused overall what the comment refers to, but I
> also haven't yet looked through the libfuse change yet for this
> patchset.

Sorry, it is a typo, it is supposed to be 'sizeof(struct fuse_ring_req)'.

> 
>> +       size_t req_buf_sz;
>> +
>> +       /* max number of background requests per queue */
>> +       size_t max_nr_async;
>> +
>> +       /* max number of foreground requests */
> 
> nit: for consistency with the comment for max_nr_async,
> s/requests/"requests per queue"
> 
>> +       size_t max_nr_sync;
> 
> It's interesting to me that this can get configured by userspace for
> background requests vs foreground requests. My perspective was that
> from the userspace POV, there's no differentiation between background
> vs foreground requests. Personally, I'm still not really even sure yet
> which of the read requests are async vs sync when I do a 8 MiB read
> call for example (iirc, I was seeing both, when it first tried the
> readahead path). It seems a bit like overkill to me but maybe there
> are some servers that actually do care a lot about this.

I think I need to rework this a bit. What I actually want is credits. With /dev/fuse bg requests get moved into the main request list and can then block everything. To keep the series small, maybe better if I entirely remove that in v4.

> 
>> +
>> +       /* size of struct fuse_ring_queue + queue-depth * entry-size */
>> +       size_t queue_size;
>> +
>> +       /* one queue per core or a single queue only ? */
>> +       unsigned int per_core_queue : 1;
>> +
>> +       /* numa aware memory allocation */
>> +       unsigned int numa_aware : 1;
>> +
>> +       struct fuse_ring_queue queues[] ____cacheline_aligned_in_smp;
>> +};
>> +
>> +void fuse_uring_abort_end_requests(struct fuse_ring *ring);
> 
> nit: I think it'd be a bit cleaner if this got moved to patch 12/17
> (fuse: {uring} Handle teardown of ring entries) when it gets used
> 
>> +int fuse_uring_conn_cfg(struct file *file, void __user *argp);
>> +
>> +static inline void fuse_uring_conn_destruct(struct fuse_conn *fc)
>> +{
>> +       if (fc->ring == NULL)
> 
> nit: if (!fc->ring)

Maybe I find a cocinelle script or write one for such things, 
checkpatch.pl doesn't annotate it.

> 
>> +               return;
>> +
>> +       kvfree(fc->ring);
>> +       fc->ring = NULL;
>> +}
>> +
>> +static inline struct fuse_ring_queue *
>> +fuse_uring_get_queue(struct fuse_ring *ring, int qid)
>> +{
>> +       char *ptr = (char *)ring->queues;
> 
> Do we need to cast this to char * or can we just do the math below as
> return ring->queues + qid;

It is qid * ring->queue_size, as we have the variable length 
array 'struct fuse_ring_ent ring_ent[]'. I'm still looking for a better
name for 'ring->queue_size'. Meaning is 
sizeof(struct fuse_ring_queue) + queue_depth * sizeof(struct fuse_ring_ent)


> 
>> +
>> +       if (WARN_ON(qid > ring->nr_queues))
> 
> Should this be >= since qid is 0-indexed?

Ouch, yeah.

> 
> we should never reach here, but it feels like if we do, we should just
> automatically return NULL.
> 
>> +               qid = 0;
>> +
>> +       return (struct fuse_ring_queue *)(ptr + qid * ring->queue_size);
>> +}
>> +
>> +#else /* CONFIG_FUSE_IO_URING */
>> +
>> +struct fuse_ring;
>> +
>> +static inline void fuse_uring_conn_init(struct fuse_ring *ring,
>> +                                       struct fuse_conn *fc)
>> +{
>> +}
>> +
>> +static inline void fuse_uring_conn_destruct(struct fuse_conn *fc)
>> +{
>> +}
>> +
>> +#endif /* CONFIG_FUSE_IO_URING */
>> +
>> +#endif /* _FS_FUSE_DEV_URING_I_H */
>> diff --git a/fs/fuse/fuse_dev_i.h b/fs/fuse/fuse_dev_i.h
>> index 6c506f040d5f..e6289bafb788 100644
>> --- a/fs/fuse/fuse_dev_i.h
>> +++ b/fs/fuse/fuse_dev_i.h
>> @@ -7,6 +7,7 @@
>>  #define _FS_FUSE_DEV_I_H
>>
>>  #include <linux/types.h>
>> +#include <linux/fs.h>
> 
> I think you accidentally included this.
> 

When I remove it:

bschubert2@imesrv6 linux.git>make M=fs/fuse/
  CC [M]  fs/fuse/dev_uring.o
In file included from fs/fuse/dev_uring.c:7:
fs/fuse/fuse_dev_i.h:15:52: warning: declaration of 'struct file' will not be visible outside of this function [-Wvisibility]
static inline struct fuse_dev *fuse_get_dev(struct file *file)
                                                   ^
fs/fuse/fuse_dev_i.h:21:9: error: call to undeclared function 'READ_ONCE'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
        return READ_ONCE(file->private_data);
               ^
fs/fuse/fuse_dev_i.h:21:23: error: incomplete definition of type 'struct file'
        return READ_ONCE(file->private_data);
                         ~~~~^


I could also include <linux/fs.h> in dev_uring.c, but isn't it cleaner 
to have the include in fuse_dev_i.h as it is that file that
adds dependencies?

>>
>>  /* Ordinary requests have even IDs, while interrupts IDs are odd */
>>  #define FUSE_INT_REQ_BIT (1ULL << 0)
>> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
>> index f23919610313..33e81b895fee 100644
>> --- a/fs/fuse/fuse_i.h
>> +++ b/fs/fuse/fuse_i.h
>> @@ -917,6 +917,11 @@ struct fuse_conn {
>>         /** IDR for backing files ids */
>>         struct idr backing_files_map;
>>  #endif
>> +
>> +#ifdef CONFIG_FUSE_IO_URING
>> +       /**  uring connection information*/
> nit: need extra space between information and */
>> +       struct fuse_ring *ring;
>> +#endif
>>  };
>>
>>  /*
>> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
>> index 99e44ea7d875..33a080b24d65 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>
>> @@ -947,6 +948,8 @@ static void delayed_release(struct rcu_head *p)
>>  {
>>         struct fuse_conn *fc = container_of(p, struct fuse_conn, rcu);
>>
>> +       fuse_uring_conn_destruct(fc);
> 
> I think it's cleaner if this is moved to fuse_free_conn than here.
> 
>> +
>>         put_user_ns(fc->user_ns);
>>         fc->release(fc);
>>  }
>> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
>> index d08b99d60f6f..a1c35e0338f0 100644
>> --- a/include/uapi/linux/fuse.h
>> +++ b/include/uapi/linux/fuse.h
>> @@ -1079,12 +1079,53 @@ struct fuse_backing_map {
>>         uint64_t        padding;
>>  };
>>
>> +enum fuse_uring_ioctl_cmd {
> 
> Do you have a link to the libfuse side? I'm not seeing these get used
> in the patchset - I'm curious how libfuse will be using these then?

I had written it in the introduction

https://github.com/bsbernd/libfuse/tree/uring

Don't look at the individual patches please, I will clean that up,
once we agree on the basic approach.

> 
>> +       /* not correctly initialized when set */
>> +       FUSE_URING_IOCTL_CMD_INVALID    = 0,
>> +
>> +       /* Ioctl to prepare communucation with io-uring */
> 
> nit: communication spelling
> 
>> +       FUSE_URING_IOCTL_CMD_RING_CFG   = 1,
>> +
>> +       /* Ring queue configuration ioctl */
>> +       FUSE_URING_IOCTL_CMD_QUEUE_CFG  = 2,
>> +};
>> +
>> +enum fuse_uring_cfg_flags {
>> +       /* server/daemon side requests numa awareness */
>> +       FUSE_URING_WANT_NUMA = 1ul << 0,
> 
> nit: 1UL for consistency
> 
>> +};
>> +
>> +struct fuse_ring_config {
>> +       /* number of queues */
>> +       uint32_t nr_queues;
>> +
>> +       /* number of foreground entries per queue */
>> +       uint32_t sync_queue_depth;
>> +
>> +       /* number of background entries per queue */
>> +       uint32_t async_queue_depth;
>> +
>> +       /*
>> +        * buffer size userspace allocated per request buffer
>> +        * from the mmaped queue buffer
>> +        */
>> +       uint32_t user_req_buf_sz;
>> +
>> +       /* ring config flags */
>> +       uint64_t numa_aware:1;
>> +
>> +       /* for future extensions */
>> +       uint8_t padding[64];
>> +};
>> +
>>  /* Device ioctls: */
>>  #define FUSE_DEV_IOC_MAGIC             229
>>  #define FUSE_DEV_IOC_CLONE             _IOR(FUSE_DEV_IOC_MAGIC, 0, uint32_t)
>>  #define FUSE_DEV_IOC_BACKING_OPEN      _IOW(FUSE_DEV_IOC_MAGIC, 1, \
>>                                              struct fuse_backing_map)
>>  #define FUSE_DEV_IOC_BACKING_CLOSE     _IOW(FUSE_DEV_IOC_MAGIC, 2, uint32_t)
>> +#define FUSE_DEV_IOC_URING_CFG         _IOR(FUSE_DEV_IOC_MAGIC, 3, \
>> +                                            struct fuse_ring_config)
>>
>>  struct fuse_lseek_in {
>>         uint64_t        fh;
>> @@ -1186,4 +1227,10 @@ struct fuse_supp_groups {
>>         uint32_t        groups[];
>>  };
>>
>> +/**
>> + * Size of the ring buffer header
>> + */
>> +#define FUSE_RING_HEADER_BUF_SIZE 4096
>> +#define FUSE_RING_MIN_IN_OUT_ARG_SIZE 4096
> 
> I think this'd be cleaner to review if this got moved to the patch
> where this gets used
> 
>> +
>>  #endif /* _LINUX_FUSE_H */
>>
>> --
>> 2.43.0
>>

I will get it all fixed later this week! I will also review my own
patches before v4, I just wanted to get v3 out asap as it was already
taking so much time after v2.


Thanks,
Bernd