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@xxxxxxxx>
>>> ---
>>> 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
>
Attachment:
signature.asc
Description: OpenPGP digital signature
