[08/20] ext4: get rid of ppath in ext4_find_extent()

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.

Getting rid of ppath in ext4_find_extent() requires its caller to update
ppath. These ppaths will also be dropped later. No functional changes.

Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/ext4.h        |  2 +-
 fs/ext4/extents.c     | 52 +++++++++++++++++++++++--------------------
 fs/ext4/move_extent.c |  6 ++---
 3 files changed, 32 insertions(+), 28 deletions(-)

On Wed 10-07-24 12:06:42, 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.
> 
> Getting rid of ppath in ext4_find_extent() requires its caller to update
> ppath. These ppaths will also be dropped later. No functional changes.
> 
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

One nit below, otherwise feel free to add:

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

> @@ -3260,11 +3257,12 @@ static int ext4_split_extent_at(handle_t *handle,
>  	 * WARN_ON may be triggered in ext4_da_update_reserve_space() due to
>  	 * an incorrect ee_len causing the i_reserved_data_blocks exception.
>  	 */
> -	path = ext4_find_extent(inode, ee_block, ppath,
> +	path = ext4_find_extent(inode, ee_block, *ppath,
>  				flags | EXT4_EX_NOFAIL);
>  	if (IS_ERR(path)) {
>  		EXT4_ERROR_INODE(inode, "Failed split extent on %u, err %ld",
>  				 split, PTR_ERR(path));
> +		*ppath = NULL;
>  		return PTR_ERR(path);
>  	}

I think here you forgot to update ppath on success case. It will go away by
the end of the series but still it's good to keep thing consistent...

								Honza

On 2024/7/25 18:38, Jan Kara wrote:
> On Wed 10-07-24 12:06:42, 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.
>>
>> Getting rid of ppath in ext4_find_extent() requires its caller to update
>> ppath. These ppaths will also be dropped later. No functional changes.
>>
>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> One nit below, otherwise feel free to add:
>
> Reviewed-by: Jan Kara <jack@suse.cz>
>
>> @@ -3260,11 +3257,12 @@ static int ext4_split_extent_at(handle_t *handle,
>>   	 * WARN_ON may be triggered in ext4_da_update_reserve_space() due to
>>   	 * an incorrect ee_len causing the i_reserved_data_blocks exception.
>>   	 */
>> -	path = ext4_find_extent(inode, ee_block, ppath,
>> +	path = ext4_find_extent(inode, ee_block, *ppath,
>>   				flags | EXT4_EX_NOFAIL);
>>   	if (IS_ERR(path)) {
>>   		EXT4_ERROR_INODE(inode, "Failed split extent on %u, err %ld",
>>   				 split, PTR_ERR(path));
>> +		*ppath = NULL;
>>   		return PTR_ERR(path);
>>   	}
> I think here you forgot to update ppath on success case. It will go away by
> the end of the series but still it's good to keep thing consistent...
>
> 								Honza

Hi Honza,

Thank you for your review！

In patch 5, the ppath is already updated in case of success, so there
is no need to add it here. This update was not shown when the patch
was made and it looks like this：

-       path = ext4_find_extent(inode, ee_block, ppath,
+       path = ext4_find_extent(inode, ee_block, *ppath,
                                 flags | EXT4_EX_NOFAIL);
         if (IS_ERR(path)) {
                 EXT4_ERROR_INODE(inode, "Failed split extent on %u, err 
%ld",
                                  split, PTR_ERR(path));
+               *ppath = NULL;
                 return PTR_ERR(path);
         }
         depth = ext_depth(inode);
         ex = path[depth].p_ext;
         *ppath = path;

On Sat 27-07-24 14:18:33, Baokun Li wrote:
> On 2024/7/25 18:38, Jan Kara wrote:
> > On Wed 10-07-24 12:06:42, 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.
> > > 
> > > Getting rid of ppath in ext4_find_extent() requires its caller to update
> > > ppath. These ppaths will also be dropped later. No functional changes.
> > > 
> > > Signed-off-by: Baokun Li <libaokun1@huawei.com>
> > One nit below, otherwise feel free to add:
> > 
> > Reviewed-by: Jan Kara <jack@suse.cz>
> > 
> > > @@ -3260,11 +3257,12 @@ static int ext4_split_extent_at(handle_t *handle,
> > >   	 * WARN_ON may be triggered in ext4_da_update_reserve_space() due to
> > >   	 * an incorrect ee_len causing the i_reserved_data_blocks exception.
> > >   	 */
> > > -	path = ext4_find_extent(inode, ee_block, ppath,
> > > +	path = ext4_find_extent(inode, ee_block, *ppath,
> > >   				flags | EXT4_EX_NOFAIL);
> > >   	if (IS_ERR(path)) {
> > >   		EXT4_ERROR_INODE(inode, "Failed split extent on %u, err %ld",
> > >   				 split, PTR_ERR(path));
> > > +		*ppath = NULL;
> > >   		return PTR_ERR(path);
> > >   	}
> > I think here you forgot to update ppath on success case. It will go away by
> > the end of the series but still it's good to keep thing consistent...
> > 
> > 								Honza
> 
> Hi Honza,
> 
> Thank you for your review！
> 
> In patch 5, the ppath is already updated in case of success, so there
> is no need to add it here. This update was not shown when the patch
> was made and it looks like this：
> 
> -       path = ext4_find_extent(inode, ee_block, ppath,
> +       path = ext4_find_extent(inode, ee_block, *ppath,
>                                 flags | EXT4_EX_NOFAIL);
>         if (IS_ERR(path)) {
>                 EXT4_ERROR_INODE(inode, "Failed split extent on %u, err
> %ld",
>                                  split, PTR_ERR(path));
> +               *ppath = NULL;
>                 return PTR_ERR(path);
>         }
>         depth = ext_depth(inode);
>         ex = path[depth].p_ext;
>         *ppath = path;

Yes, you are right. I didn't realize the update was already there. So I
withdraw my comment :)

								Honza

On Wed, Jul 10, 2024 at 12:06:42PM +0800, 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.
> 
> Getting rid of ppath in ext4_find_extent() requires its caller to update
> ppath. These ppaths will also be dropped later. No functional changes.
> 
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> ---
>  fs/ext4/ext4.h        |  2 +-
>  fs/ext4/extents.c     | 52 +++++++++++++++++++++++--------------------
>  fs/ext4/move_extent.c |  6 ++---
>  3 files changed, 32 insertions(+), 28 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 8007abd4972d..cbe8d6062c52 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -3714,7 +3714,7 @@ extern int ext4_ext_insert_extent(handle_t *, struct inode *,
>  				  struct ext4_ext_path **,
>  				  struct ext4_extent *, int);
>  extern struct ext4_ext_path *ext4_find_extent(struct inode *, ext4_lblk_t,
> -					      struct ext4_ext_path **,
> +					      struct ext4_ext_path *,
>  					      int flags);
>  extern void ext4_free_ext_path(struct ext4_ext_path *);
>  extern int ext4_ext_check_inode(struct inode *inode);
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index b1cfce5b57d2..5217e6f53467 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -884,11 +884,10 @@ void ext4_ext_tree_init(handle_t *handle, struct inode *inode)
>  
>  struct ext4_ext_path *
>  ext4_find_extent(struct inode *inode, ext4_lblk_t block,
> -		 struct ext4_ext_path **orig_path, int flags)
> +		 struct ext4_ext_path *path, int flags)
>  {
>  	struct ext4_extent_header *eh;
>  	struct buffer_head *bh;
> -	struct ext4_ext_path *path = orig_path ? *orig_path : NULL;
>  	short int depth, i, ppos = 0;
>  	int ret;
>  	gfp_t gfp_flags = GFP_NOFS;
> @@ -909,7 +908,7 @@ ext4_find_extent(struct inode *inode, ext4_lblk_t block,
>  		ext4_ext_drop_refs(path);
>  		if (depth > path[0].p_maxdepth) {
>  			kfree(path);
> -			*orig_path = path = NULL;
> +			path = NULL;
>  		}
>  	}
>  	if (!path) {
> @@ -964,8 +963,6 @@ ext4_find_extent(struct inode *inode, ext4_lblk_t block,
>  
>  err:
>  	ext4_free_ext_path(path);
> -	if (orig_path)
> -		*orig_path = NULL;
>  	return ERR_PTR(ret);
>  }
>  
> @@ -1430,7 +1427,7 @@ static int ext4_ext_create_new_leaf(handle_t *handle, struct inode *inode,
>  		/* refill path */
>  		path = ext4_find_extent(inode,
>  				    (ext4_lblk_t)le32_to_cpu(newext->ee_block),
> -				    ppath, gb_flags);
> +				    path, gb_flags);
>  		if (IS_ERR(path))
>  			err = PTR_ERR(path);
>  	} else {
> @@ -1442,7 +1439,7 @@ static int ext4_ext_create_new_leaf(handle_t *handle, struct inode *inode,
>  		/* refill path */
>  		path = ext4_find_extent(inode,
>  				   (ext4_lblk_t)le32_to_cpu(newext->ee_block),
> -				    ppath, gb_flags);
> +				    path, gb_flags);
>  		if (IS_ERR(path)) {
>  			err = PTR_ERR(path);
>  			goto out;
> @@ -1458,8 +1455,8 @@ static int ext4_ext_create_new_leaf(handle_t *handle, struct inode *inode,
>  			goto repeat;
>  		}
>  	}
> -
>  out:
> +	*ppath = IS_ERR(path) ? NULL : path;
>  	return err;
>  }
>  
> @@ -3260,11 +3257,12 @@ static int ext4_split_extent_at(handle_t *handle,
>  	 * WARN_ON may be triggered in ext4_da_update_reserve_space() due to
>  	 * an incorrect ee_len causing the i_reserved_data_blocks exception.
>  	 */
> -	path = ext4_find_extent(inode, ee_block, ppath,
> +	path = ext4_find_extent(inode, ee_block, *ppath,
>  				flags | EXT4_EX_NOFAIL);
>  	if (IS_ERR(path)) {
>  		EXT4_ERROR_INODE(inode, "Failed split extent on %u, err %ld",
>  				 split, PTR_ERR(path));
> +		*ppath = NULL;
>  		return PTR_ERR(path);
>  	}
>  	depth = ext_depth(inode);
> @@ -3379,9 +3377,12 @@ static int ext4_split_extent(handle_t *handle,
>  	 * Update path is required because previous ext4_split_extent_at() may
>  	 * result in split of original leaf or extent zeroout.
>  	 */
> -	path = ext4_find_extent(inode, map->m_lblk, ppath, flags);
> -	if (IS_ERR(path))
> +	path = ext4_find_extent(inode, map->m_lblk, *ppath, flags);
> +	if (IS_ERR(path)) {
> +		*ppath = NULL;
>  		return PTR_ERR(path);
> +	}
> +	*ppath = path;
>  	depth = ext_depth(inode);
>  	ex = path[depth].p_ext;
>  	if (!ex) {
> @@ -3767,9 +3768,12 @@ static int ext4_convert_unwritten_extents_endio(handle_t *handle,
>  						 EXT4_GET_BLOCKS_CONVERT);
>  		if (err < 0)
>  			return err;
> -		path = ext4_find_extent(inode, map->m_lblk, ppath, 0);
> -		if (IS_ERR(path))
> +		path = ext4_find_extent(inode, map->m_lblk, *ppath, 0);
> +		if (IS_ERR(path)) {
> +			*ppath = NULL;
>  			return PTR_ERR(path);
> +		}
> +		*ppath = path;
>  		depth = ext_depth(inode);
>  		ex = path[depth].p_ext;
>  	}
> @@ -3825,9 +3829,12 @@ convert_initialized_extent(handle_t *handle, struct inode *inode,
>  				EXT4_GET_BLOCKS_CONVERT_UNWRITTEN);
>  		if (err < 0)
>  			return err;
> -		path = ext4_find_extent(inode, map->m_lblk, ppath, 0);
> -		if (IS_ERR(path))
> +		path = ext4_find_extent(inode, map->m_lblk, *ppath, 0);
> +		if (IS_ERR(path)) {
> +			*ppath = NULL;
>  			return PTR_ERR(path);
> +		}
> +		*ppath = path;
>  		depth = ext_depth(inode);
>  		ex = path[depth].p_ext;
>  		if (!ex) {
> @@ -5224,7 +5231,7 @@ ext4_ext_shift_extents(struct inode *inode, handle_t *handle,
>  	* won't be shifted beyond EXT_MAX_BLOCKS.
>  	*/
>  	if (SHIFT == SHIFT_LEFT) {
> -		path = ext4_find_extent(inode, start - 1, &path,
> +		path = ext4_find_extent(inode, start - 1, path,
>  					EXT4_EX_NOCACHE);
>  		if (IS_ERR(path))
>  			return PTR_ERR(path);
> @@ -5273,7 +5280,7 @@ ext4_ext_shift_extents(struct inode *inode, handle_t *handle,
>  	 * becomes NULL to indicate the end of the loop.
>  	 */
>  	while (iterator && start <= stop) {
> -		path = ext4_find_extent(inode, *iterator, &path,
> +		path = ext4_find_extent(inode, *iterator, path,
>  					EXT4_EX_NOCACHE);
>  		if (IS_ERR(path))
>  			return PTR_ERR(path);
> @@ -5854,11 +5861,8 @@ int ext4_clu_mapped(struct inode *inode, ext4_lblk_t lclu)
>  
>  	/* search for the extent closest to the first block in the cluster */
>  	path = ext4_find_extent(inode, EXT4_C2B(sbi, lclu), NULL, 0);
> -	if (IS_ERR(path)) {
> -		err = PTR_ERR(path);
> -		path = NULL;
> -		goto out;
> -	}
> +	if (IS_ERR(path))
> +		return PTR_ERR(path);
>  
>  	depth = ext_depth(inode);
>  
> @@ -5942,7 +5946,7 @@ int ext4_ext_replay_update_ex(struct inode *inode, ext4_lblk_t start,
>  		if (ret)
>  			goto out;
>  
> -		path = ext4_find_extent(inode, start, &path, 0);
> +		path = ext4_find_extent(inode, start, path, 0);
>  		if (IS_ERR(path))
>  			return PTR_ERR(path);
>  		ex = path[path->p_depth].p_ext;
> @@ -5956,7 +5960,7 @@ int ext4_ext_replay_update_ex(struct inode *inode, ext4_lblk_t start,
>  			if (ret)
>  				goto out;
>  
> -			path = ext4_find_extent(inode, start, &path, 0);
> +			path = ext4_find_extent(inode, start, path, 0);
>  			if (IS_ERR(path))
>  				return PTR_ERR(path);
>  			ex = path[path->p_depth].p_ext;
> diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
> index 204f53b23622..b0ab19a913bf 100644
> --- a/fs/ext4/move_extent.c
> +++ b/fs/ext4/move_extent.c
> @@ -26,14 +26,14 @@ static inline int
>  get_ext_path(struct inode *inode, ext4_lblk_t lblock,
>  		struct ext4_ext_path **ppath)
>  {
> -	struct ext4_ext_path *path;
> +	struct ext4_ext_path *path = *ppath;
>  
> -	path = ext4_find_extent(inode, lblock, ppath, EXT4_EX_NOCACHE);
> +	*ppath = NULL;
> +	path = ext4_find_extent(inode, lblock, path, EXT4_EX_NOCACHE);
>  	if (IS_ERR(path))
>  		return PTR_ERR(path);
>  	if (path[ext_depth(inode)].p_ext == NULL) {
>  		ext4_free_ext_path(path);
> -		*ppath = NULL;
>  		return -ENODATA;
>  	}
>  	*ppath = path;
Hi Baokun,

The changes look good to me, afaict we've handled updating ppath in all
the places correctly.

Feel free to add:

Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>

Regards,
ojaswin
> -- 
> 2.39.2

>