On 3/3/20 9:59 AM, David Sterba wrote:
On Mon, Mar 02, 2020 at 01:47:51PM -0500, Josef Bacik wrote:
If we have an error while building the backref tree in relocation we'll
process all the pending edges and then free the node. This isn't quite
right however as the node could be integrated into the existing cache
partially, linking children within itself into the cache, but not
properly linked into the cache itself.
I'm missing description of what's the problem. Something is linked and
then freed, followed by 'fixed by'.
The fix for this is simple, use
remove_backref_node() instead of free_backref_node(), which will clean
up the cache related to this node completely.
So this means that some entries are left in the cache? Leaked memory or
something else?
Yeah leaked memory and root references, I'll update the changelog to be more clear.
Signed-off-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
---
fs/btrfs/relocation.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 4fb7e3cc2aca..507361e99316 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -1244,7 +1244,7 @@ struct backref_node *build_backref_tree(struct reloc_control *rc,
free_backref_node(cache, lower);
}
- free_backref_node(cache, node);
+ remove_backref_node(cache, node);
return ERR_PTR(err);
}
ASSERT(!node || !node->detached);
There's a similar pattern in clone_backref_node
1317 fail:
1318 while (!list_empty(&new_node->lower)) {
1319 new_edge = list_entry(new_node->lower.next,
1320 struct backref_edge, list[UPPER]);
1321 list_del(&new_edge->list[UPPER]);
1322 free_backref_edge(cache, new_edge);
1323 }
1324 free_backref_node(cache, new_node);
Does this also need to be fixed?
No this is fine, this essentially does what remove_backref_node() does. The
build_backref_tree() cleanup just handles the local lists, not the edges
attached to the node. clone_backref_node does the cleanup properly. Thanks,
Josef
