Re: [PATCH v2 08/10] btrfs: relocation: Remove the open-coded goto loop for breadth-first search

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

On 2020/3/4 下午10:24, Nikolay Borisov wrote:
>
[...]
>> +	int err = 0;
>> +
>> +	iter = btrfs_backref_iter_alloc(rc->extent_root->fs_info, GFP_NOFS);
>> +	if (!iter)
>> +		return ERR_PTR(-ENOMEM);
>
> This iterator can be made private to handle_one_tree_block as I don't see it being used outside of that function.

It's kinda a performance optimization.

Instead of allocating memory for each loop, we allocate the memory just
once, and reuse it until the whole backref map for the bytenr is built.
>
>> +	path = btrfs_alloc_path();
>> +	if (!path) {
>> +		err = -ENOMEM;
>> +		goto out;
>> +	}
>
> Same thing with this path. Overall this will reduce the argument to handle_one_tree_block by 2.

Same performance optimization here.

>
>> +	path->reada = READA_FORWARD;
>> +
>> +	node = alloc_backref_node(cache, bytenr, level);
>> +	if (!node) {
>> +		err = -ENOMEM;
>> +		goto out;
>> +	}
>> +
>> +	node->lowest = 1;
>> +	cur = node;
>> +
>> +	/* Breadth-first search to build backref cache */
>> +	while (1) {
>> +		ret = handle_one_tree_block(rc, &useless, &list, path, iter,
>> +					    node_key, cur);
>> +		if (ret < 0) {
>> +			err = ret;
>> +			goto out;
>> +		}
>> +		/* the pending list isn't empty, take the first block to process */
>> +		if (!list_empty(&list)) {
>> +			edge = list_entry(list.next, struct backref_edge, list[UPPER]);
>
> Use list_first_entry_or_null or it would become:
>
> edge = list_first_entry_or_null();
> if (edge) {
> list_del_init(&edge->list[UPPER]);
> cur = edge->node[UPPER]
> } else {
> breakl
> }

That's an interesting wrapper. Would go that way.

>
> or simply if (!edge)
> break;
>
> Also this loop can be rewritten as a do {} while() and it will look:

Yep, but I'm not sure if such do {} while() loop is preferred.
IIRC there are some docs saying to avoid such loop?

If there is no such restriction, I would be pretty happy to go that way.

Thanks,
Qu

>
>         /* Breadth-first search to build backref cache */
>         do {
>                 ret = handle_one_tree_block(rc, &useless, &list, path, iter,
>                                             node_key, cur);
>                 if (ret < 0) {
>                         err = ret;
>                         goto out;
>                 }
>                 edge = list_first_entry_or_null(&list, struct backref_edge,
>                                                 list[UPPER]);
>                 /* the pending list isn't empty, take the first block to process */
>                 if (edge) {
>                         list_del_init(&edge->list[UPPER]);
>                         cur = edge->node[UPPER];
>                 }
>         } while (edge)
>
> IMO this is shorter than the original version and it's very expicit about it's terminating conditions:
> a). handle_one_tree_block returns an error
> b) list becomes empty.
>
> Alternatively list being empty is really a proxy for "is cur a valid inode". We know it's always
> valid on the first iteration since it's passed form the caller, subsequent iterations assign cur
> to edge->node[UPPER] so it could even be
>
> while(cur) {}
>
> In my opinion reducing while(1) loops where it makes sense (as in this case) is preferable.
>
> NB: I've only compile-tested it.
>
>> +			list_del_init(&edge->list[UPPER]);
>> +			cur = edge->node[UPPER];
>> +		} else {
>> +			break;
>> +		}
>>  	}
>>
>>  	/*
>>