On Tue, Jul 02, 2024 at 01:31:06PM -0400, Josef Bacik wrote: > On Tue, Jul 02, 2024 at 01:07:57PM -0400, 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> > > This is sort of a subtle interaction, it took me a bit to piece it together, > could you add a comment to the sections indicating the purpose of the extra > check? Thanks, > Sure.. I briefly considered whether something like a d_is_lru() or some such helper might be more useful and/or descriptive, but I didn't think too hard on it. That would also leave one place to add a comment instead of two, but that's not a big deal either. I'll wait a bit for any additional feedback and send a v2 with that tweak. Thanks for the review. Brian > Josef >
