btrfs-progs: add a free_root_extent_buffers helper

Commit Message

Josef Bacik Aug. 25, 2023, 1:17 p.m. UTC

Our CI started failing a bunch because I accidentally introduced an
extent buffer leak.  This is because we haphazardly have ->commit_roots
used in btrfs-progs, and they get freed when the transaction commits and
then they're cleared out.  In the kernel we make sure to free all this
when we free the root, but we don't have the same thing in btrfs-progs.
Fix this by bringing over the free_root_extent_buffers helper and use
this for free'ing up all the roots.  This brings us inline with the
kernel more and eliminates the extent buffer leak.

Signed-off-by: Josef Bacik <[e-mail]>
---
 kernel-shared/disk-io.c | 48 +++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 26 deletions(-)

Comments

Anand Jain Aug. 29, 2023, 12:52 p.m. UTC | #1

LGTM

Reviewed-by: Anand Jain <[e-mail]>

David Sterba Aug. 29, 2023, 5:45 p.m. UTC | #2

On Fri, Aug 25, 2023 at 09:17:45AM -0400, Josef Bacik wrote:
> Our CI started failing a bunch because I accidentally introduced an
> extent buffer leak.  This is because we haphazardly have ->commit_roots
> used in btrfs-progs, and they get freed when the transaction commits and
> then they're cleared out.  In the kernel we make sure to free all this
> when we free the root, but we don't have the same thing in btrfs-progs.
> Fix this by bringing over the free_root_extent_buffers helper and use
> this for free'ing up all the roots.  This brings us inline with the
> kernel more and eliminates the extent buffer leak.

With this patch applied in devel I still see the leaks after mkfs.

David Sterba Sept. 5, 2023, 7:03 p.m. UTC | #3

On Tue, Aug 29, 2023 at 07:45:34PM +0200, David Sterba wrote:
> On Fri, Aug 25, 2023 at 09:17:45AM -0400, Josef Bacik wrote:
> > Our CI started failing a bunch because I accidentally introduced an
> > extent buffer leak.  This is because we haphazardly have ->commit_roots
> > used in btrfs-progs, and they get freed when the transaction commits and
> > then they're cleared out.  In the kernel we make sure to free all this
> > when we free the root, but we don't have the same thing in btrfs-progs.
> > Fix this by bringing over the free_root_extent_buffers helper and use
> > this for free'ing up all the roots.  This brings us inline with the
> > kernel more and eliminates the extent buffer leak.
> 
> With this patch applied in devel I still see the leaks after mkfs.

I've removed this patch, there are more patches introducing the leaks so
I'd like to get a clean series without the fixups.

Patch

diff --git a/kernel-shared/disk-io.c b/kernel-shared/disk-io.c
index 7916bf5c..9c987f7d 100644
--- a/kernel-shared/disk-io.c
+++ b/kernel-shared/disk-io.c
@@ -563,12 +563,21 @@  static int find_and_setup_log_root(struct btrfs_root *tree_root,
 	return 0;
 }
 
+static void free_root_extent_buffers(struct btrfs_root *root)
+{
+	if (root) {
+		if (root->node)
+			free_extent_buffer(root->node);
+		if (root->commit_root)
+			free_extent_buffer(root->commit_root);
+		root->node = NULL;
+		root->commit_root = NULL;
+	}
+}
+
 int btrfs_free_fs_root(struct btrfs_root *root)
 {
-	if (root->node)
-		free_extent_buffer(root->node);
-	if (root->commit_root)
-		free_extent_buffer(root->commit_root);
+	free_root_extent_buffers(root);
 	kfree(root);
 	return 0;
 }
@@ -1291,32 +1300,20 @@  static void release_global_roots(struct btrfs_fs_info *fs_info)
 
 	for (n = rb_first(&fs_info->global_roots_tree); n; n = rb_next(n)) {
 		root = rb_entry(n, struct btrfs_root, rb_node);
-		if (root->node)
-			free_extent_buffer(root->node);
-		if (root->commit_root)
-			free_extent_buffer(root->commit_root);
-		root->node = NULL;
-		root->commit_root = NULL;
+		free_root_extent_buffers(root);
 	}
 }
 
 void btrfs_release_all_roots(struct btrfs_fs_info *fs_info)
 {
 	release_global_roots(fs_info);
-	if (fs_info->block_group_root)
-		free_extent_buffer(fs_info->block_group_root->node);
-	if (fs_info->quota_root)
-		free_extent_buffer(fs_info->quota_root->node);
-	if (fs_info->dev_root)
-		free_extent_buffer(fs_info->dev_root->node);
-	if (fs_info->tree_root)
-		free_extent_buffer(fs_info->tree_root->node);
-	if (fs_info->log_root_tree)
-		free_extent_buffer(fs_info->log_root_tree->node);
-	if (fs_info->chunk_root)
-		free_extent_buffer(fs_info->chunk_root->node);
-	if (fs_info->uuid_root)
-		free_extent_buffer(fs_info->uuid_root->node);
+	free_root_extent_buffers(fs_info->block_group_root);
+	free_root_extent_buffers(fs_info->quota_root);
+	free_root_extent_buffers(fs_info->dev_root);
+	free_root_extent_buffers(fs_info->tree_root);
+	free_root_extent_buffers(fs_info->log_root_tree);
+	free_root_extent_buffers(fs_info->chunk_root);
+	free_root_extent_buffers(fs_info->uuid_root);
 }
 
 static void free_map_lookup(struct cache_extent *ce)
@@ -2300,8 +2297,7 @@  int btrfs_delete_and_free_root(struct btrfs_trans_handle *trans,
 		return ret;
 	if (is_global_root(root))
 		rb_erase(&root->rb_node, &fs_info->global_roots_tree);
-	free_extent_buffer(root->node);
-	free_extent_buffer(root->commit_root);
+	free_root_extent_buffers(root);
 	kfree(root);
 	return 0;
 }