[RFC PATCH v5 0/2] fuse: work queues to invalided dentries

[RFC PATCH v5 0/2] fuse: work queues to invalided dentries

[Luis Henriques]

Hi Miklos,

Since there are huge changes in v5, I decided to send an early version of
this patch(set).  I'm sure there are bugs and stupid mistakes, but I'd
love to get some early feedback before further testing it.  And that's why
I also decided to add back the RFC tag to the subject.

One annoying effect of making the dentries trees global data structures is
that it makes things more complicated to handle epoch changes (2nd patch).
It forces the work queue to walk through *all* the dentries in *all* the
trees.

Changes since v4:

- Dropped extra check in fuse_dentry_tree_add_node() (Chunsheng)
- Make the dentries trees global instead of per fuse_conn (Miklos)
- Protect trees with hashed locking instead of a single lock (Miklos)
- Added new work queue (2nd patch) specifically to handle epoch (Miklos)

Changes since v3:

- Use of need_resched() instead of limiting the work queue to run for 5
  seconds
- Restore usage of union with rcu_head, in struct fuse_dentry
- Minor changes in comments (e.g. s/workqueue/work queue/)

Changes since v2:

- Major rework, the dentries tree nodes are now in fuse_dentry and they are
  tied to the actual dentry lifetime
- Mount option is now a module parameter
- workqueue now runs for at most 5 seconds before rescheduling



Luis Henriques (2):
  fuse: new work queue to periodically invalidate expired dentries
  fuse: new work queue to invalidate dentries from old epochs

 fs/fuse/dev.c    |   7 +-
 fs/fuse/dir.c    | 251 +++++++++++++++++++++++++++++++++++++++++++----
 fs/fuse/fuse_i.h |  13 +++
 fs/fuse/inode.c  |  48 +++++----
 4 files changed, 276 insertions(+), 43 deletions(-)

[RFC PATCH v5 1/2] fuse: new work queue to periodically invalidate expired dentries

[Luis Henriques]

This patch adds the infrastructure to keep track of all dentries created
for FUSE file systems.  A set of rbtrees will be used to keep dentries.

A new module parameter 'inval_wq' is also added.  When set, it will start
a work queue which will periodically invalidate expired dentries.  The
value of this new parameter is the period, in seconds, for this work
queue.  When it is set, every new dentry will be added to one of the
rbtree, sorted by its expiry time.

When the work queue is executed, it will check all the trees where the
dentries are kept and will invalidate those that have timed-out.

The work queue period can not be smaller than 5 seconds, but can be
disabled by setting 'inval_wq' to zero (which is the default).

Signed-off-by: Luis Henriques <luis@igalia.com>
---
 fs/fuse/dir.c    | 217 ++++++++++++++++++++++++++++++++++++++++++-----
 fs/fuse/fuse_i.h |   9 ++
 fs/fuse/inode.c  |   7 ++
 3 files changed, 212 insertions(+), 21 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 2d817d7cab26..b7ddf0f2b3a4 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -27,6 +27,62 @@ module_param(allow_sys_admin_access, bool, 0644);
 MODULE_PARM_DESC(allow_sys_admin_access,
 		 "Allow users with CAP_SYS_ADMIN in initial userns to bypass allow_other access check");
 
+struct dentry_bucket {
+	struct rb_root tree;
+	spinlock_t lock;
+};
+
+#define HASH_BITS	12
+#define HASH_SIZE	(1 << HASH_BITS)
+static struct dentry_bucket dentry_hash[HASH_SIZE];
+struct delayed_work dentry_tree_work;
+
+/* Minimum invalidation work queue frequency */
+#define FUSE_DENTRY_INVAL_FREQ_MIN 5
+
+unsigned __read_mostly inval_wq;
+static int inval_wq_set(const char *val, const struct kernel_param *kp)
+{
+	unsigned int num;
+	unsigned int old = inval_wq;
+	int ret;
+
+	if (!val)
+		return -EINVAL;
+
+	ret = kstrtouint(val, 0, &num);
+	if (ret)
+		return ret;
+
+	if ((num < FUSE_DENTRY_INVAL_FREQ_MIN) && (num != 0))
+		return -EINVAL;
+
+	*((unsigned int *)kp->arg) = num;
+
+	if (num && !old)
+		schedule_delayed_work(&dentry_tree_work,
+				      secs_to_jiffies(num));
+	else if (!num && old)
+		cancel_delayed_work_sync(&dentry_tree_work);
+
+	return 0;
+}
+static const struct kernel_param_ops inval_wq_ops = {
+	.set = inval_wq_set,
+	.get = param_get_uint,
+};
+module_param_cb(inval_wq, &inval_wq_ops, &inval_wq, 0644);
+__MODULE_PARM_TYPE(inval_wq, "uint");
+MODULE_PARM_DESC(inval_wq,
+		 "Dentries invalidation work queue period in secs (>= 5).");
+
+static inline struct dentry_bucket *get_dentry_bucket(struct dentry *dentry)
+{
+	int i = hash_ptr(dentry, HASH_BITS);
+
+	return &dentry_hash[i];
+}
+
 static void fuse_advise_use_readdirplus(struct inode *dir)
 {
 	struct fuse_inode *fi = get_fuse_inode(dir);
@@ -34,33 +90,137 @@ static void fuse_advise_use_readdirplus(struct inode *dir)
 	set_bit(FUSE_I_ADVISE_RDPLUS, &fi->state);
 }
 
-#if BITS_PER_LONG >= 64
-static inline void __fuse_dentry_settime(struct dentry *entry, u64 time)
+struct fuse_dentry {
+	u64 time;
+	union {
+		struct rcu_head rcu;
+		struct rb_node node;
+	};
+	struct dentry *dentry;
+};
+
+static void __fuse_dentry_tree_del_node(struct fuse_dentry *fd,
+					struct dentry_bucket *bucket)
 {
-	entry->d_fsdata = (void *) time;
+	if (!RB_EMPTY_NODE(&fd->node)) {
+		rb_erase(&fd->node, &bucket->tree);
+		RB_CLEAR_NODE(&fd->node);
+	}
 }
 
-static inline u64 fuse_dentry_time(const struct dentry *entry)
+static void fuse_dentry_tree_del_node(struct dentry *dentry)
 {
-	return (u64)entry->d_fsdata;
+	struct fuse_dentry *fd = dentry->d_fsdata;
+	struct dentry_bucket *bucket = get_dentry_bucket(dentry);
+
+	if (!inval_wq && RB_EMPTY_NODE(&fd->node))
+		return;
+
+	spin_lock(&bucket->lock);
+	__fuse_dentry_tree_del_node(fd, bucket);
+	spin_unlock(&bucket->lock);
 }
 
-#else
-union fuse_dentry {
-	u64 time;
-	struct rcu_head rcu;
-};
+static void fuse_dentry_tree_add_node(struct dentry *dentry)
+{
+	struct fuse_dentry *fd = dentry->d_fsdata;
+	struct dentry_bucket *bucket = get_dentry_bucket(dentry);
+	struct fuse_dentry *cur;
+	struct rb_node **p, *parent = NULL;
+
+	if (!inval_wq)
+		return;
+
+	spin_lock(&bucket->lock);
+
+	__fuse_dentry_tree_del_node(fd, bucket);
+
+	p = &bucket->tree.rb_node;
+	while (*p) {
+		parent = *p;
+		cur = rb_entry(*p, struct fuse_dentry, node);
+		if (fd->time < cur->time)
+			p = &(*p)->rb_left;
+		else if (fd->time > cur->time)
+			p = &(*p)->rb_right;
+		else
+			break;
+	}
+	rb_link_node(&fd->node, parent, p);
+	rb_insert_color(&fd->node, &bucket->tree);
+	spin_unlock(&bucket->lock);
+}
+
+/*
+ * work queue which, when enabled, will periodically check for expired dentries
+ * in the dentries tree.
+ */
+static void fuse_dentry_tree_work(struct work_struct *work)
+{
+	struct fuse_dentry *fd;
+	struct rb_node *node;
+	int i;
+
+	for (i = 0; i < HASH_SIZE; i++) {
+		spin_lock(&dentry_hash[i].lock);
+		node = rb_first(&dentry_hash[i].tree);
+		while (node && !need_resched()) {
+			fd = rb_entry(node, struct fuse_dentry, node);
+			if (time_after64(get_jiffies_64(), fd->time)) {
+				rb_erase(&fd->node, &dentry_hash[i].tree);
+				RB_CLEAR_NODE(&fd->node);
+				spin_unlock(&dentry_hash[i].lock);
+				d_invalidate(fd->dentry);
+				spin_lock(&dentry_hash[i].lock);
+			} else
+				break;
+			node = rb_first(&dentry_hash[i].tree);
+		}
+		spin_unlock(&dentry_hash[i].lock);
+	}
+
+	schedule_delayed_work(&dentry_tree_work, secs_to_jiffies(inval_wq));
+}
+
+void fuse_dentry_tree_init(void)
+{
+	int i;
+
+	for (i = 0; i < HASH_SIZE; i++) {
+		spin_lock_init(&dentry_hash[i].lock);
+		dentry_hash[i].tree = RB_ROOT;
+	}
+	INIT_DELAYED_WORK(&dentry_tree_work, fuse_dentry_tree_work);
+}
+
+void fuse_dentry_tree_cleanup(void)
+{
+	struct rb_node *n;
+	int i;
+
+	inval_wq = 0;
+	cancel_delayed_work_sync(&dentry_tree_work);
+
+	for (i = 0; i < HASH_SIZE; i++) {
+		spin_lock(&dentry_hash[i].lock);
+		while (!RB_EMPTY_ROOT(&dentry_hash[i].tree)) {
+			n = rb_first(&dentry_hash[i].tree);
+			rb_erase(n, &dentry_hash[i].tree);
+			RB_CLEAR_NODE(n);
+		}
+		spin_unlock(&dentry_hash[i].lock);
+	}
+}
 
 static inline void __fuse_dentry_settime(struct dentry *dentry, u64 time)
 {
-	((union fuse_dentry *) dentry->d_fsdata)->time = time;
+	((struct fuse_dentry *) dentry->d_fsdata)->time = time;
 }
 
 static inline u64 fuse_dentry_time(const struct dentry *entry)
 {
-	return ((union fuse_dentry *) entry->d_fsdata)->time;
+	return ((struct fuse_dentry *) entry->d_fsdata)->time;
 }
-#endif
 
 static void fuse_dentry_settime(struct dentry *dentry, u64 time)
 {
@@ -81,6 +241,7 @@ static void fuse_dentry_settime(struct dentry *dentry, u64 time)
 	}
 
 	__fuse_dentry_settime(dentry, time);
+	fuse_dentry_tree_add_node(dentry);
 }
 
 /*
@@ -283,21 +444,36 @@ static int fuse_dentry_revalidate(struct inode *dir, const struct qstr *name,
 	goto out;
 }
 
-#if BITS_PER_LONG < 64
 static int fuse_dentry_init(struct dentry *dentry)
 {
-	dentry->d_fsdata = kzalloc(sizeof(union fuse_dentry),
-				   GFP_KERNEL_ACCOUNT | __GFP_RECLAIMABLE);
+	struct fuse_dentry *fd;
 
-	return dentry->d_fsdata ? 0 : -ENOMEM;
+	fd = kzalloc(sizeof(struct fuse_dentry),
+			  GFP_KERNEL_ACCOUNT | __GFP_RECLAIMABLE);
+	if (!fd)
+		return -ENOMEM;
+
+	fd->dentry = dentry;
+	RB_CLEAR_NODE(&fd->node);
+	dentry->d_fsdata = fd;
+
+	return 0;
+}
+
+static void fuse_dentry_prune(struct dentry *dentry)
+{
+	struct fuse_dentry *fd = dentry->d_fsdata;
+
+	if (!RB_EMPTY_NODE(&fd->node))
+		fuse_dentry_tree_del_node(dentry);
 }
+
 static void fuse_dentry_release(struct dentry *dentry)
 {
-	union fuse_dentry *fd = dentry->d_fsdata;
+	struct fuse_dentry *fd = dentry->d_fsdata;
 
 	kfree_rcu(fd, rcu);
 }
-#endif
 
 static int fuse_dentry_delete(const struct dentry *dentry)
 {
@@ -331,10 +507,9 @@ static struct vfsmount *fuse_dentry_automount(struct path *path)
 const struct dentry_operations fuse_dentry_operations = {
 	.d_revalidate	= fuse_dentry_revalidate,
 	.d_delete	= fuse_dentry_delete,
-#if BITS_PER_LONG < 64
 	.d_init		= fuse_dentry_init,
+	.d_prune	= fuse_dentry_prune,
 	.d_release	= fuse_dentry_release,
-#endif
 	.d_automount	= fuse_dentry_automount,
 };
 
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index ec248d13c8bf..214162f12353 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -54,6 +54,12 @@
 /** Frequency (in jiffies) of request timeout checks, if opted into */
 extern const unsigned long fuse_timeout_timer_freq;
 
+/*
+ * Dentries invalidation workqueue period, in seconds.  It shall be >= 5
+ * seconds, or 0 (zero), in which case no workqueue will be created.
+ */
+extern unsigned inval_wq __read_mostly;
+
 /** Maximum of max_pages received in init_out */
 extern unsigned int fuse_max_pages_limit;
 /*
@@ -1252,6 +1258,9 @@ void fuse_wait_aborted(struct fuse_conn *fc);
 /* Check if any requests timed out */
 void fuse_check_timeout(struct work_struct *work);
 
+void fuse_dentry_tree_init(void);
+void fuse_dentry_tree_cleanup(void);
+
 /**
  * Invalidate inode attributes
  */
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 67c2318bfc42..25e51efc82ee 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -2045,6 +2045,10 @@ void fuse_conn_destroy(struct fuse_mount *fm)
 
 	fuse_abort_conn(fc);
 	fuse_wait_aborted(fc);
+	/*
+	 * XXX prune dentries:
+	 * fuse_dentry_tree_prune(fc);
+	 */
 
 	if (!list_empty(&fc->entry)) {
 		mutex_lock(&fuse_mutex);
@@ -2240,6 +2244,8 @@ static int __init fuse_init(void)
 	if (res)
 		goto err_sysfs_cleanup;
 
+	fuse_dentry_tree_init();
+
 	sanitize_global_limit(&max_user_bgreq);
 	sanitize_global_limit(&max_user_congthresh);
 
@@ -2259,6 +2265,7 @@ static void __exit fuse_exit(void)
 {
 	pr_debug("exit\n");
 
+	fuse_dentry_tree_cleanup();
 	fuse_ctl_cleanup();
 	fuse_sysfs_cleanup();
 	fuse_fs_cleanup();

[RFC PATCH v5 2/2] fuse: new work queue to invalidate dentries from old epochs

[Luis Henriques]

With the infrastructure introduced to periodically invalidate expired
dentries, it is now possible to add an extra work queue to invalidate
dentries when an epoch is incremented.  This work queue will only be
triggered when the 'inval_wq' parameter is set.

Signed-off-by: Luis Henriques <luis@igalia.com>
---
 fs/fuse/dev.c    |  7 ++++---
 fs/fuse/dir.c    | 34 ++++++++++++++++++++++++++++++++++
 fs/fuse/fuse_i.h |  4 ++++
 fs/fuse/inode.c  | 41 ++++++++++++++++++++++-------------------
 4 files changed, 64 insertions(+), 22 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index e80cd8f2c049..48c5c01c3e5b 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -2033,13 +2033,14 @@ static int fuse_notify_resend(struct fuse_conn *fc)
 
 /*
  * Increments the fuse connection epoch.  This will result of dentries from
- * previous epochs to be invalidated.
- *
- * XXX optimization: add call to shrink_dcache_sb()?
+ * previous epochs to be invalidated.  Additionally, if inval_wq is set, a work
+ * queue is scheduled to trigger the invalidation.
  */
 static int fuse_notify_inc_epoch(struct fuse_conn *fc)
 {
 	atomic_inc(&fc->epoch);
+	if (inval_wq)
+		schedule_work(&fc->epoch_work);
 
 	return 0;
 }
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index b7ddf0f2b3a4..f05cb8f4c555 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -182,6 +182,40 @@ static void fuse_dentry_tree_work(struct work_struct *work)
 	schedule_delayed_work(&dentry_tree_work, secs_to_jiffies(inval_wq));
 }
 
+void fuse_epoch_work(struct work_struct *work)
+{
+	struct fuse_conn *fc = container_of(work, struct fuse_conn,
+					    epoch_work);
+	struct fuse_dentry *fd;
+	struct rb_node *node, *next;
+	struct dentry *dentry;
+	int epoch;
+	int i;
+
+	epoch = atomic_read(&fc->epoch);
+
+	for (i = 0; i < HASH_SIZE; i++) {
+		spin_lock(&dentry_hash[i].lock);
+		node = rb_first(&dentry_hash[i].tree);
+		while (node) {
+			next = rb_next(node);
+			fd = rb_entry(node, struct fuse_dentry, node);
+			dentry = fd->dentry;
+			if ((fc == get_fuse_conn_super(dentry->d_sb)) &&
+			    (dentry->d_time < epoch)) {
+				rb_erase(&fd->node, &dentry_hash[i].tree);
+				RB_CLEAR_NODE(&fd->node);
+				/* XXX use tmp tree and d_invalidate all at once? */
+				spin_unlock(&dentry_hash[i].lock);
+				d_invalidate(fd->dentry);
+				spin_lock(&dentry_hash[i].lock);
+			}
+			node = next;
+		}
+		spin_unlock(&dentry_hash[i].lock);
+	}
+}
+
 void fuse_dentry_tree_init(void)
 {
 	int i;
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 214162f12353..1f102ecdb9ab 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -642,6 +642,8 @@ struct fuse_conn {
 	/** Current epoch for up-to-date dentries */
 	atomic_t epoch;
 
+	struct work_struct epoch_work;
+
 	struct rcu_head rcu;
 
 	/** The user id for this mount */
@@ -1261,6 +1263,8 @@ void fuse_check_timeout(struct work_struct *work);
 void fuse_dentry_tree_init(void);
 void fuse_dentry_tree_cleanup(void);
 
+void fuse_epoch_work(struct work_struct *work);
+
 /**
  * Invalidate inode attributes
  */
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 25e51efc82ee..853220413fd8 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -962,6 +962,7 @@ void fuse_conn_init(struct fuse_conn *fc, struct fuse_mount *fm,
 	refcount_set(&fc->count, 1);
 	atomic_set(&fc->dev_count, 1);
 	atomic_set(&fc->epoch, 1);
+	INIT_WORK(&fc->epoch_work, fuse_epoch_work);
 	init_waitqueue_head(&fc->blocked_waitq);
 	fuse_iqueue_init(&fc->iq, fiq_ops, fiq_priv);
 	INIT_LIST_HEAD(&fc->bg_queue);
@@ -1006,26 +1007,28 @@ static void delayed_release(struct rcu_head *p)
 
 void fuse_conn_put(struct fuse_conn *fc)
 {
-	if (refcount_dec_and_test(&fc->count)) {
-		struct fuse_iqueue *fiq = &fc->iq;
-		struct fuse_sync_bucket *bucket;
-
-		if (IS_ENABLED(CONFIG_FUSE_DAX))
-			fuse_dax_conn_free(fc);
-		if (fc->timeout.req_timeout)
-			cancel_delayed_work_sync(&fc->timeout.work);
-		if (fiq->ops->release)
-			fiq->ops->release(fiq);
-		put_pid_ns(fc->pid_ns);
-		bucket = rcu_dereference_protected(fc->curr_bucket, 1);
-		if (bucket) {
-			WARN_ON(atomic_read(&bucket->count) != 1);
-			kfree(bucket);
-		}
-		if (IS_ENABLED(CONFIG_FUSE_PASSTHROUGH))
-			fuse_backing_files_free(fc);
-		call_rcu(&fc->rcu, delayed_release);
+	struct fuse_iqueue *fiq = &fc->iq;
+	struct fuse_sync_bucket *bucket;
+
+	if (!refcount_dec_and_test(&fc->count))
+		return;
+
+	if (IS_ENABLED(CONFIG_FUSE_DAX))
+		fuse_dax_conn_free(fc);
+	if (fc->timeout.req_timeout)
+		cancel_delayed_work_sync(&fc->timeout.work);
+	cancel_work_sync(&fc->epoch_work);
+	if (fiq->ops->release)
+		fiq->ops->release(fiq);
+	put_pid_ns(fc->pid_ns);
+	bucket = rcu_dereference_protected(fc->curr_bucket, 1);
+	if (bucket) {
+		WARN_ON(atomic_read(&bucket->count) != 1);
+		kfree(bucket);
 	}
+	if (IS_ENABLED(CONFIG_FUSE_PASSTHROUGH))
+		fuse_backing_files_free(fc);
+	call_rcu(&fc->rcu, delayed_release);
 }
 EXPORT_SYMBOL_GPL(fuse_conn_put);
 

Re: [RFC PATCH v5 1/2] fuse: new work queue to periodically invalidate expired dentries

[Miklos Szeredi]

On Thu, 28 Aug 2025 at 18:30, Luis Henriques <luis@igalia.com> wrote:

> +#define HASH_BITS      12

Definitely too large.  My gut feeling gives 5, but it obviously
depends on a lot of factors.

> +               schedule_delayed_work(&dentry_tree_work,
> +                                     secs_to_jiffies(num));

secs_to_jiffues() doesn't check overflow.  Perhaps simplest fix would
be to constrain parameter to unsigned short.

> +MODULE_PARM_DESC(inval_wq,
> +                "Dentries invalidation work queue period in secs (>= 5).");

__stringify(FUSE_DENTRY_INVAL_FREQ_MIN)

> +       if (!inval_wq && RB_EMPTY_NODE(&fd->node))
> +               return;

inval_wq can change to zero, which shouldn't prevent removing from the rbtree.

> +static void fuse_dentry_tree_work(struct work_struct *work)
> +{
> +       struct fuse_dentry *fd;
> +       struct rb_node *node;
> +       int i;
> +
> +       for (i = 0; i < HASH_SIZE; i++) {
> +               spin_lock(&dentry_hash[i].lock);
> +               node = rb_first(&dentry_hash[i].tree);
> +               while (node && !need_resched()) {

Wrong place.

> +                       fd = rb_entry(node, struct fuse_dentry, node);
> +                       if (time_after64(get_jiffies_64(), fd->time)) {
> +                               rb_erase(&fd->node, &dentry_hash[i].tree);
> +                               RB_CLEAR_NODE(&fd->node);
> +                               spin_unlock(&dentry_hash[i].lock);

cond_resched() here instead.

> +                               d_invalidate(fd->dentry);

Okay, so I understand the reasoning: the validity timeout for the
dentry expired, hence it's invalid.  The problem is, this is not quite
right.  The validity timeout says "this dentry is assumed valid for
this period", it doesn't say the dentry is invalid after the timeout.

Doing d_invalidate() means we "know the dentry is invalid", which will
get it off the hash tables, giving it a "(deleted)" tag in proc
strings, etc.  This would be wrong.

What we want here is just get rid of *unused* dentries, which don't
have any reference.  Referenced ones will get revalidated with
->d_revalidate() and if one turns out to be actually invalid, it will
then be invalidated with d_invalidate(), otherwise the timeout will
just be reset.

There doesn't seem to be a function that does this, so new
infrastructure will need to be added to fs/dcache.c.  Exporting
shrink_dentry_list() and to_shrink_list() would suffice, but I wonder
if the helpers should be a little higher level.


> +void fuse_dentry_tree_cleanup(void)
> +{
> +       struct rb_node *n;
> +       int i;
> +
> +       inval_wq = 0;
> +       cancel_delayed_work_sync(&dentry_tree_work);
> +
> +       for (i = 0; i < HASH_SIZE; i++) {

If we have anything in there at module remove, then something is
horribly broken.  A WARN_ON definitely suffices here.

> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -54,6 +54,12 @@
>  /** Frequency (in jiffies) of request timeout checks, if opted into */
>  extern const unsigned long fuse_timeout_timer_freq;
>
> +/*
> + * Dentries invalidation workqueue period, in seconds.  It shall be >= 5

If we have a definition of this constant, please refer to that
definition here too.

> @@ -2045,6 +2045,10 @@ void fuse_conn_destroy(struct fuse_mount *fm)
>
>         fuse_abort_conn(fc);
>         fuse_wait_aborted(fc);
> +       /*
> +        * XXX prune dentries:
> +        * fuse_dentry_tree_prune(fc);
> +        */

No need.

Thanks,
Miklos

Re: [RFC PATCH v5 2/2] fuse: new work queue to invalidate dentries from old epochs

[Miklos Szeredi]

On Thu, 28 Aug 2025 at 18:30, Luis Henriques <luis@igalia.com> wrote:
>
> With the infrastructure introduced to periodically invalidate expired
> dentries, it is now possible to add an extra work queue to invalidate
> dentries when an epoch is incremented.  This work queue will only be
> triggered when the 'inval_wq' parameter is set.
>
> Signed-off-by: Luis Henriques <luis@igalia.com>
> ---
>  fs/fuse/dev.c    |  7 ++++---
>  fs/fuse/dir.c    | 34 ++++++++++++++++++++++++++++++++++
>  fs/fuse/fuse_i.h |  4 ++++
>  fs/fuse/inode.c  | 41 ++++++++++++++++++++++-------------------
>  4 files changed, 64 insertions(+), 22 deletions(-)
>
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index e80cd8f2c049..48c5c01c3e5b 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -2033,13 +2033,14 @@ static int fuse_notify_resend(struct fuse_conn *fc)
>
>  /*
>   * Increments the fuse connection epoch.  This will result of dentries from
> - * previous epochs to be invalidated.
> - *
> - * XXX optimization: add call to shrink_dcache_sb()?

I guess it wouldn't hurt.   Definitely simpler, so I'd opt for this.

>  void fuse_conn_put(struct fuse_conn *fc)
>  {
> -       if (refcount_dec_and_test(&fc->count)) {
> -               struct fuse_iqueue *fiq = &fc->iq;
> -               struct fuse_sync_bucket *bucket;
> -
> -               if (IS_ENABLED(CONFIG_FUSE_DAX))
> -                       fuse_dax_conn_free(fc);
> -               if (fc->timeout.req_timeout)
> -                       cancel_delayed_work_sync(&fc->timeout.work);
> -               if (fiq->ops->release)
> -                       fiq->ops->release(fiq);
> -               put_pid_ns(fc->pid_ns);
> -               bucket = rcu_dereference_protected(fc->curr_bucket, 1);
> -               if (bucket) {
> -                       WARN_ON(atomic_read(&bucket->count) != 1);
> -                       kfree(bucket);
> -               }
> -               if (IS_ENABLED(CONFIG_FUSE_PASSTHROUGH))
> -                       fuse_backing_files_free(fc);
> -               call_rcu(&fc->rcu, delayed_release);
> +       struct fuse_iqueue *fiq = &fc->iq;
> +       struct fuse_sync_bucket *bucket;
> +
> +       if (!refcount_dec_and_test(&fc->count))
> +               return;

Please don't do this.  It's difficult to see what actually changed this way.

Thanks,
Miklos

Re: [RFC PATCH v5 1/2] fuse: new work queue to periodically invalidate expired dentries

[Luis Henriques]

Hi Miklos,

On Thu, Sep 04 2025, Miklos Szeredi wrote:

> On Thu, 28 Aug 2025 at 18:30, Luis Henriques <luis@igalia.com> wrote:
>
>> +#define HASH_BITS      12
>
> Definitely too large.  My gut feeling gives 5, but it obviously
> depends on a lot of factors.

Right, to be honest I didn't spent a lot of time thinking about the
correct value for this constant.  But sure, 5 seems to be much more
reasonable.

>> +               schedule_delayed_work(&dentry_tree_work,
>> +                                     secs_to_jiffies(num));
>
> secs_to_jiffues() doesn't check overflow.  Perhaps simplest fix would
> be to constrain parameter to unsigned short.

Sounds good, thanks.

>> +MODULE_PARM_DESC(inval_wq,
>> +                "Dentries invalidation work queue period in secs (>= 5).");
>
> __stringify(FUSE_DENTRY_INVAL_FREQ_MIN)
>
>> +       if (!inval_wq && RB_EMPTY_NODE(&fd->node))
>> +               return;
>
> inval_wq can change to zero, which shouldn't prevent removing from the rbtree.

Maybe I didn't understood your comment, but isn't that what's happening
here?  If the 'fd' is in a tree, it will be removed, independently of the
'inval_wq' value.

>> +static void fuse_dentry_tree_work(struct work_struct *work)
>> +{
>> +       struct fuse_dentry *fd;
>> +       struct rb_node *node;
>> +       int i;
>> +
>> +       for (i = 0; i < HASH_SIZE; i++) {
>> +               spin_lock(&dentry_hash[i].lock);
>> +               node = rb_first(&dentry_hash[i].tree);
>> +               while (node && !need_resched()) {
>
> Wrong place.
>
>> +                       fd = rb_entry(node, struct fuse_dentry, node);
>> +                       if (time_after64(get_jiffies_64(), fd->time)) {
>> +                               rb_erase(&fd->node, &dentry_hash[i].tree);
>> +                               RB_CLEAR_NODE(&fd->node);
>> +                               spin_unlock(&dentry_hash[i].lock);
>
> cond_resched() here instead.

/me slaps himself.

>> +                               d_invalidate(fd->dentry);
>
> Okay, so I understand the reasoning: the validity timeout for the
> dentry expired, hence it's invalid.  The problem is, this is not quite
> right.  The validity timeout says "this dentry is assumed valid for
> this period", it doesn't say the dentry is invalid after the timeout.
>
> Doing d_invalidate() means we "know the dentry is invalid", which will
> get it off the hash tables, giving it a "(deleted)" tag in proc
> strings, etc.  This would be wrong.

Understood.  Thanks a lot for taking the time to explain it.  This makes
it clear that using d_invalidate() here is incorrect, of course.

> What we want here is just get rid of *unused* dentries, which don't
> have any reference.  Referenced ones will get revalidated with
> ->d_revalidate() and if one turns out to be actually invalid, it will
> then be invalidated with d_invalidate(), otherwise the timeout will
> just be reset.
>
> There doesn't seem to be a function that does this, so new
> infrastructure will need to be added to fs/dcache.c.  Exporting
> shrink_dentry_list() and to_shrink_list() would suffice, but I wonder
> if the helpers should be a little higher level.

OK, I see how the to_shrink_list() and shrink_dentry_list() pair could
easily be used here.  This would even remove the need to do the
unlock/lock in the loop.

(By the way, I considered using mutexes here instead.  Do you have any
thoughts on this?)

What I don't understand in your comment is where you suggest these helpers
could be in a higher level.  Could you elaborate on what exactly you have
in mind?

>> +void fuse_dentry_tree_cleanup(void)
>> +{
>> +       struct rb_node *n;
>> +       int i;
>> +
>> +       inval_wq = 0;
>> +       cancel_delayed_work_sync(&dentry_tree_work);
>> +
>> +       for (i = 0; i < HASH_SIZE; i++) {
>
> If we have anything in there at module remove, then something is
> horribly broken.  A WARN_ON definitely suffices here.
>
>> --- a/fs/fuse/fuse_i.h
>> +++ b/fs/fuse/fuse_i.h
>> @@ -54,6 +54,12 @@
>>  /** Frequency (in jiffies) of request timeout checks, if opted into */
>>  extern const unsigned long fuse_timeout_timer_freq;
>>
>> +/*
>> + * Dentries invalidation workqueue period, in seconds.  It shall be >= 5
>
> If we have a definition of this constant, please refer to that
> definition here too.
>
>> @@ -2045,6 +2045,10 @@ void fuse_conn_destroy(struct fuse_mount *fm)
>>
>>         fuse_abort_conn(fc);
>>         fuse_wait_aborted(fc);
>> +       /*
>> +        * XXX prune dentries:
>> +        * fuse_dentry_tree_prune(fc);
>> +        */
>
> No need.

Yeah, I wasn't totally sure this would really be required here.

And again, thanks a lot for your review, Miklos!

Cheers,
-- 
Luís

Re: [RFC PATCH v5 2/2] fuse: new work queue to invalidate dentries from old epochs

[Luis Henriques]

On Thu, Sep 04 2025, Miklos Szeredi wrote:

> On Thu, 28 Aug 2025 at 18:30, Luis Henriques <luis@igalia.com> wrote:
>>
>> With the infrastructure introduced to periodically invalidate expired
>> dentries, it is now possible to add an extra work queue to invalidate
>> dentries when an epoch is incremented.  This work queue will only be
>> triggered when the 'inval_wq' parameter is set.
>>
>> Signed-off-by: Luis Henriques <luis@igalia.com>
>> ---
>>  fs/fuse/dev.c    |  7 ++++---
>>  fs/fuse/dir.c    | 34 ++++++++++++++++++++++++++++++++++
>>  fs/fuse/fuse_i.h |  4 ++++
>>  fs/fuse/inode.c  | 41 ++++++++++++++++++++++-------------------
>>  4 files changed, 64 insertions(+), 22 deletions(-)
>>
>> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
>> index e80cd8f2c049..48c5c01c3e5b 100644
>> --- a/fs/fuse/dev.c
>> +++ b/fs/fuse/dev.c
>> @@ -2033,13 +2033,14 @@ static int fuse_notify_resend(struct fuse_conn *fc)
>>
>>  /*
>>   * Increments the fuse connection epoch.  This will result of dentries from
>> - * previous epochs to be invalidated.
>> - *
>> - * XXX optimization: add call to shrink_dcache_sb()?
>
> I guess it wouldn't hurt.   Definitely simpler, so I'd opt for this.

So, your suggesting to have the work queue simply calling this instead of
walking through the dentries?  (Or even *not* having a work queue at all?)

>>  void fuse_conn_put(struct fuse_conn *fc)
>>  {
>> -       if (refcount_dec_and_test(&fc->count)) {
>> -               struct fuse_iqueue *fiq = &fc->iq;
>> -               struct fuse_sync_bucket *bucket;
>> -
>> -               if (IS_ENABLED(CONFIG_FUSE_DAX))
>> -                       fuse_dax_conn_free(fc);
>> -               if (fc->timeout.req_timeout)
>> -                       cancel_delayed_work_sync(&fc->timeout.work);
>> -               if (fiq->ops->release)
>> -                       fiq->ops->release(fiq);
>> -               put_pid_ns(fc->pid_ns);
>> -               bucket = rcu_dereference_protected(fc->curr_bucket, 1);
>> -               if (bucket) {
>> -                       WARN_ON(atomic_read(&bucket->count) != 1);
>> -                       kfree(bucket);
>> -               }
>> -               if (IS_ENABLED(CONFIG_FUSE_PASSTHROUGH))
>> -                       fuse_backing_files_free(fc);
>> -               call_rcu(&fc->rcu, delayed_release);
>> +       struct fuse_iqueue *fiq = &fc->iq;
>> +       struct fuse_sync_bucket *bucket;
>> +
>> +       if (!refcount_dec_and_test(&fc->count))
>> +               return;
>
> Please don't do this.  It's difficult to see what actually changed this way.

Right, that didn't occur to me.  Sorry.  I'll probably add a separate
patch to re-indent this function, which makes it easier to read in my
opinion.  Not sure that's acceptable, so feel free to drop it silently :-)

Cheers,
-- 
Luís

Re: [RFC PATCH v5 2/2] fuse: new work queue to invalidate dentries from old epochs

[Miklos Szeredi]

On Thu, 4 Sept 2025 at 16:11, Luis Henriques <luis@igalia.com> wrote:
>
> On Thu, Sep 04 2025, Miklos Szeredi wrote:
>
> > On Thu, 28 Aug 2025 at 18:30, Luis Henriques <luis@igalia.com> wrote:
> >>
> >> With the infrastructure introduced to periodically invalidate expired
> >> dentries, it is now possible to add an extra work queue to invalidate
> >> dentries when an epoch is incremented.  This work queue will only be
> >> triggered when the 'inval_wq' parameter is set.
> >>
> >> Signed-off-by: Luis Henriques <luis@igalia.com>
> >> ---
> >>  fs/fuse/dev.c    |  7 ++++---
> >>  fs/fuse/dir.c    | 34 ++++++++++++++++++++++++++++++++++
> >>  fs/fuse/fuse_i.h |  4 ++++
> >>  fs/fuse/inode.c  | 41 ++++++++++++++++++++++-------------------
> >>  4 files changed, 64 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> >> index e80cd8f2c049..48c5c01c3e5b 100644
> >> --- a/fs/fuse/dev.c
> >> +++ b/fs/fuse/dev.c
> >> @@ -2033,13 +2033,14 @@ static int fuse_notify_resend(struct fuse_conn *fc)
> >>
> >>  /*
> >>   * Increments the fuse connection epoch.  This will result of dentries from
> >> - * previous epochs to be invalidated.
> >> - *
> >> - * XXX optimization: add call to shrink_dcache_sb()?
> >
> > I guess it wouldn't hurt.   Definitely simpler, so I'd opt for this.
>
> So, your suggesting to have the work queue simply calling this instead of
> walking through the dentries?  (Or even *not* having a work queue at all?)

I think doing in in a work queue is useful, since walking the tree
might take a significant amount of time.

Not having to do the walk manually is definitely a simplification.
It might throw out dentries that got looked up since the last epoch,
but it's probably not a big loss in terms of performance.

Thanks,
Miklos

Re: [RFC PATCH v5 1/2] fuse: new work queue to periodically invalidate expired dentries

[Miklos Szeredi]

On Thu, 4 Sept 2025 at 16:00, Luis Henriques <luis@igalia.com> wrote:
>
> Hi Miklos,
>
> On Thu, Sep 04 2025, Miklos Szeredi wrote:
>
> > On Thu, 28 Aug 2025 at 18:30, Luis Henriques <luis@igalia.com> wrote:

> >> +       if (!inval_wq && RB_EMPTY_NODE(&fd->node))
> >> +               return;
> >
> > inval_wq can change to zero, which shouldn't prevent removing from the rbtree.
>
> Maybe I didn't understood your comment, but isn't that what's happening
> here?  If the 'fd' is in a tree, it will be removed, independently of the
> 'inval_wq' value.

I somehow thought it was || not &&.

But I still don't see the point.  The only caller already checked
RB_EMPTY_NODE, so that is false. No race possible since it's called
form the destruction of the dentry, and so this expression is
guaranteed to evaluate to false.

> (By the way, I considered using mutexes here instead.  Do you have any
> thoughts on this?)

Use mutex where protected code might sleep, spin lock otherwise.

>
> What I don't understand in your comment is where you suggest these helpers
> could be in a higher level.  Could you elaborate on what exactly you have
> in mind?

E.g.

void d_dispose_if_unused(struct dentry *dentry, struct list_head *dispose)
{
        spin_lock(&dentry->d_lock);
        if (!dentry->d_lockref.count)
                to_shrink_list(dentry, dispose);
        spin_unlock(&dentry->d_lock);
}

Which is in fact taken from d_prune_aliases(), which could be modified
to use this helper.

Thanks,
Miklos

Re: [RFC PATCH v5 2/2] fuse: new work queue to invalidate dentries from old epochs

[Luis Henriques]

On Thu, Sep 04 2025, Miklos Szeredi wrote:

> On Thu, 4 Sept 2025 at 16:11, Luis Henriques <luis@igalia.com> wrote:
>>
>> On Thu, Sep 04 2025, Miklos Szeredi wrote:
>>
>> > On Thu, 28 Aug 2025 at 18:30, Luis Henriques <luis@igalia.com> wrote:
>> >>
>> >> With the infrastructure introduced to periodically invalidate expired
>> >> dentries, it is now possible to add an extra work queue to invalidate
>> >> dentries when an epoch is incremented.  This work queue will only be
>> >> triggered when the 'inval_wq' parameter is set.
>> >>
>> >> Signed-off-by: Luis Henriques <luis@igalia.com>
>> >> ---
>> >>  fs/fuse/dev.c    |  7 ++++---
>> >>  fs/fuse/dir.c    | 34 ++++++++++++++++++++++++++++++++++
>> >>  fs/fuse/fuse_i.h |  4 ++++
>> >>  fs/fuse/inode.c  | 41 ++++++++++++++++++++++-------------------
>> >>  4 files changed, 64 insertions(+), 22 deletions(-)
>> >>
>> >> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
>> >> index e80cd8f2c049..48c5c01c3e5b 100644
>> >> --- a/fs/fuse/dev.c
>> >> +++ b/fs/fuse/dev.c
>> >> @@ -2033,13 +2033,14 @@ static int fuse_notify_resend(struct fuse_conn *fc)
>> >>
>> >>  /*
>> >>   * Increments the fuse connection epoch.  This will result of dentries from
>> >> - * previous epochs to be invalidated.
>> >> - *
>> >> - * XXX optimization: add call to shrink_dcache_sb()?
>> >
>> > I guess it wouldn't hurt.   Definitely simpler, so I'd opt for this.
>>
>> So, your suggesting to have the work queue simply calling this instead of
>> walking through the dentries?  (Or even *not* having a work queue at all?)
>
> I think doing in in a work queue is useful, since walking the tree
> might take a significant amount of time.
>
> Not having to do the walk manually is definitely a simplification.
> It might throw out dentries that got looked up since the last epoch,
> but it's probably not a big loss in terms of performance.

OK, so that definitely makes things simpler for v6.  Thanks!

Cheers,
-- 
Luís

Re: [RFC PATCH v5 1/2] fuse: new work queue to periodically invalidate expired dentries

[Luis Henriques]

On Thu, Sep 04 2025, Miklos Szeredi wrote:

> On Thu, 4 Sept 2025 at 16:00, Luis Henriques <luis@igalia.com> wrote:
>>
>> Hi Miklos,
>>
>> On Thu, Sep 04 2025, Miklos Szeredi wrote:
>>
>> > On Thu, 28 Aug 2025 at 18:30, Luis Henriques <luis@igalia.com> wrote:
>
>> >> +       if (!inval_wq && RB_EMPTY_NODE(&fd->node))
>> >> +               return;
>> >
>> > inval_wq can change to zero, which shouldn't prevent removing from the rbtree.
>>
>> Maybe I didn't understood your comment, but isn't that what's happening
>> here?  If the 'fd' is in a tree, it will be removed, independently of the
>> 'inval_wq' value.
>
> I somehow thought it was || not &&.
>
> But I still don't see the point.  The only caller already checked
> RB_EMPTY_NODE, so that is false. No race possible since it's called
> form the destruction of the dentry, and so this expression is
> guaranteed to evaluate to false.

Fair enough.  I'll drop that code.

>> (By the way, I considered using mutexes here instead.  Do you have any
>> thoughts on this?)
>
> Use mutex where protected code might sleep, spin lock otherwise.
>
>>
>> What I don't understand in your comment is where you suggest these helpers
>> could be in a higher level.  Could you elaborate on what exactly you have
>> in mind?
>
> E.g.
>
> void d_dispose_if_unused(struct dentry *dentry, struct list_head *dispose)
> {
>         spin_lock(&dentry->d_lock);
>         if (!dentry->d_lockref.count)
>                 to_shrink_list(dentry, dispose);
>         spin_unlock(&dentry->d_lock);
> }
>
> Which is in fact taken from d_prune_aliases(), which could be modified
> to use this helper.

Oh! OK, got it.  Thanks, I'll start working on v6 and try to include all
your suggestions.

Cheers,
-- 
Luís