[PATCH v2 0/4] fuse: Improve ftraces, per-cpu req unique and code dup removal

[PATCH v2 0/4] fuse: Improve ftraces, per-cpu req unique and code dup removal

[Bernd Schubert]

This series is mainly about improved ftraces to determine
latencies between queues and also to be able to create
latency histograms - the request unique was missing in
trace_fuse_request_send so far.
Some preparation patches are added before.

Scripts to enabled tracing and to get histograms are here
https://github.com/libfuse/libfuse/pull/1186

Signed-off-by: Bernd Schubert <bschubert@ddn.com>
---
Changes in v2:
- EDITME: describe what is new in this series revision.
- EDITME: use bulletpoints and terse descriptions.
- Link to v1: https://lore.kernel.org/r/20250402-fuse-io-uring-trace-points-v1-0-11b0211fa658@ddn.com

---
Bernd Schubert (4):
      fuse: Make the fuse unique value a per-cpu counter
      fuse: Set request unique on allocation
      fuse: {io-uring} Avoid _send code dup
      fuse: fine-grained request ftraces

 fs/fuse/dev.c        | 37 ++++++++++------------------------
 fs/fuse/dev_uring.c  | 44 ++++++++++++++++------------------------
 fs/fuse/fuse_dev_i.h |  4 ----
 fs/fuse/fuse_i.h     | 23 ++++++++++++++++-----
 fs/fuse/fuse_trace.h | 57 +++++++++++++++++++++++++++++++++++++---------------
 fs/fuse/inode.c      |  1 +
 fs/fuse/virtio_fs.c  |  3 ---
 7 files changed, 88 insertions(+), 81 deletions(-)
---
base-commit: 08733088b566b58283f0f12fb73f5db6a9a9de30
change-id: 20250402-fuse-io-uring-trace-points-690154bb72c7

Best regards,
-- 
Bernd Schubert <bschubert@ddn.com>

[PATCH v2 1/4] fuse: Make the fuse unique value a per-cpu counter

[Bernd Schubert]

No need to take lock, we can have that per cpu and
add in the current cpu as offset.

fuse-io-uring and virtiofs especially benefit from it
as they don't need the fiq lock at all.

Signed-off-by: Bernd Schubert <bschubert@ddn.com>
---
 fs/fuse/dev.c        | 24 +++---------------------
 fs/fuse/fuse_dev_i.h |  4 ----
 fs/fuse/fuse_i.h     | 23 ++++++++++++++++++-----
 fs/fuse/inode.c      |  1 +
 4 files changed, 22 insertions(+), 30 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 51e31df4c54613280a9c295f530b18e1d461a974..e9592ab092b948bacb5034018bd1f32c917d5c9f 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -204,24 +204,6 @@ unsigned int fuse_len_args(unsigned int numargs, struct fuse_arg *args)
 }
 EXPORT_SYMBOL_GPL(fuse_len_args);
 
-static u64 fuse_get_unique_locked(struct fuse_iqueue *fiq)
-{
-	fiq->reqctr += FUSE_REQ_ID_STEP;
-	return fiq->reqctr;
-}
-
-u64 fuse_get_unique(struct fuse_iqueue *fiq)
-{
-	u64 ret;
-
-	spin_lock(&fiq->lock);
-	ret = fuse_get_unique_locked(fiq);
-	spin_unlock(&fiq->lock);
-
-	return ret;
-}
-EXPORT_SYMBOL_GPL(fuse_get_unique);
-
 unsigned int fuse_req_hash(u64 unique)
 {
 	return hash_long(unique & ~FUSE_INT_REQ_BIT, FUSE_PQ_HASH_BITS);
@@ -278,7 +260,7 @@ static void fuse_dev_queue_req(struct fuse_iqueue *fiq, struct fuse_req *req)
 	spin_lock(&fiq->lock);
 	if (fiq->connected) {
 		if (req->in.h.opcode != FUSE_NOTIFY_REPLY)
-			req->in.h.unique = fuse_get_unique_locked(fiq);
+			req->in.h.unique = fuse_get_unique(fiq);
 		list_add_tail(&req->list, &fiq->pending);
 		fuse_dev_wake_and_unlock(fiq);
 	} else {
@@ -1177,7 +1159,7 @@ __releases(fiq->lock)
 	struct fuse_in_header ih = {
 		.opcode = FUSE_FORGET,
 		.nodeid = forget->forget_one.nodeid,
-		.unique = fuse_get_unique_locked(fiq),
+		.unique = fuse_get_unique(fiq),
 		.len = sizeof(ih) + sizeof(arg),
 	};
 
@@ -1208,7 +1190,7 @@ __releases(fiq->lock)
 	struct fuse_batch_forget_in arg = { .count = 0 };
 	struct fuse_in_header ih = {
 		.opcode = FUSE_BATCH_FORGET,
-		.unique = fuse_get_unique_locked(fiq),
+		.unique = fuse_get_unique(fiq),
 		.len = sizeof(ih) + sizeof(arg),
 	};
 
diff --git a/fs/fuse/fuse_dev_i.h b/fs/fuse/fuse_dev_i.h
index 3b2bfe1248d3573abe3b144a6d4bf6a502f56a40..e0afd837a8024450bab77312c7eebdcc7a39bd36 100644
--- a/fs/fuse/fuse_dev_i.h
+++ b/fs/fuse/fuse_dev_i.h
@@ -8,10 +8,6 @@
 
 #include <linux/types.h>
 
-/* Ordinary requests have even IDs, while interrupts IDs are odd */
-#define FUSE_INT_REQ_BIT (1ULL << 0)
-#define FUSE_REQ_ID_STEP (1ULL << 1)
-
 struct fuse_arg;
 struct fuse_args;
 struct fuse_pqueue;
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index fee96fe7887b30cd57b8a6bbda11447a228cf446..73c612dd58e45ecde0b8f72fd58ac603d12cf202 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -9,6 +9,8 @@
 #ifndef _FS_FUSE_I_H
 #define _FS_FUSE_I_H
 
+#include "linux/percpu-defs.h"
+#include "linux/threads.h"
 #ifndef pr_fmt
 # define pr_fmt(fmt) "fuse: " fmt
 #endif
@@ -44,6 +46,10 @@
 /** Number of dentries for each connection in the control filesystem */
 #define FUSE_CTL_NUM_DENTRIES 5
 
+/* Ordinary requests have even IDs, while interrupts IDs are odd */
+#define FUSE_INT_REQ_BIT (1ULL << 0)
+#define FUSE_REQ_ID_STEP (1ULL << 1)
+
 /** Maximum of max_pages received in init_out */
 extern unsigned int fuse_max_pages_limit;
 
@@ -490,7 +496,7 @@ struct fuse_iqueue {
 	wait_queue_head_t waitq;
 
 	/** The next unique request id */
-	u64 reqctr;
+	u64 __percpu *reqctr;
 
 	/** The list of pending requests */
 	struct list_head pending;
@@ -1065,6 +1071,17 @@ static inline void fuse_sync_bucket_dec(struct fuse_sync_bucket *bucket)
 	rcu_read_unlock();
 }
 
+/**
+ * Get the next unique ID for a request
+ */
+static inline u64 fuse_get_unique(struct fuse_iqueue *fiq)
+{
+	int step = FUSE_REQ_ID_STEP * (task_cpu(current) + 1);
+	u64 cntr = this_cpu_inc_return(*fiq->reqctr);
+
+	return cntr * FUSE_REQ_ID_STEP * NR_CPUS + step;
+}
+
 /** Device operations */
 extern const struct file_operations fuse_dev_operations;
 
@@ -1415,10 +1432,6 @@ int fuse_readdir(struct file *file, struct dir_context *ctx);
  */
 unsigned int fuse_len_args(unsigned int numargs, struct fuse_arg *args);
 
-/**
- * Get the next unique ID for a request
- */
-u64 fuse_get_unique(struct fuse_iqueue *fiq);
 void fuse_free_conn(struct fuse_conn *fc);
 
 /* dax.c */
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index e9db2cb8c150878634728685af0fa15e7ade628f..12012bfbf59a93deb9d27e0e0641e4ea2ec4c233 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -930,6 +930,7 @@ static void fuse_iqueue_init(struct fuse_iqueue *fiq,
 	memset(fiq, 0, sizeof(struct fuse_iqueue));
 	spin_lock_init(&fiq->lock);
 	init_waitqueue_head(&fiq->waitq);
+	fiq->reqctr = alloc_percpu(u64);
 	INIT_LIST_HEAD(&fiq->pending);
 	INIT_LIST_HEAD(&fiq->interrupts);
 	fiq->forget_list_tail = &fiq->forget_list_head;

-- 
2.43.0

[PATCH v2 2/4] fuse: Set request unique on allocation

[Bernd Schubert]

This is especially needed for better ftrace analysis,
for example to build histograms. So far the request unique
was missing, because it was added after the first trace message.

IDs/req-unique now might not come up perfectly sequentially
anymore, but especially  with cloned device or io-uring this
did not work perfectly anyway.

Signed-off-by: Bernd Schubert <bschubert@ddn.com>
---
 fs/fuse/dev.c       | 8 +++-----
 fs/fuse/dev_uring.c | 3 ---
 fs/fuse/virtio_fs.c | 3 ---
 3 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index e9592ab092b948bacb5034018bd1f32c917d5c9f..1ccf5a9c61ae2b11bc1d0b799c08e6da908a9782 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -259,8 +259,6 @@ static void fuse_dev_queue_req(struct fuse_iqueue *fiq, struct fuse_req *req)
 {
 	spin_lock(&fiq->lock);
 	if (fiq->connected) {
-		if (req->in.h.opcode != FUSE_NOTIFY_REPLY)
-			req->in.h.unique = fuse_get_unique(fiq);
 		list_add_tail(&req->list, &fiq->pending);
 		fuse_dev_wake_and_unlock(fiq);
 	} else {
@@ -508,6 +506,9 @@ static void fuse_args_to_req(struct fuse_req *req, struct fuse_args *args)
 		req->in.h.total_extlen = args->in_args[args->ext_idx].size / 8;
 	if (args->end)
 		__set_bit(FR_ASYNC, &req->flags);
+
+	if (req->in.h.opcode != FUSE_NOTIFY_REPLY)
+		req->in.h.unique = fuse_get_unique(&req->fm->fc->iq);
 }
 
 ssize_t __fuse_simple_request(struct mnt_idmap *idmap,
@@ -555,9 +556,6 @@ ssize_t __fuse_simple_request(struct mnt_idmap *idmap,
 static bool fuse_request_queue_background_uring(struct fuse_conn *fc,
 					       struct fuse_req *req)
 {
-	struct fuse_iqueue *fiq = &fc->iq;
-
-	req->in.h.unique = fuse_get_unique(fiq);
 	req->in.h.len = sizeof(struct fuse_in_header) +
 		fuse_len_args(req->args->in_numargs,
 			      (struct fuse_arg *) req->args->in_args);
diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
index 82bf458fa9db5b2357ae2d1cf5621ed4db978892..5a05b76249d6fe6214e948955f23eed1e40bb751 100644
--- a/fs/fuse/dev_uring.c
+++ b/fs/fuse/dev_uring.c
@@ -1230,9 +1230,6 @@ void fuse_uring_queue_fuse_req(struct fuse_iqueue *fiq, struct fuse_req *req)
 	if (!queue)
 		goto err;
 
-	if (req->in.h.opcode != FUSE_NOTIFY_REPLY)
-		req->in.h.unique = fuse_get_unique(fiq);
-
 	spin_lock(&queue->lock);
 	err = -ENOTCONN;
 	if (unlikely(queue->stopped))
diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index 82afe78ec542358e2db6f4d955d521652ae363ec..ea13d57133c335554acae33b22e1604424886ac9 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -1482,9 +1482,6 @@ static void virtio_fs_send_req(struct fuse_iqueue *fiq, struct fuse_req *req)
 	struct virtio_fs_vq *fsvq;
 	int ret;
 
-	if (req->in.h.opcode != FUSE_NOTIFY_REPLY)
-		req->in.h.unique = fuse_get_unique(fiq);
-
 	clear_bit(FR_PENDING, &req->flags);
 
 	fs = fiq->priv;

-- 
2.43.0

[PATCH v2 3/4] fuse: {io-uring} Avoid _send code dup

[Bernd Schubert]

fuse_uring_send_next_to_ring() can just call into fuse_uring_send
and avoid code dup.

Signed-off-by: Bernd Schubert <bschubert@ddn.com>
---
 fs/fuse/dev_uring.c | 39 +++++++++++++++------------------------
 1 file changed, 15 insertions(+), 24 deletions(-)

diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
index 5a05b76249d6fe6214e948955f23eed1e40bb751..c5cb2aea75af523e22f539c8e18cfd0d6e771ffc 100644
--- a/fs/fuse/dev_uring.c
+++ b/fs/fuse/dev_uring.c
@@ -667,6 +667,20 @@ static int fuse_uring_prepare_send(struct fuse_ring_ent *ent,
 	return err;
 }
 
+static void fuse_uring_send(struct fuse_ring_ent *ent, struct io_uring_cmd *cmd,
+			    ssize_t ret, unsigned int issue_flags)
+{
+	struct fuse_ring_queue *queue = ent->queue;
+
+	spin_lock(&queue->lock);
+	ent->state = FRRS_USERSPACE;
+	list_move(&ent->list, &queue->ent_in_userspace);
+	ent->cmd = NULL;
+	spin_unlock(&queue->lock);
+
+	io_uring_cmd_done(cmd, ret, 0, issue_flags);
+}
+
 /*
  * Write data to the ring buffer and send the request to userspace,
  * userspace will read it
@@ -676,22 +690,13 @@ static int fuse_uring_send_next_to_ring(struct fuse_ring_ent *ent,
 					struct fuse_req *req,
 					unsigned int issue_flags)
 {
-	struct fuse_ring_queue *queue = ent->queue;
 	int err;
-	struct io_uring_cmd *cmd;
 
 	err = fuse_uring_prepare_send(ent, req);
 	if (err)
 		return err;
 
-	spin_lock(&queue->lock);
-	cmd = ent->cmd;
-	ent->cmd = NULL;
-	ent->state = FRRS_USERSPACE;
-	list_move(&ent->list, &queue->ent_in_userspace);
-	spin_unlock(&queue->lock);
-
-	io_uring_cmd_done(cmd, 0, 0, issue_flags);
+	fuse_uring_send(ent, ent->cmd, 0, issue_flags);
 	return 0;
 }
 
@@ -1151,20 +1156,6 @@ int fuse_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags)
 	return -EIOCBQUEUED;
 }
 
-static void fuse_uring_send(struct fuse_ring_ent *ent, struct io_uring_cmd *cmd,
-			    ssize_t ret, unsigned int issue_flags)
-{
-	struct fuse_ring_queue *queue = ent->queue;
-
-	spin_lock(&queue->lock);
-	ent->state = FRRS_USERSPACE;
-	list_move(&ent->list, &queue->ent_in_userspace);
-	ent->cmd = NULL;
-	spin_unlock(&queue->lock);
-
-	io_uring_cmd_done(cmd, ret, 0, issue_flags);
-}
-
 /*
  * This prepares and sends the ring request in fuse-uring task context.
  * User buffers are not mapped yet - the application does not have permission

-- 
2.43.0

[PATCH v2 4/4] fuse: fine-grained request ftraces

[Bernd Schubert]

Rename trace_fuse_request_send to trace_fuse_request_enqueue
Add trace_fuse_request_send
Add trace_fuse_request_bg_enqueue
Add trace_fuse_request_enqueue

This helps to track entire request time and time in different
queues.

Signed-off-by: Bernd Schubert <bschubert@ddn.com>
---
 fs/fuse/dev.c        |  7 ++++++-
 fs/fuse/dev_uring.c  |  2 ++
 fs/fuse/fuse_trace.h | 57 +++++++++++++++++++++++++++++++++++++---------------
 3 files changed, 49 insertions(+), 17 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 1ccf5a9c61ae2b11bc1d0b799c08e6da908a9782..8e1a95f80e5454d1351ecb90beacbb35779731bb 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -281,7 +281,9 @@ static void fuse_send_one(struct fuse_iqueue *fiq, struct fuse_req *req)
 	req->in.h.len = sizeof(struct fuse_in_header) +
 		fuse_len_args(req->args->in_numargs,
 			      (struct fuse_arg *) req->args->in_args);
-	trace_fuse_request_send(req);
+
+	/* enqueue, as it is send to "fiq->ops queue" */
+	trace_fuse_request_enqueue(req);
 	fiq->ops->send_req(fiq, req);
 }
 
@@ -580,6 +582,8 @@ static int fuse_request_queue_background(struct fuse_req *req)
 	}
 	__set_bit(FR_ISREPLY, &req->flags);
 
+	trace_fuse_request_bg_enqueue(req);
+
 #ifdef CONFIG_FUSE_IO_URING
 	if (fuse_uring_ready(fc))
 		return fuse_request_queue_background_uring(fc, req);
@@ -1314,6 +1318,7 @@ static ssize_t fuse_dev_do_read(struct fuse_dev *fud, struct file *file,
 	clear_bit(FR_PENDING, &req->flags);
 	list_del_init(&req->list);
 	spin_unlock(&fiq->lock);
+	trace_fuse_request_send(req);
 
 	args = req->args;
 	reqsize = req->in.h.len;
diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
index c5cb2aea75af523e22f539c8e18cfd0d6e771ffc..e5ed146b990e12c6cc2a18aaa9b527c276870aba 100644
--- a/fs/fuse/dev_uring.c
+++ b/fs/fuse/dev_uring.c
@@ -7,6 +7,7 @@
 #include "fuse_i.h"
 #include "dev_uring_i.h"
 #include "fuse_dev_i.h"
+#include "fuse_trace.h"
 
 #include <linux/fs.h>
 #include <linux/io_uring/cmd.h>
@@ -678,6 +679,7 @@ static void fuse_uring_send(struct fuse_ring_ent *ent, struct io_uring_cmd *cmd,
 	ent->cmd = NULL;
 	spin_unlock(&queue->lock);
 
+	trace_fuse_request_send(ent->fuse_req);
 	io_uring_cmd_done(cmd, ret, 0, issue_flags);
 }
 
diff --git a/fs/fuse/fuse_trace.h b/fs/fuse/fuse_trace.h
index bbe9ddd8c71696ddcbca055f6c4c451661bb4444..393c630e7726356da16add7da4b5913b9f725b25 100644
--- a/fs/fuse/fuse_trace.h
+++ b/fs/fuse/fuse_trace.h
@@ -77,30 +77,55 @@ OPCODES
 #define EM(a, b)	{a, b},
 #define EMe(a, b)	{a, b}
 
-TRACE_EVENT(fuse_request_send,
+#define FUSE_REQ_TRACE_FIELDS				\
+	__field(dev_t,			connection)	\
+	__field(uint64_t,		unique)		\
+	__field(enum fuse_opcode,	opcode)		\
+	__field(uint32_t,		len)		\
+
+#define FUSE_REQ_TRACE_ASSIGN(req)				\
+	do {							\
+		__entry->connection	= req->fm->fc->dev;	\
+		__entry->unique		= req->in.h.unique;	\
+		__entry->opcode		= req->in.h.opcode;	\
+		__entry->len		= req->in.h.len;	\
+	} while (0)
+
+
+TRACE_EVENT(fuse_request_enqueue,
 	TP_PROTO(const struct fuse_req *req),
-
 	TP_ARGS(req),
-
-	TP_STRUCT__entry(
-		__field(dev_t,			connection)
-		__field(uint64_t,		unique)
-		__field(enum fuse_opcode,	opcode)
-		__field(uint32_t,		len)
-	),
-
-	TP_fast_assign(
-		__entry->connection	=	req->fm->fc->dev;
-		__entry->unique		=	req->in.h.unique;
-		__entry->opcode		=	req->in.h.opcode;
-		__entry->len		=	req->in.h.len;
-	),
+	TP_STRUCT__entry(FUSE_REQ_TRACE_FIELDS),
+	TP_fast_assign(FUSE_REQ_TRACE_ASSIGN(req)),
 
 	TP_printk("connection %u req %llu opcode %u (%s) len %u ",
 		  __entry->connection, __entry->unique, __entry->opcode,
 		  __print_symbolic(__entry->opcode, OPCODES), __entry->len)
 );
 
+TRACE_EVENT(fuse_request_bg_enqueue,
+	TP_PROTO(const struct fuse_req *req),
+	TP_ARGS(req),
+	TP_STRUCT__entry(FUSE_REQ_TRACE_FIELDS),
+	TP_fast_assign(FUSE_REQ_TRACE_ASSIGN(req)),
+
+	TP_printk("connection %u req %llu opcode %u (%s) len %u ",
+		  __entry->connection, __entry->unique, __entry->opcode,
+		  __print_symbolic(__entry->opcode, OPCODES), __entry->len)
+);
+
+TRACE_EVENT(fuse_request_send,
+	TP_PROTO(const struct fuse_req *req),
+	TP_ARGS(req),
+	TP_STRUCT__entry(FUSE_REQ_TRACE_FIELDS),
+	TP_fast_assign(FUSE_REQ_TRACE_ASSIGN(req)),
+
+	TP_printk("connection %u req %llu opcode %u (%s) len %u ",
+		  __entry->connection, __entry->unique, __entry->opcode,
+		  __print_symbolic(__entry->opcode, OPCODES), __entry->len)
+);
+
+
 TRACE_EVENT(fuse_request_end,
 	TP_PROTO(const struct fuse_req *req),
 

-- 
2.43.0

Re: [PATCH v2 1/4] fuse: Make the fuse unique value a per-cpu counter

[Joanne Koong]

On Thu, Apr 3, 2025 at 6:05 AM Bernd Schubert <bschubert@ddn.com> wrote:
>
> No need to take lock, we can have that per cpu and
> add in the current cpu as offset.
>
> fuse-io-uring and virtiofs especially benefit from it
> as they don't need the fiq lock at all.
>
> Signed-off-by: Bernd Schubert <bschubert@ddn.com>
> ---
>  fs/fuse/dev.c        | 24 +++---------------------
>  fs/fuse/fuse_dev_i.h |  4 ----
>  fs/fuse/fuse_i.h     | 23 ++++++++++++++++++-----
>  fs/fuse/inode.c      |  1 +
>  4 files changed, 22 insertions(+), 30 deletions(-)
>
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index 51e31df4c54613280a9c295f530b18e1d461a974..e9592ab092b948bacb5034018bd1f32c917d5c9f 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -204,24 +204,6 @@ unsigned int fuse_len_args(unsigned int numargs, struct fuse_arg *args)
>  }
>  EXPORT_SYMBOL_GPL(fuse_len_args);
>
> -static u64 fuse_get_unique_locked(struct fuse_iqueue *fiq)
> -{
> -       fiq->reqctr += FUSE_REQ_ID_STEP;
> -       return fiq->reqctr;
> -}
> -
> -u64 fuse_get_unique(struct fuse_iqueue *fiq)
> -{
> -       u64 ret;
> -
> -       spin_lock(&fiq->lock);
> -       ret = fuse_get_unique_locked(fiq);
> -       spin_unlock(&fiq->lock);
> -
> -       return ret;
> -}
> -EXPORT_SYMBOL_GPL(fuse_get_unique);
> -
>  unsigned int fuse_req_hash(u64 unique)
>  {
>         return hash_long(unique & ~FUSE_INT_REQ_BIT, FUSE_PQ_HASH_BITS);
> @@ -278,7 +260,7 @@ static void fuse_dev_queue_req(struct fuse_iqueue *fiq, struct fuse_req *req)
>         spin_lock(&fiq->lock);
>         if (fiq->connected) {
>                 if (req->in.h.opcode != FUSE_NOTIFY_REPLY)
> -                       req->in.h.unique = fuse_get_unique_locked(fiq);
> +                       req->in.h.unique = fuse_get_unique(fiq);
>                 list_add_tail(&req->list, &fiq->pending);
>                 fuse_dev_wake_and_unlock(fiq);
>         } else {
> @@ -1177,7 +1159,7 @@ __releases(fiq->lock)
>         struct fuse_in_header ih = {
>                 .opcode = FUSE_FORGET,
>                 .nodeid = forget->forget_one.nodeid,
> -               .unique = fuse_get_unique_locked(fiq),
> +               .unique = fuse_get_unique(fiq),
>                 .len = sizeof(ih) + sizeof(arg),
>         };
>
> @@ -1208,7 +1190,7 @@ __releases(fiq->lock)
>         struct fuse_batch_forget_in arg = { .count = 0 };
>         struct fuse_in_header ih = {
>                 .opcode = FUSE_BATCH_FORGET,
> -               .unique = fuse_get_unique_locked(fiq),
> +               .unique = fuse_get_unique(fiq),
>                 .len = sizeof(ih) + sizeof(arg),
>         };
>
> diff --git a/fs/fuse/fuse_dev_i.h b/fs/fuse/fuse_dev_i.h
> index 3b2bfe1248d3573abe3b144a6d4bf6a502f56a40..e0afd837a8024450bab77312c7eebdcc7a39bd36 100644
> --- a/fs/fuse/fuse_dev_i.h
> +++ b/fs/fuse/fuse_dev_i.h
> @@ -8,10 +8,6 @@
>
>  #include <linux/types.h>
>
> -/* Ordinary requests have even IDs, while interrupts IDs are odd */
> -#define FUSE_INT_REQ_BIT (1ULL << 0)
> -#define FUSE_REQ_ID_STEP (1ULL << 1)
> -
>  struct fuse_arg;
>  struct fuse_args;
>  struct fuse_pqueue;
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index fee96fe7887b30cd57b8a6bbda11447a228cf446..73c612dd58e45ecde0b8f72fd58ac603d12cf202 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -9,6 +9,8 @@
>  #ifndef _FS_FUSE_I_H
>  #define _FS_FUSE_I_H
>
> +#include "linux/percpu-defs.h"

Think the convention is #include <linux/percpu-defs.h> though I wonder
if you even need this. I see other filesystems using percpu counters
but they don't explicitly include this header. Compilation seems fine
without it.

> +#include "linux/threads.h"

Do you need threads.h?

>  #ifndef pr_fmt
>  # define pr_fmt(fmt) "fuse: " fmt
>  #endif
> @@ -44,6 +46,10 @@
>  /** Number of dentries for each connection in the control filesystem */
>  #define FUSE_CTL_NUM_DENTRIES 5
>
> +/* Ordinary requests have even IDs, while interrupts IDs are odd */
> +#define FUSE_INT_REQ_BIT (1ULL << 0)
> +#define FUSE_REQ_ID_STEP (1ULL << 1)
> +
>  /** Maximum of max_pages received in init_out */
>  extern unsigned int fuse_max_pages_limit;
>
> @@ -490,7 +496,7 @@ struct fuse_iqueue {
>         wait_queue_head_t waitq;
>
>         /** The next unique request id */
> -       u64 reqctr;
> +       u64 __percpu *reqctr;
>
>         /** The list of pending requests */
>         struct list_head pending;
> @@ -1065,6 +1071,17 @@ static inline void fuse_sync_bucket_dec(struct fuse_sync_bucket *bucket)
>         rcu_read_unlock();
>  }
>
> +/**
> + * Get the next unique ID for a request
> + */
> +static inline u64 fuse_get_unique(struct fuse_iqueue *fiq)
> +{
> +       int step = FUSE_REQ_ID_STEP * (task_cpu(current) + 1);

I don't think you need the + 1 here. This works fine even if
task_cpu() returns 0.

> +       u64 cntr = this_cpu_inc_return(*fiq->reqctr);
> +
> +       return cntr * FUSE_REQ_ID_STEP * NR_CPUS + step;

if you want to save a multiplication, I think we could just do

 static inline u64 fuse_get_unique(struct fuse_iqueue *fiq) {
   u64 cntr = this_cpu_inc_return(*fiq->reqctr);
   return (cntr * NR_CPUS + task_cpu(current)) * FUSE_REQ_ID_STEP;
}

> +}
> +
>  /** Device operations */
>  extern const struct file_operations fuse_dev_operations;
>
> @@ -1415,10 +1432,6 @@ int fuse_readdir(struct file *file, struct dir_context *ctx);
>   */
>  unsigned int fuse_len_args(unsigned int numargs, struct fuse_arg *args);
>
> -/**
> - * Get the next unique ID for a request
> - */
> -u64 fuse_get_unique(struct fuse_iqueue *fiq);
>  void fuse_free_conn(struct fuse_conn *fc);
>
>  /* dax.c */
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index e9db2cb8c150878634728685af0fa15e7ade628f..12012bfbf59a93deb9d27e0e0641e4ea2ec4c233 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -930,6 +930,7 @@ static void fuse_iqueue_init(struct fuse_iqueue *fiq,
>         memset(fiq, 0, sizeof(struct fuse_iqueue));
>         spin_lock_init(&fiq->lock);
>         init_waitqueue_head(&fiq->waitq);
> +       fiq->reqctr = alloc_percpu(u64);
>         INIT_LIST_HEAD(&fiq->pending);
>         INIT_LIST_HEAD(&fiq->interrupts);
>         fiq->forget_list_tail = &fiq->forget_list_head;
>

I think we need a free_percpu(fiq->reqctr); as well when the last ref
on the connection is dropped or else this is leaked

Thanks,
Joanne

> --
> 2.43.0
>

Re: [PATCH v2 1/4] fuse: Make the fuse unique value a per-cpu counter

[Bernd Schubert]

Thanks for your review Joanne!

On 4/3/25 20:27, Joanne Koong wrote:
> On Thu, Apr 3, 2025 at 6:05 AM Bernd Schubert <bschubert@ddn.com> wrote:
>>
>> No need to take lock, we can have that per cpu and
>> add in the current cpu as offset.
>>
>> fuse-io-uring and virtiofs especially benefit from it
>> as they don't need the fiq lock at all.
>>
>> Signed-off-by: Bernd Schubert <bschubert@ddn.com>
>> ---
>>  fs/fuse/dev.c        | 24 +++---------------------
>>  fs/fuse/fuse_dev_i.h |  4 ----
>>  fs/fuse/fuse_i.h     | 23 ++++++++++++++++++-----
>>  fs/fuse/inode.c      |  1 +
>>  4 files changed, 22 insertions(+), 30 deletions(-)
>>
>> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
>> index 51e31df4c54613280a9c295f530b18e1d461a974..e9592ab092b948bacb5034018bd1f32c917d5c9f 100644
>> --- a/fs/fuse/dev.c
>> +++ b/fs/fuse/dev.c
>> @@ -204,24 +204,6 @@ unsigned int fuse_len_args(unsigned int numargs, struct fuse_arg *args)
>>  }
>>  EXPORT_SYMBOL_GPL(fuse_len_args);
>>
>> -static u64 fuse_get_unique_locked(struct fuse_iqueue *fiq)
>> -{
>> -       fiq->reqctr += FUSE_REQ_ID_STEP;
>> -       return fiq->reqctr;
>> -}
>> -
>> -u64 fuse_get_unique(struct fuse_iqueue *fiq)
>> -{
>> -       u64 ret;
>> -
>> -       spin_lock(&fiq->lock);
>> -       ret = fuse_get_unique_locked(fiq);
>> -       spin_unlock(&fiq->lock);
>> -
>> -       return ret;
>> -}
>> -EXPORT_SYMBOL_GPL(fuse_get_unique);
>> -
>>  unsigned int fuse_req_hash(u64 unique)
>>  {
>>         return hash_long(unique & ~FUSE_INT_REQ_BIT, FUSE_PQ_HASH_BITS);
>> @@ -278,7 +260,7 @@ static void fuse_dev_queue_req(struct fuse_iqueue *fiq, struct fuse_req *req)
>>         spin_lock(&fiq->lock);
>>         if (fiq->connected) {
>>                 if (req->in.h.opcode != FUSE_NOTIFY_REPLY)
>> -                       req->in.h.unique = fuse_get_unique_locked(fiq);
>> +                       req->in.h.unique = fuse_get_unique(fiq);
>>                 list_add_tail(&req->list, &fiq->pending);
>>                 fuse_dev_wake_and_unlock(fiq);
>>         } else {
>> @@ -1177,7 +1159,7 @@ __releases(fiq->lock)
>>         struct fuse_in_header ih = {
>>                 .opcode = FUSE_FORGET,
>>                 .nodeid = forget->forget_one.nodeid,
>> -               .unique = fuse_get_unique_locked(fiq),
>> +               .unique = fuse_get_unique(fiq),
>>                 .len = sizeof(ih) + sizeof(arg),
>>         };
>>
>> @@ -1208,7 +1190,7 @@ __releases(fiq->lock)
>>         struct fuse_batch_forget_in arg = { .count = 0 };
>>         struct fuse_in_header ih = {
>>                 .opcode = FUSE_BATCH_FORGET,
>> -               .unique = fuse_get_unique_locked(fiq),
>> +               .unique = fuse_get_unique(fiq),
>>                 .len = sizeof(ih) + sizeof(arg),
>>         };
>>
>> diff --git a/fs/fuse/fuse_dev_i.h b/fs/fuse/fuse_dev_i.h
>> index 3b2bfe1248d3573abe3b144a6d4bf6a502f56a40..e0afd837a8024450bab77312c7eebdcc7a39bd36 100644
>> --- a/fs/fuse/fuse_dev_i.h
>> +++ b/fs/fuse/fuse_dev_i.h
>> @@ -8,10 +8,6 @@
>>
>>  #include <linux/types.h>
>>
>> -/* Ordinary requests have even IDs, while interrupts IDs are odd */
>> -#define FUSE_INT_REQ_BIT (1ULL << 0)
>> -#define FUSE_REQ_ID_STEP (1ULL << 1)
>> -
>>  struct fuse_arg;
>>  struct fuse_args;
>>  struct fuse_pqueue;
>> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
>> index fee96fe7887b30cd57b8a6bbda11447a228cf446..73c612dd58e45ecde0b8f72fd58ac603d12cf202 100644
>> --- a/fs/fuse/fuse_i.h
>> +++ b/fs/fuse/fuse_i.h
>> @@ -9,6 +9,8 @@
>>  #ifndef _FS_FUSE_I_H
>>  #define _FS_FUSE_I_H
>>
>> +#include "linux/percpu-defs.h"
> 
> Think the convention is #include <linux/percpu-defs.h> though I wonder
> if you even need this. I see other filesystems using percpu counters
> but they don't explicitly include this header. Compilation seems fine
> without it.
> 
>> +#include "linux/threads.h"
> 
> Do you need threads.h?

Oh, I fixed my .clangd settings, it had added headers itself.

> 
>>  #ifndef pr_fmt
>>  # define pr_fmt(fmt) "fuse: " fmt
>>  #endif
>> @@ -44,6 +46,10 @@
>>  /** Number of dentries for each connection in the control filesystem */
>>  #define FUSE_CTL_NUM_DENTRIES 5
>>
>> +/* Ordinary requests have even IDs, while interrupts IDs are odd */
>> +#define FUSE_INT_REQ_BIT (1ULL << 0)
>> +#define FUSE_REQ_ID_STEP (1ULL << 1)
>> +
>>  /** Maximum of max_pages received in init_out */
>>  extern unsigned int fuse_max_pages_limit;
>>
>> @@ -490,7 +496,7 @@ struct fuse_iqueue {
>>         wait_queue_head_t waitq;
>>
>>         /** The next unique request id */
>> -       u64 reqctr;
>> +       u64 __percpu *reqctr;
>>
>>         /** The list of pending requests */
>>         struct list_head pending;
>> @@ -1065,6 +1071,17 @@ static inline void fuse_sync_bucket_dec(struct fuse_sync_bucket *bucket)
>>         rcu_read_unlock();
>>  }
>>
>> +/**
>> + * Get the next unique ID for a request
>> + */
>> +static inline u64 fuse_get_unique(struct fuse_iqueue *fiq)
>> +{
>> +       int step = FUSE_REQ_ID_STEP * (task_cpu(current) + 1);
> 
> I don't think you need the + 1 here. This works fine even if
> task_cpu() returns 0.

Yeah right, I had a version that was multiplying by the step

> 
>> +       u64 cntr = this_cpu_inc_return(*fiq->reqctr);
>> +
>> +       return cntr * FUSE_REQ_ID_STEP * NR_CPUS + step;
> 
> if you want to save a multiplication, I think we could just do
> 
>  static inline u64 fuse_get_unique(struct fuse_iqueue *fiq) {
>    u64 cntr = this_cpu_inc_return(*fiq->reqctr);
>    return (cntr * NR_CPUS + task_cpu(current)) * FUSE_REQ_ID_STEP;
> }
> 

I find this harder to read - the compiler will optimize that anyway?



>> +}
>> +
>>  /** Device operations */
>>  extern const struct file_operations fuse_dev_operations;
>>
>> @@ -1415,10 +1432,6 @@ int fuse_readdir(struct file *file, struct dir_context *ctx);
>>   */
>>  unsigned int fuse_len_args(unsigned int numargs, struct fuse_arg *args);
>>
>> -/**
>> - * Get the next unique ID for a request
>> - */
>> -u64 fuse_get_unique(struct fuse_iqueue *fiq);
>>  void fuse_free_conn(struct fuse_conn *fc);
>>
>>  /* dax.c */
>> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
>> index e9db2cb8c150878634728685af0fa15e7ade628f..12012bfbf59a93deb9d27e0e0641e4ea2ec4c233 100644
>> --- a/fs/fuse/inode.c
>> +++ b/fs/fuse/inode.c
>> @@ -930,6 +930,7 @@ static void fuse_iqueue_init(struct fuse_iqueue *fiq,
>>         memset(fiq, 0, sizeof(struct fuse_iqueue));
>>         spin_lock_init(&fiq->lock);
>>         init_waitqueue_head(&fiq->waitq);
>> +       fiq->reqctr = alloc_percpu(u64);
>>         INIT_LIST_HEAD(&fiq->pending);
>>         INIT_LIST_HEAD(&fiq->interrupts);
>>         fiq->forget_list_tail = &fiq->forget_list_head;
>>
> 
> I think we need a free_percpu(fiq->reqctr); as well when the last ref
> on the connection is dropped or else this is leaked

Right, totally forgot.


Thanks,
Bernd