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

Nikolay Borisov
2020-03-04T13:06:45Z

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>