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.
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
