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

[Nikolay Borisov <nborisov@suse.com>]

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

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>