lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Zq07D/rySkSKxsw3@li-bb2b2a4c-3307-11b2-a85c-8fa5c3a69313.ibm.com>
Date: Sat, 3 Aug 2024 01:31:19 +0530
From: Ojaswin Mujoo <ojaswin@...ux.ibm.com>
To: libaokun@...weicloud.com
Cc: linux-ext4@...r.kernel.org, tytso@....edu, adilger.kernel@...ger.ca,
        jack@...e.cz, ritesh.list@...il.com, linux-kernel@...r.kernel.org,
        yi.zhang@...wei.com, yangerkun@...wei.com,
        Baokun Li <libaokun1@...wei.com>
Subject: Re: [PATCH 13/20] ext4: get rid of ppath in
 ext4_force_split_extent_at()

On Wed, Jul 10, 2024 at 12:06:47PM +0800, libaokun@...weicloud.com wrote:
> From: Baokun Li <libaokun1@...wei.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.

Looks good Baokun, feel free to add:

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

One small comment below..

> 
> Signed-off-by: Baokun Li <libaokun1@...wei.com>
> ---
>  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

.. snip ..

> @@ -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;
>  }
>  

In ext4_swap_extents, maybe we should keep the refactoring to a separate
patch than the changes needed to get rid of ppath in
ext4_force_split_extent_at(), the commits would look a bit cleaner and
easier to read that way. I don't feel too strongly about it tho and 
I'll let you take a call.

Regards,
ojaswin

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ