Re: [PATCH v2 07/10] btrfs: relocation: Specify essential members for alloc_backref_node()

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2020/3/4 下午9:06, Nikolay Borisov wrote:
>
>
> On 2.03.20 г. 11:45 ч., Qu Wenruo wrote:
>> Bytenr and level are essential parameters for backref_node, thus it
>> makes sense to initial them at alloc time.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
>
>> ---
>>  fs/btrfs/relocation.c | 25 ++++++++++++-------------
>>  1 file changed, 12 insertions(+), 13 deletions(-)
>>
>> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
>> index c76849409c81..67a4a61eb86a 100644
>> --- a/fs/btrfs/relocation.c
>> +++ b/fs/btrfs/relocation.c
>> @@ -308,10 +308,12 @@ static void backref_cache_cleanup(struct backref_cache *cache)
>>  	ASSERT(!cache->nr_edges);
>>  }
>>
>> -static struct backref_node *alloc_backref_node(struct backref_cache *cache)
>> +static struct backref_node *alloc_backref_node(struct backref_cache *cache,
>> +						u64 bytenr, int level)
>>  {
>>  	struct backref_node *node;
>>
>> +	ASSERT(level >= 0 && level < BTRFS_MAX_LEVEL);
>>  	node = kzalloc(sizeof(*node), GFP_NOFS);
>>  	if (node) {
>>  		INIT_LIST_HEAD(&node->list);
>> @@ -319,6 +321,9 @@ static struct backref_node *alloc_backref_node(struct backref_cache *cache)
>>  		INIT_LIST_HEAD(&node->lower);
>>  		RB_CLEAR_NODE(&node->rb_node);
>>  		cache->nr_nodes++;
>> +
>> +		node->level = level;
>> +		node->bytenr = bytenr;
>>  	}
>
> nit: One suggestion for this function, feel free to ignore:
>
> What if you do it:
> if (!node)
> 	return NULL

I didn't notice we can do that.

Looks like the style I prefer too.

I'll use that style in next version.

Thanks,
Qu
>
> and then have the initialization happen with just 1 level of nesting
> inside the function and not 2. That function is short and easy to
> understand but it might be a good time to refactor like that. If you
> think it's redundant then don't bother.
>
>>  	return node;
>>  }
>
> <snip>
>