Re: [PATCH 01/20] ext4: refactor ext4_ext_rm_idx() to index 'path'

[Ojaswin Mujoo]

On Wed, Jul 10, 2024 at 12:06:35PM +0800, libaokun@xxxxxxxxxxxxxxx wrote:
> From: Baokun Li <libaokun1@xxxxxxxxxx>
> 
> As suggested by Honza in Link，modify ext4_ext_rm_idx() to leave 'path'
> alone and just index it like ext4_ext_correct_indexes() does it. This
> facilitates adding error handling later. No functional changes.
> 
> Suggested-by: Jan Kara <jack@xxxxxxx>
> Link: https://lore.kernel.org/all/20230216130305.nrbtd42tppxhbynn@quack3/
> Signed-off-by: Baokun Li <libaokun1@xxxxxxxxxx>
> ---
>  fs/ext4/extents.c | 32 +++++++++++++++-----------------
>  1 file changed, 15 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index e067f2dd0335..bff3666c891a 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -2279,27 +2279,26 @@ static int ext4_ext_rm_idx(handle_t *handle, struct inode *inode,
>  {
>  	int err;
>  	ext4_fsblk_t leaf;
> +	int k = depth - 1;
>  
>  	/* free index block */
> -	depth--;
> -	path = path + depth;
> -	leaf = ext4_idx_pblock(path->p_idx);
> -	if (unlikely(path->p_hdr->eh_entries == 0)) {
> -		EXT4_ERROR_INODE(inode, "path->p_hdr->eh_entries == 0");
> +	leaf = ext4_idx_pblock(path[k].p_idx);
> +	if (unlikely(path[k].p_hdr->eh_entries == 0)) {
> +		EXT4_ERROR_INODE(inode, "path[%d].p_hdr->eh_entries == 0", k);
>  		return -EFSCORRUPTED;
>  	}
> -	err = ext4_ext_get_access(handle, inode, path);
> +	err = ext4_ext_get_access(handle, inode, path + k);
>  	if (err)
>  		return err;
>  
> -	if (path->p_idx != EXT_LAST_INDEX(path->p_hdr)) {
> -		int len = EXT_LAST_INDEX(path->p_hdr) - path->p_idx;
> +	if (path[k].p_idx != EXT_LAST_INDEX(path[k].p_hdr)) {
> +		int len = EXT_LAST_INDEX(path[k].p_hdr) - path[k].p_idx;
>  		len *= sizeof(struct ext4_extent_idx);
> -		memmove(path->p_idx, path->p_idx + 1, len);
> +		memmove(path[k].p_idx, path[k].p_idx + 1, len);
>  	}
>  
> -	le16_add_cpu(&path->p_hdr->eh_entries, -1);
> -	err = ext4_ext_dirty(handle, inode, path);
> +	le16_add_cpu(&path[k].p_hdr->eh_entries, -1);
> +	err = ext4_ext_dirty(handle, inode, path + k);
>  	if (err)
>  		return err;
>  	ext_debug(inode, "index is empty, remove it, free block %llu\n", leaf);
> @@ -2308,15 +2307,14 @@ static int ext4_ext_rm_idx(handle_t *handle, struct inode *inode,
>  	ext4_free_blocks(handle, inode, NULL, leaf, 1,
>  			 EXT4_FREE_BLOCKS_METADATA | EXT4_FREE_BLOCKS_FORGET);
>  
> -	while (--depth >= 0) {
> -		if (path->p_idx != EXT_FIRST_INDEX(path->p_hdr))
> +	while (--k >= 0) {
> +		if (path[k + 1].p_idx != EXT_FIRST_INDEX(path[k + 1].p_hdr))
>  			break;
> -		path--;
> -		err = ext4_ext_get_access(handle, inode, path);
> +		err = ext4_ext_get_access(handle, inode, path + k);
>  		if (err)
>  			break;
> -		path->p_idx->ei_block = (path+1)->p_idx->ei_block;
> -		err = ext4_ext_dirty(handle, inode, path);
> +		path[k].p_idx->ei_block = path[k + 1].p_idx->ei_block;
> +		err = ext4_ext_dirty(handle, inode, path + k);
>  		if (err)
>  			break;
>  	}

Look good to me as well. Feel free to add:

Reviewed-by: Ojaswin Mujoo <ojaswin@xxxxxxxxxxxxx>

Regards,
ojaswin
> -- 
> 2.39.2
>