[PATCH 00/10] Static inline cleanups

[PATCH 00/10] Static inline cleanups

[David Sterba]

Historically there are many static inlines in headers that do not
qualify as such, move them to .c. Some can be open coded.

David Sterba (10):
  btrfs: move balance args conversion helpers to volumes.c
  btrfs: open code btrfs_backref_iter_free()
  btrfs: open code btrfs_backref_get_eb()
  btrfs: uninline some static inline helpers from backref.h
  btrfs: uninline btrfs_init_delayed_root()
  btrfs: drop static inline specifiers from tree-mod-log.c
  btrfs: uninline some static inline helpers from tree-log.h
  btrfs: simplify conditions in btrfs_free_chunk_map()
  btrfs: open code trivial btrfs_lru_cache_size()
  btrfs: uninline some static inline helpers from delayed-ref.h

 fs/btrfs/accessors.h     |  39 -------------
 fs/btrfs/backref.c       |  94 +++++++++++++++++++++++++++++-
 fs/btrfs/backref.h       | 120 ++++-----------------------------------
 fs/btrfs/delayed-inode.c |  11 ++++
 fs/btrfs/delayed-inode.h |  13 +----
 fs/btrfs/delayed-ref.c   |  65 +++++++++++++++++++++
 fs/btrfs/delayed-ref.h   |  72 +++--------------------
 fs/btrfs/lru_cache.h     |   5 --
 fs/btrfs/relocation.c    |   3 +-
 fs/btrfs/send.c          |   7 +--
 fs/btrfs/tree-log.c      |  46 +++++++++++++++
 fs/btrfs/tree-log.h      |  48 +---------------
 fs/btrfs/tree-mod-log.c  |  13 ++---
 fs/btrfs/volumes.c       |  41 ++++++++++++-
 fs/btrfs/volumes.h       |   2 +-
 fs/btrfs/zoned.c         |   3 +-
 16 files changed, 290 insertions(+), 292 deletions(-)

-- 
2.42.1

[PATCH 01/10] btrfs: move balance args conversion helpers to volumes.c

[David Sterba]

The from/to CPU/disk helpers for balance args are used only in volumes,
no need to define them in accessors.h.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/accessors.h | 39 ---------------------------------------
 fs/btrfs/volumes.c   | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 39 deletions(-)

diff --git a/fs/btrfs/accessors.h b/fs/btrfs/accessors.h
index fa099f61fc8c..6fce3e8d3dac 100644
--- a/fs/btrfs/accessors.h
+++ b/fs/btrfs/accessors.h
@@ -853,45 +853,6 @@ static inline void btrfs_set_balance_sys(struct extent_buffer *eb,
 	write_eb_member(eb, bi, struct btrfs_balance_item, sys, ba);
 }
 
-static inline void btrfs_disk_balance_args_to_cpu(struct btrfs_balance_args *cpu,
-			       const struct btrfs_disk_balance_args *disk)
-{
-	memset(cpu, 0, sizeof(*cpu));
-
-	cpu->profiles = le64_to_cpu(disk->profiles);
-	cpu->usage = le64_to_cpu(disk->usage);
-	cpu->devid = le64_to_cpu(disk->devid);
-	cpu->pstart = le64_to_cpu(disk->pstart);
-	cpu->pend = le64_to_cpu(disk->pend);
-	cpu->vstart = le64_to_cpu(disk->vstart);
-	cpu->vend = le64_to_cpu(disk->vend);
-	cpu->target = le64_to_cpu(disk->target);
-	cpu->flags = le64_to_cpu(disk->flags);
-	cpu->limit = le64_to_cpu(disk->limit);
-	cpu->stripes_min = le32_to_cpu(disk->stripes_min);
-	cpu->stripes_max = le32_to_cpu(disk->stripes_max);
-}
-
-static inline void btrfs_cpu_balance_args_to_disk(
-				struct btrfs_disk_balance_args *disk,
-				const struct btrfs_balance_args *cpu)
-{
-	memset(disk, 0, sizeof(*disk));
-
-	disk->profiles = cpu_to_le64(cpu->profiles);
-	disk->usage = cpu_to_le64(cpu->usage);
-	disk->devid = cpu_to_le64(cpu->devid);
-	disk->pstart = cpu_to_le64(cpu->pstart);
-	disk->pend = cpu_to_le64(cpu->pend);
-	disk->vstart = cpu_to_le64(cpu->vstart);
-	disk->vend = cpu_to_le64(cpu->vend);
-	disk->target = cpu_to_le64(cpu->target);
-	disk->flags = cpu_to_le64(cpu->flags);
-	disk->limit = cpu_to_le64(cpu->limit);
-	disk->stripes_min = cpu_to_le32(cpu->stripes_min);
-	disk->stripes_max = cpu_to_le32(cpu->stripes_max);
-}
-
 /* struct btrfs_super_block */
 BTRFS_SETGET_STACK_FUNCS(super_bytenr, struct btrfs_super_block, bytenr, 64);
 BTRFS_SETGET_STACK_FUNCS(super_flags, struct btrfs_super_block, flags, 64);
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index bfe54745eae3..752144a31d79 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3512,6 +3512,44 @@ static int btrfs_may_alloc_data_chunk(struct btrfs_fs_info *fs_info,
 	return 0;
 }
 
+static void btrfs_disk_balance_args_to_cpu(struct btrfs_balance_args *cpu,
+					   const struct btrfs_disk_balance_args *disk)
+{
+	memset(cpu, 0, sizeof(*cpu));
+
+	cpu->profiles = le64_to_cpu(disk->profiles);
+	cpu->usage = le64_to_cpu(disk->usage);
+	cpu->devid = le64_to_cpu(disk->devid);
+	cpu->pstart = le64_to_cpu(disk->pstart);
+	cpu->pend = le64_to_cpu(disk->pend);
+	cpu->vstart = le64_to_cpu(disk->vstart);
+	cpu->vend = le64_to_cpu(disk->vend);
+	cpu->target = le64_to_cpu(disk->target);
+	cpu->flags = le64_to_cpu(disk->flags);
+	cpu->limit = le64_to_cpu(disk->limit);
+	cpu->stripes_min = le32_to_cpu(disk->stripes_min);
+	cpu->stripes_max = le32_to_cpu(disk->stripes_max);
+}
+
+static void btrfs_cpu_balance_args_to_disk(struct btrfs_disk_balance_args *disk,
+					   const struct btrfs_balance_args *cpu)
+{
+	memset(disk, 0, sizeof(*disk));
+
+	disk->profiles = cpu_to_le64(cpu->profiles);
+	disk->usage = cpu_to_le64(cpu->usage);
+	disk->devid = cpu_to_le64(cpu->devid);
+	disk->pstart = cpu_to_le64(cpu->pstart);
+	disk->pend = cpu_to_le64(cpu->pend);
+	disk->vstart = cpu_to_le64(cpu->vstart);
+	disk->vend = cpu_to_le64(cpu->vend);
+	disk->target = cpu_to_le64(cpu->target);
+	disk->flags = cpu_to_le64(cpu->flags);
+	disk->limit = cpu_to_le64(cpu->limit);
+	disk->stripes_min = cpu_to_le32(cpu->stripes_min);
+	disk->stripes_max = cpu_to_le32(cpu->stripes_max);
+}
+
 static int insert_balance_item(struct btrfs_fs_info *fs_info,
 			       struct btrfs_balance_control *bctl)
 {
-- 
2.42.1

[PATCH 02/10] btrfs: open code btrfs_backref_iter_free()

[David Sterba]

The helper is trivial and used only once, open code it. It's safe to
remove the 'if', the pointer is validated in build_backref_tree().

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/backref.h    | 8 --------
 fs/btrfs/relocation.c | 3 ++-
 2 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/backref.h b/fs/btrfs/backref.h
index 523e594ac753..493ea47db426 100644
--- a/fs/btrfs/backref.h
+++ b/fs/btrfs/backref.h
@@ -283,14 +283,6 @@ struct btrfs_backref_iter {
 
 struct btrfs_backref_iter *btrfs_backref_iter_alloc(struct btrfs_fs_info *fs_info);
 
-static inline void btrfs_backref_iter_free(struct btrfs_backref_iter *iter)
-{
-	if (!iter)
-		return;
-	btrfs_free_path(iter->path);
-	kfree(iter);
-}
-
 static inline struct extent_buffer *btrfs_backref_get_eb(
 		struct btrfs_backref_iter *iter)
 {
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 2fca67f2b39b..f96f267fb4aa 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -523,7 +523,8 @@ static noinline_for_stack struct btrfs_backref_node *build_backref_tree(
 	if (handle_useless_nodes(rc, node))
 		node = NULL;
 out:
-	btrfs_backref_iter_free(iter);
+	btrfs_free_path(iter->path);
+	kfree(iter);
 	btrfs_free_path(path);
 	if (err) {
 		btrfs_backref_error_cleanup(cache, node);
-- 
2.42.1

[PATCH 03/10] btrfs: open code btrfs_backref_get_eb()

[David Sterba]

The helper is trivial, we can inline it. It's safe to remove the 'if' as
the iterator is always valid when used, the potential NULL was never
checked anyway.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/backref.c | 4 ++--
 fs/btrfs/backref.h | 8 --------
 2 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index fe05e2f55bf7..1b57c8289de6 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -2960,7 +2960,7 @@ int btrfs_backref_iter_start(struct btrfs_backref_iter *iter, u64 bytenr)
  */
 int btrfs_backref_iter_next(struct btrfs_backref_iter *iter)
 {
-	struct extent_buffer *eb = btrfs_backref_get_eb(iter);
+	struct extent_buffer *eb = iter->path->nodes[0];
 	struct btrfs_root *extent_root;
 	struct btrfs_path *path = iter->path;
 	struct btrfs_extent_inline_ref *iref;
@@ -3438,7 +3438,7 @@ int btrfs_backref_add_tree_node(struct btrfs_trans_handle *trans,
 		int type;
 
 		cond_resched();
-		eb = btrfs_backref_get_eb(iter);
+		eb = iter->path->nodes[0];
 
 		key.objectid = iter->bytenr;
 		if (btrfs_backref_iter_is_inline_ref(iter)) {
diff --git a/fs/btrfs/backref.h b/fs/btrfs/backref.h
index 493ea47db426..04b82c512bf1 100644
--- a/fs/btrfs/backref.h
+++ b/fs/btrfs/backref.h
@@ -283,14 +283,6 @@ struct btrfs_backref_iter {
 
 struct btrfs_backref_iter *btrfs_backref_iter_alloc(struct btrfs_fs_info *fs_info);
 
-static inline struct extent_buffer *btrfs_backref_get_eb(
-		struct btrfs_backref_iter *iter)
-{
-	if (!iter)
-		return NULL;
-	return iter->path->nodes[0];
-}
-
 /*
  * For metadata with EXTENT_ITEM key (non-skinny) case, the first inline data
  * is btrfs_tree_block_info, without a btrfs_extent_inline_ref header.
-- 
2.42.1

[PATCH 04/10] btrfs: uninline some static inline helpers from backref.h

[David Sterba]

There are many helpers doing simple things but not simple enough to
justify the static inline. None of them seems to be on a hot path so
move them to .c.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/backref.c |  90 +++++++++++++++++++++++++++++++++++++++
 fs/btrfs/backref.h | 104 ++++++---------------------------------------
 2 files changed, 102 insertions(+), 92 deletions(-)

diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index 1b57c8289de6..6514cb1d404a 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -2856,6 +2856,16 @@ struct btrfs_backref_iter *btrfs_backref_iter_alloc(struct btrfs_fs_info *fs_inf
 	return ret;
 }
 
+static void btrfs_backref_iter_release(struct btrfs_backref_iter *iter)
+{
+	iter->bytenr = 0;
+	iter->item_ptr = 0;
+	iter->cur_ptr = 0;
+	iter->end_ptr = 0;
+	btrfs_release_path(iter->path);
+	memset(&iter->cur_key, 0, sizeof(iter->cur_key));
+}
+
 int btrfs_backref_iter_start(struct btrfs_backref_iter *iter, u64 bytenr)
 {
 	struct btrfs_fs_info *fs_info = iter->fs_info;
@@ -2948,6 +2958,14 @@ int btrfs_backref_iter_start(struct btrfs_backref_iter *iter, u64 bytenr)
 	return ret;
 }
 
+static bool btrfs_backref_iter_is_inline_ref(struct btrfs_backref_iter *iter)
+{
+	if (iter->cur_key.type == BTRFS_EXTENT_ITEM_KEY ||
+	    iter->cur_key.type == BTRFS_METADATA_ITEM_KEY)
+		return true;
+	return false;
+}
+
 /*
  * Go to the next backref item of current bytenr, can be either inlined or
  * keyed.
@@ -3048,6 +3066,19 @@ struct btrfs_backref_node *btrfs_backref_alloc_node(
 	return node;
 }
 
+void btrfs_backref_free_node(struct btrfs_backref_cache *cache,
+			     struct btrfs_backref_node *node)
+{
+	if (node) {
+		ASSERT(list_empty(&node->list));
+		ASSERT(list_empty(&node->lower));
+		ASSERT(node->eb == NULL);
+		cache->nr_nodes--;
+		btrfs_put_root(node->root);
+		kfree(node);
+	}
+}
+
 struct btrfs_backref_edge *btrfs_backref_alloc_edge(
 		struct btrfs_backref_cache *cache)
 {
@@ -3059,6 +3090,52 @@ struct btrfs_backref_edge *btrfs_backref_alloc_edge(
 	return edge;
 }
 
+void btrfs_backref_free_edge(struct btrfs_backref_cache *cache,
+			     struct btrfs_backref_edge *edge)
+{
+	if (edge) {
+		cache->nr_edges--;
+		kfree(edge);
+	}
+}
+
+void btrfs_backref_unlock_node_buffer(struct btrfs_backref_node *node)
+{
+	if (node->locked) {
+		btrfs_tree_unlock(node->eb);
+		node->locked = 0;
+	}
+}
+
+void btrfs_backref_drop_node_buffer(struct btrfs_backref_node *node)
+{
+	if (node->eb) {
+		btrfs_backref_unlock_node_buffer(node);
+		free_extent_buffer(node->eb);
+		node->eb = NULL;
+	}
+}
+
+/*
+ * Drop the backref node from cache without cleaning up its children
+ * edges.
+ *
+ * This can only be called on node without parent edges.
+ * The children edges are still kept as is.
+ */
+void btrfs_backref_drop_node(struct btrfs_backref_cache *tree,
+			     struct btrfs_backref_node *node)
+{
+	ASSERT(list_empty(&node->upper));
+
+	btrfs_backref_drop_node_buffer(node);
+	list_del_init(&node->list);
+	list_del_init(&node->lower);
+	if (!RB_EMPTY_NODE(&node->rb_node))
+		rb_erase(&node->rb_node, &tree->rb_root);
+	btrfs_backref_free_node(tree, node);
+}
+
 /*
  * Drop the backref node from cache, also cleaning up all its
  * upper edges and any uncached nodes in the path.
@@ -3130,6 +3207,19 @@ void btrfs_backref_release_cache(struct btrfs_backref_cache *cache)
 	ASSERT(!cache->nr_edges);
 }
 
+void btrfs_backref_link_edge(struct btrfs_backref_edge *edge,
+			     struct btrfs_backref_node *lower,
+			     struct btrfs_backref_node *upper,
+			     int link_which)
+{
+	ASSERT(upper && lower && upper->level == lower->level + 1);
+	edge->node[LOWER] = lower;
+	edge->node[UPPER] = upper;
+	if (link_which & LINK_LOWER)
+		list_add_tail(&edge->list[LOWER], &lower->upper);
+	if (link_which & LINK_UPPER)
+		list_add_tail(&edge->list[UPPER], &upper->lower);
+}
 /*
  * Handle direct tree backref
  *
diff --git a/fs/btrfs/backref.h b/fs/btrfs/backref.h
index 04b82c512bf1..e8c22cccb5c1 100644
--- a/fs/btrfs/backref.h
+++ b/fs/btrfs/backref.h
@@ -302,25 +302,6 @@ int btrfs_backref_iter_start(struct btrfs_backref_iter *iter, u64 bytenr);
 
 int btrfs_backref_iter_next(struct btrfs_backref_iter *iter);
 
-static inline bool btrfs_backref_iter_is_inline_ref(
-		struct btrfs_backref_iter *iter)
-{
-	if (iter->cur_key.type == BTRFS_EXTENT_ITEM_KEY ||
-	    iter->cur_key.type == BTRFS_METADATA_ITEM_KEY)
-		return true;
-	return false;
-}
-
-static inline void btrfs_backref_iter_release(struct btrfs_backref_iter *iter)
-{
-	iter->bytenr = 0;
-	iter->item_ptr = 0;
-	iter->cur_ptr = 0;
-	iter->end_ptr = 0;
-	btrfs_release_path(iter->path);
-	memset(&iter->cur_key, 0, sizeof(iter->cur_key));
-}
-
 /*
  * Backref cache related structures
  *
@@ -448,83 +429,22 @@ struct btrfs_backref_edge *btrfs_backref_alloc_edge(
 
 #define		LINK_LOWER	(1 << 0)
 #define		LINK_UPPER	(1 << 1)
-static inline void btrfs_backref_link_edge(struct btrfs_backref_edge *edge,
-					   struct btrfs_backref_node *lower,
-					   struct btrfs_backref_node *upper,
-					   int link_which)
-{
-	ASSERT(upper && lower && upper->level == lower->level + 1);
-	edge->node[LOWER] = lower;
-	edge->node[UPPER] = upper;
-	if (link_which & LINK_LOWER)
-		list_add_tail(&edge->list[LOWER], &lower->upper);
-	if (link_which & LINK_UPPER)
-		list_add_tail(&edge->list[UPPER], &upper->lower);
-}
 
-static inline void btrfs_backref_free_node(struct btrfs_backref_cache *cache,
-					   struct btrfs_backref_node *node)
-{
-	if (node) {
-		ASSERT(list_empty(&node->list));
-		ASSERT(list_empty(&node->lower));
-		ASSERT(node->eb == NULL);
-		cache->nr_nodes--;
-		btrfs_put_root(node->root);
-		kfree(node);
-	}
-}
-
-static inline void btrfs_backref_free_edge(struct btrfs_backref_cache *cache,
-					   struct btrfs_backref_edge *edge)
-{
-	if (edge) {
-		cache->nr_edges--;
-		kfree(edge);
-	}
-}
-
-static inline void btrfs_backref_unlock_node_buffer(
-		struct btrfs_backref_node *node)
-{
-	if (node->locked) {
-		btrfs_tree_unlock(node->eb);
-		node->locked = 0;
-	}
-}
-
-static inline void btrfs_backref_drop_node_buffer(
-		struct btrfs_backref_node *node)
-{
-	if (node->eb) {
-		btrfs_backref_unlock_node_buffer(node);
-		free_extent_buffer(node->eb);
-		node->eb = NULL;
-	}
-}
-
-/*
- * Drop the backref node from cache without cleaning up its children
- * edges.
- *
- * This can only be called on node without parent edges.
- * The children edges are still kept as is.
- */
-static inline void btrfs_backref_drop_node(struct btrfs_backref_cache *tree,
-					   struct btrfs_backref_node *node)
-{
-	ASSERT(list_empty(&node->upper));
-
-	btrfs_backref_drop_node_buffer(node);
-	list_del_init(&node->list);
-	list_del_init(&node->lower);
-	if (!RB_EMPTY_NODE(&node->rb_node))
-		rb_erase(&node->rb_node, &tree->rb_root);
-	btrfs_backref_free_node(tree, node);
-}
+void btrfs_backref_link_edge(struct btrfs_backref_edge *edge,
+			     struct btrfs_backref_node *lower,
+			     struct btrfs_backref_node *upper,
+			     int link_which);
+void btrfs_backref_free_node(struct btrfs_backref_cache *cache,
+			     struct btrfs_backref_node *node);
+void btrfs_backref_free_edge(struct btrfs_backref_cache *cache,
+			     struct btrfs_backref_edge *edge);
+void btrfs_backref_unlock_node_buffer(struct btrfs_backref_node *node);
+void btrfs_backref_drop_node_buffer(struct btrfs_backref_node *node);
 
 void btrfs_backref_cleanup_node(struct btrfs_backref_cache *cache,
 				struct btrfs_backref_node *node);
+void btrfs_backref_drop_node(struct btrfs_backref_cache *tree,
+			     struct btrfs_backref_node *node);
 
 void btrfs_backref_release_cache(struct btrfs_backref_cache *cache);
 
-- 
2.42.1

[PATCH 05/10] btrfs: uninline btrfs_init_delayed_root()

[David Sterba]

This is a simple initializer and not on any hot path, it does not need
to be static inline.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/delayed-inode.c | 11 +++++++++++
 fs/btrfs/delayed-inode.h | 13 +------------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
index efe435403b77..920225658fb1 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -43,6 +43,17 @@ void __cold btrfs_delayed_inode_exit(void)
 	kmem_cache_destroy(delayed_node_cache);
 }
 
+void btrfs_init_delayed_root(struct btrfs_delayed_root *delayed_root)
+{
+	atomic_set(&delayed_root->items, 0);
+	atomic_set(&delayed_root->items_seq, 0);
+	delayed_root->nodes = 0;
+	spin_lock_init(&delayed_root->lock);
+	init_waitqueue_head(&delayed_root->wait);
+	INIT_LIST_HEAD(&delayed_root->node_list);
+	INIT_LIST_HEAD(&delayed_root->prepare_list);
+}
+
 static inline void btrfs_init_delayed_node(
 				struct btrfs_delayed_node *delayed_node,
 				struct btrfs_root *root, u64 inode_id)
diff --git a/fs/btrfs/delayed-inode.h b/fs/btrfs/delayed-inode.h
index 3870a4bf7189..64e115d97499 100644
--- a/fs/btrfs/delayed-inode.h
+++ b/fs/btrfs/delayed-inode.h
@@ -106,18 +106,7 @@ struct btrfs_delayed_item {
 	char data[] __counted_by(data_len);
 };
 
-static inline void btrfs_init_delayed_root(
-				struct btrfs_delayed_root *delayed_root)
-{
-	atomic_set(&delayed_root->items, 0);
-	atomic_set(&delayed_root->items_seq, 0);
-	delayed_root->nodes = 0;
-	spin_lock_init(&delayed_root->lock);
-	init_waitqueue_head(&delayed_root->wait);
-	INIT_LIST_HEAD(&delayed_root->node_list);
-	INIT_LIST_HEAD(&delayed_root->prepare_list);
-}
-
+void btrfs_init_delayed_root(struct btrfs_delayed_root *delayed_root);
 int btrfs_insert_delayed_dir_index(struct btrfs_trans_handle *trans,
 				   const char *name, int name_len,
 				   struct btrfs_inode *dir,
-- 
2.42.1

[PATCH 06/10] btrfs: drop static inline specifiers from tree-mod-log.c

[David Sterba]

Using static inline in a .c file should be justified, e.g. when
functions are on a hot path but none of the affected functions seem to
be. As it's all in one compilation unit let the compiler decide.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/tree-mod-log.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/tree-mod-log.c b/fs/btrfs/tree-mod-log.c
index 3df6153d5d5a..43b3accbed7a 100644
--- a/fs/btrfs/tree-mod-log.c
+++ b/fs/btrfs/tree-mod-log.c
@@ -44,7 +44,7 @@ struct tree_mod_elem {
 /*
  * Pull a new tree mod seq number for our operation.
  */
-static inline u64 btrfs_inc_tree_mod_seq(struct btrfs_fs_info *fs_info)
+static u64 btrfs_inc_tree_mod_seq(struct btrfs_fs_info *fs_info)
 {
 	return atomic64_inc_return(&fs_info->tree_mod_seq);
 }
@@ -170,8 +170,7 @@ static noinline int tree_mod_log_insert(struct btrfs_fs_info *fs_info,
  * this until all tree mod log insertions are recorded in the rb tree and then
  * write unlock fs_info::tree_mod_log_lock.
  */
-static inline bool tree_mod_dont_log(struct btrfs_fs_info *fs_info,
-				    struct extent_buffer *eb)
+static bool tree_mod_dont_log(struct btrfs_fs_info *fs_info, struct extent_buffer *eb)
 {
 	if (!test_bit(BTRFS_FS_TREE_MOD_LOG_USERS, &fs_info->flags))
 		return true;
@@ -188,7 +187,7 @@ static inline bool tree_mod_dont_log(struct btrfs_fs_info *fs_info,
 }
 
 /* Similar to tree_mod_dont_log, but doesn't acquire any locks. */
-static inline bool tree_mod_need_log(const struct btrfs_fs_info *fs_info,
+static bool tree_mod_need_log(const struct btrfs_fs_info *fs_info,
 				    struct extent_buffer *eb)
 {
 	if (!test_bit(BTRFS_FS_TREE_MOD_LOG_USERS, &fs_info->flags))
@@ -367,9 +366,9 @@ int btrfs_tree_mod_log_insert_move(struct extent_buffer *eb,
 	return ret;
 }
 
-static inline int tree_mod_log_free_eb(struct btrfs_fs_info *fs_info,
-				       struct tree_mod_elem **tm_list,
-				       int nritems)
+static int tree_mod_log_free_eb(struct btrfs_fs_info *fs_info,
+				struct tree_mod_elem **tm_list,
+				int nritems)
 {
 	int i, j;
 	int ret;
-- 
2.42.1

[PATCH 07/10] btrfs: uninline some static inline helpers from tree-log.h

[David Sterba]

The helpers are doing an initialization or release work, none of which
is performance critical that it would require a static inline, so move
them to the .c file.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/tree-log.c | 46 +++++++++++++++++++++++++++++++++++++++++++
 fs/btrfs/tree-log.h | 48 +++------------------------------------------
 2 files changed, 49 insertions(+), 45 deletions(-)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index d7693368f34f..472918a5bc73 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -2818,6 +2818,52 @@ static void wait_for_writer(struct btrfs_root *root)
 	finish_wait(&root->log_writer_wait, &wait);
 }
 
+void btrfs_init_log_ctx(struct btrfs_log_ctx *ctx, struct inode *inode)
+{
+	ctx->log_ret = 0;
+	ctx->log_transid = 0;
+	ctx->log_new_dentries = false;
+	ctx->logging_new_name = false;
+	ctx->logging_new_delayed_dentries = false;
+	ctx->logged_before = false;
+	ctx->inode = inode;
+	INIT_LIST_HEAD(&ctx->list);
+	INIT_LIST_HEAD(&ctx->ordered_extents);
+	INIT_LIST_HEAD(&ctx->conflict_inodes);
+	ctx->num_conflict_inodes = 0;
+	ctx->logging_conflict_inodes = false;
+	ctx->scratch_eb = NULL;
+}
+
+void btrfs_init_log_ctx_scratch_eb(struct btrfs_log_ctx *ctx)
+{
+	struct btrfs_inode *inode = BTRFS_I(ctx->inode);
+
+	if (!test_bit(BTRFS_INODE_NEEDS_FULL_SYNC, &inode->runtime_flags) &&
+	    !test_bit(BTRFS_INODE_COPY_EVERYTHING, &inode->runtime_flags))
+		return;
+
+	/*
+	 * Don't care about allocation failure. This is just for optimization,
+	 * if we fail to allocate here, we will try again later if needed.
+	 */
+	ctx->scratch_eb = alloc_dummy_extent_buffer(inode->root->fs_info, 0);
+}
+
+void btrfs_release_log_ctx_extents(struct btrfs_log_ctx *ctx)
+{
+	struct btrfs_ordered_extent *ordered;
+	struct btrfs_ordered_extent *tmp;
+
+	ASSERT(inode_is_locked(ctx->inode));
+
+	list_for_each_entry_safe(ordered, tmp, &ctx->ordered_extents, log_list) {
+		list_del_init(&ordered->log_list);
+		btrfs_put_ordered_extent(ordered);
+	}
+}
+
+
 static inline void btrfs_remove_log_ctx(struct btrfs_root *root,
 					struct btrfs_log_ctx *ctx)
 {
diff --git a/fs/btrfs/tree-log.h b/fs/btrfs/tree-log.h
index 254082a189c3..22e9cbc81577 100644
--- a/fs/btrfs/tree-log.h
+++ b/fs/btrfs/tree-log.h
@@ -55,51 +55,9 @@ struct btrfs_log_ctx {
 	struct extent_buffer *scratch_eb;
 };
 
-static inline void btrfs_init_log_ctx(struct btrfs_log_ctx *ctx,
-				      struct inode *inode)
-{
-	ctx->log_ret = 0;
-	ctx->log_transid = 0;
-	ctx->log_new_dentries = false;
-	ctx->logging_new_name = false;
-	ctx->logging_new_delayed_dentries = false;
-	ctx->logged_before = false;
-	ctx->inode = inode;
-	INIT_LIST_HEAD(&ctx->list);
-	INIT_LIST_HEAD(&ctx->ordered_extents);
-	INIT_LIST_HEAD(&ctx->conflict_inodes);
-	ctx->num_conflict_inodes = 0;
-	ctx->logging_conflict_inodes = false;
-	ctx->scratch_eb = NULL;
-}
-
-static inline void btrfs_init_log_ctx_scratch_eb(struct btrfs_log_ctx *ctx)
-{
-	struct btrfs_inode *inode = BTRFS_I(ctx->inode);
-
-	if (!test_bit(BTRFS_INODE_NEEDS_FULL_SYNC, &inode->runtime_flags) &&
-	    !test_bit(BTRFS_INODE_COPY_EVERYTHING, &inode->runtime_flags))
-		return;
-
-	/*
-	 * Don't care about allocation failure. This is just for optimization,
-	 * if we fail to allocate here, we will try again later if needed.
-	 */
-	ctx->scratch_eb = alloc_dummy_extent_buffer(inode->root->fs_info, 0);
-}
-
-static inline void btrfs_release_log_ctx_extents(struct btrfs_log_ctx *ctx)
-{
-	struct btrfs_ordered_extent *ordered;
-	struct btrfs_ordered_extent *tmp;
-
-	ASSERT(inode_is_locked(ctx->inode));
-
-	list_for_each_entry_safe(ordered, tmp, &ctx->ordered_extents, log_list) {
-		list_del_init(&ordered->log_list);
-		btrfs_put_ordered_extent(ordered);
-	}
-}
+void btrfs_init_log_ctx(struct btrfs_log_ctx *ctx, struct inode *inode);
+void btrfs_init_log_ctx_scratch_eb(struct btrfs_log_ctx *ctx);
+void btrfs_release_log_ctx_extents(struct btrfs_log_ctx *ctx);
 
 static inline void btrfs_set_log_full_commit(struct btrfs_trans_handle *trans)
 {
-- 
2.42.1

[PATCH 08/10] btrfs: simplify conditions in btrfs_free_chunk_map()

[David Sterba]

The helper is simple enough for inlining, we can further simplify it by
removing the check for map pointer validity. After this patch all
callers always pass a valid pointer.

The changes to achieve this:

- in verify_one_dev_extent() return and don't jump to the out label
  that could potentially pass a NULL pointer to btrfs_free_chunk_map

- in btrfs_load_block_group_zone_info() add a label that specifically
  clears the map and does not go through label out that could encounter
  a NULL pointer in cache->physical_map

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/volumes.c | 3 +--
 fs/btrfs/volumes.h | 2 +-
 fs/btrfs/zoned.c   | 3 ++-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 752144a31d79..55b91807aba4 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -7979,8 +7979,7 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
 		btrfs_err(fs_info,
 "dev extent physical offset %llu on devid %llu doesn't have corresponding chunk",
 			  physical_offset, devid);
-		ret = -EUCLEAN;
-		goto out;
+		return -EUCLEAN;
 	}
 
 	stripe_len = btrfs_calc_stripe_length(map);
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 21d4de0e3f1f..ce1aa7684c74 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -566,7 +566,7 @@ struct btrfs_chunk_map {
 
 static inline void btrfs_free_chunk_map(struct btrfs_chunk_map *map)
 {
-	if (map && refcount_dec_and_test(&map->refs)) {
+	if (refcount_dec_and_test(&map->refs)) {
 		ASSERT(RB_EMPTY_NODE(&map->rb_node));
 		kfree(map);
 	}
diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index 3317bebfca95..b9346ca82c47 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -1564,7 +1564,7 @@ int btrfs_load_block_group_zone_info(struct btrfs_block_group *cache, bool new)
 	cache->physical_map = btrfs_clone_chunk_map(map, GFP_NOFS);
 	if (!cache->physical_map) {
 		ret = -ENOMEM;
-		goto out;
+		goto out_free_map;
 	}
 
 	zone_info = kcalloc(map->num_stripes, sizeof(*zone_info), GFP_NOFS);
@@ -1677,6 +1677,7 @@ int btrfs_load_block_group_zone_info(struct btrfs_block_group *cache, bool new)
 	}
 	bitmap_free(active);
 	kfree(zone_info);
+out_free_map:
 	btrfs_free_chunk_map(map);
 
 	return ret;
-- 
2.42.1

[PATCH 09/10] btrfs: open code trivial btrfs_lru_cache_size()

[David Sterba]

The helper is really trivial, reading a cache size can be done directly.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/lru_cache.h | 5 -----
 fs/btrfs/send.c      | 7 +++----
 2 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/lru_cache.h b/fs/btrfs/lru_cache.h
index 390a12b61fd2..e32906ab6faa 100644
--- a/fs/btrfs/lru_cache.h
+++ b/fs/btrfs/lru_cache.h
@@ -52,11 +52,6 @@ struct btrfs_lru_cache {
 #define btrfs_lru_cache_for_each_entry_safe(cache, entry, tmp)		\
 	list_for_each_entry_safe_reverse((entry), (tmp), &(cache)->lru_list, lru_list)
 
-static inline unsigned int btrfs_lru_cache_size(const struct btrfs_lru_cache *cache)
-{
-	return cache->size;
-}
-
 static inline struct btrfs_lru_cache_entry *btrfs_lru_cache_lru_entry(
 					      struct btrfs_lru_cache *cache)
 {
diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index e96d511f9dd9..e6a9e976916c 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -1418,7 +1418,7 @@ static bool lookup_backref_cache(u64 leaf_bytenr, void *ctx,
 	struct btrfs_lru_cache_entry *raw_entry;
 	struct backref_cache_entry *entry;
 
-	if (btrfs_lru_cache_size(&sctx->backref_cache) == 0)
+	if (sctx->backref_cache.size == 0)
 		return false;
 
 	/*
@@ -1516,7 +1516,7 @@ static void store_backref_cache(u64 leaf_bytenr, const struct ulist *root_ids,
 	 * transaction handle or holding fs_info->commit_root_sem, so no need
 	 * to take any lock here.
 	 */
-	if (btrfs_lru_cache_size(&sctx->backref_cache) == 1)
+	if (sctx->backref_cache.size == 1)
 		sctx->backref_cache_last_reloc_trans = fs_info->last_reloc_trans;
 }
 
@@ -2821,8 +2821,7 @@ static int cache_dir_utimes(struct send_ctx *sctx, u64 dir, u64 gen)
 
 static int trim_dir_utimes_cache(struct send_ctx *sctx)
 {
-	while (btrfs_lru_cache_size(&sctx->dir_utimes_cache) >
-	       SEND_MAX_DIR_UTIMES_CACHE_SIZE) {
+	while (sctx->dir_utimes_cache.size > SEND_MAX_DIR_UTIMES_CACHE_SIZE) {
 		struct btrfs_lru_cache_entry *lru;
 		int ret;
 
-- 
2.42.1

[PATCH 10/10] btrfs: uninline some static inline helpers from delayed-ref.h

[David Sterba]

The helpers are doing an initialization or release work, none of which
is performance critical that it would require a static inline, so move
them to the .c file.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/delayed-ref.c | 65 ++++++++++++++++++++++++++++++++++++++
 fs/btrfs/delayed-ref.h | 72 ++++--------------------------------------
 2 files changed, 72 insertions(+), 65 deletions(-)

diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
index 891ea2fa263c..7cfaec5dc806 100644
--- a/fs/btrfs/delayed-ref.c
+++ b/fs/btrfs/delayed-ref.c
@@ -1004,6 +1004,52 @@ static void init_delayed_ref_common(struct btrfs_fs_info *fs_info,
 	INIT_LIST_HEAD(&ref->add_list);
 }
 
+void btrfs_init_generic_ref(struct btrfs_ref *generic_ref, int action, u64 bytenr,
+			    u64 len, u64 parent, u64 owning_root)
+{
+	generic_ref->action = action;
+	generic_ref->bytenr = bytenr;
+	generic_ref->len = len;
+	generic_ref->parent = parent;
+	generic_ref->owning_root = owning_root;
+}
+
+void btrfs_init_tree_ref(struct btrfs_ref *generic_ref, int level, u64 root,
+			 u64 mod_root, bool skip_qgroup)
+{
+#ifdef CONFIG_BTRFS_FS_REF_VERIFY
+	/* If @real_root not set, use @root as fallback */
+	generic_ref->real_root = mod_root ?: root;
+#endif
+	generic_ref->tree_ref.level = level;
+	generic_ref->tree_ref.ref_root = root;
+	generic_ref->type = BTRFS_REF_METADATA;
+	if (skip_qgroup || !(is_fstree(root) &&
+			     (!mod_root || is_fstree(mod_root))))
+		generic_ref->skip_qgroup = true;
+	else
+		generic_ref->skip_qgroup = false;
+
+}
+
+void btrfs_init_data_ref(struct btrfs_ref *generic_ref, u64 ref_root, u64 ino,
+			 u64 offset, u64 mod_root, bool skip_qgroup)
+{
+#ifdef CONFIG_BTRFS_FS_REF_VERIFY
+	/* If @real_root not set, use @root as fallback */
+	generic_ref->real_root = mod_root ?: ref_root;
+#endif
+	generic_ref->data_ref.ref_root = ref_root;
+	generic_ref->data_ref.ino = ino;
+	generic_ref->data_ref.offset = offset;
+	generic_ref->type = BTRFS_REF_DATA;
+	if (skip_qgroup || !(is_fstree(ref_root) &&
+			     (!mod_root || is_fstree(mod_root))))
+		generic_ref->skip_qgroup = true;
+	else
+		generic_ref->skip_qgroup = false;
+}
+
 /*
  * add a delayed tree ref.  This does all of the accounting required
  * to make sure the delayed ref is eventually processed before this
@@ -1220,6 +1266,25 @@ int btrfs_add_delayed_extent_op(struct btrfs_trans_handle *trans,
 	return 0;
 }
 
+void btrfs_put_delayed_ref(struct btrfs_delayed_ref_node *ref)
+{
+	if (refcount_dec_and_test(&ref->refs)) {
+		WARN_ON(!RB_EMPTY_NODE(&ref->ref_node));
+		switch (ref->type) {
+		case BTRFS_TREE_BLOCK_REF_KEY:
+		case BTRFS_SHARED_BLOCK_REF_KEY:
+			kmem_cache_free(btrfs_delayed_tree_ref_cachep, ref);
+			break;
+		case BTRFS_EXTENT_DATA_REF_KEY:
+		case BTRFS_SHARED_DATA_REF_KEY:
+			kmem_cache_free(btrfs_delayed_data_ref_cachep, ref);
+			break;
+		default:
+			BUG();
+		}
+	}
+}
+
 /*
  * This does a simple search for the head node for a given extent.  Returns the
  * head node if found, or NULL if not.
diff --git a/fs/btrfs/delayed-ref.h b/fs/btrfs/delayed-ref.h
index cbd632f145f0..b291147cb8ab 100644
--- a/fs/btrfs/delayed-ref.h
+++ b/fs/btrfs/delayed-ref.h
@@ -318,53 +318,12 @@ static inline u64 btrfs_calc_delayed_ref_csum_bytes(const struct btrfs_fs_info *
 	return btrfs_calc_metadata_size(fs_info, num_csum_items);
 }
 
-static inline void btrfs_init_generic_ref(struct btrfs_ref *generic_ref,
-					  int action, u64 bytenr, u64 len,
-					  u64 parent, u64 owning_root)
-{
-	generic_ref->action = action;
-	generic_ref->bytenr = bytenr;
-	generic_ref->len = len;
-	generic_ref->parent = parent;
-	generic_ref->owning_root = owning_root;
-}
-
-static inline void btrfs_init_tree_ref(struct btrfs_ref *generic_ref, int level,
-				       u64 root, u64 mod_root, bool skip_qgroup)
-{
-#ifdef CONFIG_BTRFS_FS_REF_VERIFY
-	/* If @real_root not set, use @root as fallback */
-	generic_ref->real_root = mod_root ?: root;
-#endif
-	generic_ref->tree_ref.level = level;
-	generic_ref->tree_ref.ref_root = root;
-	generic_ref->type = BTRFS_REF_METADATA;
-	if (skip_qgroup || !(is_fstree(root) &&
-			     (!mod_root || is_fstree(mod_root))))
-		generic_ref->skip_qgroup = true;
-	else
-		generic_ref->skip_qgroup = false;
-
-}
-
-static inline void btrfs_init_data_ref(struct btrfs_ref *generic_ref,
-				u64 ref_root, u64 ino, u64 offset, u64 mod_root,
-				bool skip_qgroup)
-{
-#ifdef CONFIG_BTRFS_FS_REF_VERIFY
-	/* If @real_root not set, use @root as fallback */
-	generic_ref->real_root = mod_root ?: ref_root;
-#endif
-	generic_ref->data_ref.ref_root = ref_root;
-	generic_ref->data_ref.ino = ino;
-	generic_ref->data_ref.offset = offset;
-	generic_ref->type = BTRFS_REF_DATA;
-	if (skip_qgroup || !(is_fstree(ref_root) &&
-			     (!mod_root || is_fstree(mod_root))))
-		generic_ref->skip_qgroup = true;
-	else
-		generic_ref->skip_qgroup = false;
-}
+void btrfs_init_generic_ref(struct btrfs_ref *generic_ref, int action, u64 bytenr,
+			    u64 len, u64 parent, u64 owning_root);
+void btrfs_init_tree_ref(struct btrfs_ref *generic_ref, int level, u64 root,
+			 u64 mod_root, bool skip_qgroup);
+void btrfs_init_data_ref(struct btrfs_ref *generic_ref, u64 ref_root, u64 ino,
+			 u64 offset, u64 mod_root, bool skip_qgroup);
 
 static inline struct btrfs_delayed_extent_op *
 btrfs_alloc_delayed_extent_op(void)
@@ -379,24 +338,7 @@ btrfs_free_delayed_extent_op(struct btrfs_delayed_extent_op *op)
 		kmem_cache_free(btrfs_delayed_extent_op_cachep, op);
 }
 
-static inline void btrfs_put_delayed_ref(struct btrfs_delayed_ref_node *ref)
-{
-	if (refcount_dec_and_test(&ref->refs)) {
-		WARN_ON(!RB_EMPTY_NODE(&ref->ref_node));
-		switch (ref->type) {
-		case BTRFS_TREE_BLOCK_REF_KEY:
-		case BTRFS_SHARED_BLOCK_REF_KEY:
-			kmem_cache_free(btrfs_delayed_tree_ref_cachep, ref);
-			break;
-		case BTRFS_EXTENT_DATA_REF_KEY:
-		case BTRFS_SHARED_DATA_REF_KEY:
-			kmem_cache_free(btrfs_delayed_data_ref_cachep, ref);
-			break;
-		default:
-			BUG();
-		}
-	}
-}
+void btrfs_put_delayed_ref(struct btrfs_delayed_ref_node *ref);
 
 static inline u64 btrfs_ref_head_to_space_flags(
 				struct btrfs_delayed_ref_head *head_ref)
-- 
2.42.1

Re: [PATCH 08/10] btrfs: simplify conditions in btrfs_free_chunk_map()

[Filipe Manana]

On Mon, Feb 19, 2024 at 11:14 AM David Sterba <dsterba@suse.com> wrote:
>
> The helper is simple enough for inlining, we can further simplify it by
> removing the check for map pointer validity. After this patch all
> callers always pass a valid pointer.

So by making btrfs_free_chunk_map() to not ignore a NULL pointer, we
are adding rather
surprising behaviour and inconsistency.

Most free functions ignore a NULL pointer, take the example of the
kfree() family and even free() family in the standard C library,
as well as most of the free functions we have in btrfs as well, which
are modeled on that common pattern.

Ignoring NULL makes error handling simpler, by not having the need to
take special care to call the free function with a non-NULL pointer.

Besides that, this change doesn't seem to improve anything.

Thanks.

>
> The changes to achieve this:
>
> - in verify_one_dev_extent() return and don't jump to the out label
>   that could potentially pass a NULL pointer to btrfs_free_chunk_map
>
> - in btrfs_load_block_group_zone_info() add a label that specifically
>   clears the map and does not go through label out that could encounter
>   a NULL pointer in cache->physical_map
>
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/volumes.c | 3 +--
>  fs/btrfs/volumes.h | 2 +-
>  fs/btrfs/zoned.c   | 3 ++-
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 752144a31d79..55b91807aba4 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -7979,8 +7979,7 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
>                 btrfs_err(fs_info,
>  "dev extent physical offset %llu on devid %llu doesn't have corresponding chunk",
>                           physical_offset, devid);
> -               ret = -EUCLEAN;
> -               goto out;
> +               return -EUCLEAN;
>         }
>
>         stripe_len = btrfs_calc_stripe_length(map);
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 21d4de0e3f1f..ce1aa7684c74 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -566,7 +566,7 @@ struct btrfs_chunk_map {
>
>  static inline void btrfs_free_chunk_map(struct btrfs_chunk_map *map)
>  {
> -       if (map && refcount_dec_and_test(&map->refs)) {
> +       if (refcount_dec_and_test(&map->refs)) {
>                 ASSERT(RB_EMPTY_NODE(&map->rb_node));
>                 kfree(map);
>         }
> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
> index 3317bebfca95..b9346ca82c47 100644
> --- a/fs/btrfs/zoned.c
> +++ b/fs/btrfs/zoned.c
> @@ -1564,7 +1564,7 @@ int btrfs_load_block_group_zone_info(struct btrfs_block_group *cache, bool new)
>         cache->physical_map = btrfs_clone_chunk_map(map, GFP_NOFS);
>         if (!cache->physical_map) {
>                 ret = -ENOMEM;
> -               goto out;
> +               goto out_free_map;
>         }
>
>         zone_info = kcalloc(map->num_stripes, sizeof(*zone_info), GFP_NOFS);
> @@ -1677,6 +1677,7 @@ int btrfs_load_block_group_zone_info(struct btrfs_block_group *cache, bool new)
>         }
>         bitmap_free(active);
>         kfree(zone_info);
> +out_free_map:
>         btrfs_free_chunk_map(map);
>
>         return ret;
> --
> 2.42.1
>
>

Re: [PATCH 08/10] btrfs: simplify conditions in btrfs_free_chunk_map()

[David Sterba]

On Mon, Feb 19, 2024 at 12:27:44PM +0000, Filipe Manana wrote:
> On Mon, Feb 19, 2024 at 11:14 AM David Sterba <dsterba@suse.com> wrote:
> >
> > The helper is simple enough for inlining, we can further simplify it by
> > removing the check for map pointer validity. After this patch all
> > callers always pass a valid pointer.
> 
> So by making btrfs_free_chunk_map() to not ignore a NULL pointer, we
> are adding rather
> surprising behaviour and inconsistency.
> 
> Most free functions ignore a NULL pointer, take the example of the
> kfree() family and even free() family in the standard C library,
> as well as most of the free functions we have in btrfs as well, which
> are modeled on that common pattern.
> 
> Ignoring NULL makes error handling simpler, by not having the need to
> take special care to call the free function with a non-NULL pointer.
> 
> Besides that, this change doesn't seem to improve anything.

The goal is to reduce a static inline function size as its code is
duplicated many times (in this case 36x) so anything that does not need
to be there is removed. The improvement is smaller code size, one less
condition to check.

OTOH that it's a freeing function and would not accept a NULL pointer is
indeed inconsistent and potentially problematic so I'll drop the patch.

Re: [PATCH 08/10] btrfs: simplify conditions in btrfs_free_chunk_map()

[kernel test robot]

Hello,

kernel test robot noticed "dmesg.BUG:KASAN:null-ptr-deref_in_btrfs_put_block_group" on:

commit: 1511810d056bc04fc0aed7a2b20d09b170da3e86 ("[PATCH 08/10] btrfs: simplify conditions in btrfs_free_chunk_map()")
url: https://github.com/intel-lab-lkp/linux/commits/David-Sterba/btrfs-move-balance-args-conversion-helpers-to-volumes-c/20240219-191714
base: https://git.kernel.org/cgit/linux/kernel/git/kdave/linux.git for-next
patch link: https://lore.kernel.org/all/cd9ae501762221ffca5408ffb59f1a3b990de14e.1708339010.git.dsterba@suse.com/
patch subject: [PATCH 08/10] btrfs: simplify conditions in btrfs_free_chunk_map()

in testcase: xfstests
version: xfstests-x86_64-c46ca4d1-1_20240205
with following parameters:

	disk: 4HDD
	fs: btrfs
	test: generic-group-34



compiler: gcc-12
test machine: 4 threads Intel(R) Core(TM) i5-6500 CPU @ 3.20GHz (Skylake) with 32G memory

(please refer to attached dmesg/kmsg for entire log/backtrace)



If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202402261652.bcd6d27d-lkp@intel.com



[   55.292606][ T1454] BTRFS info (device sda1): last unmount of filesystem b71ba2d6-b44f-48b2-b855-8d320c026d64
[   55.376758][ T1454] ==================================================================
[   55.384644][ T1454] BUG: KASAN: null-ptr-deref in btrfs_put_block_group+0x15a/0x2c0 [btrfs]
[   55.393037][ T1454] Write of size 4 at addr 000000000000001c by task umount/1454
[   55.400400][ T1454] 
[   55.402575][ T1454] CPU: 1 PID: 1454 Comm: umount Tainted: G S        I        6.8.0-rc4-00127-g1511810d056b #1
[   55.412614][ T1454] Hardware name: Dell Inc. OptiPlex 7040/0Y7WYT, BIOS 1.1.1 10/07/2015
[   55.420665][ T1454] Call Trace:
[   55.423806][ T1454]  <TASK>
[   55.426586][ T1454]  dump_stack_lvl+0x36/0x50
[   55.430927][ T1454]  kasan_report+0xc7/0x100
[   55.435178][ T1454]  ? btrfs_put_block_group+0x15a/0x2c0 [btrfs]
[   55.441237][ T1454]  kasan_check_range+0xfc/0x1a0
[   55.445928][ T1454]  btrfs_put_block_group+0x15a/0x2c0 [btrfs]
[   55.451831][ T1454]  btrfs_free_block_groups+0x7fd/0x10f0 [btrfs]
[   55.457992][ T1454]  ? free_root_pointers+0x759/0xa10 [btrfs]
[   55.463785][ T1454]  close_ctree+0x87c/0xcf0 [btrfs]
[   55.468842][ T1454]  ? _btrfs_printk+0x1e8/0x430 [btrfs]
[   55.474214][ T1454]  ? preempt_notifier_dec+0x20/0x20
[   55.479245][ T1454]  ? btrfs_cleanup_transaction+0xae0/0xae0 [btrfs]
[   55.486236][ T1454]  ? fsnotify_sb_delete+0x2ab/0x420
[   55.491265][ T1454]  ? fsnotify+0x14d0/0x1550
[   55.495604][ T1454]  ? dispose_list+0x1b0/0x1b0
[   55.500118][ T1454]  generic_shutdown_super+0x13f/0x370
[   55.505320][ T1454]  kill_anon_super+0x3a/0x90
[   55.509745][ T1454]  btrfs_kill_super+0x3b/0x50 [btrfs]
[   55.515033][ T1454]  deactivate_locked_super+0xa2/0x190
[   55.520235][ T1454]  cleanup_mnt+0x1e5/0x3f0
[   55.524487][ T1454]  task_work_run+0x119/0x200
[   55.528911][ T1454]  ? task_work_cancel+0x20/0x20
[   55.533592][ T1454]  ? __x64_sys_umount+0x119/0x140
[   55.538447][ T1454]  ? __ia32_sys_oldumount+0xf0/0xf0
[   55.543475][ T1454]  syscall_exit_to_user_mode+0x1fa/0x200
[   55.548936][ T1454]  do_syscall_64+0x6f/0x170
[   55.553272][ T1454]  entry_SYSCALL_64_after_hwframe+0x63/0x6b
[   55.558994][ T1454] RIP: 0033:0x7fcb7e405a67
[   55.563244][ T1454] Code: 24 0d 00 f7 d8 64 89 01 48 83 c8 ff c3 66 0f 1f 44 00 00 31 f6 e9 09 00 00 00 66 0f 1f 84 00 00 00 00 00 b8 a6 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d f9 23 0d 00 f7 d8 64 89 01 48
[   55.582617][ T1454] RSP: 002b:00007ffd2ff1b1d8 EFLAGS: 00000246 ORIG_RAX: 00000000000000a6
[   55.590846][ T1454] RAX: 0000000000000000 RBX: 00007fcb7e53a264 RCX: 00007fcb7e405a67
[   55.598639][ T1454] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000564579779b90
[   55.606431][ T1454] RBP: 0000564579779960 R08: 0000000000000000 R09: 00007ffd2ff19f80
[   55.614225][ T1454] R10: 00007fcb7e498fc0 R11: 0000000000000246 R12: 0000000000000000
[   55.622019][ T1454] R13: 0000564579779b90 R14: 0000564579779a70 R15: 0000000000000000
[   55.629829][ T1454]  </TASK>
[   55.632697][ T1454] ==================================================================
[   55.640659][ T1454] Disabling lock debugging due to kernel taint
[   55.646643][ T1454] BUG: kernel NULL pointer dereference, address: 000000000000001c
[   55.654266][ T1454] #PF: supervisor write access in kernel mode
[   55.660162][ T1454] #PF: error_code(0x0002) - not-present page
[   55.665971][ T1454] PGD 0 P4D 0 
[   55.669189][ T1454] Oops: 0002 [#1] PREEMPT SMP KASAN PTI
[   55.674565][ T1454] CPU: 1 PID: 1454 Comm: umount Tainted: G S  B     I        6.8.0-rc4-00127-g1511810d056b #1
[   55.684619][ T1454] Hardware name: Dell Inc. OptiPlex 7040/0Y7WYT, BIOS 1.1.1 10/07/2015
[   55.692684][ T1454] RIP: 0010:btrfs_put_block_group+0x15f/0x2c0 [btrfs]
[   55.699363][ T1454] Code: c1 ea 03 80 3c 02 00 0f 85 31 01 00 00 48 8b ab 28 02 00 00 be 04 00 00 00 4c 8d 65 1c 4c 89 e7 e8 86 cc e6 bf b8 ff ff ff ff <f0> 0f c1 45 1c 83 f8 01 74 7e 85 c0 0f 8e 9b 00 00 00 48 89 df 5b
[   55.718754][ T1454] RSP: 0018:ffffc90001317b78 EFLAGS: 00010246
[   55.724647][ T1454] RAX: 00000000ffffffff RBX: ffff8881eae12000 RCX: 0000000000000001
[   55.732455][ T1454] RDX: fffffbfff0c59f01 RSI: 0000000000000008 RDI: ffffffff862cf800
[   55.740260][ T1454] RBP: 0000000000000000 R08: 0000000000000001 R09: fffffbfff0c59f00
[   55.748054][ T1454] R10: ffffffff862cf807 R11: 0000000000000001 R12: 000000000000001c
[   55.755848][ T1454] R13: ffff88818c5da090 R14: ffff8881eae12100 R15: ffff8881eae120d8
[   55.763642][ T1454] FS:  00007fcb7e1c8840(0000) GS:ffff8887ee280000(0000) knlGS:0000000000000000
[   55.772406][ T1454] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   55.778826][ T1454] CR2: 000000000000001c CR3: 00000001e5a68006 CR4: 00000000003706f0
[   55.786620][ T1454] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   55.794416][ T1454] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[   55.802223][ T1454] Call Trace:
[   55.805352][ T1454]  <TASK>
[   55.808132][ T1454]  ? __die+0x23/0x70
[   55.811871][ T1454]  ? page_fault_oops+0x136/0x240
[   55.816655][ T1454]  ? show_fault_oops+0x780/0x780
[   55.821426][ T1454]  ? exc_page_fault+0x5c/0xc0
[   55.825938][ T1454]  ? asm_exc_page_fault+0x26/0x30
[   55.830797][ T1454]  ? btrfs_put_block_group+0x15f/0x2c0 [btrfs]
[   55.836867][ T1454]  ? btrfs_put_block_group+0x15a/0x2c0 [btrfs]
[   55.842937][ T1454]  btrfs_free_block_groups+0x7fd/0x10f0 [btrfs]
[   55.849077][ T1454]  ? free_root_pointers+0x759/0xa10 [btrfs]
[   55.854884][ T1454]  close_ctree+0x87c/0xcf0 [btrfs]
[   55.859891][ T1454]  ? _btrfs_printk+0x1e8/0x430 [btrfs]
[   55.865252][ T1454]  ? preempt_notifier_dec+0x20/0x20
[   55.870283][ T1454]  ? btrfs_cleanup_transaction+0xae0/0xae0 [btrfs]
[   55.877277][ T1454]  ? fsnotify_sb_delete+0x2ab/0x420
[   55.882308][ T1454]  ? fsnotify+0x14d0/0x1550
[   55.886645][ T1454]  ? dispose_list+0x1b0/0x1b0
[   55.891156][ T1454]  generic_shutdown_super+0x13f/0x370
[   55.896358][ T1454]  kill_anon_super+0x3a/0x90
[   55.900785][ T1454]  btrfs_kill_super+0x3b/0x50 [btrfs]
[   55.906047][ T1454]  deactivate_locked_super+0xa2/0x190
[   55.911249][ T1454]  cleanup_mnt+0x1e5/0x3f0
[   55.915516][ T1454]  task_work_run+0x119/0x200
[   55.919957][ T1454]  ? task_work_cancel+0x20/0x20
[   55.924651][ T1454]  ? __x64_sys_umount+0x119/0x140
[   55.929522][ T1454]  ? __ia32_sys_oldumount+0xf0/0xf0
[   55.934575][ T1454]  syscall_exit_to_user_mode+0x1fa/0x200
[   55.940044][ T1454]  do_syscall_64+0x6f/0x170
[   55.944391][ T1454]  entry_SYSCALL_64_after_hwframe+0x63/0x6b
[   55.950115][ T1454] RIP: 0033:0x7fcb7e405a67
[   55.954369][ T1454] Code: 24 0d 00 f7 d8 64 89 01 48 83 c8 ff c3 66 0f 1f 44 00 00 31 f6 e9 09 00 00 00 66 0f 1f 84 00 00 00 00 00 b8 a6 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d f9 23 0d 00 f7 d8 64 89 01 48
[   55.973758][ T1454] RSP: 002b:00007ffd2ff1b1d8 EFLAGS: 00000246 ORIG_RAX: 00000000000000a6
[   55.981985][ T1454] RAX: 0000000000000000 RBX: 00007fcb7e53a264 RCX: 00007fcb7e405a67
[   55.989779][ T1454] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000564579779b90
[   55.997574][ T1454] RBP: 0000564579779960 R08: 0000000000000000 R09: 00007ffd2ff19f80
[   56.005367][ T1454] R10: 00007fcb7e498fc0 R11: 0000000000000246 R12: 0000000000000000
[   56.013160][ T1454] R13: 0000564579779b90 R14: 0000564579779a70 R15: 0000000000000000
[   56.020959][ T1454]  </TASK>
[   56.023826][ T1454] Modules linked in: dm_mod btrfs blake2b_generic xor raid6_pq zstd_compress intel_rapl_msr libcrc32c intel_rapl_common x86_pkg_temp_thermal intel_powerclamp coretemp kvm_intel sd_mod t10_pi crc64_rocksoft_generic kvm crc64_rocksoft crc64 irqbypass crct10dif_pclmul sg crc32_pclmul crc32c_intel ipmi_devintf ipmi_msghandler ghash_clmulni_intel sha512_ssse3 i915 mei_wdt rapl ahci wmi_bmof intel_cstate drm_buddy intel_gtt drm_display_helper libahci intel_uncore ttm libata mei_me mei drm_kms_helper intel_pch_thermal video acpi_pad wmi drm fuse ip_tables
[   56.073766][ T1454] CR2: 000000000000001c
[   56.077761][ T1454] ---[ end trace 0000000000000000 ]---
[   56.083048][ T1454] RIP: 0010:btrfs_put_block_group+0x15f/0x2c0 [btrfs]
[   56.089714][ T1454] Code: c1 ea 03 80 3c 02 00 0f 85 31 01 00 00 48 8b ab 28 02 00 00 be 04 00 00 00 4c 8d 65 1c 4c 89 e7 e8 86 cc e6 bf b8 ff ff ff ff <f0> 0f c1 45 1c 83 f8 01 74 7e 85 c0 0f 8e 9b 00 00 00 48 89 df 5b
[   56.109089][ T1454] RSP: 0018:ffffc90001317b78 EFLAGS: 00010246
[   56.114983][ T1454] RAX: 00000000ffffffff RBX: ffff8881eae12000 RCX: 0000000000000001
[   56.122778][ T1454] RDX: fffffbfff0c59f01 RSI: 0000000000000008 RDI: ffffffff862cf800
[   56.130571][ T1454] RBP: 0000000000000000 R08: 0000000000000001 R09: fffffbfff0c59f00
[   56.138364][ T1454] R10: ffffffff862cf807 R11: 0000000000000001 R12: 000000000000001c
[   56.146158][ T1454] R13: ffff88818c5da090 R14: ffff8881eae12100 R15: ffff8881eae120d8
[   56.153955][ T1454] FS:  00007fcb7e1c8840(0000) GS:ffff8887ee280000(0000) knlGS:0000000000000000
[   56.162715][ T1454] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   56.169149][ T1454] CR2: 000000000000001c CR3: 00000001e5a68006 CR4: 00000000003706f0
[   56.175890][  T271] result_service: raw_upload, RESULT_MNT: /internal-lkp-server/result, RESULT_ROOT: /internal-lkp-server/result/xfstests/4HDD-btrfs-generic-group-34/lkp-skl-d02/debian-11.1-x86_64-20220510.cgz/x86_64-rhel-8.3-func/gcc-12/1511810d056bc04fc0aed7a2b20d09b170da3e86/3, TMP_RESULT_ROOT: /tmp/lkp/result
[   56.176952][ T1454] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   56.176953][ T1454] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[   56.176955][ T1454] Kernel panic - not syncing: Fatal exception
[   56.204637][ T1454] Kernel Offset: disabled


The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20240226/202402261652.bcd6d27d-lkp@intel.com



-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki