Btrfs: don't leak reloc root nodes on error

PATCH In review Josef Bacik 3 participants 6 messages Sep 2, 2016 – Sep 21, 2016 Btrfs file system

Share & tools

Josef Bacik

author

2016-09-02 19:25 UTC #0 lore raw

PATCH 1/1 Btrfs: don't leak reloc root nodes on error

We don't track the reloc roots in any sort of normal way, so the only way the root/commit_root nodes get free'd is if the relocation finishes successfully and the reloc root is deleted. Fix this by free'ing them in free_reloc_roots. Thanks,

Signed-off-by Josef Bacik <[email protected]>

---
 fs/btrfs/relocation.c | 4 ++++
 1 file changed, 4 insertions(+)

fs/btrfs/relocation.c +4 -0

index 7fc6ea7..62dfc2c 100644
@@ -2349,6 +2349,10 @@ void free_reloc_roots(struct list_head *list)
 	while (!list_empty(list)) {
 		reloc_root = list_entry(list->next, struct btrfs_root,
 					root_list);
+		free_extent_buffer(reloc_root->node);
+		free_extent_buffer(reloc_root->commit_root);
+		reloc_root->node = NULL;
+		reloc_root->commit_root = NULL;
 		__del_reloc_root(reloc_root);
 	}
 }

Signature · 1 line

2.7.4

Liu Bo

2016-09-03 01:08 UTC #1 lore raw

Quoted 4 lines · Josef Bacik

We don't track the reloc roots in any sort of normal way, so the only way the root/commit_root nodes get free'd is if the relocation finishes successfully and the reloc root is deleted. Fix this by free'ing them in free_reloc_roots. Thanks,

Looks good.

Quoted 11 lines

Signed-off-by Josef Bacik <[email protected]>
---
 fs/btrfs/relocation.c | 4 ++++
 1 file changed, 4 insertions(+)
fs/btrfs/relocation.c +0 -0
index 7fc6ea7..62dfc2c 100644

fs/btrfs/relocation.c void free_reloc_roots(struct list_head *list) · line 2355

Quoted 7 lines

fs/btrfs/relocation.c +4 -0

@@ -2349,6 +2349,10 @@ void free_reloc_roots(struct list_head *list)
 	while (!list_empty(list)) {
 		reloc_root = list_entry(list->next, struct btrfs_root,
 					root_list);
+		free_extent_buffer(reloc_root->node);
+		free_extent_buffer(reloc_root->commit_root);
+		reloc_root->node = NULL;
+		reloc_root->commit_root = NULL;

What about reloc_root itself?

Thanks,

-liubo

Quoted 10 lines

fs/btrfs/relocation.c +0 -0

 		__del_reloc_root(reloc_root);
 	}
 }

Signature · 6 lines

2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

David Sterba

2016-09-05 15:22 UTC #2 lore raw

Quoted 14 lines · Liu Bo

Quoted 4 lines · Josef Bacik
We don't track the reloc roots in any sort of normal way, so the only way the root/commit_root nodes get free'd is if the relocation finishes successfully and the reloc root is deleted. Fix this by free'ing them in free_reloc_roots. Thanks,
Looks good.
Quoted 6 lines
Signed-off-by Josef Bacik <[email protected]>
---
 fs/btrfs/relocation.c | 4 ++++
 1 file changed, 4 insertions(+)

Quoted 16 lines

fs/btrfs/relocation.c void free_reloc_roots(struct list_head *list) · line 2355

Quoted 12 lines

fs/btrfs/relocation.c +4 -0

index 7fc6ea7..62dfc2c 100644
@@ -2349,6 +2349,10 @@ void free_reloc_roots(struct list_head *list)
 	while (!list_empty(list)) {
 		reloc_root = list_entry(list->next, struct btrfs_root,
 					root_list);
+		free_extent_buffer(reloc_root->node);
+		free_extent_buffer(reloc_root->commit_root);
+		reloc_root->node = NULL;
+		reloc_root->commit_root = NULL;

What about reloc_root itself?

Quoted 1 line

fs/btrfs/relocation.c +0 -0
 		__del_reloc_root(reloc_root);

It's deleted at the end of __del_reloc_root

Liu Bo

2016-09-06 19:22 UTC #3 lore raw

Quoted 30 lines · David Sterba

Quoted 28 lines · Liu Bo
Quoted 4 lines · Josef Bacik
We don't track the reloc roots in any sort of normal way, so the only way the root/commit_root nodes get free'd is if the relocation finishes successfully and the reloc root is deleted. Fix this by free'ing them in free_reloc_roots. Thanks,
Looks good.
Quoted 11 lines
Signed-off-by Josef Bacik <[email protected]>
---
 fs/btrfs/relocation.c | 4 ++++
 1 file changed, 4 insertions(+)
fs/btrfs/relocation.c +0 -0
index 7fc6ea7..62dfc2c 100644
fs/btrfs/relocation.c void free_reloc_roots(struct list_head *list) · line 2355
Quoted 7 lines
fs/btrfs/relocation.c +4 -0
@@ -2349,6 +2349,10 @@ void free_reloc_roots(struct list_head *list)
 	while (!list_empty(list)) {
 		reloc_root = list_entry(list->next, struct btrfs_root,
 					root_list);
+		free_extent_buffer(reloc_root->node);
+		free_extent_buffer(reloc_root->commit_root);
+		reloc_root->node = NULL;
+		reloc_root->commit_root = NULL;
What about reloc_root itself?

Quoted 3 lines

Quoted 1 line
fs/btrfs/relocation.c void free_reloc_roots(struct list_head *list) · line 2356
Quoted 1 line
fs/btrfs/relocation.c +0 -0
 		__del_reloc_root(reloc_root);
It's deleted at the end of __del_reloc_root

__del_reloc_root() is dealing with the associated rb_node, not reloc_root.

I posted a patch to free reloc_root while free'ing its corresponding fs root, but it'd be good if we can free it here.

Thanks,

-liubo

David Sterba

2016-09-21 14:02 UTC #4 lore raw

Quoted 33 lines · Liu Bo

Quoted 32 lines · David Sterba
Quoted 30 lines · Liu Bo
Quoted 4 lines · Josef Bacik
We don't track the reloc roots in any sort of normal way, so the only way the root/commit_root nodes get free'd is if the relocation finishes successfully and the reloc root is deleted. Fix this by free'ing them in free_reloc_roots. Thanks,
Looks good.
Quoted 11 lines
Signed-off-by Josef Bacik <[email protected]>
---
 fs/btrfs/relocation.c | 4 ++++
 1 file changed, 4 insertions(+)
fs/btrfs/relocation.c +0 -0
index 7fc6ea7..62dfc2c 100644
fs/btrfs/relocation.c void free_reloc_roots(struct list_head *list) · line 2355
Quoted 7 lines
fs/btrfs/relocation.c +4 -0
@@ -2349,6 +2349,10 @@ void free_reloc_roots(struct list_head *list)
 	while (!list_empty(list)) {
 		reloc_root = list_entry(list->next, struct btrfs_root,
 					root_list);
+		free_extent_buffer(reloc_root->node);
+		free_extent_buffer(reloc_root->commit_root);
+		reloc_root->node = NULL;
+		reloc_root->commit_root = NULL;
What about reloc_root itself?
Quoted 1 line
fs/btrfs/relocation.c +0 -0
 		__del_reloc_root(reloc_root);

Quoted 7 lines

Quoted 1 line
It's deleted at the end of __del_reloc_root
__del_reloc_root() is dealing with the associated rb_node, not reloc_root.
I posted a patch to free reloc_root while free'ing its corresponding fs root, but it'd be good if we can free it here.

Can you please point me to the patch?

Liu Bo

2016-09-21 22:45 UTC #5 lore raw

Re: [PATCH] Btrfs: don't leak reloc root nodes on errorg

Quoted 36 lines · David Sterba

Quoted 35 lines · Liu Bo
Quoted 33 lines · David Sterba
Quoted 14 lines · Liu Bo
Quoted 4 lines · Josef Bacik
We don't track the reloc roots in any sort of normal way, so the only way the root/commit_root nodes get free'd is if the relocation finishes successfully and the reloc root is deleted. Fix this by free'ing them in free_reloc_roots. Thanks,
Looks good.
Quoted 6 lines
Signed-off-by Josef Bacik <[email protected]>
---
 fs/btrfs/relocation.c | 4 ++++
 1 file changed, 4 insertions(+)
Quoted 16 lines
fs/btrfs/relocation.c void free_reloc_roots(struct list_head *list) · line 2355
Quoted 12 lines
fs/btrfs/relocation.c +4 -0
index 7fc6ea7..62dfc2c 100644
@@ -2349,6 +2349,10 @@ void free_reloc_roots(struct list_head *list)
 	while (!list_empty(list)) {
 		reloc_root = list_entry(list->next, struct btrfs_root,
 					root_list);
+		free_extent_buffer(reloc_root->node);
+		free_extent_buffer(reloc_root->commit_root);
+		reloc_root->node = NULL;
+		reloc_root->commit_root = NULL;
What about reloc_root itself?
Quoted 1 line
fs/btrfs/relocation.c +0 -0
 		__del_reloc_root(reloc_root);
It's deleted at the end of __del_reloc_root

Quoted 7 lines

Quoted 5 lines
__del_reloc_root() is dealing with the associated rb_node, not reloc_root.
I posted a patch to free reloc_root while free'ing its corresponding fs root, but it'd be good if we can free it here.
Can you please point me to the patch?

Btrfs: fix memory leak of reloc_root
https://patchwork.kernel.org/patch/9238383/

Thanks,

-liubo