On 2020/4/2 上午7:40, Qu Wenruo wrote:
> 
> 
> On 2020/4/1 下午11:48, David Sterba wrote:
>> 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.
> 
> 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.
> 
>>
>>>  
>>>  	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.

My bad, the order is still a pretty important thing...

Thus we still need to keep everything in their correct order to make the
code work...

> 
> 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?
> 
> Thanks,
> Qu
>