Re: [PATCH] vfs: don't mod negative dentry count when on shrinker list

[Brian Foster <bfoster@redhat.com>]

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@redhat.com>
> 
> 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
>