Re: [PATCH RFC v3 06/17] fuse: Add the queue configuration ioctl

[Joanne Koong <joannelkoong@gmail.com>]

On Sun, Sep 1, 2024 at 6:37 AM Bernd Schubert <bschubert@ddn.com> wrote:
>
> Signed-off-by: Bernd Schubert <bschubert@ddn.com>
> ---
>  fs/fuse/dev.c             | 30 ++++++++++++++++++++++++++++++
>  fs/fuse/dev_uring.c       |  2 ++
>  fs/fuse/dev_uring_i.h     | 13 +++++++++++++
>  fs/fuse/fuse_i.h          |  4 ++++
>  include/uapi/linux/fuse.h | 39 +++++++++++++++++++++++++++++++++++++++
>  5 files changed, 88 insertions(+)
>
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index 6489179e7260..06ea4dc5ffe1 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -2379,6 +2379,33 @@ static long fuse_dev_ioctl_backing_close(struct file *file, __u32 __user *argp)
>         return fuse_backing_close(fud->fc, backing_id);
>  }
>
> +#ifdef CONFIG_FUSE_IO_URING
> +static long fuse_uring_queue_ioc(struct file *file, __u32 __user *argp)
> +{
> +       int res = 0;
> +       struct fuse_dev *fud;
> +       struct fuse_conn *fc;
> +       struct fuse_ring_queue_config qcfg;
> +
> +       res = copy_from_user(&qcfg, (void *)argp, sizeof(qcfg));
> +       if (res != 0)
> +               return -EFAULT;
> +
> +       res = _fuse_dev_ioctl_clone(file, qcfg.control_fd);

I'm confused how this works for > 1 queues. If I'm understanding this
correctly, if a system has multiple cores and the server would like
multi-queues, then the server needs to call the ioctl
FUSE_DEV_IOC_URING_QUEUE_CFG multiple times (each with a different
qid).

In this handler, when we get to _fuse_dev_ioctl_clone() ->
fuse_device_clone(), it allocates and installs a new fud and then sets
file->private_data to fud, but isn't this underlying file the same for
all of the queues since they are using the same fd for the ioctl
calls? It seems like every queue after the 1st would fail with -EINVAL
from the "if (new->private_data)" check in fuse_device_clone()?

Not sure if I'm missing something or if this intentionally doesn't
support multi-queue yet. If the latter, then I'm curious how you're
planning to get the fud for a specific queue given that
file->private_data and fuse_get_dev() only can support the single
queue case.

Thanks,
Joanne

> +       if (res != 0)
> +               return res;
> +
> +       fud = fuse_get_dev(file);
> +       if (fud == NULL)
> +               return -ENODEV;
> +       fc = fud->fc;
> +
> +       fud->ring_q = fuse_uring_get_queue(fc->ring, qcfg.qid);
> +
> +       return 0;
> +}
> +#endif
> +
>  static long
>  fuse_dev_ioctl(struct file *file, unsigned int cmd,
>                unsigned long arg)
> @@ -2398,6 +2425,9 @@ fuse_dev_ioctl(struct file *file, unsigned int cmd,
>  #ifdef CONFIG_FUSE_IO_URING
>         case FUSE_DEV_IOC_URING_CFG:
>                 return fuse_uring_conn_cfg(file, argp);
> +
> +       case FUSE_DEV_IOC_URING_QUEUE_CFG:
> +               return fuse_uring_queue_ioc(file, argp);
>  #endif
>         default:
>                 return -ENOTTY;
> diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
> index 4e7518ef6527..4dcb4972242e 100644
> --- a/fs/fuse/dev_uring.c
> +++ b/fs/fuse/dev_uring.c
> @@ -42,6 +42,8 @@ static void fuse_uring_queue_cfg(struct fuse_ring_queue *queue, int qid,
>
>                 ent->queue = queue;
>                 ent->tag = tag;
> +
> +               ent->state = FRRS_INIT;
>         }
>  }
>
> diff --git a/fs/fuse/dev_uring_i.h b/fs/fuse/dev_uring_i.h
> index d4eff87bcd1f..301b37d16506 100644
> --- a/fs/fuse/dev_uring_i.h
> +++ b/fs/fuse/dev_uring_i.h
> @@ -14,6 +14,13 @@
>  /* IORING_MAX_ENTRIES */
>  #define FUSE_URING_MAX_QUEUE_DEPTH 32768
>
> +enum fuse_ring_req_state {
> +
> +       /* request is basially initialized */
> +       FRRS_INIT = 1,
> +
> +};
> +
>  /* A fuse ring entry, part of the ring queue */
>  struct fuse_ring_ent {
>         /* back pointer */
> @@ -21,6 +28,12 @@ struct fuse_ring_ent {
>
>         /* array index in the ring-queue */
>         unsigned int tag;
> +
> +       /*
> +        * state the request is currently in
> +        * (enum fuse_ring_req_state)
> +        */
> +       unsigned long state;
>  };
>
>  struct fuse_ring_queue {
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 33e81b895fee..5eb8552d9d7f 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -540,6 +540,10 @@ struct fuse_dev {
>
>         /** list entry on fc->devices */
>         struct list_head entry;
> +
> +#ifdef CONFIG_FUSE_IO_URING
> +       struct fuse_ring_queue *ring_q;
> +#endif
>  };
>
>  enum fuse_dax_mode {
> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> index a1c35e0338f0..143ed3c1c7b3 100644
> --- a/include/uapi/linux/fuse.h
> +++ b/include/uapi/linux/fuse.h
> @@ -1118,6 +1118,18 @@ struct fuse_ring_config {
>         uint8_t padding[64];
>  };
>
> +struct fuse_ring_queue_config {
> +       /* qid the command is for */
> +       uint32_t qid;
> +
> +       /* /dev/fuse fd that initiated the mount. */
> +       uint32_t control_fd;
> +
> +       /* 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)
> @@ -1126,6 +1138,8 @@ struct fuse_ring_config {
>  #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)
> +#define FUSE_DEV_IOC_URING_QUEUE_CFG   _IOR(FUSE_DEV_IOC_MAGIC, 3, \
> +                                            struct fuse_ring_queue_config)
>
>  struct fuse_lseek_in {
>         uint64_t        fh;
> @@ -1233,4 +1247,29 @@ struct fuse_supp_groups {
>  #define FUSE_RING_HEADER_BUF_SIZE 4096
>  #define FUSE_RING_MIN_IN_OUT_ARG_SIZE 4096
>
> +/**
> + * This structure mapped onto the
> + */
> +struct fuse_ring_req {
> +       union {
> +               /* The first 4K are command data */
> +               char ring_header[FUSE_RING_HEADER_BUF_SIZE];
> +
> +               struct {
> +                       uint64_t flags;
> +
> +                       uint32_t in_out_arg_len;
> +                       uint32_t padding;
> +
> +                       /* kernel fills in, reads out */
> +                       union {
> +                               struct fuse_in_header in;
> +                               struct fuse_out_header out;
> +                       };
> +               };
> +       };
> +
> +       char in_out_arg[];
> +};
> +
>  #endif /* _LINUX_FUSE_H */
>
> --
> 2.43.0
>