Re: [PATCH 13/20] ext4: get rid of ppath in ext4_force_split_extent_at()

[Jan Kara <jack@suse.cz>]

On Wed 10-07-24 12:06:47, libaokun@huaweicloud.com wrote:
> From: Baokun Li <libaokun1@huawei.com>
> 
> The use of path and ppath is now very confusing, so to make the code more
> readable, pass path between functions uniformly, and get rid of ppath.
> 
> To get rid of the ppath in ext4_force_split_extent_at(), the following is
> done here:
> 
>  * The ext4_find_extent() can update the extent path so it doesn't have to
>    allocate and free path repeatedly, thus reducing the consumption of
>    memory allocation and freeing in ext4_swap_extents().
> 
> No functional changes.
> 
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/extents.c | 117 ++++++++++++++++++++++++----------------------
>  1 file changed, 60 insertions(+), 57 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index c86b1bb7720f..0bd068ed324f 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -328,27 +328,20 @@ static inline int ext4_ext_space_root_idx(struct inode *inode, int check)
>  	return size;
>  }
>  
> -static inline int
> +static inline struct ext4_ext_path *
>  ext4_force_split_extent_at(handle_t *handle, struct inode *inode,
> -			   struct ext4_ext_path **ppath, ext4_lblk_t lblk,
> +			   struct ext4_ext_path *path, ext4_lblk_t lblk,
>  			   int nofail)
>  {
> -	struct ext4_ext_path *path = *ppath;
>  	int unwritten = ext4_ext_is_unwritten(path[path->p_depth].p_ext);
>  	int flags = EXT4_EX_NOCACHE | EXT4_GET_BLOCKS_PRE_IO;
>  
>  	if (nofail)
>  		flags |= EXT4_GET_BLOCKS_METADATA_NOFAIL | EXT4_EX_NOFAIL;
>  
> -	path = ext4_split_extent_at(handle, inode, path, lblk, unwritten ?
> +	return ext4_split_extent_at(handle, inode, path, lblk, unwritten ?
>  			EXT4_EXT_MARK_UNWRIT1|EXT4_EXT_MARK_UNWRIT2 : 0,
>  			flags);
> -	if (IS_ERR(path)) {
> -		*ppath = NULL;
> -		return PTR_ERR(path);
> -	}
> -	*ppath = path;
> -	return 0;
>  }
>  
>  static int
> @@ -2906,11 +2899,12 @@ int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start,
>  			 * fail removing space due to ENOSPC so try to use
>  			 * reserved block if that happens.
>  			 */
> -			err = ext4_force_split_extent_at(handle, inode, &path,
> -							 end + 1, 1);
> -			if (err < 0)
> +			path = ext4_force_split_extent_at(handle, inode, path,
> +							  end + 1, 1);
> +			if (IS_ERR(path)) {
> +				err = PTR_ERR(path);
>  				goto out;
> -
> +			}
>  		} else if (sbi->s_cluster_ratio > 1 && end >= ex_end &&
>  			   partial.state == initial) {
>  			/*
> @@ -5707,25 +5701,21 @@ ext4_swap_extents(handle_t *handle, struct inode *inode1,
>  		int e1_len, e2_len, len;
>  		int split = 0;
>  
> -		path1 = ext4_find_extent(inode1, lblk1, NULL, EXT4_EX_NOCACHE);
> +		path1 = ext4_find_extent(inode1, lblk1, path1, EXT4_EX_NOCACHE);
>  		if (IS_ERR(path1)) {
>  			*erp = PTR_ERR(path1);
> -			path1 = NULL;
> -		finish:
> -			count = 0;
> -			goto repeat;
> +			goto errout;
>  		}
> -		path2 = ext4_find_extent(inode2, lblk2, NULL, EXT4_EX_NOCACHE);
> +		path2 = ext4_find_extent(inode2, lblk2, path2, EXT4_EX_NOCACHE);
>  		if (IS_ERR(path2)) {
>  			*erp = PTR_ERR(path2);
> -			path2 = NULL;
> -			goto finish;
> +			goto errout;
>  		}
>  		ex1 = path1[path1->p_depth].p_ext;
>  		ex2 = path2[path2->p_depth].p_ext;
>  		/* Do we have something to swap ? */
>  		if (unlikely(!ex2 || !ex1))
> -			goto finish;
> +			goto errout;
>  
>  		e1_blk = le32_to_cpu(ex1->ee_block);
>  		e2_blk = le32_to_cpu(ex2->ee_block);
> @@ -5747,7 +5737,7 @@ ext4_swap_extents(handle_t *handle, struct inode *inode1,
>  				next2 = e2_blk;
>  			/* Do we have something to swap */
>  			if (next1 == EXT_MAX_BLOCKS || next2 == EXT_MAX_BLOCKS)
> -				goto finish;
> +				goto errout;
>  			/* Move to the rightest boundary */
>  			len = next1 - lblk1;
>  			if (len < next2 - lblk2)
> @@ -5757,28 +5747,32 @@ ext4_swap_extents(handle_t *handle, struct inode *inode1,
>  			lblk1 += len;
>  			lblk2 += len;
>  			count -= len;
> -			goto repeat;
> +			continue;
>  		}
>  
>  		/* Prepare left boundary */
>  		if (e1_blk < lblk1) {
>  			split = 1;
> -			*erp = ext4_force_split_extent_at(handle, inode1,
> -						&path1, lblk1, 0);
> -			if (unlikely(*erp))
> -				goto finish;
> +			path1 = ext4_force_split_extent_at(handle, inode1,
> +							   path1, lblk1, 0);
> +			if (IS_ERR(path1)) {
> +				*erp = PTR_ERR(path1);
> +				goto errout;
> +			}
>  		}
>  		if (e2_blk < lblk2) {
>  			split = 1;
> -			*erp = ext4_force_split_extent_at(handle, inode2,
> -						&path2,  lblk2, 0);
> -			if (unlikely(*erp))
> -				goto finish;
> +			path2 = ext4_force_split_extent_at(handle, inode2,
> +							   path2, lblk2, 0);
> +			if (IS_ERR(path2)) {
> +				*erp = PTR_ERR(path2);
> +				goto errout;
> +			}
>  		}
>  		/* ext4_split_extent_at() may result in leaf extent split,
>  		 * path must to be revalidated. */
>  		if (split)
> -			goto repeat;
> +			continue;
>  
>  		/* Prepare right boundary */
>  		len = count;
> @@ -5789,30 +5783,34 @@ ext4_swap_extents(handle_t *handle, struct inode *inode1,
>  
>  		if (len != e1_len) {
>  			split = 1;
> -			*erp = ext4_force_split_extent_at(handle, inode1,
> -						&path1, lblk1 + len, 0);
> -			if (unlikely(*erp))
> -				goto finish;
> +			path1 = ext4_force_split_extent_at(handle, inode1,
> +							path1, lblk1 + len, 0);
> +			if (IS_ERR(path1)) {
> +				*erp = PTR_ERR(path1);
> +				goto errout;
> +			}
>  		}
>  		if (len != e2_len) {
>  			split = 1;
> -			*erp = ext4_force_split_extent_at(handle, inode2,
> -						&path2, lblk2 + len, 0);
> -			if (*erp)
> -				goto finish;
> +			path2 = ext4_force_split_extent_at(handle, inode2,
> +							path2, lblk2 + len, 0);
> +			if (IS_ERR(path2)) {
> +				*erp = PTR_ERR(path2);
> +				goto errout;
> +			}
>  		}
>  		/* ext4_split_extent_at() may result in leaf extent split,
>  		 * path must to be revalidated. */
>  		if (split)
> -			goto repeat;
> +			continue;
>  
>  		BUG_ON(e2_len != e1_len);
>  		*erp = ext4_ext_get_access(handle, inode1, path1 + path1->p_depth);
>  		if (unlikely(*erp))
> -			goto finish;
> +			goto errout;
>  		*erp = ext4_ext_get_access(handle, inode2, path2 + path2->p_depth);
>  		if (unlikely(*erp))
> -			goto finish;
> +			goto errout;
>  
>  		/* Both extents are fully inside boundaries. Swap it now */
>  		tmp_ex = *ex1;
> @@ -5830,7 +5828,7 @@ ext4_swap_extents(handle_t *handle, struct inode *inode1,
>  		*erp = ext4_ext_dirty(handle, inode2, path2 +
>  				      path2->p_depth);
>  		if (unlikely(*erp))
> -			goto finish;
> +			goto errout;
>  		*erp = ext4_ext_dirty(handle, inode1, path1 +
>  				      path1->p_depth);
>  		/*
> @@ -5840,17 +5838,17 @@ ext4_swap_extents(handle_t *handle, struct inode *inode1,
>  		 * aborted anyway.
>  		 */
>  		if (unlikely(*erp))
> -			goto finish;
> +			goto errout;
> +
>  		lblk1 += len;
>  		lblk2 += len;
>  		replaced_count += len;
>  		count -= len;
> -
> -	repeat:
> -		ext4_free_ext_path(path1);
> -		ext4_free_ext_path(path2);
> -		path1 = path2 = NULL;
>  	}
> +
> +errout:
> +	ext4_free_ext_path(path1);
> +	ext4_free_ext_path(path2);
>  	return replaced_count;
>  }
>  
> @@ -5965,24 +5963,29 @@ int ext4_ext_replay_update_ex(struct inode *inode, ext4_lblk_t start,
>  		ext4_ext_get_actual_len(ex) != len) {
>  		/* We need to split this extent to match our extent first */
>  		down_write(&EXT4_I(inode)->i_data_sem);
> -		ret = ext4_force_split_extent_at(NULL, inode, &path, start, 1);
> +		path = ext4_force_split_extent_at(NULL, inode, path, start, 1);
>  		up_write(&EXT4_I(inode)->i_data_sem);
> -		if (ret)
> +		if (IS_ERR(path)) {
> +			ret = PTR_ERR(path);
>  			goto out;
> +		}
>  
>  		path = ext4_find_extent(inode, start, path, 0);
>  		if (IS_ERR(path))
>  			return PTR_ERR(path);
> +
>  		ex = path[path->p_depth].p_ext;
>  		WARN_ON(le32_to_cpu(ex->ee_block) != start);
>  
>  		if (ext4_ext_get_actual_len(ex) != len) {
>  			down_write(&EXT4_I(inode)->i_data_sem);
> -			ret = ext4_force_split_extent_at(NULL, inode, &path,
> -							 start + len, 1);
> +			path = ext4_force_split_extent_at(NULL, inode, path,
> +							  start + len, 1);
>  			up_write(&EXT4_I(inode)->i_data_sem);
> -			if (ret)
> +			if (IS_ERR(path)) {
> +				ret = PTR_ERR(path);
>  				goto out;
> +			}
>  
>  			path = ext4_find_extent(inode, start, path, 0);
>  			if (IS_ERR(path))
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR