Re: [RFC PATCH] vfs: limit directory child dentry retention

[Ian Kent <raven@themaw.net>]

On 31/3/26 17:39, Christian Brauner wrote:
> On Tue, Mar 31, 2026 at 09:29:09AM +0800, Ian Kent wrote:
>> If there's a very large number of children present in a directory dentry
>> then the benifit from retaining stale child dentries for re-use can
>> become ineffective. Even hashed lookup can become ineffective as hash
>> chains grow, time taken to umount a file system can increase a lot, as
>> well as child dentry traversals resulting in lock held too long log
>> messages.
> Fwiw, there's also e6957c99dca5 ("vfs: Add a sysctl for automated deletion of dentry")

I'm pretty sure I saw that earlier on but had forgotten about it when I

reviewed the bug this time around. It is essentially 681ce8623567 ("vfs:

Delete the associated dentry when deleting a file") with opt-in of course.


>
> This patch introduces the concept conditionally, where the associated
> dentry is deleted only when the user explicitly opts for it during file
> removal. A new sysctl fs.automated_deletion_of_dentry is added for this
> purpose. Its default value is set to 0.

I meant to update Documentation/admin-guide/sysctl/fs.rst to also say that

setting dir-stale-max to 0 disables it. I also get the impression you might

feel better about this if the default was 0 as well.


The thing that I don't much like with the d_delete() approach is that it

fails to cater for files that have been closed and are otherwise unused

who's dentries make there way to the LRU eventually resulting in the bad

behaviour being discussed.


>
> I have no massive objections to your approach. It feels a bit hacky tbh
> as it seems to degrade performance for new workloads in favor old
> workloads. The LRU should sort this out though.

My aim was to improve performance so I'm a bit puzzled by the comment.


The problem is the sheer number of dentry objects and the consequences

of that. Hash table chains growing will affect performance, umounting

the mount will take ages, and there are cases of child dentry traversals

in the VFS. Once you get a large number of stale dentries that necessarily

need to stay linked into the structures to get the benefit of caching your

exposed to this problem.


The LRU mechanism is so far unable to cope with this.


Ian

>
>> But when a directory dentry has a very large number of children the
>> parent dentry reference count is dominated by the contribution of its
>> children. So it makes sense to not retain dentries if the parent
>> reference count is large.
>>
>> Setting some large high water mark (eg. 500000) over which dentries
>> are discarded instead of retained on final dput() would help a lot
>> by preventing dentry caching contributing to the problem.
>>
>> Signed-off-by: Ian Kent <raven@themaw.net>
>> ---
>>   Documentation/admin-guide/sysctl/fs.rst |  7 +++++++
>>   fs/dcache.c                             | 28 +++++++++++++++++++++++++
>>   2 files changed, 35 insertions(+)
>>
>> diff --git a/Documentation/admin-guide/sysctl/fs.rst b/Documentation/admin-guide/sysctl/fs.rst
>> index 9b7f65c3efd8..7649254f2d0d 100644
>> --- a/Documentation/admin-guide/sysctl/fs.rst
>> +++ b/Documentation/admin-guide/sysctl/fs.rst
>> @@ -75,6 +75,13 @@ negative dentries which do not map to any files. Instead,
>>   they help speeding up rejection of non-existing files provided
>>   by the users.
>>   
>> +dir-stale-max
>> +-------------
>> +
>> +Used to limit the number of stale child dentries retained in a
>> +directory before the benifit of caching the dentry is negated by
>> +the cost of traversing hash buckets during lookups or enumerating
>> +the directory children. Initially set to 500000.
>>   
>>   file-max & file-nr
>>   ------------------
>> diff --git a/fs/dcache.c b/fs/dcache.c
>> index 7ba1801d8132..298b4c3b1493 100644
>> --- a/fs/dcache.c
>> +++ b/fs/dcache.c
>> @@ -86,6 +86,14 @@ __cacheline_aligned_in_smp DEFINE_SEQLOCK(rename_lock);
>>   
>>   EXPORT_SYMBOL(rename_lock);
>>   
>> +static long dsm_zero = 0;
>> +static long dsm_max = ULONG_MAX/2;
>> +
>> +/* Highwater mark for number of stale entries in a directory (loosely
>> + * measured by parent dentry reference count).
>> + */
>> +static unsigned long dir_stale_max __read_mostly = 500000;
>> +
>>   static struct kmem_cache *__dentry_cache __ro_after_init;
>>   #define dentry_cache runtime_const_ptr(__dentry_cache)
>>   
>> @@ -216,6 +224,15 @@ static const struct ctl_table fs_dcache_sysctls[] = {
>>   		.extra1		= SYSCTL_ZERO,
>>   		.extra2		= SYSCTL_ONE,
>>   	},
>> +	{
>> +		.procname	= "dir-stale-max",
>> +		.data		= &dir_stale_max,
>> +		.maxlen		= sizeof(dir_stale_max),
>> +		.mode		= 0644,
>> +		.proc_handler	= proc_doulongvec_minmax,
>> +		.extra1		= &dsm_zero,
>> +		.extra2		= &dsm_max,
>> +	},
>>   };
>>   
>>   static const struct ctl_table vm_dcache_sysctls[] = {
>> @@ -768,6 +785,17 @@ static inline bool retain_dentry(struct dentry *dentry, bool locked)
>>   	if (unlikely(d_flags & DCACHE_DONTCACHE))
>>   		return false;
>>   
>> +	if (dir_stale_max) {
>> +		unsigned long p_count;
>> +
>> +		// If the parent reference count is higher than some large value
>> +		// its dominated by the contribution of its children so there's
>> +		// no benefit caching the dentry over re-allocating it.
>> +		p_count = READ_ONCE(dentry->d_parent->d_lockref.count);
>> +		if (unlikely(p_count > dir_stale_max))
>> +			return false;
>> +	}
>> +
>>   	// At this point it looks like we ought to keep it.  We also might
>>   	// need to do something - put it on LRU if it wasn't there already
>>   	// and mark it referenced if it was on LRU, but not marked yet.
>> -- 
>> 2.53.0
>>