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

[Josef Bacik <josef@toxicpanda.com>]

On Tue, Aug 20, 2024 at 01:13:18PM -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.
> 
> ---
>  fs/btrfs/inode.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index a8ad540d6de2..f2959803f9d7 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,20 +3812,13 @@ static int btrfs_read_locked_inode(struct inode *inode,
>  	if (!ret)
>  		filled = true;
>  
> -	if (!path) {
> -		path = btrfs_alloc_path();
> -		if (!path)
> -			return -ENOMEM;
> -	}
> +	ASSERT(path);
>  
>  	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);
> +	if (ret)
>  		return ret;
> -	}
>  
>  	leaf = path->nodes[0];
>  
> @@ -3960,8 +3952,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);
> @@ -5632,7 +5622,17 @@ 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;
> +	struct inode *inode;
> +
> +	path = btrfs_alloc_path();
> +	if (!path)
> +		return ERR_PTR(-ENOMEM);
> +

Actually now that I look at it, I don't want to do it this way.  Sorry about
that because I'm the one who suggested cleaning this all up, and I missed an
important piece here.

With btrfs_iget_path() we're doing the btrfs_iget_locked() first, which will
find the inode in cache if it can, so the path allocation isn't necessary in
that case.  I missed this when I was looking at this, so I think it's better to
move the path allocation out of btrfs_read_locked_inode(), but push it into
btrfs_iget_path() instead.  So btrfs_iget_path() will handle path == NULL the
way that btrfs_read_locked_inode() currently does, allocating if it has to and
freeing if it's necessary.  The second patch is still good.  Thanks,

Josef