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
