* [PATCH v2 1/6] rbtree: add rb_find_add_cached() to rbtree.h
2024-12-15 15:26 [PATCH v2 0/6] reduce boilerplate code within btrfs Roger L. Beckermeyer III
@ 2024-12-15 15:26 ` Roger L. Beckermeyer III
2024-12-15 15:26 ` [PATCH v2 2/6] btrfs: update btrfs_add_block_group_cache() to use rb helper Roger L. Beckermeyer III
` (5 subsequent siblings)
6 siblings, 0 replies; 16+ messages in thread
From: Roger L. Beckermeyer III @ 2024-12-15 15:26 UTC (permalink / raw)
To: linux-btrfs; +Cc: Roger L. Beckermeyer III, Josef Bacik, Peter Zijlstra
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
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v2 2/6] btrfs: update btrfs_add_block_group_cache() to use rb helper
2024-12-15 15:26 [PATCH v2 0/6] reduce boilerplate code within btrfs Roger L. Beckermeyer III
2024-12-15 15:26 ` [PATCH v2 1/6] rbtree: add rb_find_add_cached() to rbtree.h Roger L. Beckermeyer III
@ 2024-12-15 15:26 ` Roger L. Beckermeyer III
2024-12-15 22:23 ` Qu Wenruo
2024-12-15 15:26 ` [PATCH v2 3/6] btrfs: update prelim_ref_insert() to use rb helpers Roger L. Beckermeyer III
` (4 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Roger L. Beckermeyer III @ 2024-12-15 15:26 UTC (permalink / raw)
To: linux-btrfs; +Cc: Roger L. Beckermeyer III, Josef Bacik
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
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v2 2/6] btrfs: update btrfs_add_block_group_cache() to use rb helper
2024-12-15 15:26 ` [PATCH v2 2/6] btrfs: update btrfs_add_block_group_cache() to use rb helper Roger L. Beckermeyer III
@ 2024-12-15 22:23 ` Qu Wenruo
[not found] ` <CAMNkDpC3gC0Eu5VWLbtvjgj8vj2ckxYo5CZJ_394y_rXDQ7FXg@mail.gmail.com>
0 siblings, 1 reply; 16+ messages in thread
From: Qu Wenruo @ 2024-12-15 22:23 UTC (permalink / raw)
To: Roger L. Beckermeyer III, linux-btrfs; +Cc: Josef Bacik
在 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;
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 3/6] btrfs: update prelim_ref_insert() to use rb helpers
2024-12-15 15:26 [PATCH v2 0/6] reduce boilerplate code within btrfs Roger L. Beckermeyer III
2024-12-15 15:26 ` [PATCH v2 1/6] rbtree: add rb_find_add_cached() to rbtree.h Roger L. Beckermeyer III
2024-12-15 15:26 ` [PATCH v2 2/6] btrfs: update btrfs_add_block_group_cache() to use rb helper Roger L. Beckermeyer III
@ 2024-12-15 15:26 ` Roger L. Beckermeyer III
2024-12-15 15:26 ` [PATCH v2 4/6] btrfs: update __btrfs_add_delayed_item() to use rb helper Roger L. Beckermeyer III
` (3 subsequent siblings)
6 siblings, 0 replies; 16+ messages in thread
From: Roger L. Beckermeyer III @ 2024-12-15 15:26 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v2 4/6] btrfs: update __btrfs_add_delayed_item() to use rb helper
2024-12-15 15:26 [PATCH v2 0/6] reduce boilerplate code within btrfs Roger L. Beckermeyer III
` (2 preceding siblings ...)
2024-12-15 15:26 ` [PATCH v2 3/6] btrfs: update prelim_ref_insert() to use rb helpers Roger L. Beckermeyer III
@ 2024-12-15 15:26 ` Roger L. Beckermeyer III
2024-12-15 15:26 ` [PATCH v2 5/6] btrfs: update btrfs_add_chunk_map() to use rb helpers Roger L. Beckermeyer III
` (2 subsequent siblings)
6 siblings, 0 replies; 16+ messages in thread
From: Roger L. Beckermeyer III @ 2024-12-15 15:26 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v2 5/6] btrfs: update btrfs_add_chunk_map() to use rb helpers
2024-12-15 15:26 [PATCH v2 0/6] reduce boilerplate code within btrfs Roger L. Beckermeyer III
` (3 preceding siblings ...)
2024-12-15 15:26 ` [PATCH v2 4/6] btrfs: update __btrfs_add_delayed_item() to use rb helper Roger L. Beckermeyer III
@ 2024-12-15 15:26 ` Roger L. Beckermeyer III
2024-12-15 15:26 ` [PATCH v2 6/6] btrfs: update tree_insert() " Roger L. Beckermeyer III
2024-12-16 21:20 ` [PATCH v2 0/6] reduce boilerplate code within btrfs Qu Wenruo
6 siblings, 0 replies; 16+ messages in thread
From: Roger L. Beckermeyer III @ 2024-12-15 15:26 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v2 6/6] btrfs: update tree_insert() to use rb helpers
2024-12-15 15:26 [PATCH v2 0/6] reduce boilerplate code within btrfs Roger L. Beckermeyer III
` (4 preceding siblings ...)
2024-12-15 15:26 ` [PATCH v2 5/6] btrfs: update btrfs_add_chunk_map() to use rb helpers Roger L. Beckermeyer III
@ 2024-12-15 15:26 ` Roger L. Beckermeyer III
2024-12-16 21:20 ` [PATCH v2 0/6] reduce boilerplate code within btrfs Qu Wenruo
6 siblings, 0 replies; 16+ messages in thread
From: Roger L. Beckermeyer III @ 2024-12-15 15:26 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v2 0/6] reduce boilerplate code within btrfs
2024-12-15 15:26 [PATCH v2 0/6] reduce boilerplate code within btrfs Roger L. Beckermeyer III
` (5 preceding siblings ...)
2024-12-15 15:26 ` [PATCH v2 6/6] btrfs: update tree_insert() " Roger L. Beckermeyer III
@ 2024-12-16 21:20 ` Qu Wenruo
2024-12-16 22:06 ` Qu Wenruo
6 siblings, 1 reply; 16+ messages in thread
From: Qu Wenruo @ 2024-12-16 21:20 UTC (permalink / raw)
To: Roger L. Beckermeyer III, linux-btrfs
在 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(-)
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 0/6] reduce boilerplate code within btrfs
2024-12-16 21:20 ` [PATCH v2 0/6] reduce boilerplate code within btrfs Qu Wenruo
@ 2024-12-16 22:06 ` Qu Wenruo
2024-12-17 15:09 ` Filipe Manana
0 siblings, 1 reply; 16+ messages in thread
From: Qu Wenruo @ 2024-12-16 22:06 UTC (permalink / raw)
To: Roger L. Beckermeyer III, linux-btrfs
在 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(-)
>>
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 0/6] reduce boilerplate code within btrfs
2024-12-16 22:06 ` Qu Wenruo
@ 2024-12-17 15:09 ` Filipe Manana
2024-12-17 20:36 ` Lee Beckermeyer
2024-12-17 20:42 ` Qu Wenruo
0 siblings, 2 replies; 16+ messages in thread
From: Filipe Manana @ 2024-12-17 15:09 UTC (permalink / raw)
To: Qu Wenruo; +Cc: Roger L. Beckermeyer III, linux-btrfs
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(-)
> >>
> >
> >
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 0/6] reduce boilerplate code within btrfs
2024-12-17 15:09 ` Filipe Manana
@ 2024-12-17 20:36 ` Lee Beckermeyer
2024-12-17 20:49 ` Qu Wenruo
2024-12-17 20:42 ` Qu Wenruo
1 sibling, 1 reply; 16+ messages in thread
From: Lee Beckermeyer @ 2024-12-17 20:36 UTC (permalink / raw)
To: Filipe Manana; +Cc: Qu Wenruo, linux-btrfs
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(-)
> > >>
> > >
> > >
> >
> >
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/6] reduce boilerplate code within btrfs
2024-12-17 20:36 ` Lee Beckermeyer
@ 2024-12-17 20:49 ` Qu Wenruo
2024-12-19 16:07 ` Lee Beckermeyer
0 siblings, 1 reply; 16+ messages in thread
From: Qu Wenruo @ 2024-12-17 20:49 UTC (permalink / raw)
To: Lee Beckermeyer, Filipe Manana; +Cc: linux-btrfs
在 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(-)
>>>>>
>>>>
>>>>
>>>
>>>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 0/6] reduce boilerplate code within btrfs
2024-12-17 20:49 ` Qu Wenruo
@ 2024-12-19 16:07 ` Lee Beckermeyer
0 siblings, 0 replies; 16+ messages in thread
From: Lee Beckermeyer @ 2024-12-19 16:07 UTC (permalink / raw)
To: Qu Wenruo; +Cc: Filipe Manana, linux-btrfs
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(-)
> >>>>>
> >>>>
> >>>>
> >>>
> >>>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/6] reduce boilerplate code within btrfs
2024-12-17 15:09 ` Filipe Manana
2024-12-17 20:36 ` Lee Beckermeyer
@ 2024-12-17 20:42 ` Qu Wenruo
1 sibling, 0 replies; 16+ messages in thread
From: Qu Wenruo @ 2024-12-17 20:42 UTC (permalink / raw)
To: Filipe Manana; +Cc: Roger L. Beckermeyer III, linux-btrfs
在 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(-)
>>>>
>>>
>>>
>>
>>
^ permalink raw reply [flat|nested] 16+ messages in thread