[PATCH v2 0/6] reduce boilerplate code within btrfs

[PATCH v2 0/6] reduce boilerplate code within btrfs

[Roger L. Beckermeyer III]

The goal of this patch series is to reduce boilerplate code
within btrfs. To accomplish this rb_find_add_cached() was added
to linux/include/rbtree.h. Any replaceable functions were then 
replaced within btrfs.

changelog: 
updated if() statements to utilize newer error checking
resolved lock error on 0002
edited title of patches to utilize update instead of edit
added Acked-by from Peter Zijlstra to 0001
eliminated extra variables throughout the patch series

Roger L. Beckermeyer III (6):
  rbtree: add rb_find_add_cached() to rbtree.h
  btrfs: update btrfs_add_block_group_cache() to use rb helper
  btrfs: update prelim_ref_insert() to use rb helpers
  btrfs: update __btrfs_add_delayed_item() to use rb helper
  btrfs: update btrfs_add_chunk_map() to use rb helpers
  btrfs: update tree_insert() to use rb helpers

 fs/btrfs/backref.c       | 71 ++++++++++++++++++++--------------------
 fs/btrfs/block-group.c   | 41 ++++++++++-------------
 fs/btrfs/delayed-inode.c | 40 +++++++++-------------
 fs/btrfs/delayed-ref.c   | 39 +++++++++-------------
 fs/btrfs/volumes.c       | 39 ++++++++++------------
 include/linux/rbtree.h   | 37 +++++++++++++++++++++
 6 files changed, 141 insertions(+), 126 deletions(-)

-- 
2.45.2

[PATCH v2 1/6] rbtree: add rb_find_add_cached() to rbtree.h

[Roger L. Beckermeyer III]

Adds rb_find_add_cached() as a helper function for use with
red-black trees. Used in btrfs to reduce boilerplate code.

Suggested-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Roger L. Beckermeyer III <beckerlee3@gmail.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/rbtree.h | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/include/linux/rbtree.h b/include/linux/rbtree.h
index 7c173aa64e1e..0d4444c0cfb3 100644
--- a/include/linux/rbtree.h
+++ b/include/linux/rbtree.h
@@ -210,6 +210,43 @@ rb_add(struct rb_node *node, struct rb_root *tree,
 	rb_insert_color(node, tree);
 }
 
+/**
+ * rb_find_add_cached() - find equivalent @node in @tree, or add @node
+ * @node: node to look-for / insert
+ * @tree: tree to search / modify
+ * @cmp: operator defining the node order
+ *
+ * Returns the rb_node matching @node, or NULL when no match is found and @node
+ * is inserted.
+ */
+static __always_inline struct rb_node *
+rb_find_add_cached(struct rb_node *node, struct rb_root_cached *tree,
+	    int (*cmp)(struct rb_node *, const struct rb_node *))
+{
+	bool leftmost = true;
+	struct rb_node **link = &tree->rb_root.rb_node;
+	struct rb_node *parent = NULL;
+	int c;
+
+	while (*link) {
+		parent = *link;
+		c = cmp(node, parent);
+
+		if (c < 0) {
+			link = &parent->rb_left;
+		} else if (c > 0) {
+			link = &parent->rb_right;
+			leftmost = false;
+		} else {
+			return parent;
+		}
+	}
+
+	rb_link_node(node, parent, link);
+	rb_insert_color_cached(node, tree, leftmost);
+	return NULL;
+}
+
 /**
  * rb_find_add() - find equivalent @node in @tree, or add @node
  * @node: node to look-for / insert
-- 
2.45.2

[PATCH v2 2/6] btrfs: update btrfs_add_block_group_cache() to use rb helper

[Roger L. Beckermeyer III]

update fs/btrfs/block-group.c to use rb_find_add_cached(),
also implements btrfs_bg_start_cmp() for use with
rb_find_add_cached().

Suggested-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Roger L. Beckermeyer III <beckerlee3@gmail.com>
---
 fs/btrfs/block-group.c | 41 ++++++++++++++++++-----------------------
 1 file changed, 18 insertions(+), 23 deletions(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 5be029734cfa..a8d51023f7a4 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -173,40 +173,35 @@ void btrfs_put_block_group(struct btrfs_block_group *cache)
 	}
 }
 
+static int btrfs_bg_start_cmp(struct rb_node *new, const struct rb_node *exist)
+{
+	struct btrfs_block_group *cmp1 = rb_entry(new, struct btrfs_block_group, cache_node);
+	const struct btrfs_block_group *cmp2 = rb_entry(exist, struct btrfs_block_group, cache_node);
+
+	if (cmp1->start < cmp2->start)
+		return -1;
+	if (cmp1->start > cmp2->start)
+		return 1;
+	return 0;
+}
+
 /*
  * This adds the block group to the fs_info rb tree for the block group cache
  */
 static int btrfs_add_block_group_cache(struct btrfs_fs_info *info,
 				       struct btrfs_block_group *block_group)
 {
-	struct rb_node **p;
-	struct rb_node *parent = NULL;
-	struct btrfs_block_group *cache;
-	bool leftmost = true;
+	struct rb_node *exist;
 
 	ASSERT(block_group->length != 0);
 
 	write_lock(&info->block_group_cache_lock);
-	p = &info->block_group_cache_tree.rb_root.rb_node;
-
-	while (*p) {
-		parent = *p;
-		cache = rb_entry(parent, struct btrfs_block_group, cache_node);
-		if (block_group->start < cache->start) {
-			p = &(*p)->rb_left;
-		} else if (block_group->start > cache->start) {
-			p = &(*p)->rb_right;
-			leftmost = false;
-		} else {
-			write_unlock(&info->block_group_cache_lock);
-			return -EEXIST;
-		}
-	}
-
-	rb_link_node(&block_group->cache_node, parent, p);
-	rb_insert_color_cached(&block_group->cache_node,
-			       &info->block_group_cache_tree, leftmost);
 
+	exist = rb_find_add_cached(&block_group->cache_node,
+			&info->block_group_cache_tree, btrfs_bg_start_cmp);
+	if (exist)
+		write_unlock(&info->block_group_cache_lock);
+		return -EEXIST;
 	write_unlock(&info->block_group_cache_lock);
 
 	return 0;
-- 
2.45.2

[PATCH v2 3/6] btrfs: update prelim_ref_insert() to use rb helpers

[Roger L. Beckermeyer III]

update prelim_ref_insert() to use rb_find_add_cached()
adds prelim_ref_cmp for use with rb_find_add_cached()

Signed-off-by: Roger L. Beckermeyer III <beckerlee3@gmail.com>
---
 fs/btrfs/backref.c | 71 +++++++++++++++++++++++-----------------------
 1 file changed, 36 insertions(+), 35 deletions(-)

diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index 6d9f39c1d89c..1326872f172e 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -250,6 +250,17 @@ static int prelim_ref_compare(const struct prelim_ref *ref1,
 	return 0;
 }
 
+static int prelim_ref_cmp(struct rb_node *node, const struct rb_node *exist)
+{
+	int result;
+	struct prelim_ref *ref1 = rb_entry(node, struct prelim_ref, rbnode);
+	struct prelim_ref *ref2 = rb_entry(exist, struct prelim_ref, rbnode);
+
+	result = prelim_ref_compare(ref1, ref2);
+
+	return result;
+}
+
 static void update_share_count(struct share_check *sc, int oldcount,
 			       int newcount, const struct prelim_ref *newref)
 {
@@ -281,52 +292,42 @@ static void prelim_ref_insert(const struct btrfs_fs_info *fs_info,
 	struct rb_node **p;
 	struct rb_node *parent = NULL;
 	struct prelim_ref *ref;
-	int result;
-	bool leftmost = true;
+	struct rb_node *exist;
 
 	root = &preftree->root;
 	p = &root->rb_root.rb_node;
+	parent = *p;
+	ref = rb_entry(parent, struct prelim_ref, rbnode);
 
-	while (*p) {
-		parent = *p;
-		ref = rb_entry(parent, struct prelim_ref, rbnode);
-		result = prelim_ref_compare(ref, newref);
-		if (result < 0) {
-			p = &(*p)->rb_left;
-		} else if (result > 0) {
-			p = &(*p)->rb_right;
-			leftmost = false;
-		} else {
-			/* Identical refs, merge them and free @newref */
-			struct extent_inode_elem *eie = ref->inode_list;
+	exist = rb_find_add_cached(&newref->rbnode, root, prelim_ref_cmp);
+	if (exist) {
+		/* Identical refs, merge them and free @newref */
+		struct extent_inode_elem *eie = ref->inode_list;
 
-			while (eie && eie->next)
-				eie = eie->next;
+		while (eie && eie->next)
+			eie = eie->next;
 
-			if (!eie)
-				ref->inode_list = newref->inode_list;
-			else
-				eie->next = newref->inode_list;
-			trace_btrfs_prelim_ref_merge(fs_info, ref, newref,
-						     preftree->count);
-			/*
-			 * A delayed ref can have newref->count < 0.
-			 * The ref->count is updated to follow any
-			 * BTRFS_[ADD|DROP]_DELAYED_REF actions.
-			 */
-			update_share_count(sc, ref->count,
-					   ref->count + newref->count, newref);
-			ref->count += newref->count;
-			free_pref(newref);
-			return;
-		}
+		if (!eie)
+			ref->inode_list = newref->inode_list;
+		else
+			eie->next = newref->inode_list;
+		trace_btrfs_prelim_ref_merge(fs_info, ref, newref,
+							preftree->count);
+		/*
+		 * A delayed ref can have newref->count < 0.
+		 * The ref->count is updated to follow any
+		 * BTRFS_[ADD|DROP]_DELAYED_REF actions.
+		 */
+		update_share_count(sc, ref->count,
+					ref->count + newref->count, newref);
+		ref->count += newref->count;
+		free_pref(newref);
+		return;
 	}
 
 	update_share_count(sc, 0, newref->count, newref);
 	preftree->count++;
 	trace_btrfs_prelim_ref_insert(fs_info, newref, NULL, preftree->count);
-	rb_link_node(&newref->rbnode, parent, p);
-	rb_insert_color_cached(&newref->rbnode, root, leftmost);
 }
 
 /*
-- 
2.45.2

[PATCH v2 4/6] btrfs: update __btrfs_add_delayed_item() to use rb helper

[Roger L. Beckermeyer III]

update __btrfs_add_delayed_item() to use rb_find_add_cached()
add btrfs_delayed_item_cmp() to use with rb_find_add_cached()

Signed-off-by: Roger L. Beckermeyer III <beckerlee3@gmail.com>
---
 fs/btrfs/delayed-inode.c | 40 ++++++++++++++++------------------------
 1 file changed, 16 insertions(+), 24 deletions(-)

diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
index 508bdbae29a0..4bd02e566ab2 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -366,40 +366,32 @@ static struct btrfs_delayed_item *__btrfs_lookup_delayed_item(
 	return NULL;
 }
 
+static int btrfs_delayed_item_cmp(struct rb_node *rb_node, const struct rb_node *exist_node)
+{
+	struct btrfs_delayed_item *item = rb_entry(rb_node, struct btrfs_delayed_item, rb_node);
+	const struct btrfs_delayed_item *exist = rb_entry(exist_node, struct btrfs_delayed_item, rb_node);
+
+	if (item->index < exist->index)
+		return -1;
+	if (item->index > exist->index)
+		return 1;
+	return 0;
+}
+
 static int __btrfs_add_delayed_item(struct btrfs_delayed_node *delayed_node,
 				    struct btrfs_delayed_item *ins)
 {
-	struct rb_node **p, *node;
-	struct rb_node *parent_node = NULL;
 	struct rb_root_cached *root;
-	struct btrfs_delayed_item *item;
-	bool leftmost = true;
+	struct rb_node *exist;
 
 	if (ins->type == BTRFS_DELAYED_INSERTION_ITEM)
 		root = &delayed_node->ins_root;
 	else
 		root = &delayed_node->del_root;
 
-	p = &root->rb_root.rb_node;
-	node = &ins->rb_node;
-
-	while (*p) {
-		parent_node = *p;
-		item = rb_entry(parent_node, struct btrfs_delayed_item,
-				 rb_node);
-
-		if (item->index < ins->index) {
-			p = &(*p)->rb_right;
-			leftmost = false;
-		} else if (item->index > ins->index) {
-			p = &(*p)->rb_left;
-		} else {
-			return -EEXIST;
-		}
-	}
-
-	rb_link_node(node, parent_node, p);
-	rb_insert_color_cached(node, root, leftmost);
+	exist = rb_find_add_cached(&ins->rb_node, root, btrfs_delayed_item_cmp);
+	if (exist)
+		return -EEXIST;
 
 	if (ins->type == BTRFS_DELAYED_INSERTION_ITEM &&
 	    ins->index >= delayed_node->index_cnt)
-- 
2.45.2

[PATCH v2 5/6] btrfs: update btrfs_add_chunk_map() to use rb helpers

[Roger L. Beckermeyer III]

update btrfs_add_chunk_map() to use rb_find_add_cached(). add
btrfs_chunk_map_cmp to use with rb_find_add_cached().

Signed-off-by: Roger L. Beckermeyer III <beckerlee3@gmail.com>
---
 fs/btrfs/volumes.c | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 1cccaf9c2b0d..d51badec5520 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -5513,33 +5513,30 @@ void btrfs_remove_chunk_map(struct btrfs_fs_info *fs_info, struct btrfs_chunk_ma
 	btrfs_free_chunk_map(map);
 }
 
+static int btrfs_chunk_map_cmp(struct rb_node *rb_node, const struct rb_node *exist_node)
+{
+	struct btrfs_chunk_map *map = rb_entry(rb_node, struct btrfs_chunk_map, rb_node);
+	const struct btrfs_chunk_map *exist = rb_entry(exist_node, struct btrfs_chunk_map, rb_node);
+
+	if (map->start == exist->start)
+		return 0;
+	if (map->start < exist->start)
+		return -1;
+	return 1;
+}
+
 EXPORT_FOR_TESTS
 int btrfs_add_chunk_map(struct btrfs_fs_info *fs_info, struct btrfs_chunk_map *map)
 {
-	struct rb_node **p;
-	struct rb_node *parent = NULL;
-	bool leftmost = true;
+	struct rb_node *exist;
 
 	write_lock(&fs_info->mapping_tree_lock);
-	p = &fs_info->mapping_tree.rb_root.rb_node;
-	while (*p) {
-		struct btrfs_chunk_map *entry;
-
-		parent = *p;
-		entry = rb_entry(parent, struct btrfs_chunk_map, rb_node);
-
-		if (map->start < entry->start) {
-			p = &(*p)->rb_left;
-		} else if (map->start > entry->start) {
-			p = &(*p)->rb_right;
-			leftmost = false;
-		} else {
-			write_unlock(&fs_info->mapping_tree_lock);
-			return -EEXIST;
-		}
+	exist = rb_find_add_cached(&map->rb_node, &fs_info->mapping_tree, btrfs_chunk_map_cmp);
+
+	if (exist) {
+		write_unlock(&fs_info->mapping_tree_lock);
+		return -EEXIST;
 	}
-	rb_link_node(&map->rb_node, parent, p);
-	rb_insert_color_cached(&map->rb_node, &fs_info->mapping_tree, leftmost);
 	chunk_map_device_set_bits(map, CHUNK_ALLOCATED);
 	chunk_map_device_clear_bits(map, CHUNK_TRIMMED);
 	write_unlock(&fs_info->mapping_tree_lock);
-- 
2.45.2

[PATCH v2 6/6] btrfs: update tree_insert() to use rb helpers

[Roger L. Beckermeyer III]

update tree_insert() to use rb_find_add_cached().
add cmp_refs_node in rb_find_add_cached() to compare.

Signed-off-by: Roger L. Beckermeyer III <beckerlee3@gmail.com>
---
 fs/btrfs/delayed-ref.c | 39 ++++++++++++++++-----------------------
 1 file changed, 16 insertions(+), 23 deletions(-)

diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
index 30f7079fa28e..dcacce2c2096 100644
--- a/fs/btrfs/delayed-ref.c
+++ b/fs/btrfs/delayed-ref.c
@@ -317,34 +317,27 @@ static int comp_refs(struct btrfs_delayed_ref_node *ref1,
 	return 0;
 }
 
+static int cmp_refs_node(struct rb_node *node, const struct rb_node *node2)
+{
+	struct btrfs_delayed_ref_node *ref1;
+	struct btrfs_delayed_ref_node *ref2;
+	bool check_seq = true;
+
+	ref1 = rb_entry(node, struct btrfs_delayed_ref_node, ref_node);
+	ref2 = rb_entry(node2, struct btrfs_delayed_ref_node, ref_node);
+
+	return comp_refs(ref1, ref2, check_seq);
+}
+
 static struct btrfs_delayed_ref_node* tree_insert(struct rb_root_cached *root,
 		struct btrfs_delayed_ref_node *ins)
 {
-	struct rb_node **p = &root->rb_root.rb_node;
 	struct rb_node *node = &ins->ref_node;
-	struct rb_node *parent_node = NULL;
-	struct btrfs_delayed_ref_node *entry;
-	bool leftmost = true;
-
-	while (*p) {
-		int comp;
-
-		parent_node = *p;
-		entry = rb_entry(parent_node, struct btrfs_delayed_ref_node,
-				 ref_node);
-		comp = comp_refs(ins, entry, true);
-		if (comp < 0) {
-			p = &(*p)->rb_left;
-		} else if (comp > 0) {
-			p = &(*p)->rb_right;
-			leftmost = false;
-		} else {
-			return entry;
-		}
-	}
+	struct rb_node *exist;
 
-	rb_link_node(node, parent_node, p);
-	rb_insert_color_cached(node, root, leftmost);
+	exist = rb_find_add_cached(node, root, cmp_refs_node);
+	if (exist)
+		return rb_entry(exist, struct btrfs_delayed_ref_node, ref_node);
 	return NULL;
 }
 
-- 
2.45.2

Re: [PATCH v2 2/6] btrfs: update btrfs_add_block_group_cache() to use rb helper

[Qu Wenruo]

在 2024/12/16 01:56, Roger L. Beckermeyer III 写道:
> update fs/btrfs/block-group.c to use rb_find_add_cached(),
> also implements btrfs_bg_start_cmp() for use with
> rb_find_add_cached().
>
> Suggested-by: Josef Bacik <josef@toxicpanda.com>
> Signed-off-by: Roger L. Beckermeyer III <beckerlee3@gmail.com>
> ---
>   fs/btrfs/block-group.c | 41 ++++++++++++++++++-----------------------
>   1 file changed, 18 insertions(+), 23 deletions(-)
>
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index 5be029734cfa..a8d51023f7a4 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -173,40 +173,35 @@ void btrfs_put_block_group(struct btrfs_block_group *cache)
>   	}
>   }
>
> +static int btrfs_bg_start_cmp(struct rb_node *new, const struct rb_node *exist)
> +{
> +	struct btrfs_block_group *cmp1 = rb_entry(new, struct btrfs_block_group, cache_node);
> +	const struct btrfs_block_group *cmp2 = rb_entry(exist, struct btrfs_block_group, cache_node);
> +
> +	if (cmp1->start < cmp2->start)
> +		return -1;
> +	if (cmp1->start > cmp2->start)
> +		return 1;
> +	return 0;
> +}
> +
>   /*
>    * This adds the block group to the fs_info rb tree for the block group cache
>    */
>   static int btrfs_add_block_group_cache(struct btrfs_fs_info *info,
>   				       struct btrfs_block_group *block_group)
>   {
> -	struct rb_node **p;
> -	struct rb_node *parent = NULL;
> -	struct btrfs_block_group *cache;
> -	bool leftmost = true;
> +	struct rb_node *exist;
>
>   	ASSERT(block_group->length != 0);
>
>   	write_lock(&info->block_group_cache_lock);
> -	p = &info->block_group_cache_tree.rb_root.rb_node;
> -
> -	while (*p) {
> -		parent = *p;
> -		cache = rb_entry(parent, struct btrfs_block_group, cache_node);
> -		if (block_group->start < cache->start) {
> -			p = &(*p)->rb_left;
> -		} else if (block_group->start > cache->start) {
> -			p = &(*p)->rb_right;
> -			leftmost = false;
> -		} else {
> -			write_unlock(&info->block_group_cache_lock);
> -			return -EEXIST;
> -		}
> -	}
> -
> -	rb_link_node(&block_group->cache_node, parent, p);
> -	rb_insert_color_cached(&block_group->cache_node,
> -			       &info->block_group_cache_tree, leftmost);
>
> +	exist = rb_find_add_cached(&block_group->cache_node,
> +			&info->block_group_cache_tree, btrfs_bg_start_cmp);
> +	if (exist)
> +		write_unlock(&info->block_group_cache_lock);
> +		return -EEXIST;

IIRC latest GCC would already warn about such indent.
This will cause the function to always return -EEXIST.

Thanks,
Qu

>   	write_unlock(&info->block_group_cache_lock);
>
>   	return 0;

Re: [PATCH v2 2/6] btrfs: update btrfs_add_block_group_cache() to use rb helper

[Qu Wenruo]

在 2024/12/17 06:51, Lee Beckermeyer 写道:
>
>
> On Sun, Dec 15, 2024 at 4:23 PM Qu Wenruo <quwenruo.btrfs@gmx.com
> <mailto:quwenruo.btrfs@gmx.com>> wrote:
>
>
>
>     在 2024/12/16 01:56, Roger L. Beckermeyer III 写道:
>      > update fs/btrfs/block-group.c to use rb_find_add_cached(),
>      > also implements btrfs_bg_start_cmp() for use with
>      > rb_find_add_cached().
>      >
>      > Suggested-by: Josef Bacik <josef@toxicpanda.com
>     <mailto:josef@toxicpanda.com>>
>      > Signed-off-by: Roger L. Beckermeyer III <beckerlee3@gmail.com
>     <mailto:beckerlee3@gmail.com>>
>      > ---
>      >   fs/btrfs/block-group.c | 41 +++++++++++++++++
>     +-----------------------
>      >   1 file changed, 18 insertions(+), 23 deletions(-)
>      >
>      > diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
>      > index 5be029734cfa..a8d51023f7a4 100644
>      > --- a/fs/btrfs/block-group.c
>      > +++ b/fs/btrfs/block-group.c
>      > @@ -173,40 +173,35 @@ void btrfs_put_block_group(struct
>     btrfs_block_group *cache)
>      >       }
>      >   }
>      >
>      > +static int btrfs_bg_start_cmp(struct rb_node *new, const struct
>     rb_node *exist)
>      > +{
>      > +     struct btrfs_block_group *cmp1 = rb_entry(new, struct
>     btrfs_block_group, cache_node);
>      > +     const struct btrfs_block_group *cmp2 = rb_entry(exist,
>     struct btrfs_block_group, cache_node);
>      > +
>      > +     if (cmp1->start < cmp2->start)
>      > +             return -1;
>      > +     if (cmp1->start > cmp2->start)
>      > +             return 1;
>      > +     return 0;
>      > +}
>      > +
>      >   /*
>      >    * This adds the block group to the fs_info rb tree for the
>     block group cache
>      >    */
>      >   static int btrfs_add_block_group_cache(struct btrfs_fs_info *info,
>      >                                      struct btrfs_block_group
>     *block_group)
>      >   {
>      > -     struct rb_node **p;
>      > -     struct rb_node *parent = NULL;
>      > -     struct btrfs_block_group *cache;
>      > -     bool leftmost = true;
>      > +     struct rb_node *exist;
>      >
>      >       ASSERT(block_group->length != 0);
>      >
>      >       write_lock(&info->block_group_cache_lock);
>      > -     p = &info->block_group_cache_tree.rb_root.rb_node;
>      > -
>      > -     while (*p) {
>      > -             parent = *p;
>      > -             cache = rb_entry(parent, struct btrfs_block_group,
>     cache_node);
>      > -             if (block_group->start < cache->start) {
>      > -                     p = &(*p)->rb_left;
>      > -             } else if (block_group->start > cache->start) {
>      > -                     p = &(*p)->rb_right;
>      > -                     leftmost = false;
>      > -             } else {
>      > -                     write_unlock(&info->block_group_cache_lock);
>      > -                     return -EEXIST;
>      > -             }
>      > -     }
>      > -
>      > -     rb_link_node(&block_group->cache_node, parent, p);
>      > -     rb_insert_color_cached(&block_group->cache_node,
>      > -                            &info->block_group_cache_tree,
>     leftmost);
>      >
>      > +     exist = rb_find_add_cached(&block_group->cache_node,
>      > +                     &info->block_group_cache_tree,
>     btrfs_bg_start_cmp);
>      > +     if (exist)
>      > +             write_unlock(&info->block_group_cache_lock);
>      > +             return -EEXIST;
>
>     IIRC latest GCC would already warn about such indent.
>     This will cause the function to always return -EEXIST.
>
>     Thanks,
>     Qu
>
> Oof, that's what I get for not compiling it again. reran all fstests
> again just to make sure, got similar results to the first time. Want me
> to just resend the whole email chain again?

I can fix this at merge time.

Thanks,
Qu
>
>
>      >       write_unlock(&info->block_group_cache_lock);
>      >
>      >       return 0;
>

Re: [PATCH v2 0/6] reduce boilerplate code within btrfs

[Qu Wenruo]

在 2024/12/16 01:56, Roger L. Beckermeyer III 写道:
> The goal of this patch series is to reduce boilerplate code
> within btrfs. To accomplish this rb_find_add_cached() was added
> to linux/include/rbtree.h. Any replaceable functions were then
> replaced within btrfs.

Since Peter has acknowledged the change in rbtree, the remaining part
looks fine to me.

The mentioned error handling bug will be fixed when I merge the series.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
>
> changelog:
> updated if() statements to utilize newer error checking
> resolved lock error on 0002
> edited title of patches to utilize update instead of edit
> added Acked-by from Peter Zijlstra to 0001
> eliminated extra variables throughout the patch series
>
> Roger L. Beckermeyer III (6):
>    rbtree: add rb_find_add_cached() to rbtree.h
>    btrfs: update btrfs_add_block_group_cache() to use rb helper
>    btrfs: update prelim_ref_insert() to use rb helpers
>    btrfs: update __btrfs_add_delayed_item() to use rb helper
>    btrfs: update btrfs_add_chunk_map() to use rb helpers
>    btrfs: update tree_insert() to use rb helpers
>
>   fs/btrfs/backref.c       | 71 ++++++++++++++++++++--------------------
>   fs/btrfs/block-group.c   | 41 ++++++++++-------------
>   fs/btrfs/delayed-inode.c | 40 +++++++++-------------
>   fs/btrfs/delayed-ref.c   | 39 +++++++++-------------
>   fs/btrfs/volumes.c       | 39 ++++++++++------------
>   include/linux/rbtree.h   | 37 +++++++++++++++++++++
>   6 files changed, 141 insertions(+), 126 deletions(-)
>

Re: [PATCH v2 0/6] reduce boilerplate code within btrfs

[Qu Wenruo]

在 2024/12/17 07:50, Qu Wenruo 写道:
>
>
> 在 2024/12/16 01:56, Roger L. Beckermeyer III 写道:
>> The goal of this patch series is to reduce boilerplate code
>> within btrfs. To accomplish this rb_find_add_cached() was added
>> to linux/include/rbtree.h. Any replaceable functions were then
>> replaced within btrfs.
>
> Since Peter has acknowledged the change in rbtree, the remaining part
> looks fine to me.
>
> The mentioned error handling bug will be fixed when I merge the series.
>
> Reviewed-by: Qu Wenruo <wqu@suse.com>

Well, during merge I found some extra things that you can improve in the
future.

- The length of each code line
   Although we no longer have the older strict 80 chars length limit,
   check_patch.pl will still warn about lines over 100 chars.
   Several patches triggered this.

   So please use check_patch.pl or just use btrfs workflow instead.

- The incorrect drop of const prefix in the last patch
   Since the comparison function accepts one regular node and one const
   node, the last patch drops the second const prefix, mostly due to
   the factg that comp_refs() doesn't have const prefix at all for both
   parameters.

   The proper fix is to add const prefixes for involved functions, not
   dropping the existing const prefixes.

   I have also make all internal structure inside those helpers to be
   const.
   (Personally speaking I also want to check if the less() and cmp() can
    be converted to accept both parameters as const in the future)

- Upper case for the first letter of a sentence
   I'm not good at English either, but at least for the commit message,
   the first letter of a sentence should be in upper case.

- Minor code style problems
   IIRC others have already address the problem like:

	int result;

	result = some_function();
	return result;

   Which can be done by a simple "return some_function();".

Thanks,
Qu

>
> Thanks,
> Qu
>>
>> changelog:
>> updated if() statements to utilize newer error checking
>> resolved lock error on 0002
>> edited title of patches to utilize update instead of edit
>> added Acked-by from Peter Zijlstra to 0001
>> eliminated extra variables throughout the patch series
>>
>> Roger L. Beckermeyer III (6):
>>    rbtree: add rb_find_add_cached() to rbtree.h
>>    btrfs: update btrfs_add_block_group_cache() to use rb helper
>>    btrfs: update prelim_ref_insert() to use rb helpers
>>    btrfs: update __btrfs_add_delayed_item() to use rb helper
>>    btrfs: update btrfs_add_chunk_map() to use rb helpers
>>    btrfs: update tree_insert() to use rb helpers
>>
>>   fs/btrfs/backref.c       | 71 ++++++++++++++++++++--------------------
>>   fs/btrfs/block-group.c   | 41 ++++++++++-------------
>>   fs/btrfs/delayed-inode.c | 40 +++++++++-------------
>>   fs/btrfs/delayed-ref.c   | 39 +++++++++-------------
>>   fs/btrfs/volumes.c       | 39 ++++++++++------------
>>   include/linux/rbtree.h   | 37 +++++++++++++++++++++
>>   6 files changed, 141 insertions(+), 126 deletions(-)
>>
>
>

Re: [PATCH v2 0/6] reduce boilerplate code within btrfs

[Filipe Manana]

On Mon, Dec 16, 2024 at 10:07 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>
>
>
> 在 2024/12/17 07:50, Qu Wenruo 写道:
> >
> >
> > 在 2024/12/16 01:56, Roger L. Beckermeyer III 写道:
> >> The goal of this patch series is to reduce boilerplate code
> >> within btrfs. To accomplish this rb_find_add_cached() was added
> >> to linux/include/rbtree.h. Any replaceable functions were then
> >> replaced within btrfs.
> >
> > Since Peter has acknowledged the change in rbtree, the remaining part
> > looks fine to me.
> >
> > The mentioned error handling bug will be fixed when I merge the series.
> >
> > Reviewed-by: Qu Wenruo <wqu@suse.com>
>
> Well, during merge I found some extra things that you can improve in the
> future.

One more thing to improve in the future:

- Run fstests and check that that are no tests failing after the changes.

There's at least 1 test case failing after this patchset.
The patch "btrfs: update prelim_ref_insert() to use rb helpers" breaks
btrfs/287:

$ ./check btrfs/287
FSTYP         -- btrfs
PLATFORM      -- Linux/x86_64 debian0 6.13.0-rc2-btrfs-next-182+ #2
SMP PREEMPT_DYNAMIC Tue Dec 17 11:02:25 WET 2024
MKFS_OPTIONS  -- /dev/sdc
MOUNT_OPTIONS -- /dev/sdc /home/fdmanana/btrfs-tests/scratch_1

btrfs/287 1s ... - output mismatch (see
/home/fdmanana/git/hub/xfstests/results//btrfs/287.out.bad)
    --- tests/btrfs/287.out 2024-10-30 07:42:47.901514035 +0000
    +++ /home/fdmanana/git/hub/xfstests/results//btrfs/287.out.bad
2024-12-17 15:00:35.341110069 +0000
    @@ -8,82 +8,82 @@
     linked 8388608/8388608 bytes at offset 16777216
     XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
     resolve first extent:
    -inode 257 offset 16777216 root 5
    -inode 257 offset 8388608 root 5
     inode 257 offset 0 root 5
    +inode 257 offset 8388608 root 5
    ...
    (Run 'diff -u /home/fdmanana/git/hub/xfstests/tests/btrfs/287.out
/home/fdmanana/git/hub/xfstests/results//btrfs/287.out.bad'  to see
the entire diff)

HINT: You _MAY_ be missing kernel fix:
      0cad8f14d70c btrfs: fix backref walking not returning all inode refs

Ran: btrfs/287
Failures: btrfs/287
Failed 1 of 1 tests

So please fix the patch (or patches) and resend the updated version once fixed.
Meanwhile it should be dropped from for-next.

Thanks.


>
> - The length of each code line
>    Although we no longer have the older strict 80 chars length limit,
>    check_patch.pl will still warn about lines over 100 chars.
>    Several patches triggered this.
>
>    So please use check_patch.pl or just use btrfs workflow instead.
>
> - The incorrect drop of const prefix in the last patch
>    Since the comparison function accepts one regular node and one const
>    node, the last patch drops the second const prefix, mostly due to
>    the factg that comp_refs() doesn't have const prefix at all for both
>    parameters.
>
>    The proper fix is to add const prefixes for involved functions, not
>    dropping the existing const prefixes.
>
>    I have also make all internal structure inside those helpers to be
>    const.
>    (Personally speaking I also want to check if the less() and cmp() can
>     be converted to accept both parameters as const in the future)
>
> - Upper case for the first letter of a sentence
>    I'm not good at English either, but at least for the commit message,
>    the first letter of a sentence should be in upper case.
>
> - Minor code style problems
>    IIRC others have already address the problem like:
>
>         int result;
>
>         result = some_function();
>         return result;
>
>    Which can be done by a simple "return some_function();".
>
> Thanks,
> Qu
>
> >
> > Thanks,
> > Qu
> >>
> >> changelog:
> >> updated if() statements to utilize newer error checking
> >> resolved lock error on 0002
> >> edited title of patches to utilize update instead of edit
> >> added Acked-by from Peter Zijlstra to 0001
> >> eliminated extra variables throughout the patch series
> >>
> >> Roger L. Beckermeyer III (6):
> >>    rbtree: add rb_find_add_cached() to rbtree.h
> >>    btrfs: update btrfs_add_block_group_cache() to use rb helper
> >>    btrfs: update prelim_ref_insert() to use rb helpers
> >>    btrfs: update __btrfs_add_delayed_item() to use rb helper
> >>    btrfs: update btrfs_add_chunk_map() to use rb helpers
> >>    btrfs: update tree_insert() to use rb helpers
> >>
> >>   fs/btrfs/backref.c       | 71 ++++++++++++++++++++--------------------
> >>   fs/btrfs/block-group.c   | 41 ++++++++++-------------
> >>   fs/btrfs/delayed-inode.c | 40 +++++++++-------------
> >>   fs/btrfs/delayed-ref.c   | 39 +++++++++-------------
> >>   fs/btrfs/volumes.c       | 39 ++++++++++------------
> >>   include/linux/rbtree.h   | 37 +++++++++++++++++++++
> >>   6 files changed, 141 insertions(+), 126 deletions(-)
> >>
> >
> >
>
>

Re: [PATCH v2 0/6] reduce boilerplate code within btrfs

[Lee Beckermeyer]

On Tue, Dec 17, 2024 at 9:09 AM Filipe Manana <fdmanana@kernel.org> wrote:
>
> On Mon, Dec 16, 2024 at 10:07 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
> >
> >
> >
> > 在 2024/12/17 07:50, Qu Wenruo 写道:
> > >
> > >
> > > 在 2024/12/16 01:56, Roger L. Beckermeyer III 写道:
> > >> The goal of this patch series is to reduce boilerplate code
> > >> within btrfs. To accomplish this rb_find_add_cached() was added
> > >> to linux/include/rbtree.h. Any replaceable functions were then
> > >> replaced within btrfs.
> > >
> > > Since Peter has acknowledged the change in rbtree, the remaining part
> > > looks fine to me.
> > >
> > > The mentioned error handling bug will be fixed when I merge the series.
> > >
> > > Reviewed-by: Qu Wenruo <wqu@suse.com>
> >
> > Well, during merge I found some extra things that you can improve in the
> > future.
>
> One more thing to improve in the future:
>
> - Run fstests and check that that are no tests failing after the changes.
>
> There's at least 1 test case failing after this patchset.
> The patch "btrfs: update prelim_ref_insert() to use rb helpers" breaks
> btrfs/287:
>
> $ ./check btrfs/287
> FSTYP         -- btrfs
> PLATFORM      -- Linux/x86_64 debian0 6.13.0-rc2-btrfs-next-182+ #2
> SMP PREEMPT_DYNAMIC Tue Dec 17 11:02:25 WET 2024
> MKFS_OPTIONS  -- /dev/sdc
> MOUNT_OPTIONS -- /dev/sdc /home/fdmanana/btrfs-tests/scratch_1
>
> btrfs/287 1s ... - output mismatch (see
> /home/fdmanana/git/hub/xfstests/results//btrfs/287.out.bad)
>     --- tests/btrfs/287.out 2024-10-30 07:42:47.901514035 +0000
>     +++ /home/fdmanana/git/hub/xfstests/results//btrfs/287.out.bad
> 2024-12-17 15:00:35.341110069 +0000
>     @@ -8,82 +8,82 @@
>      linked 8388608/8388608 bytes at offset 16777216
>      XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>      resolve first extent:
>     -inode 257 offset 16777216 root 5
>     -inode 257 offset 8388608 root 5
>      inode 257 offset 0 root 5
>     +inode 257 offset 8388608 root 5
>     ...
>     (Run 'diff -u /home/fdmanana/git/hub/xfstests/tests/btrfs/287.out
> /home/fdmanana/git/hub/xfstests/results//btrfs/287.out.bad'  to see
> the entire diff)
>
> HINT: You _MAY_ be missing kernel fix:
>       0cad8f14d70c btrfs: fix backref walking not returning all inode refs
>
> Ran: btrfs/287
> Failures: btrfs/287
> Failed 1 of 1 tests
>
> So please fix the patch (or patches) and resend the updated version once fixed.
> Meanwhile it should be dropped from for-next.
>
> Thanks.
>
Found the problem, it's in prelim_ref_cmp in fs/btrfs/backref.c, if
you invert the comparison between the parent and node it passes the
test. e.g. prelim_ref_compare(ref2, ref1); instead of
prelim_ref_compare(ref1, ref2);

I can dig into it but I've got a couple of other things in the queue
right now so it might be a little bit. prelim_ref_cmp() could do with
a logic rework as well as prelim_ref_compare() is only used in 2
places within backref.c. It's just outside the scope of this patch
series so I didn't dig too deep into it.
>
> >
> > - The length of each code line
> >    Although we no longer have the older strict 80 chars length limit,
> >    check_patch.pl will still warn about lines over 100 chars.
> >    Several patches triggered this.
> >
> >    So please use check_patch.pl or just use btrfs workflow instead.
> >
yee, I haven't built a really good workflow for kernel submissions yet.
> > - The incorrect drop of const prefix in the last patch
> >    Since the comparison function accepts one regular node and one const
> >    node, the last patch drops the second const prefix, mostly due to
> >    the factg that comp_refs() doesn't have const prefix at all for both
> >    parameters.
> >
> >    The proper fix is to add const prefixes for involved functions, not
> >    dropping the existing const prefixes.
> >
Okay, however, what do const keywords do that would require this
habit? There's also a place in backref.c where I didn't implement
const keywords.
> >    I have also make all internal structure inside those helpers to be
> >    const.
> >    (Personally speaking I also want to check if the less() and cmp() can
> >     be converted to accept both parameters as const in the future)
> >
> > - Upper case for the first letter of a sentence
> >    I'm not good at English either, but at least for the commit message,
> >    the first letter of a sentence should be in upper case.
Gotcha, I can work on rewording those descriptions, just didn't seem
very important compared to validating everything works properly.
> >
> > - Minor code style problems
> >    IIRC others have already address the problem like:
> >
> >         int result;
> >
> >         result = some_function();
> >         return result;
> >
> >    Which can be done by a simple "return some_function();".
Where was this? I probably missed one when I went through these, sorry.

Just to summarize, would you like me to resend the patch series with
the below changes?
- updated prelim_ref_compare to eliminate comparison error
- rescanned with scripts/checkpatch.pl
- whatever needs to be done with consts.

Thanks for your time!
Lee
> >
> > Thanks,
> > Qu
> >
> > >
> > > Thanks,
> > > Qu
> > >>
> > >> changelog:
> > >> updated if() statements to utilize newer error checking
> > >> resolved lock error on 0002
> > >> edited title of patches to utilize update instead of edit
> > >> added Acked-by from Peter Zijlstra to 0001
> > >> eliminated extra variables throughout the patch series
> > >>
> > >> Roger L. Beckermeyer III (6):
> > >>    rbtree: add rb_find_add_cached() to rbtree.h
> > >>    btrfs: update btrfs_add_block_group_cache() to use rb helper
> > >>    btrfs: update prelim_ref_insert() to use rb helpers
> > >>    btrfs: update __btrfs_add_delayed_item() to use rb helper
> > >>    btrfs: update btrfs_add_chunk_map() to use rb helpers
> > >>    btrfs: update tree_insert() to use rb helpers
> > >>
> > >>   fs/btrfs/backref.c       | 71 ++++++++++++++++++++--------------------
> > >>   fs/btrfs/block-group.c   | 41 ++++++++++-------------
> > >>   fs/btrfs/delayed-inode.c | 40 +++++++++-------------
> > >>   fs/btrfs/delayed-ref.c   | 39 +++++++++-------------
> > >>   fs/btrfs/volumes.c       | 39 ++++++++++------------
> > >>   include/linux/rbtree.h   | 37 +++++++++++++++++++++
> > >>   6 files changed, 141 insertions(+), 126 deletions(-)
> > >>
> > >
> > >
> >
> >

Re: [PATCH v2 0/6] reduce boilerplate code within btrfs

[Qu Wenruo]

在 2024/12/18 01:39, Filipe Manana 写道:
> On Mon, Dec 16, 2024 at 10:07 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>>
>>
>>
>> 在 2024/12/17 07:50, Qu Wenruo 写道:
>>>
>>>
>>> 在 2024/12/16 01:56, Roger L. Beckermeyer III 写道:
>>>> The goal of this patch series is to reduce boilerplate code
>>>> within btrfs. To accomplish this rb_find_add_cached() was added
>>>> to linux/include/rbtree.h. Any replaceable functions were then
>>>> replaced within btrfs.
>>>
>>> Since Peter has acknowledged the change in rbtree, the remaining part
>>> looks fine to me.
>>>
>>> The mentioned error handling bug will be fixed when I merge the series.
>>>
>>> Reviewed-by: Qu Wenruo <wqu@suse.com>
>>
>> Well, during merge I found some extra things that you can improve in the
>> future.
>
> One more thing to improve in the future:
>
> - Run fstests and check that that are no tests failing after the changes.
>
> There's at least 1 test case failing after this patchset.
> The patch "btrfs: update prelim_ref_insert() to use rb helpers" breaks
> btrfs/287:
>
> $ ./check btrfs/287
> FSTYP         -- btrfs
> PLATFORM      -- Linux/x86_64 debian0 6.13.0-rc2-btrfs-next-182+ #2
> SMP PREEMPT_DYNAMIC Tue Dec 17 11:02:25 WET 2024
> MKFS_OPTIONS  -- /dev/sdc
> MOUNT_OPTIONS -- /dev/sdc /home/fdmanana/btrfs-tests/scratch_1
>
> btrfs/287 1s ... - output mismatch (see
> /home/fdmanana/git/hub/xfstests/results//btrfs/287.out.bad)
>      --- tests/btrfs/287.out 2024-10-30 07:42:47.901514035 +0000
>      +++ /home/fdmanana/git/hub/xfstests/results//btrfs/287.out.bad
> 2024-12-17 15:00:35.341110069 +0000
>      @@ -8,82 +8,82 @@
>       linked 8388608/8388608 bytes at offset 16777216
>       XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>       resolve first extent:
>      -inode 257 offset 16777216 root 5
>      -inode 257 offset 8388608 root 5
>       inode 257 offset 0 root 5
>      +inode 257 offset 8388608 root 5
>      ...
>      (Run 'diff -u /home/fdmanana/git/hub/xfstests/tests/btrfs/287.out
> /home/fdmanana/git/hub/xfstests/results//btrfs/287.out.bad'  to see
> the entire diff)
>
> HINT: You _MAY_ be missing kernel fix:
>        0cad8f14d70c btrfs: fix backref walking not returning all inode refs
>
> Ran: btrfs/287
> Failures: btrfs/287
> Failed 1 of 1 tests

I thought the failure is caused by the missing fix, unfortunately I
didn't notice the fix is already merged upstream.

>
> So please fix the patch (or patches) and resend the updated version once fixed.
> Meanwhile it should be dropped from for-next.

Now reverted.

Thanks,
Qu

>
> Thanks.
>
>
>>
>> - The length of each code line
>>     Although we no longer have the older strict 80 chars length limit,
>>     check_patch.pl will still warn about lines over 100 chars.
>>     Several patches triggered this.
>>
>>     So please use check_patch.pl or just use btrfs workflow instead.
>>
>> - The incorrect drop of const prefix in the last patch
>>     Since the comparison function accepts one regular node and one const
>>     node, the last patch drops the second const prefix, mostly due to
>>     the factg that comp_refs() doesn't have const prefix at all for both
>>     parameters.
>>
>>     The proper fix is to add const prefixes for involved functions, not
>>     dropping the existing const prefixes.
>>
>>     I have also make all internal structure inside those helpers to be
>>     const.
>>     (Personally speaking I also want to check if the less() and cmp() can
>>      be converted to accept both parameters as const in the future)
>>
>> - Upper case for the first letter of a sentence
>>     I'm not good at English either, but at least for the commit message,
>>     the first letter of a sentence should be in upper case.
>>
>> - Minor code style problems
>>     IIRC others have already address the problem like:
>>
>>          int result;
>>
>>          result = some_function();
>>          return result;
>>
>>     Which can be done by a simple "return some_function();".
>>
>> Thanks,
>> Qu
>>
>>>
>>> Thanks,
>>> Qu
>>>>
>>>> changelog:
>>>> updated if() statements to utilize newer error checking
>>>> resolved lock error on 0002
>>>> edited title of patches to utilize update instead of edit
>>>> added Acked-by from Peter Zijlstra to 0001
>>>> eliminated extra variables throughout the patch series
>>>>
>>>> Roger L. Beckermeyer III (6):
>>>>     rbtree: add rb_find_add_cached() to rbtree.h
>>>>     btrfs: update btrfs_add_block_group_cache() to use rb helper
>>>>     btrfs: update prelim_ref_insert() to use rb helpers
>>>>     btrfs: update __btrfs_add_delayed_item() to use rb helper
>>>>     btrfs: update btrfs_add_chunk_map() to use rb helpers
>>>>     btrfs: update tree_insert() to use rb helpers
>>>>
>>>>    fs/btrfs/backref.c       | 71 ++++++++++++++++++++--------------------
>>>>    fs/btrfs/block-group.c   | 41 ++++++++++-------------
>>>>    fs/btrfs/delayed-inode.c | 40 +++++++++-------------
>>>>    fs/btrfs/delayed-ref.c   | 39 +++++++++-------------
>>>>    fs/btrfs/volumes.c       | 39 ++++++++++------------
>>>>    include/linux/rbtree.h   | 37 +++++++++++++++++++++
>>>>    6 files changed, 141 insertions(+), 126 deletions(-)
>>>>
>>>
>>>
>>
>>

Re: [PATCH v2 0/6] reduce boilerplate code within btrfs

[Qu Wenruo]

在 2024/12/18 07:06, Lee Beckermeyer 写道:
> On Tue, Dec 17, 2024 at 9:09 AM Filipe Manana <fdmanana@kernel.org> wrote:
>>
>> On Mon, Dec 16, 2024 at 10:07 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>>>
>>>
>>>
>>> 在 2024/12/17 07:50, Qu Wenruo 写道:
>>>>
>>>>
>>>> 在 2024/12/16 01:56, Roger L. Beckermeyer III 写道:
>>>>> The goal of this patch series is to reduce boilerplate code
>>>>> within btrfs. To accomplish this rb_find_add_cached() was added
>>>>> to linux/include/rbtree.h. Any replaceable functions were then
>>>>> replaced within btrfs.
>>>>
>>>> Since Peter has acknowledged the change in rbtree, the remaining part
>>>> looks fine to me.
>>>>
>>>> The mentioned error handling bug will be fixed when I merge the series.
>>>>
>>>> Reviewed-by: Qu Wenruo <wqu@suse.com>
>>>
>>> Well, during merge I found some extra things that you can improve in the
>>> future.
>>
>> One more thing to improve in the future:
>>
>> - Run fstests and check that that are no tests failing after the changes.
>>
>> There's at least 1 test case failing after this patchset.
>> The patch "btrfs: update prelim_ref_insert() to use rb helpers" breaks
>> btrfs/287:
>>
>> $ ./check btrfs/287
>> FSTYP         -- btrfs
>> PLATFORM      -- Linux/x86_64 debian0 6.13.0-rc2-btrfs-next-182+ #2
>> SMP PREEMPT_DYNAMIC Tue Dec 17 11:02:25 WET 2024
>> MKFS_OPTIONS  -- /dev/sdc
>> MOUNT_OPTIONS -- /dev/sdc /home/fdmanana/btrfs-tests/scratch_1
>>
>> btrfs/287 1s ... - output mismatch (see
>> /home/fdmanana/git/hub/xfstests/results//btrfs/287.out.bad)
>>      --- tests/btrfs/287.out 2024-10-30 07:42:47.901514035 +0000
>>      +++ /home/fdmanana/git/hub/xfstests/results//btrfs/287.out.bad
>> 2024-12-17 15:00:35.341110069 +0000
>>      @@ -8,82 +8,82 @@
>>       linked 8388608/8388608 bytes at offset 16777216
>>       XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>       resolve first extent:
>>      -inode 257 offset 16777216 root 5
>>      -inode 257 offset 8388608 root 5
>>       inode 257 offset 0 root 5
>>      +inode 257 offset 8388608 root 5
>>      ...
>>      (Run 'diff -u /home/fdmanana/git/hub/xfstests/tests/btrfs/287.out
>> /home/fdmanana/git/hub/xfstests/results//btrfs/287.out.bad'  to see
>> the entire diff)
>>
>> HINT: You _MAY_ be missing kernel fix:
>>        0cad8f14d70c btrfs: fix backref walking not returning all inode refs
>>
>> Ran: btrfs/287
>> Failures: btrfs/287
>> Failed 1 of 1 tests
>>
>> So please fix the patch (or patches) and resend the updated version once fixed.
>> Meanwhile it should be dropped from for-next.
>>
>> Thanks.
>>
> Found the problem, it's in prelim_ref_cmp in fs/btrfs/backref.c, if
> you invert the comparison between the parent and node it passes the
> test. e.g. prelim_ref_compare(ref2, ref1); instead of
> prelim_ref_compare(ref1, ref2);
>
> I can dig into it but I've got a couple of other things in the queue
> right now so it might be a little bit. prelim_ref_cmp() could do with
> a logic rework as well as prelim_ref_compare() is only used in 2
> places within backref.c. It's just outside the scope of this patch
> series so I didn't dig too deep into it.

It's not the parameter order, it's the problem related to the parent/ref
usage.

Previously during the insert we update the parent/ref pointer during the
search, but it's no longer the case, thus the whole if (exist) {} branch
is wrong.

>>
>>>
>>> - The length of each code line
>>>     Although we no longer have the older strict 80 chars length limit,
>>>     check_patch.pl will still warn about lines over 100 chars.
>>>     Several patches triggered this.
>>>
>>>     So please use check_patch.pl or just use btrfs workflow instead.
>>>
> yee, I haven't built a really good workflow for kernel submissions yet.
>>> - The incorrect drop of const prefix in the last patch
>>>     Since the comparison function accepts one regular node and one const
>>>     node, the last patch drops the second const prefix, mostly due to
>>>     the factg that comp_refs() doesn't have const prefix at all for both
>>>     parameters.
>>>
>>>     The proper fix is to add const prefixes for involved functions, not
>>>     dropping the existing const prefixes.
>>>
> Okay, however, what do const keywords do that would require this
> habit? There's also a place in backref.c where I didn't implement
> const keywords.
>>>     I have also make all internal structure inside those helpers to be
>>>     const.
>>>     (Personally speaking I also want to check if the less() and cmp() can
>>>      be converted to accept both parameters as const in the future)
>>>
>>> - Upper case for the first letter of a sentence
>>>     I'm not good at English either, but at least for the commit message,
>>>     the first letter of a sentence should be in upper case.
> Gotcha, I can work on rewording those descriptions, just didn't seem
> very important compared to validating everything works properly.
>>>
>>> - Minor code style problems
>>>     IIRC others have already address the problem like:
>>>
>>>          int result;
>>>
>>>          result = some_function();
>>>          return result;
>>>
>>>     Which can be done by a simple "return some_function();".
> Where was this? I probably missed one when I went through these, sorry.
>
> Just to summarize, would you like me to resend the patch series with
> the below changes?
> - updated prelim_ref_compare to eliminate comparison error
> - rescanned with scripts/checkpatch.pl
> - whatever needs to be done with consts.

I'll handle the error fix.

Since I have already changed the series a lot, your update will get all
the existing modification lost.

Thanks,
Qu

>
> Thanks for your time!
> Lee
>>>
>>> Thanks,
>>> Qu
>>>
>>>>
>>>> Thanks,
>>>> Qu
>>>>>
>>>>> changelog:
>>>>> updated if() statements to utilize newer error checking
>>>>> resolved lock error on 0002
>>>>> edited title of patches to utilize update instead of edit
>>>>> added Acked-by from Peter Zijlstra to 0001
>>>>> eliminated extra variables throughout the patch series
>>>>>
>>>>> Roger L. Beckermeyer III (6):
>>>>>     rbtree: add rb_find_add_cached() to rbtree.h
>>>>>     btrfs: update btrfs_add_block_group_cache() to use rb helper
>>>>>     btrfs: update prelim_ref_insert() to use rb helpers
>>>>>     btrfs: update __btrfs_add_delayed_item() to use rb helper
>>>>>     btrfs: update btrfs_add_chunk_map() to use rb helpers
>>>>>     btrfs: update tree_insert() to use rb helpers
>>>>>
>>>>>    fs/btrfs/backref.c       | 71 ++++++++++++++++++++--------------------
>>>>>    fs/btrfs/block-group.c   | 41 ++++++++++-------------
>>>>>    fs/btrfs/delayed-inode.c | 40 +++++++++-------------
>>>>>    fs/btrfs/delayed-ref.c   | 39 +++++++++-------------
>>>>>    fs/btrfs/volumes.c       | 39 ++++++++++------------
>>>>>    include/linux/rbtree.h   | 37 +++++++++++++++++++++
>>>>>    6 files changed, 141 insertions(+), 126 deletions(-)
>>>>>
>>>>
>>>>
>>>
>>>

Re: [PATCH v2 0/6] reduce boilerplate code within btrfs

[Lee Beckermeyer]

On Tue, Dec 17, 2024 at 2:49 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>
>
>
> 在 2024/12/18 07:06, Lee Beckermeyer 写道:
> > On Tue, Dec 17, 2024 at 9:09 AM Filipe Manana <fdmanana@kernel.org> wrote:
> >>
> >> On Mon, Dec 16, 2024 at 10:07 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
> >>>
> >>>
> >>>
> >>> 在 2024/12/17 07:50, Qu Wenruo 写道:
> >>>>
> >>>>
> >>>> 在 2024/12/16 01:56, Roger L. Beckermeyer III 写道:
> >>>>> The goal of this patch series is to reduce boilerplate code
> >>>>> within btrfs. To accomplish this rb_find_add_cached() was added
> >>>>> to linux/include/rbtree.h. Any replaceable functions were then
> >>>>> replaced within btrfs.
> >>>>
> >>>> Since Peter has acknowledged the change in rbtree, the remaining part
> >>>> looks fine to me.
> >>>>
> >>>> The mentioned error handling bug will be fixed when I merge the series.
> >>>>
> >>>> Reviewed-by: Qu Wenruo <wqu@suse.com>
> >>>
> >>> Well, during merge I found some extra things that you can improve in the
> >>> future.
> >>
> >> One more thing to improve in the future:
> >>
> >> - Run fstests and check that that are no tests failing after the changes.
> >>
> >> There's at least 1 test case failing after this patchset.
> >> The patch "btrfs: update prelim_ref_insert() to use rb helpers" breaks
> >> btrfs/287:
> >>
> >> $ ./check btrfs/287
> >> FSTYP         -- btrfs
> >> PLATFORM      -- Linux/x86_64 debian0 6.13.0-rc2-btrfs-next-182+ #2
> >> SMP PREEMPT_DYNAMIC Tue Dec 17 11:02:25 WET 2024
> >> MKFS_OPTIONS  -- /dev/sdc
> >> MOUNT_OPTIONS -- /dev/sdc /home/fdmanana/btrfs-tests/scratch_1
> >>
> >> btrfs/287 1s ... - output mismatch (see
> >> /home/fdmanana/git/hub/xfstests/results//btrfs/287.out.bad)
> >>      --- tests/btrfs/287.out 2024-10-30 07:42:47.901514035 +0000
> >>      +++ /home/fdmanana/git/hub/xfstests/results//btrfs/287.out.bad
> >> 2024-12-17 15:00:35.341110069 +0000
> >>      @@ -8,82 +8,82 @@
> >>       linked 8388608/8388608 bytes at offset 16777216
> >>       XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> >>       resolve first extent:
> >>      -inode 257 offset 16777216 root 5
> >>      -inode 257 offset 8388608 root 5
> >>       inode 257 offset 0 root 5
> >>      +inode 257 offset 8388608 root 5
> >>      ...
> >>      (Run 'diff -u /home/fdmanana/git/hub/xfstests/tests/btrfs/287.out
> >> /home/fdmanana/git/hub/xfstests/results//btrfs/287.out.bad'  to see
> >> the entire diff)
> >>
> >> HINT: You _MAY_ be missing kernel fix:
> >>        0cad8f14d70c btrfs: fix backref walking not returning all inode refs
> >>
> >> Ran: btrfs/287
> >> Failures: btrfs/287
> >> Failed 1 of 1 tests
> >>
> >> So please fix the patch (or patches) and resend the updated version once fixed.
> >> Meanwhile it should be dropped from for-next.
> >>
> >> Thanks.
> >>
> > Found the problem, it's in prelim_ref_cmp in fs/btrfs/backref.c, if
> > you invert the comparison between the parent and node it passes the
> > test. e.g. prelim_ref_compare(ref2, ref1); instead of
> > prelim_ref_compare(ref1, ref2);
> >
> > I can dig into it but I've got a couple of other things in the queue
> > right now so it might be a little bit. prelim_ref_cmp() could do with
> > a logic rework as well as prelim_ref_compare() is only used in 2
> > places within backref.c. It's just outside the scope of this patch
> > series so I didn't dig too deep into it.
>
> It's not the parameter order, it's the problem related to the parent/ref
> usage.
>
> Previously during the insert we update the parent/ref pointer during the
> search, but it's no longer the case, thus the whole if (exist) {} branch
> is wrong.
>
> >>
> >>>
> >>> - The length of each code line
> >>>     Although we no longer have the older strict 80 chars length limit,
> >>>     check_patch.pl will still warn about lines over 100 chars.
> >>>     Several patches triggered this.
> >>>
> >>>     So please use check_patch.pl or just use btrfs workflow instead.
> >>>
> > yee, I haven't built a really good workflow for kernel submissions yet.
> >>> - The incorrect drop of const prefix in the last patch
> >>>     Since the comparison function accepts one regular node and one const
> >>>     node, the last patch drops the second const prefix, mostly due to
> >>>     the factg that comp_refs() doesn't have const prefix at all for both
> >>>     parameters.
> >>>
> >>>     The proper fix is to add const prefixes for involved functions, not
> >>>     dropping the existing const prefixes.
> >>>
> > Okay, however, what do const keywords do that would require this
> > habit? There's also a place in backref.c where I didn't implement
> > const keywords.
> >>>     I have also make all internal structure inside those helpers to be
> >>>     const.
> >>>     (Personally speaking I also want to check if the less() and cmp() can
> >>>      be converted to accept both parameters as const in the future)
> >>>
> >>> - Upper case for the first letter of a sentence
> >>>     I'm not good at English either, but at least for the commit message,
> >>>     the first letter of a sentence should be in upper case.
> > Gotcha, I can work on rewording those descriptions, just didn't seem
> > very important compared to validating everything works properly.
> >>>
> >>> - Minor code style problems
> >>>     IIRC others have already address the problem like:
> >>>
> >>>          int result;
> >>>
> >>>          result = some_function();
> >>>          return result;
> >>>
> >>>     Which can be done by a simple "return some_function();".
> > Where was this? I probably missed one when I went through these, sorry.
> >
> > Just to summarize, would you like me to resend the patch series with
> > the below changes?
> > - updated prelim_ref_compare to eliminate comparison error
> > - rescanned with scripts/checkpatch.pl
> > - whatever needs to be done with consts.
>
> I'll handle the error fix.
>
> Since I have already changed the series a lot, your update will get all
> the existing modification lost.
>
> Thanks,
> Qu
Sounds good, let me know if you need anything else on this patch
series from me then.

Thanks,
Lee
>
> >
> > Thanks for your time!
> > Lee
> >>>
> >>> Thanks,
> >>> Qu
> >>>
> >>>>
> >>>> Thanks,
> >>>> Qu
> >>>>>
> >>>>> changelog:
> >>>>> updated if() statements to utilize newer error checking
> >>>>> resolved lock error on 0002
> >>>>> edited title of patches to utilize update instead of edit
> >>>>> added Acked-by from Peter Zijlstra to 0001
> >>>>> eliminated extra variables throughout the patch series
> >>>>>
> >>>>> Roger L. Beckermeyer III (6):
> >>>>>     rbtree: add rb_find_add_cached() to rbtree.h
> >>>>>     btrfs: update btrfs_add_block_group_cache() to use rb helper
> >>>>>     btrfs: update prelim_ref_insert() to use rb helpers
> >>>>>     btrfs: update __btrfs_add_delayed_item() to use rb helper
> >>>>>     btrfs: update btrfs_add_chunk_map() to use rb helpers
> >>>>>     btrfs: update tree_insert() to use rb helpers
> >>>>>
> >>>>>    fs/btrfs/backref.c       | 71 ++++++++++++++++++++--------------------
> >>>>>    fs/btrfs/block-group.c   | 41 ++++++++++-------------
> >>>>>    fs/btrfs/delayed-inode.c | 40 +++++++++-------------
> >>>>>    fs/btrfs/delayed-ref.c   | 39 +++++++++-------------
> >>>>>    fs/btrfs/volumes.c       | 39 ++++++++++------------
> >>>>>    include/linux/rbtree.h   | 37 +++++++++++++++++++++
> >>>>>    6 files changed, 141 insertions(+), 126 deletions(-)
> >>>>>
> >>>>
> >>>>
> >>>
> >>>
>