[PATCH 0/7] More btrfs_path auto cleaning

[PATCH 0/7] More btrfs_path auto cleaning

[David Sterba]

A few more conversions to automatic freeing of btrfs_path, same
separation as last time, first patch with the trivial ones and separated
when there are goto/return conversion.

David Sterba (7):
  btrfs: do more trivial BTRFS_PATH_AUTO_FREE conversions
  btrfs: use BTRFS_PATH_AUTO_FREE in may_destroy_subvol()
  btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_set_inode_index_count()
  btrfs: use BTRFS_PATH_AUTO_FREE in can_nocow_extent()
  btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_encoded_read_inline()
  btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_del_inode_extref()
  btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_insert_inode_extref()

 fs/btrfs/block-group.c      |   3 +-
 fs/btrfs/fiemap.c           |   3 +-
 fs/btrfs/file-item.c        |   3 +-
 fs/btrfs/file.c             |   3 +-
 fs/btrfs/free-space-cache.c |   3 +-
 fs/btrfs/inode-item.c       |  31 ++++-----
 fs/btrfs/inode.c            | 123 ++++++++++++++----------------------
 7 files changed, 65 insertions(+), 104 deletions(-)

-- 
2.48.1

[PATCH 1/7] btrfs: do more trivial BTRFS_PATH_AUTO_FREE conversions

[David Sterba]

The most trivial pattern for the auto freeing when the variable is
declared with the macro and the final btrfs_free_path() is removed.
There are almost none goto -> return conversions and there's no other
function cleanup.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/block-group.c      |  3 +--
 fs/btrfs/fiemap.c           |  3 +--
 fs/btrfs/file-item.c        |  3 +--
 fs/btrfs/file.c             |  3 +--
 fs/btrfs/free-space-cache.c |  3 +--
 fs/btrfs/inode.c            | 27 +++++++++------------------
 6 files changed, 14 insertions(+), 28 deletions(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index a38578c60f34e6..3eba00da9fc7a5 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -700,7 +700,7 @@ static int load_extent_tree_free(struct btrfs_caching_control *caching_ctl)
 	struct btrfs_block_group *block_group = caching_ctl->block_group;
 	struct btrfs_fs_info *fs_info = block_group->fs_info;
 	struct btrfs_root *extent_root;
-	struct btrfs_path *path;
+	BTRFS_PATH_AUTO_FREE(path);
 	struct extent_buffer *leaf;
 	struct btrfs_key key;
 	u64 total_found = 0;
@@ -827,7 +827,6 @@ static int load_extent_tree_free(struct btrfs_caching_control *caching_ctl)
 				       block_group->start + block_group->length,
 				       NULL);
 out:
-	btrfs_free_path(path);
 	return ret;
 }
 
diff --git a/fs/btrfs/fiemap.c b/fs/btrfs/fiemap.c
index b80c07ad8c5e71..7715e30508c575 100644
--- a/fs/btrfs/fiemap.c
+++ b/fs/btrfs/fiemap.c
@@ -634,7 +634,7 @@ static int extent_fiemap(struct btrfs_inode *inode,
 	const u64 ino = btrfs_ino(inode);
 	struct extent_state *cached_state = NULL;
 	struct extent_state *delalloc_cached_state = NULL;
-	struct btrfs_path *path;
+	BTRFS_PATH_AUTO_FREE(path);
 	struct fiemap_cache cache = { 0 };
 	struct btrfs_backref_share_check_ctx *backref_ctx;
 	u64 last_extent_end = 0;
@@ -874,7 +874,6 @@ static int extent_fiemap(struct btrfs_inode *inode,
 	free_extent_state(delalloc_cached_state);
 	kfree(cache.entries);
 	btrfs_free_backref_share_ctx(backref_ctx);
-	btrfs_free_path(path);
 	return ret;
 }
 
diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
index 344b4db487a0c6..c191be6bcefbd2 100644
--- a/fs/btrfs/file-item.c
+++ b/fs/btrfs/file-item.c
@@ -1048,7 +1048,7 @@ int btrfs_csum_file_blocks(struct btrfs_trans_handle *trans,
 	struct btrfs_fs_info *fs_info = root->fs_info;
 	struct btrfs_key file_key;
 	struct btrfs_key found_key;
-	struct btrfs_path *path;
+	BTRFS_PATH_AUTO_FREE(path);
 	struct btrfs_csum_item *item;
 	struct btrfs_csum_item *item_end;
 	struct extent_buffer *leaf = NULL;
@@ -1259,7 +1259,6 @@ int btrfs_csum_file_blocks(struct btrfs_trans_handle *trans,
 		goto again;
 	}
 out:
-	btrfs_free_path(path);
 	return ret;
 }
 
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index fe81129cfbf1b7..e7e8c477f83701 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -553,7 +553,7 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
 {
 	struct btrfs_root *root = inode->root;
 	struct extent_buffer *leaf;
-	struct btrfs_path *path;
+	BTRFS_PATH_AUTO_FREE(path);
 	struct btrfs_file_extent_item *fi;
 	struct btrfs_ref ref = { 0 };
 	struct btrfs_key key;
@@ -791,7 +791,6 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
 		}
 	}
 out:
-	btrfs_free_path(path);
 	return ret;
 }
 
diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
index 1b3082e81220d2..f66a0a6e505079 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -308,7 +308,7 @@ int btrfs_truncate_free_space_cache(struct btrfs_trans_handle *trans,
 	bool locked = false;
 
 	if (block_group) {
-		struct btrfs_path *path = btrfs_alloc_path();
+		BTRFS_PATH_AUTO_FREE(path);
 
 		if (!path) {
 			ret = -ENOMEM;
@@ -330,7 +330,6 @@ int btrfs_truncate_free_space_cache(struct btrfs_trans_handle *trans,
 		spin_lock(&block_group->lock);
 		block_group->disk_cache_state = BTRFS_DC_CLEAR;
 		spin_unlock(&block_group->lock);
-		btrfs_free_path(path);
 	}
 
 	btrfs_i_size_write(inode, 0);
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 520b12638ee49c..db989572cba419 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2933,7 +2933,7 @@ static int insert_reserved_file_extent(struct btrfs_trans_handle *trans,
 {
 	struct btrfs_root *root = inode->root;
 	const u64 sectorsize = root->fs_info->sectorsize;
-	struct btrfs_path *path;
+	BTRFS_PATH_AUTO_FREE(path);
 	struct extent_buffer *leaf;
 	struct btrfs_key ins;
 	u64 disk_num_bytes = btrfs_stack_file_extent_disk_num_bytes(stack_fi);
@@ -3015,8 +3015,6 @@ static int insert_reserved_file_extent(struct btrfs_trans_handle *trans,
 					       file_pos - offset,
 					       qgroup_reserved, &ins);
 out:
-	btrfs_free_path(path);
-
 	return ret;
 }
 
@@ -3540,7 +3538,7 @@ static int btrfs_orphan_del(struct btrfs_trans_handle *trans,
 int btrfs_orphan_cleanup(struct btrfs_root *root)
 {
 	struct btrfs_fs_info *fs_info = root->fs_info;
-	struct btrfs_path *path;
+	BTRFS_PATH_AUTO_FREE(path);
 	struct extent_buffer *leaf;
 	struct btrfs_key key, found_key;
 	struct btrfs_trans_handle *trans;
@@ -3730,7 +3728,6 @@ int btrfs_orphan_cleanup(struct btrfs_root *root)
 out:
 	if (ret)
 		btrfs_err(fs_info, "could not do orphan cleanup %d", ret);
-	btrfs_free_path(path);
 	return ret;
 }
 
@@ -4123,7 +4120,7 @@ static noinline int btrfs_update_inode_item(struct btrfs_trans_handle *trans,
 					    struct btrfs_inode *inode)
 {
 	struct btrfs_inode_item *inode_item;
-	struct btrfs_path *path;
+	BTRFS_PATH_AUTO_FREE(path);
 	struct extent_buffer *leaf;
 	struct btrfs_key key;
 	int ret;
@@ -4137,7 +4134,7 @@ static noinline int btrfs_update_inode_item(struct btrfs_trans_handle *trans,
 	if (ret) {
 		if (ret > 0)
 			ret = -ENOENT;
-		goto failed;
+		return ret;
 	}
 
 	leaf = path->nodes[0];
@@ -4146,10 +4143,7 @@ static noinline int btrfs_update_inode_item(struct btrfs_trans_handle *trans,
 
 	fill_inode_item(trans, leaf, inode_item, &inode->vfs_inode);
 	btrfs_set_inode_last_trans(trans, inode);
-	ret = 0;
-failed:
-	btrfs_free_path(path);
-	return ret;
+	return 0;
 }
 
 /*
@@ -5456,7 +5450,7 @@ static int btrfs_inode_by_name(struct btrfs_inode *dir, struct dentry *dentry,
 			       struct btrfs_key *location, u8 *type)
 {
 	struct btrfs_dir_item *di;
-	struct btrfs_path *path;
+	BTRFS_PATH_AUTO_FREE(path);
 	struct btrfs_root *root = dir->root;
 	int ret = 0;
 	struct fscrypt_name fname;
@@ -5467,7 +5461,7 @@ static int btrfs_inode_by_name(struct btrfs_inode *dir, struct dentry *dentry,
 
 	ret = fscrypt_setup_filename(&dir->vfs_inode, &dentry->d_name, 1, &fname);
 	if (ret < 0)
-		goto out;
+		return ret;
 	/*
 	 * fscrypt_setup_filename() should never return a positive value, but
 	 * gcc on sparc/parisc thinks it can, so assert that doesn't happen.
@@ -5496,7 +5490,6 @@ static int btrfs_inode_by_name(struct btrfs_inode *dir, struct dentry *dentry,
 		*type = btrfs_dir_ftype(path->nodes[0], di);
 out:
 	fscrypt_free_filename(&fname);
-	btrfs_free_path(path);
 	return ret;
 }
 
@@ -5511,7 +5504,7 @@ static int fixup_tree_root_location(struct btrfs_fs_info *fs_info,
 				    struct btrfs_key *location,
 				    struct btrfs_root **sub_root)
 {
-	struct btrfs_path *path;
+	BTRFS_PATH_AUTO_FREE(path);
 	struct btrfs_root *new_root;
 	struct btrfs_root_ref *ref;
 	struct extent_buffer *leaf;
@@ -5567,7 +5560,6 @@ static int fixup_tree_root_location(struct btrfs_fs_info *fs_info,
 	location->offset = 0;
 	err = 0;
 out:
-	btrfs_free_path(path);
 	fscrypt_free_filename(&fname);
 	return err;
 }
@@ -5988,7 +5980,7 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
 	struct btrfs_dir_item *di;
 	struct btrfs_key key;
 	struct btrfs_key found_key;
-	struct btrfs_path *path;
+	BTRFS_PATH_AUTO_FREE(path);
 	void *addr;
 	LIST_HEAD(ins_list);
 	LIST_HEAD(del_list);
@@ -6101,7 +6093,6 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
 err:
 	if (put)
 		btrfs_readdir_put_delayed_items(BTRFS_I(inode), &ins_list, &del_list);
-	btrfs_free_path(path);
 	return ret;
 }
 
-- 
2.48.1

[PATCH 2/7] btrfs: use BTRFS_PATH_AUTO_FREE in may_destroy_subvol()

[David Sterba]

This is the trivial pattern for path auto free, initialize at the
beginning and free at the end with simple goto -> return conversions.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/inode.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index db989572cba419..b3c2847ddae274 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4477,7 +4477,7 @@ static int btrfs_unlink_subvol(struct btrfs_trans_handle *trans,
 static noinline int may_destroy_subvol(struct btrfs_root *root)
 {
 	struct btrfs_fs_info *fs_info = root->fs_info;
-	struct btrfs_path *path;
+	BTRFS_PATH_AUTO_FREE(path);
 	struct btrfs_dir_item *di;
 	struct btrfs_key key;
 	struct fscrypt_str name = FSTR_INIT("default", 7);
@@ -4499,7 +4499,7 @@ static noinline int may_destroy_subvol(struct btrfs_root *root)
 			btrfs_err(fs_info,
 				  "deleting default subvolume %llu is not allowed",
 				  key.objectid);
-			goto out;
+			return ret;
 		}
 		btrfs_release_path(path);
 	}
@@ -4510,14 +4510,13 @@ static noinline int may_destroy_subvol(struct btrfs_root *root)
 
 	ret = btrfs_search_slot(NULL, fs_info->tree_root, &key, path, 0, 0);
 	if (ret < 0)
-		goto out;
+		return ret;
 	if (ret == 0) {
 		/*
 		 * Key with offset -1 found, there would have to exist a root
 		 * with such id, but this is out of valid range.
 		 */
-		ret = -EUCLEAN;
-		goto out;
+		return -EUCLEAN;
 	}
 
 	ret = 0;
@@ -4527,8 +4526,7 @@ static noinline int may_destroy_subvol(struct btrfs_root *root)
 		if (key.objectid == btrfs_root_id(root) && key.type == BTRFS_ROOT_REF_KEY)
 			ret = -ENOTEMPTY;
 	}
-out:
-	btrfs_free_path(path);
+
 	return ret;
 }
 
-- 
2.48.1

[PATCH 3/7] btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_set_inode_index_count()

[David Sterba]

This is the trivial pattern for path auto free, initialize at the
beginning and free at the end with simple goto -> return conversions.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/inode.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index b3c2847ddae274..4f862fc8ee52a3 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -5836,7 +5836,7 @@ static int btrfs_set_inode_index_count(struct btrfs_inode *inode)
 {
 	struct btrfs_root *root = inode->root;
 	struct btrfs_key key, found_key;
-	struct btrfs_path *path;
+	BTRFS_PATH_AUTO_FREE(path);
 	struct extent_buffer *leaf;
 	int ret;
 
@@ -5850,15 +5850,14 @@ static int btrfs_set_inode_index_count(struct btrfs_inode *inode)
 
 	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
 	if (ret < 0)
-		goto out;
+		return ret;
 	/* FIXME: we should be able to handle this */
 	if (ret == 0)
-		goto out;
-	ret = 0;
+		return ret;
 
 	if (path->slots[0] == 0) {
 		inode->index_cnt = BTRFS_DIR_START_INDEX;
-		goto out;
+		return 0;
 	}
 
 	path->slots[0]--;
@@ -5869,13 +5868,12 @@ static int btrfs_set_inode_index_count(struct btrfs_inode *inode)
 	if (found_key.objectid != btrfs_ino(inode) ||
 	    found_key.type != BTRFS_DIR_INDEX_KEY) {
 		inode->index_cnt = BTRFS_DIR_START_INDEX;
-		goto out;
+		return 0;
 	}
 
 	inode->index_cnt = found_key.offset + 1;
-out:
-	btrfs_free_path(path);
-	return ret;
+
+	return 0;
 }
 
 static int btrfs_get_dir_last_index(struct btrfs_inode *dir, u64 *index)
-- 
2.48.1

[PATCH 4/7] btrfs: use BTRFS_PATH_AUTO_FREE in can_nocow_extent()

[David Sterba]

This is the trivial pattern for path auto free, initialize at the
beginning and free at the end with simple goto -> return conversions.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/inode.c | 37 +++++++++++++++----------------------
 1 file changed, 15 insertions(+), 22 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 4f862fc8ee52a3..3d0f1aedfa7e23 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -7088,7 +7088,7 @@ noinline int can_nocow_extent(struct btrfs_inode *inode, u64 offset, u64 *len,
 	struct btrfs_root *root = inode->root;
 	struct btrfs_fs_info *fs_info = root->fs_info;
 	struct can_nocow_file_extent_args nocow_args = { 0 };
-	struct btrfs_path *path;
+	BTRFS_PATH_AUTO_FREE(path);
 	int ret;
 	struct extent_buffer *leaf;
 	struct extent_io_tree *io_tree = &inode->io_tree;
@@ -7104,13 +7104,12 @@ noinline int can_nocow_extent(struct btrfs_inode *inode, u64 offset, u64 *len,
 	ret = btrfs_lookup_file_extent(NULL, root, path, btrfs_ino(inode),
 				       offset, 0);
 	if (ret < 0)
-		goto out;
+		return ret;
 
 	if (ret == 1) {
 		if (path->slots[0] == 0) {
-			/* can't find the item, must cow */
-			ret = 0;
-			goto out;
+			/* Can't find the item, must COW. */
+			return 0;
 		}
 		path->slots[0]--;
 	}
@@ -7119,17 +7118,17 @@ noinline int can_nocow_extent(struct btrfs_inode *inode, u64 offset, u64 *len,
 	btrfs_item_key_to_cpu(leaf, &key, path->slots[0]);
 	if (key.objectid != btrfs_ino(inode) ||
 	    key.type != BTRFS_EXTENT_DATA_KEY) {
-		/* not our file or wrong item type, must cow */
-		goto out;
+		/* Not our file or wrong item type, must COW. */
+		return 0;
 	}
 
 	if (key.offset > offset) {
-		/* Wrong offset, must cow */
-		goto out;
+		/* Wrong offset, must COW. */
+		return 0;
 	}
 
 	if (btrfs_file_extent_end(path) <= offset)
-		goto out;
+		return 0;
 
 	fi = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_file_extent_item);
 	found_type = btrfs_file_extent_type(leaf, fi);
@@ -7144,15 +7143,13 @@ noinline int can_nocow_extent(struct btrfs_inode *inode, u64 offset, u64 *len,
 
 	if (ret != 1) {
 		/* Treat errors as not being able to NOCOW. */
-		ret = 0;
-		goto out;
+		return 0;
 	}
 
-	ret = 0;
 	if (btrfs_extent_readonly(fs_info,
 				  nocow_args.file_extent.disk_bytenr +
 				  nocow_args.file_extent.offset))
-		goto out;
+		return 0;
 
 	if (!(inode->flags & BTRFS_INODE_NODATACOW) &&
 	    found_type == BTRFS_FILE_EXTENT_PREALLOC) {
@@ -7161,20 +7158,16 @@ noinline int can_nocow_extent(struct btrfs_inode *inode, u64 offset, u64 *len,
 		range_end = round_up(offset + nocow_args.file_extent.num_bytes,
 				     root->fs_info->sectorsize) - 1;
 		ret = test_range_bit_exists(io_tree, offset, range_end, EXTENT_DELALLOC);
-		if (ret) {
-			ret = -EAGAIN;
-			goto out;
-		}
+		if (ret)
+			return -EAGAIN;
 	}
 
 	if (file_extent)
 		memcpy(file_extent, &nocow_args.file_extent, sizeof(*file_extent));
 
 	*len = nocow_args.file_extent.num_bytes;
-	ret = 1;
-out:
-	btrfs_free_path(path);
-	return ret;
+
+	return 1;
 }
 
 /* The callers of this must take lock_extent() */
-- 
2.48.1

[PATCH 5/7] btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_encoded_read_inline()

[David Sterba]

This is the trivial pattern for path auto free, initialize at the
beginning and free at the end with simple goto -> return conversions.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/inode.c | 31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 3d0f1aedfa7e23..a4e2193d4f7171 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -9080,7 +9080,7 @@ static ssize_t btrfs_encoded_read_inline(
 	struct btrfs_root *root = inode->root;
 	struct btrfs_fs_info *fs_info = root->fs_info;
 	struct extent_io_tree *io_tree = &inode->io_tree;
-	struct btrfs_path *path;
+	BTRFS_PATH_AUTO_FREE(path);
 	struct extent_buffer *leaf;
 	struct btrfs_file_extent_item *item;
 	u64 ram_bytes;
@@ -9090,10 +9090,8 @@ static ssize_t btrfs_encoded_read_inline(
 	const bool nowait = (iocb->ki_flags & IOCB_NOWAIT);
 
 	path = btrfs_alloc_path();
-	if (!path) {
-		ret = -ENOMEM;
-		goto out;
-	}
+	if (!path)
+		return -ENOMEM;
 
 	path->nowait = nowait;
 
@@ -9102,9 +9100,9 @@ static ssize_t btrfs_encoded_read_inline(
 	if (ret) {
 		if (ret > 0) {
 			/* The extent item disappeared? */
-			ret = -EIO;
+			return -EIO;
 		}
-		goto out;
+		return ret;
 	}
 	leaf = path->nodes[0];
 	item = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_file_extent_item);
@@ -9117,17 +9115,16 @@ static ssize_t btrfs_encoded_read_inline(
 	ret = btrfs_encoded_io_compression_from_extent(fs_info,
 				 btrfs_file_extent_compression(leaf, item));
 	if (ret < 0)
-		goto out;
+		return ret;
 	encoded->compression = ret;
 	if (encoded->compression) {
 		size_t inline_size;
 
 		inline_size = btrfs_file_extent_inline_item_len(leaf,
 								path->slots[0]);
-		if (inline_size > count) {
-			ret = -ENOBUFS;
-			goto out;
-		}
+		if (inline_size > count)
+			return -ENOBUFS;
+
 		count = inline_size;
 		encoded->unencoded_len = ram_bytes;
 		encoded->unencoded_offset = iocb->ki_pos - extent_start;
@@ -9139,10 +9136,9 @@ static ssize_t btrfs_encoded_read_inline(
 	}
 
 	tmp = kmalloc(count, GFP_NOFS);
-	if (!tmp) {
-		ret = -ENOMEM;
-		goto out;
-	}
+	if (!tmp)
+		return -ENOMEM;
+
 	read_extent_buffer(leaf, tmp, ptr, count);
 	btrfs_release_path(path);
 	unlock_extent(io_tree, start, lockend, cached_state);
@@ -9153,8 +9149,7 @@ static ssize_t btrfs_encoded_read_inline(
 	if (ret != count)
 		ret = -EFAULT;
 	kfree(tmp);
-out:
-	btrfs_free_path(path);
+
 	return ret;
 }
 
-- 
2.48.1

[PATCH 6/7] btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_del_inode_extref()

[David Sterba]

This is the trivial pattern for path auto free, initialize at the
beginning and free at the end with simple goto -> return conversions.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/inode-item.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/fs/btrfs/inode-item.c b/fs/btrfs/inode-item.c
index 3530de0618c8be..b920250e9e8ff7 100644
--- a/fs/btrfs/inode-item.c
+++ b/fs/btrfs/inode-item.c
@@ -109,7 +109,7 @@ static int btrfs_del_inode_extref(struct btrfs_trans_handle *trans,
 				  u64 inode_objectid, u64 ref_objectid,
 				  u64 *index)
 {
-	struct btrfs_path *path;
+	BTRFS_PATH_AUTO_FREE(path);
 	struct btrfs_key key;
 	struct btrfs_inode_extref *extref;
 	struct extent_buffer *leaf;
@@ -129,9 +129,9 @@ static int btrfs_del_inode_extref(struct btrfs_trans_handle *trans,
 
 	ret = btrfs_search_slot(trans, root, &key, path, -1, 1);
 	if (ret > 0)
-		ret = -ENOENT;
+		return -ENOENT;
 	if (ret < 0)
-		goto out;
+		return ret;
 
 	/*
 	 * Sanity check - did we find the right item for this name?
@@ -142,8 +142,7 @@ static int btrfs_del_inode_extref(struct btrfs_trans_handle *trans,
 						ref_objectid, name);
 	if (!extref) {
 		btrfs_abort_transaction(trans, -ENOENT);
-		ret = -ENOENT;
-		goto out;
+		return -ENOENT;
 	}
 
 	leaf = path->nodes[0];
@@ -152,12 +151,8 @@ static int btrfs_del_inode_extref(struct btrfs_trans_handle *trans,
 		*index = btrfs_inode_extref_index(leaf, extref);
 
 	if (del_len == item_size) {
-		/*
-		 * Common case only one ref in the item, remove the
-		 * whole item.
-		 */
-		ret = btrfs_del_item(trans, root, path);
-		goto out;
+		/* Common case only one ref in the item, remove the whole item. */
+		return btrfs_del_item(trans, root, path);
 	}
 
 	ptr = (unsigned long)extref;
@@ -168,9 +163,6 @@ static int btrfs_del_inode_extref(struct btrfs_trans_handle *trans,
 
 	btrfs_truncate_item(trans, path, item_size - del_len, 1);
 
-out:
-	btrfs_free_path(path);
-
 	return ret;
 }
 
-- 
2.48.1

[PATCH 7/7] btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_insert_inode_extref()

[David Sterba]

This is the trivial pattern for path auto free, initialize at the
beginning and free at the end with simple goto -> return conversions.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/inode-item.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/inode-item.c b/fs/btrfs/inode-item.c
index b920250e9e8ff7..a61c3540d67bad 100644
--- a/fs/btrfs/inode-item.c
+++ b/fs/btrfs/inode-item.c
@@ -252,7 +252,7 @@ static int btrfs_insert_inode_extref(struct btrfs_trans_handle *trans,
 	int ret;
 	int ins_len = name->len + sizeof(*extref);
 	unsigned long ptr;
-	struct btrfs_path *path;
+	BTRFS_PATH_AUTO_FREE(path);
 	struct btrfs_key key;
 	struct extent_buffer *leaf;
 
@@ -271,13 +271,13 @@ static int btrfs_insert_inode_extref(struct btrfs_trans_handle *trans,
 						   path->slots[0],
 						   ref_objectid,
 						   name))
-			goto out;
+			return ret;
 
 		btrfs_extend_item(trans, path, ins_len);
 		ret = 0;
 	}
 	if (ret < 0)
-		goto out;
+		return ret;
 
 	leaf = path->nodes[0];
 	ptr = (unsigned long)btrfs_item_ptr(leaf, path->slots[0], char);
@@ -290,9 +290,8 @@ static int btrfs_insert_inode_extref(struct btrfs_trans_handle *trans,
 
 	ptr = (unsigned long)&extref->name;
 	write_extent_buffer(path->nodes[0], name->name, ptr, name->len);
-out:
-	btrfs_free_path(path);
-	return ret;
+
+	return 0;
 }
 
 /* Will return 0, -ENOMEM, -EMLINK, or -EEXIST or anything from the CoW path */
-- 
2.48.1

Re: [PATCH 1/7] btrfs: do more trivial BTRFS_PATH_AUTO_FREE conversions

[David Disseldorp]

Hi David

On Wed,  2 Apr 2025 01:18:06 +0200, David Sterba wrote:

> @@ -308,7 +308,7 @@ int btrfs_truncate_free_space_cache(struct btrfs_trans_handle *trans,
>  	bool locked = false;
>  
>  	if (block_group) {
> -		struct btrfs_path *path = btrfs_alloc_path();
> +		BTRFS_PATH_AUTO_FREE(path);
>  
>  		if (!path) {
>  			ret = -ENOMEM;

This one looks broken. btrfs_search_slot() needs it allocated.

Re: [PATCH 1/7] btrfs: do more trivial BTRFS_PATH_AUTO_FREE conversions

[David Sterba]

On Wed, Apr 02, 2025 at 11:39:51AM +1100, David Disseldorp wrote:
> Hi David
> 
> On Wed,  2 Apr 2025 01:18:06 +0200, David Sterba wrote:
> 
> > @@ -308,7 +308,7 @@ int btrfs_truncate_free_space_cache(struct btrfs_trans_handle *trans,
> >  	bool locked = false;
> >  
> >  	if (block_group) {
> > -		struct btrfs_path *path = btrfs_alloc_path();
> > +		BTRFS_PATH_AUTO_FREE(path);
> >  
> >  		if (!path) {
> >  			ret = -ENOMEM;
> 
> This one looks broken. btrfs_search_slot() needs it allocated.

Sorry, I don't see what you mean. There's no btrfs_search_slot() in
btrfs_truncate_free_space_cache(), perhaps you mean a different
function?

The macro BTRFS_PATH_AUTO_FREE still declares a pointer, only adds the
__cleanup parameter that calls btrfs_free_path() when the variable
leaves its scope. It's not declaring 'path' as plain variable, if I
interpret the comment 'needs it allocated' in that context.

Re: [PATCH 1/7] btrfs: do more trivial BTRFS_PATH_AUTO_FREE conversions

[David Disseldorp]

On Thu, 3 Apr 2025 00:02:25 +0200, David Sterba wrote:

> On Wed, Apr 02, 2025 at 11:39:51AM +1100, David Disseldorp wrote:
> > Hi David
> > 
> > On Wed,  2 Apr 2025 01:18:06 +0200, David Sterba wrote:
> >   
> > > @@ -308,7 +308,7 @@ int btrfs_truncate_free_space_cache(struct btrfs_trans_handle *trans,
> > >  	bool locked = false;
> > >  
> > >  	if (block_group) {
> > > -		struct btrfs_path *path = btrfs_alloc_path();
> > > +		BTRFS_PATH_AUTO_FREE(path);
> > >  
> > >  		if (!path) {
> > >  			ret = -ENOMEM;  
> > 
> > This one looks broken. btrfs_search_slot() needs it allocated.  
> 
> Sorry, I don't see what you mean. There's no btrfs_search_slot() in
> btrfs_truncate_free_space_cache(), perhaps you mean a different
> function?

This will jump straight through to the -ENOMEM goto fail path... What am
I missing here?
With 91e5bfe317d8f8471fbaa3e70cf66cae1314a516 I see:
#define BTRFS_PATH_AUTO_FREE(path_name)                                 \              
        struct btrfs_path *path_name __free(btrfs_free_path) = NULL 

I would expect your change to instead be something like:

   BTRFS_PATH_AUTO_FREE(path);
   path = btrfs_alloc_path();

Re: [PATCH 1/7] btrfs: do more trivial BTRFS_PATH_AUTO_FREE conversions

[David Sterba]

On Thu, Apr 03, 2025 at 10:14:09AM +1100, David Disseldorp wrote:
> On Thu, 3 Apr 2025 00:02:25 +0200, David Sterba wrote:
> 
> > On Wed, Apr 02, 2025 at 11:39:51AM +1100, David Disseldorp wrote:
> > > Hi David
> > > 
> > > On Wed,  2 Apr 2025 01:18:06 +0200, David Sterba wrote:
> > >   
> > > > @@ -308,7 +308,7 @@ int btrfs_truncate_free_space_cache(struct btrfs_trans_handle *trans,
> > > >  	bool locked = false;
> > > >  
> > > >  	if (block_group) {
> > > > -		struct btrfs_path *path = btrfs_alloc_path();
> > > > +		BTRFS_PATH_AUTO_FREE(path);
> > > >  
> > > >  		if (!path) {
> > > >  			ret = -ENOMEM;  
> > > 
> > > This one looks broken. btrfs_search_slot() needs it allocated.  
> > 
> > Sorry, I don't see what you mean. There's no btrfs_search_slot() in
> > btrfs_truncate_free_space_cache(), perhaps you mean a different
> > function?
> 
> This will jump straight through to the -ENOMEM goto fail path... What am
> I missing here?
> With 91e5bfe317d8f8471fbaa3e70cf66cae1314a516 I see:
> #define BTRFS_PATH_AUTO_FREE(path_name)                                 \              
>         struct btrfs_path *path_name __free(btrfs_free_path) = NULL 
> 
> I would expect your change to instead be something like:
> 
>    BTRFS_PATH_AUTO_FREE(path);
>    path = btrfs_alloc_path();

Duh, of course, you're right, thanks for catching it.

Re: [PATCH 0/7] More btrfs_path auto cleaning

[Daniel Vacek]

On Wed, 2 Apr 2025 at 01:18, David Sterba <dsterba@suse.com> wrote:
>
> A few more conversions to automatic freeing of btrfs_path, same
> separation as last time, first patch with the trivial ones and separated
> when there are goto/return conversion.
>
> David Sterba (7):
>   btrfs: do more trivial BTRFS_PATH_AUTO_FREE conversions
>   btrfs: use BTRFS_PATH_AUTO_FREE in may_destroy_subvol()
>   btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_set_inode_index_count()
>   btrfs: use BTRFS_PATH_AUTO_FREE in can_nocow_extent()
>   btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_encoded_read_inline()
>   btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_del_inode_extref()
>   btrfs: use BTRFS_PATH_AUTO_FREE in btrfs_insert_inode_extref()

With what David mentioned for 1/7 the series looks good to me.

Reviewed-by: Daniel Vacek <neelx@suse.com>

--nX

>
>  fs/btrfs/block-group.c      |   3 +-
>  fs/btrfs/fiemap.c           |   3 +-
>  fs/btrfs/file-item.c        |   3 +-
>  fs/btrfs/file.c             |   3 +-
>  fs/btrfs/free-space-cache.c |   3 +-
>  fs/btrfs/inode-item.c       |  31 ++++-----
>  fs/btrfs/inode.c            | 123 ++++++++++++++----------------------
>  7 files changed, 65 insertions(+), 104 deletions(-)
>
> --
> 2.48.1
>
>

Re: [PATCH 0/7] More btrfs_path auto cleaning

[Sun YangKai]

I'm new in lore, not having many experience in kernel development. I'm 
currently learning by read the code and patches. I'm just wondering why we 
cannot just make these `btrfs_path` become local variables so they can get 
freed automatically? Is it because the size of `btrfs_path` is too large to 
put them on stack?

Sorry for bothering here. I'm glad to move to else where if it is not prorate 
to discuss here.

Re: [PATCH 0/7] More btrfs_path auto cleaning

[David Sterba]

On Sat, Apr 05, 2025 at 12:13:58PM +0800, Sun YangKai wrote:
> I'm new in lore, not having many experience in kernel development. I'm 
> currently learning by read the code and patches. I'm just wondering why we 
> cannot just make these `btrfs_path` become local variables so they can get 
> freed automatically? Is it because the size of `btrfs_path` is too large to 
> put them on stack?

Yes, this is the reason. The size of the structure is 112 bytes, stack
size is fixed to 16K and is considered a scarce resource. It's not that
the filesystem would use the whole 16K of the stack but in connection
with other internal subsystems like memory management, block layer,
drivers and with other layers like device mapper or NFS, the overall
stack consumption matters and each layer should not waste it.

In some known cases like ioctl handlers that run from the process
context it's known that there's more stack left, basically only the
layers below, so there are on-stack structures that do not need to be
allocated.

> Sorry for bothering here. I'm glad to move to else where if it is not prorate 
> to discuss here.

No problem, it's a valid question and keeping a structure on stack may
simplify some things so it's something to look for, but as can be
demonstrated on this example it's not always safe. The btrfs_path cannot
be made smaller, the first member 'nodes' is already 64 bytes, the
remaining members can be possibly squeezed when smaller int types would
be used but this generates worse code and it's one of most frequently
used structures.

Re: [PATCH 0/7] More btrfs_path auto cleaning

[Sun YangKai]

Thank you for your kind reply. I've learned a lot here.