Hi Miklos,
On Thu, Sep 04 2025, Miklos Szeredi wrote:
> On Thu, 28 Aug 2025 at 18:30, Luis Henriques <luis@xxxxxxxxxx> 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
