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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87bjq2k0tm.fsf@igalia.com>
Date: Wed, 02 Jul 2025 17:35:33 +0100
From: Luis Henriques <luis@...lia.com>
To: Miklos Szeredi <miklos@...redi.hu>
Cc: Bernd Schubert <bernd@...ernd.com>,  Laura Promberger
 <laura.promberger@...n.ch>,  Dave Chinner <david@...morbit.com>,  Matt
 Harvey <mharvey@...ptrading.com>,  linux-fsdevel@...r.kernel.org,
  linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] fuse: new workqueue to periodically invalidate
 expired dentries

On Wed, Jul 02 2025, Miklos Szeredi wrote:

> On Tue, 20 May 2025 at 17:42, Luis Henriques <luis@...lia.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