Re: [PATCH RFC v2 07/19] fuse uring: Add an mmap method

[Josef Bacik <josef@toxicpanda.com>]

On Wed, May 29, 2024 at 08:00:42PM +0200, Bernd Schubert wrote:
> Signed-off-by: Bernd Schubert <bschubert@ddn.com>
> ---
>  fs/fuse/dev.c             |   3 ++
>  fs/fuse/dev_uring.c       | 114 ++++++++++++++++++++++++++++++++++++++++++++++
>  fs/fuse/dev_uring_i.h     |  22 +++++++++
>  include/uapi/linux/fuse.h |   3 ++
>  4 files changed, 142 insertions(+)
> 
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index bc77413932cf..349c1d16b0df 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -2470,6 +2470,9 @@ const struct file_operations fuse_dev_operations = {
>  	.fasync		= fuse_dev_fasync,
>  	.unlocked_ioctl = fuse_dev_ioctl,
>  	.compat_ioctl   = compat_ptr_ioctl,
> +#if IS_ENABLED(CONFIG_FUSE_IO_URING)

I'm loathe to use

#if IS_ENABLED()

when we can use

#ifdef CONFIG_FUSE_IO_URING

which is more standard across the kernel.

> +	.mmap		= fuse_uring_mmap,
> +#endif
>  };
>  EXPORT_SYMBOL_GPL(fuse_dev_operations);
>  
> diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
> index 702a994cf192..9491bdaa5716 100644
> --- a/fs/fuse/dev_uring.c
> +++ b/fs/fuse/dev_uring.c
> @@ -120,3 +120,117 @@ void fuse_uring_ring_destruct(struct fuse_ring *ring)
>  
>  	mutex_destroy(&ring->start_stop_lock);
>  }
> +
> +static inline int fuse_uring_current_nodeid(void)
> +{
> +	int cpu;
> +	const struct cpumask *proc_mask = current->cpus_ptr;
> +
> +	cpu = cpumask_first(proc_mask);
> +
> +	return cpu_to_node(cpu);

You don't need this, just use numa_node_id();

> +}
> +
> +static char *fuse_uring_alloc_queue_buf(int size, int node)
> +{
> +	char *buf;
> +
> +	if (size <= 0) {
> +		pr_info("Invalid queue buf size: %d.\n", size);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	buf = vmalloc_node_user(size, node);
> +	return buf ? buf : ERR_PTR(-ENOMEM);
> +}

This is excessive, we base size off of ring->queue_buf_size, or the
fuse_uring_mmap() size we get from the vma, which I don't think can ever be 0 or
negative.  I think we just validate that ->queue_buf_size is always correct, and
if we're really worried about it in fuse_uring_mmap we validate that sz is
correct there, and then we just use vmalloc_node_user() directly instead of
having this helper.

> +
> +/**
> + * fuse uring mmap, per ring qeuue.
> + * Userpsace maps a kernel allocated ring/queue buffer. For numa awareness,
> + * userspace needs to run the do the mapping from a core bound thread.
> + */
> +int
> +fuse_uring_mmap(struct file *filp, struct vm_area_struct *vma)

I'm not seeing anywhere else in the fuse code that has this style, I'd prefer we
keep it consistent with the rest of the kernel and have

int fuse_uring_mmap(struct file *filp, struct vm_area_struct *vma)

additionally you're using the docstyle strings without the actual docstyle
formatting, which is pissing off my git hooks that run checkpatch.  Not a big
deal, but if you're going to provide docstyle comments then please do it
formatted properly, or just do a normal

/*
 * fuse uring mmap....
 */

> +{
> +	struct fuse_dev *fud = fuse_get_dev(filp);
> +	struct fuse_conn *fc;
> +	struct fuse_ring *ring;
> +	size_t sz = vma->vm_end - vma->vm_start;
> +	int ret;
> +	struct fuse_uring_mbuf *new_node = NULL;
> +	void *buf = NULL;
> +	int nodeid;
> +
> +	if (vma->vm_pgoff << PAGE_SHIFT != FUSE_URING_MMAP_OFF) {
> +		pr_debug("Invalid offset, expected %llu got %lu\n",
> +			 FUSE_URING_MMAP_OFF, vma->vm_pgoff << PAGE_SHIFT);
> +		return -EINVAL;
> +	}
> +
> +	if (!fud)
> +		return -ENODEV;
> +	fc = fud->fc;
> +	ring = fc->ring;
> +	if (!ring)
> +		return -ENODEV;
> +
> +	nodeid = ring->numa_aware ? fuse_uring_current_nodeid() : NUMA_NO_NODE;

nodeid = ring->numa_awayre ? numa_node_id() : NUMA_NO_NODE;

> +
> +	/* check if uring is configured and if the requested size matches */
> +	if (ring->nr_queues == 0 || ring->queue_depth == 0) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	if (sz != ring->queue_buf_size) {
> +		ret = -EINVAL;
> +		pr_devel("mmap size mismatch, expected %zu got %zu\n",
> +			 ring->queue_buf_size, sz);
> +		goto out;
> +	}
> +
> +	if (current->nr_cpus_allowed != 1 && ring->numa_aware) {
> +		ret = -EINVAL;
> +		pr_debug(
> +			"Numa awareness, but thread has more than allowed cpu.\n");
> +		goto out;
> +	}
> +
> +	buf = fuse_uring_alloc_queue_buf(ring->queue_buf_size, nodeid);
> +	if (IS_ERR(buf)) {
> +		ret = PTR_ERR(buf);
> +		goto out;
> +	}

All of the above you can just return ret, you don't have to jump to out.

> +
> +	new_node = kmalloc(sizeof(*new_node), GFP_USER);
> +	if (unlikely(new_node == NULL)) {
> +		ret = -ENOMEM;
> +		goto out;

Here I would just

if (unlikely(new_node == NULL)) {
	vfree(buf);
	return -ENOMEM;
}

> +	}
> +
> +	ret = remap_vmalloc_range(vma, buf, 0);
> +	if (ret)
> +		goto out;

And since this is the only place we can fail with both things allocated I'd just

if (ret) {
	vfree(buf);
	kfree(new_node);
	return ret;
}

and then drop the bit below where you free the buffers if there's an error.

> +
> +	mutex_lock(&ring->start_stop_lock);
> +	/*
> +	 * In this function we do not know the queue the buffer belongs to.
> +	 * Later server side will pass the mmaped address, the kernel address
> +	 * will be found through the map.
> +	 */
> +	new_node->kbuf = buf;
> +	new_node->ubuf = (void *)vma->vm_start;
> +	rb_add(&new_node->rb_node, &ring->mem_buf_map,
> +	       fuse_uring_rb_tree_buf_less);
> +	mutex_unlock(&ring->start_stop_lock);
> +out:
> +	if (ret) {
> +		kfree(new_node);
> +		vfree(buf);
> +	}
> +
> +	pr_devel("%s: pid %d addr: %p sz: %zu  ret: %d\n", __func__,
> +		 current->pid, (char *)vma->vm_start, sz, ret);
> +
> +	return ret;
> +}
> diff --git a/fs/fuse/dev_uring_i.h b/fs/fuse/dev_uring_i.h
> index 58ab4671deff..c455ae0e729a 100644
> --- a/fs/fuse/dev_uring_i.h
> +++ b/fs/fuse/dev_uring_i.h
> @@ -181,6 +181,7 @@ struct fuse_ring {
>  
>  void fuse_uring_abort_end_requests(struct fuse_ring *ring);
>  int fuse_uring_conn_cfg(struct fuse_ring *ring, struct fuse_ring_config *rcfg);
> +int fuse_uring_mmap(struct file *filp, struct vm_area_struct *vma);
>  int fuse_uring_queue_cfg(struct fuse_ring *ring,
>  			 struct fuse_ring_queue_config *qcfg);
>  void fuse_uring_ring_destruct(struct fuse_ring *ring);
> @@ -208,6 +209,27 @@ static inline void fuse_uring_conn_destruct(struct fuse_conn *fc)
>  	kfree(ring);
>  }
>  
> +static inline int fuse_uring_rb_tree_buf_cmp(const void *key,
> +					     const struct rb_node *node)
> +{
> +	const struct fuse_uring_mbuf *entry =
> +		rb_entry(node, struct fuse_uring_mbuf, rb_node);
> +
> +	if (key == entry->ubuf)
> +		return 0;
> +
> +	return (unsigned long)key < (unsigned long)entry->ubuf ? -1 : 1;
> +}
> +
> +static inline bool fuse_uring_rb_tree_buf_less(struct rb_node *node1,
> +					       const struct rb_node *node2)
> +{
> +	const struct fuse_uring_mbuf *entry1 =
> +		rb_entry(node1, struct fuse_uring_mbuf, rb_node);
> +
> +	return fuse_uring_rb_tree_buf_cmp(entry1->ubuf, node2) < 0;
> +}
> +

These are only used in dev_uring.c, just put them in there instead of the header
file.  Thanks,

Josef