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

[Luis Henriques <luis@igalia.com>]

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