fs: push nr_cached_objects memcg gating into individual filesystems

Commit 0baad6f9b997 ("fs/super: skip non-memcg-aware nr_cached_objects in memcg slab shrink") added a check in fs/super.c that skips the ->nr_cached_objects() hook whenever the shrinker is invoked for a non-root memcg, because none of the current implementations (btrfs, xfs, shmem huge) honour sc->memcg.

That policy is really a filesystem-owned property: fs/super.c should not encode the assumption that these hooks are never memcg-aware, since a future implementation might legitimately filter by sc->memcg. Move the check into btrfs_nr_cached_objects(), xfs_fs_nr_cached_objects() and shmem_unused_huge_count() so each filesystem can lift the restriction independently once its underlying counters/scans become memcg-aware, without needing a coordinated change to fs/super.c.

Behaviour is unchanged: calls into these hooks from shrink_slab_memcg() still early-return 0 for non-root memcg contexts, keeping the shrinker bit clearable in each memcg's bitmap; the global (kswapd or root direct reclaim) path still drives them as before.

Signed-off-by Usama Arif <[email protected]>

---
  fs/btrfs/super.c   | 11 +++++++++++
  fs/super.c         | 19 ++-----------------
  fs/xfs/xfs_super.c | 12 ++++++++++++
  mm/shmem.c         | 11 +++++++++++
  4 files changed, 36 insertions(+), 17 deletions(-)

Commit 0baad6f9b997 ("fs/super: skip non-memcg-aware nr_cached_objects in memcg slab shrink") added a check in fs/super.c that skips the ->nr_cached_objects() hook whenever the shrinker is invoked for a non-root memcg, because none of the current implementations (btrfs, xfs, shmem huge) honour sc->memcg.

That policy is really a filesystem-owned property: fs/super.c should not encode the assumption that these hooks are never memcg-aware, since a future implementation might legitimately filter by sc->memcg. Move the check into btrfs_nr_cached_objects(), xfs_fs_nr_cached_objects() and shmem_unused_huge_count() so each filesystem can lift the restriction independently once its underlying counters/scans become memcg-aware, without needing a coordinated change to fs/super.c.

Behaviour is unchanged: calls into these hooks from shrink_slab_memcg() still early-return 0 for non-root memcg contexts, keeping the shrinker bit clearable in each memcg's bitmap; the global (kswapd or root direct reclaim) path still drives them as before.

Signed-off-by Usama Arif <[email protected]>

---
 fs/btrfs/super.c   | 11 +++++++++++
 fs/super.c         | 19 ++-----------------
 fs/xfs/xfs_super.c | 12 ++++++++++++
 mm/shmem.c         | 11 +++++++++++
 4 files changed, 36 insertions(+), 17 deletions(-)

fs/btrfs/super.c +11 -0

index a7d804219bec..16c24bccc897 100644
@@ -22,6 +22,7 @@
 #include <linux/namei.h>
 #include <linux/miscdevice.h>
 #include <linux/magic.h>
+#include <linux/memcontrol.h>
 #include <linux/slab.h>
 #include <linux/ratelimit.h>
 #include <linux/crc32c.h>

@@ -2434,6 +2435,16 @@ static long btrfs_nr_cached_objects(struct super_block *sb, struct shrink_contro
 	struct btrfs_fs_info *fs_info = btrfs_sb(sb);
 	const s64 nr = percpu_counter_read_positive(&fs_info->evictable_extent_maps);
 
+	/*
+	 * The evictable extent map counter is filesystem-global and does not
+	 * honour sc->memcg, so it is only meaningful on the global (kswapd or
+	 * root direct reclaim) shrink path. Skip the per-memcg iterations of
+	 * shrink_slab_memcg() to avoid queueing duplicate global work and
+	 * pinning the shrinker bit in every memcg's bitmap.
+	 */
+	if (sc->memcg && !mem_cgroup_is_root(sc->memcg))
+		return 0;
+
 	trace_btrfs_extent_map_shrinker_count(fs_info, nr);
 
 	return nr;

fs/super.c +2 -17

index d2d04a6f4f84..a8fd61136aaf 100644
@@ -24,7 +24,6 @@
 #include <linux/export.h>
 #include <linux/slab.h>
 #include <linux/blkdev.h>
-#include <linux/memcontrol.h>
 #include <linux/mount.h>
 #include <linux/security.h>
 #include <linux/writeback.h>		/* for the emergency remount stuff */

@@ -170,19 +169,6 @@ static void super_wake(struct super_block *sb, unsigned int flag)
 	wake_up_var(&sb->s_flags);
 }
 
-/*
- * The s_op->nr_cached_objects hooks (used for example by btrfs and xfs)
- * operate on filesystem-global state and ignore sc->memcg. Driving them
- * from per-memcg shrink_slab_memcg() invocations only burns CPU walking
- * per-cpu counters and queueing duplicate work: the actual reclaim happens on
- * the global path (kswapd or root direct reclaim) regardless. Restrict them
- * to that path.
- */
-static inline bool super_fs_objects_eligible(struct shrink_control *sc)
-{
-	return !sc->memcg || mem_cgroup_is_root(sc->memcg);
-}
-
 /*
  * One thing we have to be careful of with a per-sb shrinker is that we don't
  * drop the last active reference to the superblock from within the shrinker.

@@ -212,7 +198,7 @@ static unsigned long super_cache_scan(struct shrinker *shrink,
 	if (!super_trylock_shared(sb))
 		return SHRINK_STOP;
 
-	if (sb->s_op->nr_cached_objects && super_fs_objects_eligible(sc))
+	if (sb->s_op->nr_cached_objects)
 		fs_objects = sb->s_op->nr_cached_objects(sb, sc);
 
 	inodes = list_lru_shrink_count(&sb->s_inode_lru, sc);

@@ -273,8 +259,7 @@ static unsigned long super_cache_count(struct shrinker *shrink,
 		return 0;
 	smp_rmb();
 
-	if (sb->s_op && sb->s_op->nr_cached_objects &&
-	    super_fs_objects_eligible(sc))
+	if (sb->s_op && sb->s_op->nr_cached_objects)
 		total_objects = sb->s_op->nr_cached_objects(sb, sc);
 
 	total_objects += list_lru_shrink_count(&sb->s_dentry_lru, sc);

fs/xfs/xfs_super.c +12 -0

index eac7f9503805..f0d8251fad3b 100644
@@ -54,6 +54,7 @@
 #include "scrub/rcbag_btree.h"
 
 #include <linux/magic.h>
+#include <linux/memcontrol.h>
 #include <linux/fs_context.h>
 #include <linux/fs_parser.h>
 #include <linux/fserror.h>

@@ -1242,6 +1243,17 @@ xfs_fs_nr_cached_objects(
 	/* Paranoia: catch incorrect calls during mount setup or teardown */
 	if (WARN_ON_ONCE(!sb->s_fs_info))
 		return 0;
+
+	/*
+	 * The reclaimable inode count is filesystem-global and does not honour
+	 * sc->memcg, so it is only meaningful on the global (kswapd or root
+	 * direct reclaim) shrink path. Skip the per-memcg iterations of
+	 * shrink_slab_memcg() to avoid queueing duplicate global work and
+	 * pinning the shrinker bit in every memcg's bitmap.
+	 */
+	if (sc->memcg && !mem_cgroup_is_root(sc->memcg))
+		return 0;
+
 	return xfs_reclaim_inodes_count(XFS_M(sb));
 }
 

mm/shmem.c +11 -0

index 5789a0f5a346..3ba812e4f5ba 100644
@@ -846,6 +846,17 @@ static long shmem_unused_huge_count(struct super_block *sb,
 		struct shrink_control *sc)
 {
 	struct shmem_sb_info *sbinfo = SHMEM_SB(sb);
+
+	/*
+	 * The per-superblock shrinklist is filesystem-global and does not
+	 * honour sc->memcg, so it is only meaningful on the global (kswapd or
+	 * root direct reclaim) shrink path. Skip the per-memcg iterations of
+	 * shrink_slab_memcg() to avoid queueing duplicate global work and
+	 * pinning the shrinker bit in every memcg's bitmap.
+	 */
+	if (sc->memcg && !mem_cgroup_is_root(sc->memcg))
+		return 0;
+
 	return READ_ONCE(sbinfo->shrinklist_len);
 }
 #else /* !CONFIG_TRANSPARENT_HUGEPAGE */

Signature · 1 line

2.53.0-Meta

Commit 0baad6f9b997 ("fs/super: skip non-memcg-aware nr_cached_objects in memcg slab shrink") added a check in fs/super.c that skips the ->nr_cached_objects() hook whenever the shrinker is invoked for a non-root memcg, because none of the current implementations (btrfs, xfs, shmem huge) honour sc->memcg.

That policy is really a filesystem-owned property: fs/super.c should not encode the assumption that these hooks are never memcg-aware, since a future implementation might legitimately filter by sc->memcg. Move the check into btrfs_nr_cached_objects(), xfs_fs_nr_cached_objects() and shmem_unused_huge_count() so each filesystem can lift the restriction independently once its underlying counters/scans become memcg-aware, without needing a coordinated change to fs/super.c.

Behaviour is unchanged: calls into these hooks from shrink_slab_memcg() still early-return 0 for non-root memcg contexts, keeping the shrinker bit clearable in each memcg's bitmap; the global (kswapd or root direct reclaim) path still drives them as before.

Signed-off-by Usama Arif <[email protected]>

Commit 0baad6f9b997 ("fs/super: skip non-memcg-aware nr_cached_objects in memcg slab shrink") added a check in fs/super.c that skips the ->nr_cached_objects() hook whenever the shrinker is invoked for a non-root memcg, because none of the current implementations (btrfs, xfs, shmem huge) honour sc->memcg.

That policy is really a filesystem-owned property: fs/super.c should not encode the assumption that these hooks are never memcg-aware, since a future implementation might legitimately filter by sc->memcg. Move the check into btrfs_nr_cached_objects(), xfs_fs_nr_cached_objects() and shmem_unused_huge_count() so each filesystem can lift the restriction independently once its underlying counters/scans become memcg-aware, without needing a coordinated change to fs/super.c.

Behaviour is unchanged: calls into these hooks from shrink_slab_memcg()

still early-return 0 for non-root memcg contexts, keeping the shrinker bit clearable in each memcg's bitmap; the global (kswapd or root direct reclaim) path still drives them as before.

Signed-off-by Usama Arif <[email protected]>

fs/super.c +0 -2

@@ -170,19 +169,6 @@ static void super_wake(struct super_block *sb, unsigned int flag)
 	wake_up_var(&sb->s_flags);
 }
 
-/*
- * The s_op->nr_cached_objects hooks (used for example by btrfs and xfs)

Quoted 5 lines · Usama Arif

Commit 0baad6f9b997 ("fs/super: skip non-memcg-aware nr_cached_objects in memcg slab shrink") added a check in fs/super.c that skips the ->nr_cached_objects() hook whenever the shrinker is invoked for a non-root memcg, because none of the current implementations (btrfs, xfs, shmem huge) honour sc->memcg.

I don't see that commit in an upstream tree. I'm guessing I wasn't cc'd on it, either, because I don't recall seeing that as a patch, either.

Quoted 5 lines · Usama Arif

Commit 0baad6f9b997 ("fs/super: skip non-memcg-aware nr_cached_objects in memcg slab shrink") added a check in fs/super.c that skips the ->nr_cached_objects() hook whenever the shrinker is invoked for a non-root memcg, because none of the current implementations (btrfs, xfs, shmem huge) honour sc->memcg.

I don't see that commit in an upstream tree. I'm guessing I wasn't cc'd on it, either, because I don't recall seeing that as a patch, either.

However, if anything is gating the fs_objects callouts to the fs when memcg is being srhunk, then it is fundamentally broken and you are correct to fix it.

Quoted 8 lines

That policy is really a filesystem-owned property: fs/super.c should not encode the assumption that these hooks are never memcg-aware, since a future implementation might legitimately filter by sc->memcg. Move the check into btrfs_nr_cached_objects(), xfs_fs_nr_cached_objects() and shmem_unused_huge_count() so each filesystem can lift the restriction independently once its underlying counters/scans become memcg-aware, without needing a coordinated change to fs/super.c.

However, your assumptions here are incorrect.

That is, the XFS fs_objects implementation is intended to be called even when memcg shrinking is occurring. I architected it that way all those years ago when I introduced the fs_objects shrinker callout. i.e. XFS is not aging reclaimable inodes via this call - it is purely an expedite freeing of recently reclaimed VFS inodes when there is memory pressure of any kind.

Reclaimable XFS inodes are still charged to memcgs, but we don't track them per-memcg because it is extremely inefficient when their lifetime after being released by the VFS is usually less than 5 seconds. Indeed, this low level cache is not for working set retention; the VFS inode cache does that. The XFS inodes need to through a GC state before they can be freed, this takes a little bit of time, so we don't hold up the VFS inode cache shrinker for that.

The VFS inode cache shrinker is memcg aware, so it only pushes inodes that are being reclaimed by memcg reclaim into the the reclaimable state at the XFS level. Hence we don't need to track reclaimable inodes by memcg - we know that memcg owned inodes that are to be freed have already been pushed into reclaim by the VFS shrinker doing memcg reclaim. Hence all we need to do is sweep them and free them.

This is what the XFS fs objects shrinker does. It needs to run in conjunction with -any- shrinker context to immediately free the clean inodes that the VFS shrinker released that pass, and to release any of the recently released VFS inodes that needed GC and are now clean and can be freed.

IOWs, the XFS fs_objects callout is not "memcg-aware" in the classic sense of "it tracks objects by their owner memcg". However, the -combined superblock shrinker algorithm- is memcg aware and that results in XFS only seeing objects from the memcg being reclaimed transitioning to RECLAIMABLE state which it then immediately sweeps away.

i.e. the architecture of the VFS and XFS inode cache reclaim is entirely memcg aware, and that is why I implemented the fs_objects callout for XFS all those years ago. It works efficiently, it works correctly with memcg based reclaim, and XFS doesn't need to care about memcgs in it's low level reclaim code. Win, win, win.

Quoted 1 line

Behaviour is unchanged: calls into these hooks from shrink_slab_memcg()

Behaviour was broken by the above commit, it needs to change.

Quoted 10 lines

still early-return 0 for non-root memcg contexts, keeping the shrinker bit clearable in each memcg's bitmap; the global (kswapd or root direct reclaim) path still drives them as before.

Signed-off-by Usama Arif <[email protected]>

fs/super.c +0 -0

@@ -170,19 +169,6 @@ static void super_wake(struct super_block *sb, unsigned int flag)
 	wake_up_var(&sb->s_flags);
 }
 

fs/super.c static void super_wake(struct super_block *sb, unsigned int flag) · line 184

Quoted 12 lines

fs/super.c +0 -12

-/*
- * The s_op->nr_cached_objects hooks (used for example by btrfs and xfs)
- * operate on filesystem-global state and ignore sc->memcg. Driving them
- * from per-memcg shrink_slab_memcg() invocations only burns CPU walking
- * per-cpu counters and queueing duplicate work: the actual reclaim happens on
- * the global path (kswapd or root direct reclaim) regardless. Restrict them
- * to that path.
- */
-static inline bool super_fs_objects_eligible(struct shrink_control *sc)
-{
-	return !sc->memcg || mem_cgroup_is_root(sc->memcg);
-}

This wrapper should still exist (with a better name) so filesystems don't need to open code memcg cruft.

-Dave.