Re: [PATCH v2 05/10] btrfs: relocation: Refactor tree backref processing into its own function

Qu Wenruo
2020-03-04T12:33:35Z

On 2020/3/4 下午8:23, Nikolay Borisov wrote:
>
>
> On 2.03.20 г. 11:45 ч., Qu Wenruo wrote:
>> build_backref_tree() function is painfully long, as it has 3 big parts:
>> - Tree backref handling
>> - Weaving backref nodes
>> - Useless nodes pruning
>>
>> This patch will move the tree backref handling into its own function,
>> handle_one_tree_backref().
>>
>> And inside that function, the main works are determined by the backref
>> key:
>> - BTRFS_SHARED_BLOCK_REF_KEY
>> We know the parent node bytenr directly.
>> If the parent is cached, or it's root, call it a day.
>> If the parent is not cached, add it pending list.
>>
>> - BTRFS_TREE_BLOCK_REF_KEY
>> The most complex work.
>> We need to grab the fs root, do a tree search to locate all its
>> parent nodes, weaving all needed edges, and put all uncached edges to
>> pending edge list.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>> fs/btrfs/relocation.c | 395 +++++++++++++++++++++---------------------
>> 1 file changed, 202 insertions(+), 193 deletions(-)
>>
>> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
>> index d1e1d613ab98..04416489d87a 100644
>> --- a/fs/btrfs/relocation.c
>> +++ b/fs/btrfs/relocation.c
>> @@ -666,6 +666,206 @@ static struct btrfs_root *read_fs_root(struct btrfs_fs_info *fs_info,
>> return btrfs_get_fs_root(fs_info, &key, false);
>> }
>>
>> +static int handle_one_tree_backref(struct reloc_control *rc,
>> + struct list_head *useless_node,
>> + struct list_head *pending_edge,
>> + struct btrfs_path *path,
>> + struct btrfs_key *ref_key,
>> + struct btrfs_key *tree_key,
>> + struct backref_node *cur)
>
> That function has 7 parameters and while @rc and @path are somewhat
> self-explanatory others are not.

That's exactly what I'm fixing in the 2nd patchset.
To kill 2 parameters by moving @useless_node and @pending_edge into
backref_cache.

Since you're complaining about this, it looks like I'd better put that
patch earlier.

> Also it's really doing 2 distinct
> things which, in my opinion, would be much better split across 2
> functions - 1 handling BTRFS_SHARED_BLOCK_REF_KEY and the other handling
> BTRFS_TREE_BLOCK_REF_KEY. For example the @tree_key,@useless_node and
> @path are not used in BTRFS_SHARED_BLOCK_REF_KEY case. You'd have two
> functions named:
>
> handle_(in)?direct_tree_backref
>
> One will take only 4 parameters the other will have to, unfortunately,
> take all 7.
>

That looks reasonable.

I'll split them into two functions, and move the two lists into
backref_cache in next version.

Thanks,
Qu