On Wed, Jul 03, 2024 at 09:48:52AM GMT, Ian Kent wrote: > On 3/7/24 01:07, Brian Foster wrote: > > The nr_dentry_negative counter is intended to only account negative > > dentries that are present on the superblock LRU. Therefore, the LRU > > add, remove and isolate helpers modify the counter based on whether > > the dentry is negative, but the shrinker list related helpers do not > > modify the counter, and the paths that change a dentry between > > positive and negative only do so if DCACHE_LRU_LIST is set. > > > > The problem with this is that a dentry on a shrinker list still has > > DCACHE_LRU_LIST set to indicate ->d_lru is in use. The additional > > DCACHE_SHRINK_LIST flag denotes whether the dentry is on LRU or a > > shrink related list. Therefore if a relevant operation (i.e. unlink) > > occurs while a dentry is present on a shrinker list, and the > > associated codepath only checks for DCACHE_LRU_LIST, then it is > > technically possible to modify the negative dentry count for a > > dentry that is off the LRU. Since the shrinker list related helpers > > do not modify the negative dentry count (because non-LRU dentries > > should not be included in the count) when the dentry is ultimately > > removed from the shrinker list, this can cause the negative dentry > > count to become permanently inaccurate. > > > > This problem can be reproduced via a heavy file create/unlink vs. > > drop_caches workload. On an 80xcpu system, I start 80 tasks each > > running a 1k file create/delete loop, and one task spinning on > > drop_caches. After 10 minutes or so of runtime, the idle/clean cache > > negative dentry count increases from somewhere in the range of 5-10 > > entries to several hundred (and increasingly grows beyond > > nr_dentry_unused). > > > > Tweak the logic in the paths that turn a dentry negative or positive > > to filter out the case where the dentry is present on a shrink > > related list. This allows the above workload to maintain an accurate > > negative dentry count. > > > > Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx> > > --- > > fs/dcache.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/fs/dcache.c b/fs/dcache.c > > index 407095188f83..5305b95b3030 100644 > > --- a/fs/dcache.c > > +++ b/fs/dcache.c > > @@ -355,7 +355,7 @@ static inline void __d_clear_type_and_inode(struct dentry *dentry) > > flags &= ~DCACHE_ENTRY_TYPE; > > WRITE_ONCE(dentry->d_flags, flags); > > dentry->d_inode = NULL; > > - if (flags & DCACHE_LRU_LIST) > > + if ((flags & (DCACHE_LRU_LIST|DCACHE_SHRINK_LIST)) == DCACHE_LRU_LIST) > > this_cpu_inc(nr_dentry_negative); > > } > > @@ -1846,7 +1846,8 @@ static void __d_instantiate(struct dentry *dentry, struct inode *inode) > > /* > > * Decrement negative dentry count if it was in the LRU list. > > */ > > - if (dentry->d_flags & DCACHE_LRU_LIST) > > + if ((dentry->d_flags & > > + (DCACHE_LRU_LIST|DCACHE_SHRINK_LIST)) == DCACHE_LRU_LIST) > > this_cpu_dec(nr_dentry_negative); > > hlist_add_head(&dentry->d_u.d_alias, &inode->i_dentry); > > raw_write_seqcount_begin(&dentry->d_seq); > > > Acked-by: Ian Kent <ikent@xxxxxxxxxx> > > > Christian, just thought I'd call your attention to this since it's a bit > urgent for us to get reviews > > and hopefully merged into the VFS tree. I'm about to pick it up.
