Skip to contentSkip to conversation

btrfs: disable slab merging in debug build

PATCH In review David Sterba 4 participants 5 messages Jul 12, 2023 – Jul 13, 2023

Subsystems (2)
Memory management Btrfs file system
lore
Share & tools
lore

David Sterba

author maintainer
#0 lore raw

PATCH 1/1 btrfs: disable slab merging in debug build

The slab allocator newly allows to disable merging per-slab (since commit d0bf7d5759c1 ("mm/slab: introduce kmem_cache flag SLAB_NO_MERGE")). Set this for all caches in debug build so we can verify there are no leaks when module gets reloaded.

Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/backref.c          |  2 +-
 fs/btrfs/ctree.c            |  2 +-
 fs/btrfs/defrag.c           |  2 +-
 fs/btrfs/delayed-inode.c    |  2 +-
 fs/btrfs/delayed-ref.c      |  8 ++++----
 fs/btrfs/extent-io-tree.c   |  2 +-
 fs/btrfs/extent_io.c        |  2 +-
 fs/btrfs/extent_map.c       |  2 +-
 fs/btrfs/free-space-cache.c |  3 ++-
 fs/btrfs/inode.c            |  3 ++-
 fs/btrfs/misc.h             | 10 ++++++++++
 fs/btrfs/ordered-data.c     |  2 +-
 fs/btrfs/transaction.c      |  3 ++-
 13 files changed, 28 insertions(+), 15 deletions(-)
fs/btrfs/backref.c +1 -1
index 79336fa853db..0167c3aaf15d 100644
@@ -200,7 +200,7 @@ int __init btrfs_prelim_ref_init(void)
 	btrfs_prelim_ref_cache = kmem_cache_create("btrfs_prelim_ref",
 					sizeof(struct prelim_ref),
 					0,
-					SLAB_MEM_SPREAD,
+					BTRFS_DEBUG_SLAB_NO_MERGE | SLAB_MEM_SPREAD,
 					NULL);
 	if (!btrfs_prelim_ref_cache)
 		return -ENOMEM;
fs/btrfs/ctree.c +1 -1
index a4cb4b642987..14fc47857caf 100644
@@ -5166,7 +5166,7 @@ int __init btrfs_ctree_init(void)
 {
 	btrfs_path_cachep = kmem_cache_create("btrfs_path",
 			sizeof(struct btrfs_path), 0,
-			SLAB_MEM_SPREAD, NULL);
+			BTRFS_DEBUG_SLAB_NO_MERGE | SLAB_MEM_SPREAD, NULL);
 	if (!btrfs_path_cachep)
 		return -ENOMEM;
 	return 0;
fs/btrfs/defrag.c +1 -1
index f2ff4cbe8656..bac993bb757e 100644
@@ -1370,7 +1370,7 @@ int __init btrfs_auto_defrag_init(void)
 {
 	btrfs_inode_defrag_cachep = kmem_cache_create("btrfs_inode_defrag",
 					sizeof(struct inode_defrag), 0,
-					SLAB_MEM_SPREAD,
+					BTRFS_DEBUG_SLAB_NO_MERGE | SLAB_MEM_SPREAD,
 					NULL);
 	if (!btrfs_inode_defrag_cachep)
 		return -ENOMEM;
fs/btrfs/delayed-inode.c +1 -1
index 6b457b010cbc..c0a6a1784697 100644
@@ -31,7 +31,7 @@ int __init btrfs_delayed_inode_init(void)
 	delayed_node_cache = kmem_cache_create("btrfs_delayed_node",
 					sizeof(struct btrfs_delayed_node),
 					0,
-					SLAB_MEM_SPREAD,
+					BTRFS_DEBUG_SLAB_NO_MERGE | SLAB_MEM_SPREAD,
 					NULL);
 	if (!delayed_node_cache)
 		return -ENOMEM;
fs/btrfs/delayed-ref.c +4 -4
index 6a13cf00218b..d80ea0c52ccc 100644
@@ -1104,28 +1104,28 @@ int __init btrfs_delayed_ref_init(void)
 	btrfs_delayed_ref_head_cachep = kmem_cache_create(
 				"btrfs_delayed_ref_head",
 				sizeof(struct btrfs_delayed_ref_head), 0,
-				SLAB_MEM_SPREAD, NULL);
+				BTRFS_DEBUG_SLAB_NO_MERGE | SLAB_MEM_SPREAD, NULL);
 	if (!btrfs_delayed_ref_head_cachep)
 		goto fail;
 
 	btrfs_delayed_tree_ref_cachep = kmem_cache_create(
 				"btrfs_delayed_tree_ref",
 				sizeof(struct btrfs_delayed_tree_ref), 0,
-				SLAB_MEM_SPREAD, NULL);
+				BTRFS_DEBUG_SLAB_NO_MERGE | SLAB_MEM_SPREAD, NULL);
 	if (!btrfs_delayed_tree_ref_cachep)
 		goto fail;
 
 	btrfs_delayed_data_ref_cachep = kmem_cache_create(
 				"btrfs_delayed_data_ref",
 				sizeof(struct btrfs_delayed_data_ref), 0,
-				SLAB_MEM_SPREAD, NULL);
+				BTRFS_DEBUG_SLAB_NO_MERGE | SLAB_MEM_SPREAD, NULL);
 	if (!btrfs_delayed_data_ref_cachep)
 		goto fail;
 
 	btrfs_delayed_extent_op_cachep = kmem_cache_create(
 				"btrfs_delayed_extent_op",
 				sizeof(struct btrfs_delayed_extent_op), 0,
-				SLAB_MEM_SPREAD, NULL);
+				BTRFS_DEBUG_SLAB_NO_MERGE | SLAB_MEM_SPREAD, NULL);
 	if (!btrfs_delayed_extent_op_cachep)
 		goto fail;
fs/btrfs/extent-io-tree.c +1 -1
index a2315a4b8b75..3075d9439907 100644
@@ -1771,7 +1771,7 @@ int __init extent_state_init_cachep(void)
 {
 	extent_state_cache = kmem_cache_create("btrfs_extent_state",
 			sizeof(struct extent_state), 0,
-			SLAB_MEM_SPREAD, NULL);
+			BTRFS_DEBUG_SLAB_NO_MERGE | SLAB_MEM_SPREAD, NULL);
 	if (!extent_state_cache)
 		return -ENOMEM;
fs/btrfs/extent_io.c +1 -1
index a845a90d46f7..094004c11a50 100644
@@ -149,7 +149,7 @@ int __init extent_buffer_init_cachep(void)
 {
 	extent_buffer_cache = kmem_cache_create("btrfs_extent_buffer",
 			sizeof(struct extent_buffer), 0,
-			SLAB_MEM_SPREAD, NULL);
+			BTRFS_DEBUG_SLAB_NO_MERGE | SLAB_MEM_SPREAD, NULL);
 	if (!extent_buffer_cache)
 		return -ENOMEM;
fs/btrfs/extent_map.c +1 -1
index 0cdb3e86f29b..d66f512613f7 100644
@@ -17,7 +17,7 @@ int __init extent_map_init(void)
 {
 	extent_map_cache = kmem_cache_create("btrfs_extent_map",
 			sizeof(struct extent_map), 0,
-			SLAB_MEM_SPREAD, NULL);
+			BTRFS_DEBUG_SLAB_NO_MERGE | SLAB_MEM_SPREAD, NULL);
 	if (!extent_map_cache)
 		return -ENOMEM;
 	return 0;
fs/btrfs/free-space-cache.c +2 -1
index 880800418075..c5b70dbec245 100644
@@ -4160,12 +4160,13 @@ int __init btrfs_free_space_init(void)
 {
 	btrfs_free_space_cachep = kmem_cache_create("btrfs_free_space",
 			sizeof(struct btrfs_free_space), 0,
-			SLAB_MEM_SPREAD, NULL);
+			BTRFS_DEBUG_SLAB_NO_MERGE | SLAB_MEM_SPREAD, NULL);
 	if (!btrfs_free_space_cachep)
 		return -ENOMEM;
 
 	btrfs_free_space_bitmap_cachep = kmem_cache_create("btrfs_free_space_bitmap",
 							PAGE_SIZE, PAGE_SIZE,
+							BTRFS_DEBUG_SLAB_NO_MERGE |
 							SLAB_MEM_SPREAD, NULL);
 	if (!btrfs_free_space_bitmap_cachep) {
 		kmem_cache_destroy(btrfs_free_space_cachep);
fs/btrfs/inode.c +2 -1
index b12850b31cb3..3bde49018530 100644
@@ -8723,7 +8723,8 @@ int __init btrfs_init_cachep(void)
 {
 	btrfs_inode_cachep = kmem_cache_create("btrfs_inode",
 			sizeof(struct btrfs_inode), 0,
-			SLAB_RECLAIM_ACCOUNT | SLAB_MEM_SPREAD | SLAB_ACCOUNT,
+			BTRFS_DEBUG_SLAB_NO_MERGE | SLAB_RECLAIM_ACCOUNT |
+			SLAB_MEM_SPREAD | SLAB_ACCOUNT,
 			init_once);
 	if (!btrfs_inode_cachep)
 		goto fail;
fs/btrfs/misc.h +10 -0
index 005751a12911..9f667ab1a3d5 100644
@@ -8,6 +8,16 @@
 #include <linux/math64.h>
 #include <linux/rbtree.h>
 
+/*
+ * Don't merge slabs in debug build so we can verify there are no leaks when
+ * reloading module.
+ */
+#ifdef CONFIG_BTRFS_DEBUG
+#define BTRFS_DEBUG_SLAB_NO_MERGE		SLAB_NO_MERGE
+#else
+#define BTRFS_DEBUG_SLAB_NO_MERGE		0
+#endif
+
 #define in_range(b, first, len) ((b) >= (first) && (b) < (first) + (len))
 
 /*
fs/btrfs/ordered-data.c +1 -1
index a629532283bc..c7306e8bd877 100644
@@ -1245,7 +1245,7 @@ int __init ordered_data_init(void)
 {
 	btrfs_ordered_extent_cache = kmem_cache_create("btrfs_ordered_extent",
 				     sizeof(struct btrfs_ordered_extent), 0,
-				     SLAB_MEM_SPREAD,
+				     BTRFS_DEBUG_SLAB_NO_MERGE | SLAB_MEM_SPREAD,
 				     NULL);
 	if (!btrfs_ordered_extent_cache)
 		return -ENOMEM;
fs/btrfs/transaction.c +2 -1
index cf306351b148..e7cfc992e02a 100644
@@ -2641,7 +2641,8 @@ int __init btrfs_transaction_init(void)
 {
 	btrfs_trans_handle_cachep = kmem_cache_create("btrfs_trans_handle",
 			sizeof(struct btrfs_trans_handle), 0,
-			SLAB_TEMPORARY | SLAB_MEM_SPREAD, NULL);
+			BTRFS_DEBUG_SLAB_NO_MERGE | SLAB_TEMPORARY | SLAB_MEM_SPREAD,
+			NULL);
 	if (!btrfs_trans_handle_cachep)
 		return -ENOMEM;
 	return 0;
Signature · 1 line
2.40.0
#2 lore raw
In reply to David Sterba
Quoted 4 lines · David Sterba

The slab allocator newly allows to disable merging per-slab (since commit d0bf7d5759c1 ("mm/slab: introduce kmem_cache flag SLAB_NO_MERGE")). Set this for all caches in debug build so we can verify there are no leaks when module gets reloaded.

So we're having a discussion on linux-mm wether to just disbale slab merging by default, because it really is a pain. Maybe wait for that to settle before adding per-subsystem hacks for what really is a slab problem?

#3 lore raw
Quoted 10 lines · Christoph Hellwig
Quoted 4 lines · David Sterba

The slab allocator newly allows to disable merging per-slab (since commit d0bf7d5759c1 ("mm/slab: introduce kmem_cache flag SLAB_NO_MERGE")). Set this for all caches in debug build so we can verify there are no leaks when module gets reloaded.

So we're having a discussion on linux-mm wether to just disbale slab merging by default, because it really is a pain. Maybe wait for that to settle before adding per-subsystem hacks for what really is a slab problem?

Yeah I can wait with the patch. That slab merging is considered bad is new. I remember discussions where Linus and (maybe?) xfs guys argued pro/against merging of slabs, where xfs wanted not-merging and had to resort to hacks like empty slab constructor that would prevent it. I can't find the link but that's base of my reasoning to add a flag assuming that merging makes sense by default.

#4 lore raw
Quoted 7 lines · David Sterba
Quoted 6 lines · Christoph Hellwig
Quoted 4 lines · David Sterba

The slab allocator newly allows to disable merging per-slab (since commit d0bf7d5759c1 ("mm/slab: introduce kmem_cache flag SLAB_NO_MERGE")). Set this for all caches in debug build so we can verify there are no leaks when module gets reloaded.

Quoted 7 lines
Quoted 4 lines

So we're having a discussion on linux-mm wether to just disbale slab merging by default, because it really is a pain. Maybe wait for that to settle before adding per-subsystem hacks for what really is a slab problem?

Yeah I can wait with the patch. That slab merging is considered bad is new.

Yeah, I wouldn't say it's universally accepted. But even if we change the default, it's just a default that distros or users might not follow, so there's still a space for per-cache enforcement IMHO.

Quoted 5 lines

I remember discussions where Linus and (maybe?) xfs guys argued pro/against merging of slabs, where xfs wanted not-merging and had to resort to hacks like empty slab constructor that would prevent it. I can't find the link but that's base of my reasoning to add a flag assuming that merging makes sense by default.

Probably this discussion?

https://lore.kernel.org/all/CA+55aFyepmdpbg9U2Pvp+aHjKmmGCrTK2ywzqfmaOTMXQasYNw@mail.gmail.com/