Re: [PATCH v2 17/39] btrfs: Rename tree_entry to simple_node and export it

[David Sterba <dsterba@suse.cz>]

On Thu, Mar 26, 2020 at 04:32:54PM +0800, Qu Wenruo wrote:
> Structure tree_entry provides a very simple rb_tree which only uses
> bytenr as search index.
> 
> That tree_entry is used in 3 structures: backref_node, mapping_node and
> tree_block.
> 
> Since we're going to make backref_node independnt from relocation, it's
> a good time to extract the tree_entry into simple_node, and export it
> into misc.h.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/backref.h    |   6 ++-
>  fs/btrfs/misc.h       |  54 +++++++++++++++++++++
>  fs/btrfs/relocation.c | 109 +++++++++++++-----------------------------
>  3 files changed, 90 insertions(+), 79 deletions(-)
> 
> diff --git a/fs/btrfs/backref.h b/fs/btrfs/backref.h
> index 76858ec099d9..f3eae9e9f84b 100644
> --- a/fs/btrfs/backref.h
> +++ b/fs/btrfs/backref.h
> @@ -162,8 +162,10 @@ btrfs_backref_iter_release(struct btrfs_backref_iter *iter)
>   * present a tree block in the backref cache
>   */
>  struct btrfs_backref_node {
> -	struct rb_node rb_node;
> -	u64 bytenr;
> +	struct {
> +		struct rb_node rb_node;
> +		u64 bytenr;
> +	}; /* Use simple_node for search/insert */

Why is this anonymous struct? This should be the simple_node as I see
below. For some simple rb search API.

>  
>  	u64 new_bytenr;
>  	/* objectid of tree block owner, can be not uptodate */
> diff --git a/fs/btrfs/misc.h b/fs/btrfs/misc.h
> index 72bab64ecf60..d199bfdb210e 100644
> --- a/fs/btrfs/misc.h
> +++ b/fs/btrfs/misc.h
> @@ -6,6 +6,7 @@
>  #include <linux/sched.h>
>  #include <linux/wait.h>
>  #include <asm/div64.h>
> +#include <linux/rbtree.h>
>  
>  #define in_range(b, first, len) ((b) >= (first) && (b) < (first) + (len))
>  
> @@ -58,4 +59,57 @@ static inline bool has_single_bit_set(u64 n)
>  	return is_power_of_two_u64(n);
>  }
>  
> +/*
> + * Simple bytenr based rb_tree relate structures
> + *
> + * Any structure wants to use bytenr as single search index should have their
> + * structure start with these members.

This is not very clean coding style, relying on particular placement and
order in another struct.

> + */
> +struct simple_node {
> +	struct rb_node rb_node;
> +	u64 bytenr;
> +};
> +
> +static inline struct rb_node *simple_search(struct rb_root *root, u64 bytenr)

simple_search is IMHO too vague, it's related to a rb-tree so this could
be reflected in the name somehow.

I think it's ok if you do this as a middle step before making it a
proper struct hook and API but I don't like the end result as it's not
really an improvement.