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;
>> + }
>> }
>>
>> /*
>>
