Re: [PATCH v3] fuse: new workqueue to periodically invalidate expired dentries

[Luis Henriques <luis@igalia.com>]

On Wed, Jul 02 2025, Miklos Szeredi wrote:

> On Tue, 20 May 2025 at 17:42, Luis Henriques <luis@igalia.com> wrote:
>>
>> This patch adds a new module parameter 'inval_wq' which is used to start a
>> workqueue to periodically invalidate expired dentries.  The value of this
>> new parameter is the period, in seconds, of the workqueue.  When it is set,
>> every new dentry will be added to an rbtree, sorted by the dentry's expiry
>> time.
>>
>> When the workqueue is executed, it will check the dentries in this tree and
>> invalidate them if:
>>
>>   - The dentry has timed-out, or if
>>   - The connection epoch has been incremented.
>
> I wonder, why not make the whole infrastructure global?  There's no
> reason to have separate rb-trees and workqueues for each fuse
> instance.

Hmm... true.  My initial approach was to use a mount parameter to enabled
it for each connection.  When you suggested replacing that by a module
parameter, I should have done that too.

> Contention on the lock would be worse, but it's bad as it
> is, so need some solution, e.g. hashed lock, which is better done with
> a single instance.

Right, I'll think how to fix it (or at least reduce contention).

>> The workqueue will run for, at most, 5 seconds each time.  It will
>> reschedule itself if the dentries tree isn't empty.
>
> It should check need_resched() instead.

OK.

>> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
>> index 1fb0b15a6088..257ca2b36b94 100644
>> --- a/fs/fuse/dir.c
>> +++ b/fs/fuse/dir.c
>> @@ -34,33 +34,153 @@ 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;
>> +       struct rcu_head rcu;
>> +       struct rb_node node;
>> +       struct dentry *dentry;
>> +};
>> +
>
> You lost the union with rcu_head.   Any other field is okay, none of
> them matter in rcu protected code.  E.g.
>
> struct fuse_dentry {
>         u64 time;
>         union {
>                 struct rcu_head rcu;
>                 struct rb_node node;
>         };
>         struct dentry *dentry;
> };

Oops.  I'll fix that.

Thanks a lot for your feedback, Miklos.  Much appreciated.  I'll re-work
this patch and send a new revision shortly.

Cheers,
-- 
Luís