Message-ID: <87ldp5e925.fsf@igalia.com>
Date: Thu, 03 Jul 2025 13:46:26 +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, Luis Henriques wrote:

> 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.

While starting working on this, I realised there's an additional
complication with this approach.  Having a dentries tree per connection
allows the workqueue to stop walking through the tree once we find a
non-expired dentry: it has a valid timestamp *and* it's epoch is equal to
the connection epoch.

Moving to a global tree, I'll need to _always_ walk through all the
dentries, because the epoch for a specific connection may have been
incremented.

So, I can see two options to solve this:

1) keep the design as is (i.e. a tree/workqueue per connection), or

2) add another flag indicating whether there has been an epoch increment
   in any connection, and only keep walking through all the dentries in
   that case.

A third option could be to change dentries timestamps and re-order the
tree when there's an epoch increment.  But this would probably be messy,
and very hacky I believe.

Any thoughts?

Cheers,
-- 
Luís


>> 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

Powered by blists - more mailing lists