diff mbox series

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

Message ID 20240703121301.247680-1-bfoster@redhat.com (mailing list archive)
State New
Headers show
Series [v2] vfs: don't mod negative dentry count when on shrinker list | expand

Commit Message

Brian Foster July 3, 2024, 12:13 p.m. UTC
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.

Fixes: af0c9af1b3f6 ("fs/dcache: Track & report number of negative dentries")
Signed-off-by: Brian Foster <bfoster@redhat.com>
Acked-by: Ian Kent <ikent@redhat.com>
---

Hi Christian,

I see you already picked up v1. Josef had asked for some comment updates
so I'm posting v2 with that, but TBH I'm not sure how useful this all is
once one groks the flags. I have no strong opinion on it. I also added a
Fixes: tag for the patch that added the counter.

In short, feel free to grab this one, ignore this and stick with v1, or
maybe just pull in the Fixes: tag if you agree with it. Thanks.

Brian

v2:
- Update comments (Josef).
- Add Fixes: tag, cc Waiman.
v1: https://lore.kernel.org/linux-fsdevel/20240702170757.232130-1-bfoster@redhat.com/

 fs/dcache.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Comments

Christian Brauner July 3, 2024, 1:50 p.m. UTC | #1
> Hi Christian,
> 
> I see you already picked up v1. Josef had asked for some comment updates
> so I'm posting v2 with that, but TBH I'm not sure how useful this all is
> once one groks the flags. I have no strong opinion on it. I also added a
> Fixes: tag for the patch that added the counter.
> 
> In short, feel free to grab this one, ignore this and stick with v1, or
> maybe just pull in the Fixes: tag if you agree with it. Thanks.

Meh, I'll just take your v2. If Josef thinks the comments are useful and
you don't mind adding them it seems like the an easy thing to do. Thanks
for resending!
Christian Brauner July 3, 2024, 2:03 p.m. UTC | #2
On Wed, 03 Jul 2024 08:13:01 -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.
> 
> [...]

Applied to the vfs.fixes branch of the vfs/vfs.git tree.
Patches in the vfs.fixes branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes

[1/1] vfs: don't mod negative dentry count when on shrinker list
      https://git.kernel.org/vfs/vfs/c/dd445e65afe4
Josef Bacik July 3, 2024, 2:43 p.m. UTC | #3
On Wed, Jul 03, 2024 at 08:13:01AM -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.
> 
> Fixes: af0c9af1b3f6 ("fs/dcache: Track & report number of negative dentries")
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Acked-by: Ian Kent <ikent@redhat.com>
> ---
> 
> Hi Christian,
> 
> I see you already picked up v1. Josef had asked for some comment updates
> so I'm posting v2 with that, but TBH I'm not sure how useful this all is
> once one groks the flags. I have no strong opinion on it. I also added a
> Fixes: tag for the patch that added the counter.
> 
> In short, feel free to grab this one, ignore this and stick with v1, or
> maybe just pull in the Fixes: tag if you agree with it. Thanks.
> 
> Brian
> 
> v2:
> - Update comments (Josef).
> - Add Fixes: tag, cc Waiman.
> v1: https://lore.kernel.org/linux-fsdevel/20240702170757.232130-1-bfoster@redhat.com/

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks!

Josef
Waiman Long July 3, 2024, 5:02 p.m. UTC | #4
On 7/3/24 08:13, 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.
>
> Fixes: af0c9af1b3f6 ("fs/dcache: Track & report number of negative dentries")
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Acked-by: Ian Kent <ikent@redhat.com>
> ---
>
> Hi Christian,
>
> I see you already picked up v1. Josef had asked for some comment updates
> so I'm posting v2 with that, but TBH I'm not sure how useful this all is
> once one groks the flags. I have no strong opinion on it. I also added a
> Fixes: tag for the patch that added the counter.
>
> In short, feel free to grab this one, ignore this and stick with v1, or
> maybe just pull in the Fixes: tag if you agree with it. Thanks.
>
> Brian
>
> v2:
> - Update comments (Josef).
> - Add Fixes: tag, cc Waiman.
> v1: https://lore.kernel.org/linux-fsdevel/20240702170757.232130-1-bfoster@redhat.com/
>
>   fs/dcache.c | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 407095188f83..66515fbc9dd7 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -355,7 +355,11 @@ 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)
> +	/*
> +	 * The negative counter only tracks dentries on the LRU. Don't inc if
> +	 * d_lru is on another list.
> +	 */
> +	if ((flags & (DCACHE_LRU_LIST|DCACHE_SHRINK_LIST)) == DCACHE_LRU_LIST)
>   		this_cpu_inc(nr_dentry_negative);
>   }
>   
> @@ -1844,9 +1848,11 @@ static void __d_instantiate(struct dentry *dentry, struct inode *inode)
>   
>   	spin_lock(&dentry->d_lock);
>   	/*
> -	 * Decrement negative dentry count if it was in the LRU list.
> +	 * The negative counter only tracks dentries on the LRU. Don't dec if
> +	 * d_lru is on another 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);

Thanks for fixing it.

Reviewed-by: Waiman Long <longman@redhat.com>
Eric Sandeen July 10, 2024, 3:37 p.m. UTC | #5
On 7/3/24 7:13 AM, 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.
> 
> Fixes: af0c9af1b3f6 ("fs/dcache: Track & report number of negative dentries")
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Acked-by: Ian Kent <ikent@redhat.com>
> ---
> 
> Hi Christian,
> 
> I see you already picked up v1. Josef had asked for some comment updates
> so I'm posting v2 with that, but TBH I'm not sure how useful this all is
> once one groks the flags. I have no strong opinion on it. I also added a
> Fixes: tag for the patch that added the counter.
> 
> In short, feel free to grab this one, ignore this and stick with v1, or
> maybe just pull in the Fixes: tag if you agree with it. Thanks.

This might also want a:

Cc: stable@vger.kernel.org # v5.0+

just FWIW. Given the mild panic On The Internet about negative dentries, an
artificial and permanent increase in the counter is probably adding to the
worries.

Thanks,
-Eric
diff mbox series

Patch

diff --git a/fs/dcache.c b/fs/dcache.c
index 407095188f83..66515fbc9dd7 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -355,7 +355,11 @@  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)
+	/*
+	 * The negative counter only tracks dentries on the LRU. Don't inc if
+	 * d_lru is on another list.
+	 */
+	if ((flags & (DCACHE_LRU_LIST|DCACHE_SHRINK_LIST)) == DCACHE_LRU_LIST)
 		this_cpu_inc(nr_dentry_negative);
 }
 
@@ -1844,9 +1848,11 @@  static void __d_instantiate(struct dentry *dentry, struct inode *inode)
 
 	spin_lock(&dentry->d_lock);
 	/*
-	 * Decrement negative dentry count if it was in the LRU list.
+	 * The negative counter only tracks dentries on the LRU. Don't dec if
+	 * d_lru is on another 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);