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

[Qu Wenruo <wqu@suse.com>]

On 2020/4/2 上午9:09, David Sterba wrote:
> On Thu, Apr 02, 2020 at 07:40:29AM +0800, Qu Wenruo wrote:
>>>>  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.
>>
>> If using simple_node, we need a ton of extra wrapper to wrap things like
>> rb_entry(), rb_postorder_()
>>
>> Thus here we still want byte/rb_node directly embeded into the structure.
>>
>> The ideal method would be anonymous but typed structure.
>> Unfortunately no such C standard supports this.
> 
> My idea was to have something like this (simplified):
> 
> 	struct tree_node {
> 		struct rb_node node;
> 		u64 bytenr;
> 	};
> 
> 	struct backref_node {
> 		...
> 		struct tree_node cache_node;
> 		...
> 	};
> 
> 	struct backref_node bnode;
> 
> when the rb_node is needed, pass &bnode.cache_node.rb_node . All the
> rb_* functions should work without adding another interface layer.

The problem is function relocate_tree_blocks().

In which we call rbtree_postorder_for_each_entry_safe().

If we use tree_node directly, we need to call container_of() again to
grab the tree_block structure, which almost kills the meaning of
rbtree_postorder_for_each_entry_safe()

This also applies to rb_first() callers like free_block_list().

> 
>>>>  	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.
>>
>> Order is not a problem, since we call container_of(), thus there is no
>> need for any order or placement.
>> User can easily put rb_node at the end of the structure, and bytenr at
>> the beginning of the structure, and everything still goes well.
>>
>> The anonymous structure is mostly here to inform callers that we're
>> using simple_node structure.
>>
>>>
>>>> + */
>>>> +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.
>>>
>> That's the what I mean for "simple", it's really just a simple, not even
>> a full wrapper, for bytenr based rb tree search.
>>
>> Adding too many wrappers may simply kill the "simple" part.
>>
>> Although I have to admit, that most of the simple_node part is only to
>> reuse code across relocation.c and backref.c. Since no other users
>> utilize such simple facility.
>>
>> Any idea to improve such situation? Or we really need to go full wrappers?
> 
> If the above works we won't need to add more wrappers. But after some
> thinking I'm ok with the way you implement it as it will certainly clean
> up some things and once it's merged we'll have another chance to look at
> the code and fix up only the structures.

Looking forward to better cleanups.

Thanks,
Qu