diff mbox series

[1/2] btrfs: remove conditional path allocation from read_locked_inode, add path allocation to iget

Message ID 32dea3edada81f7901f7f7e39f4e3729888dea46.1723580508.git.loemra.dev@gmail.com (mailing list archive)
State New, archived
Headers show
Series btrfs: clean up btrfs_iget, btrfs_iget_path usage | expand

Commit Message

Leo Martins Aug. 13, 2024, 8:27 p.m. UTC 
Move the path allocation from inside btrfs_read_locked_inode to
btrfs_iget. This makes the code easier to reason about as it is clear
where the allocation occurs and who is in charge of freeing the path.
I have investigated all of the callers of btrfs_iget_path to make sure 
that it is never called with a null path with the expectation
of a path allocation. All of the null calls seem to come from btrfs_iget
so it makes sense to do the allocation within btrfs_iget.

---
 fs/btrfs/inode.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

Comments

David Sterba Aug. 13, 2024, 10:21 p.m. UTC | #1 
On Tue, Aug 13, 2024 at 01:27:33PM -0700, Leo Martins wrote:
> Move the path allocation from inside btrfs_read_locked_inode to
> btrfs_iget. This makes the code easier to reason about as it is clear
> where the allocation occurs and who is in charge of freeing the path.
> I have investigated all of the callers of btrfs_iget_path to make sure 
> that it is never called with a null path with the expectation
> of a path allocation. All of the null calls seem to come from btrfs_iget
> so it makes sense to do the allocation within btrfs_iget.

This looks like a good cleanup. The path argument was added to fix some
problem in the past, 4222ea7100c0e3 ("Btrfs: fix deadlock on tree root
leaf when finding free extent") but doing the allocation in the caller
makes things more clear.

> ---
>  fs/btrfs/inode.c | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 07858d63378f..b89b4b1bd3da 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -3790,10 +3790,9 @@ static int btrfs_init_file_extent_tree(struct btrfs_inode *inode)
>   * read an inode from the btree into the in-memory inode
>   */
>  static int btrfs_read_locked_inode(struct inode *inode,
> -				   struct btrfs_path *in_path)
> +				   struct btrfs_path *path)
>  {
>  	struct btrfs_fs_info *fs_info = inode_to_fs_info(inode);
> -	struct btrfs_path *path = in_path;
>  	struct extent_buffer *leaf;
>  	struct btrfs_inode_item *inode_item;
>  	struct btrfs_root *root = BTRFS_I(inode)->root;
> @@ -3813,18 +3812,10 @@ static int btrfs_read_locked_inode(struct inode *inode,
>  	if (!ret)
>  		filled = true;

You can add an ASSERT(path)

>  
> -	if (!path) {
> -		path = btrfs_alloc_path();
> -		if (!path)
> -			return -ENOMEM;
> -	}
> -
>  	btrfs_get_inode_key(BTRFS_I(inode), &location);
>  
>  	ret = btrfs_lookup_inode(NULL, root, path, &location, 0);
>  	if (ret) {
> -		if (path != in_path)
> -			btrfs_free_path(path);
>  		return ret;
>  	}
>  
> @@ -3960,8 +3951,6 @@ static int btrfs_read_locked_inode(struct inode *inode,
>  				  btrfs_ino(BTRFS_I(inode)),
>  				  btrfs_root_id(root), ret);
>  	}
> -	if (path != in_path)
> -		btrfs_free_path(path);
>  
>  	if (!maybe_acls)
>  		cache_no_acl(inode);
> @@ -5631,7 +5620,15 @@ struct inode *btrfs_iget_path(u64 ino, struct btrfs_root *root,
>  
>  struct inode *btrfs_iget(u64 ino, struct btrfs_root *root)
>  {
> -	return btrfs_iget_path(ino, root, NULL);
> +	struct btrfs_path *path = btrfs_alloc_path();
> +
> +	if (!path)
> +		return ERR_PTR(-ENOMEM);
> +
> +	struct inode *inode = btrfs_iget_path(ino, root, path);

In kernel the declaration block should be contiguous, ie. not mixed with
statements like that. Also, we don't do allocatios (and other
non-trivial assignments that require error handling) in the declaration
block. This may different in other parts of kernel, in btrfs it's the
preferred way.
diff mbox series

Patch

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 07858d63378f..b89b4b1bd3da 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -3790,10 +3790,9 @@  static int btrfs_init_file_extent_tree(struct btrfs_inode *inode)
  * read an inode from the btree into the in-memory inode
  */
 static int btrfs_read_locked_inode(struct inode *inode,
-				   struct btrfs_path *in_path)
+				   struct btrfs_path *path)
 {
 	struct btrfs_fs_info *fs_info = inode_to_fs_info(inode);
-	struct btrfs_path *path = in_path;
 	struct extent_buffer *leaf;
 	struct btrfs_inode_item *inode_item;
 	struct btrfs_root *root = BTRFS_I(inode)->root;
@@ -3813,18 +3812,10 @@  static int btrfs_read_locked_inode(struct inode *inode,
 	if (!ret)
 		filled = true;
 
-	if (!path) {
-		path = btrfs_alloc_path();
-		if (!path)
-			return -ENOMEM;
-	}
-
 	btrfs_get_inode_key(BTRFS_I(inode), &location);
 
 	ret = btrfs_lookup_inode(NULL, root, path, &location, 0);
 	if (ret) {
-		if (path != in_path)
-			btrfs_free_path(path);
 		return ret;
 	}
 
@@ -3960,8 +3951,6 @@  static int btrfs_read_locked_inode(struct inode *inode,
 				  btrfs_ino(BTRFS_I(inode)),
 				  btrfs_root_id(root), ret);
 	}
-	if (path != in_path)
-		btrfs_free_path(path);
 
 	if (!maybe_acls)
 		cache_no_acl(inode);
@@ -5631,7 +5620,15 @@  struct inode *btrfs_iget_path(u64 ino, struct btrfs_root *root,
 
 struct inode *btrfs_iget(u64 ino, struct btrfs_root *root)
 {
-	return btrfs_iget_path(ino, root, NULL);
+	struct btrfs_path *path = btrfs_alloc_path();
+
+	if (!path)
+		return ERR_PTR(-ENOMEM);
+
+	struct inode *inode = btrfs_iget_path(ino, root, path);
+
+	btrfs_free_path(path);
+	return inode;
 }
 
 static struct inode *new_simple_dir(struct inode *dir,