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: <20240725121803.bsfnpchwk5rvq6gg@quack3>
Date: Thu, 25 Jul 2024 14:18:03 +0200
From: Jan Kara <jack@...e.cz>
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 17/20] ext4: get rid of ppath in
 ext4_ext_convert_to_initialized()

On Wed 10-07-24 12:06:51, 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_ext_convert_to_initialized(), the following
> is done here:
> 
>  * Free the extents path when an error is encountered.
>  * Its caller needs to update ppath if it uses ppath.
>  * The 'allocated' is changed from passing a value to passing an address.
> 
> No functional changes.
> 
> Signed-off-by: Baokun Li <libaokun1@...wei.com>

Looks good to me. Feel free to add:

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

								Honza

> ---
>  fs/ext4/extents.c | 73 +++++++++++++++++++++++------------------------
>  1 file changed, 35 insertions(+), 38 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index b7f443f98e9d..59e80926fe3a 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -3437,13 +3437,11 @@ static struct ext4_ext_path *ext4_split_extent(handle_t *handle,
>   *    that are allocated and initialized.
>   *    It is guaranteed to be >= map->m_len.
>   */
> -static int ext4_ext_convert_to_initialized(handle_t *handle,
> -					   struct inode *inode,
> -					   struct ext4_map_blocks *map,
> -					   struct ext4_ext_path **ppath,
> -					   int flags)
> +static struct ext4_ext_path *
> +ext4_ext_convert_to_initialized(handle_t *handle, struct inode *inode,
> +			struct ext4_map_blocks *map, struct ext4_ext_path *path,
> +			int flags, unsigned int *allocated)
>  {
> -	struct ext4_ext_path *path = *ppath;
>  	struct ext4_sb_info *sbi;
>  	struct ext4_extent_header *eh;
>  	struct ext4_map_blocks split_map;
> @@ -3453,7 +3451,6 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,
>  	unsigned int ee_len, depth, map_len = map->m_len;
>  	int err = 0;
>  	int split_flag = EXT4_EXT_DATA_VALID2;
> -	int allocated = 0;
>  	unsigned int max_zeroout = 0;
>  
>  	ext_debug(inode, "logical block %llu, max_blocks %u\n",
> @@ -3494,6 +3491,7 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,
>  	 *  - L2: we only attempt to merge with an extent stored in the
>  	 *    same extent tree node.
>  	 */
> +	*allocated = 0;
>  	if ((map->m_lblk == ee_block) &&
>  		/* See if we can merge left */
>  		(map_len < ee_len) &&		/*L1*/
> @@ -3523,7 +3521,7 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,
>  			(prev_len < (EXT_INIT_MAX_LEN - map_len))) {	/*C4*/
>  			err = ext4_ext_get_access(handle, inode, path + depth);
>  			if (err)
> -				goto out;
> +				goto errout;
>  
>  			trace_ext4_ext_convert_to_initialized_fastpath(inode,
>  				map, ex, abut_ex);
> @@ -3538,7 +3536,7 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,
>  			abut_ex->ee_len = cpu_to_le16(prev_len + map_len);
>  
>  			/* Result: number of initialized blocks past m_lblk */
> -			allocated = map_len;
> +			*allocated = map_len;
>  		}
>  	} else if (((map->m_lblk + map_len) == (ee_block + ee_len)) &&
>  		   (map_len < ee_len) &&	/*L1*/
> @@ -3569,7 +3567,7 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,
>  		    (next_len < (EXT_INIT_MAX_LEN - map_len))) {	/*C4*/
>  			err = ext4_ext_get_access(handle, inode, path + depth);
>  			if (err)
> -				goto out;
> +				goto errout;
>  
>  			trace_ext4_ext_convert_to_initialized_fastpath(inode,
>  				map, ex, abut_ex);
> @@ -3584,18 +3582,20 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,
>  			abut_ex->ee_len = cpu_to_le16(next_len + map_len);
>  
>  			/* Result: number of initialized blocks past m_lblk */
> -			allocated = map_len;
> +			*allocated = map_len;
>  		}
>  	}
> -	if (allocated) {
> +	if (*allocated) {
>  		/* Mark the block containing both extents as dirty */
>  		err = ext4_ext_dirty(handle, inode, path + depth);
>  
>  		/* Update path to point to the right extent */
>  		path[depth].p_ext = abut_ex;
> +		if (err)
> +			goto errout;
>  		goto out;
>  	} else
> -		allocated = ee_len - (map->m_lblk - ee_block);
> +		*allocated = ee_len - (map->m_lblk - ee_block);
>  
>  	WARN_ON(map->m_lblk < ee_block);
>  	/*
> @@ -3622,21 +3622,21 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,
>  	split_map.m_lblk = map->m_lblk;
>  	split_map.m_len = map->m_len;
>  
> -	if (max_zeroout && (allocated > split_map.m_len)) {
> -		if (allocated <= max_zeroout) {
> +	if (max_zeroout && (*allocated > split_map.m_len)) {
> +		if (*allocated <= max_zeroout) {
>  			/* case 3 or 5 */
>  			zero_ex1.ee_block =
>  				 cpu_to_le32(split_map.m_lblk +
>  					     split_map.m_len);
>  			zero_ex1.ee_len =
> -				cpu_to_le16(allocated - split_map.m_len);
> +				cpu_to_le16(*allocated - split_map.m_len);
>  			ext4_ext_store_pblock(&zero_ex1,
>  				ext4_ext_pblock(ex) + split_map.m_lblk +
>  				split_map.m_len - ee_block);
>  			err = ext4_ext_zeroout(inode, &zero_ex1);
>  			if (err)
>  				goto fallback;
> -			split_map.m_len = allocated;
> +			split_map.m_len = *allocated;
>  		}
>  		if (split_map.m_lblk - ee_block + split_map.m_len <
>  								max_zeroout) {
> @@ -3654,27 +3654,24 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,
>  
>  			split_map.m_len += split_map.m_lblk - ee_block;
>  			split_map.m_lblk = ee_block;
> -			allocated = map->m_len;
> +			*allocated = map->m_len;
>  		}
>  	}
>  
>  fallback:
>  	path = ext4_split_extent(handle, inode, path, &split_map, split_flag,
>  				 flags, NULL);
> -	if (IS_ERR(path)) {
> -		err = PTR_ERR(path);
> -		*ppath = NULL;
> -		goto out;
> -	}
> -	err = 0;
> -	*ppath = path;
> +	if (IS_ERR(path))
> +		return path;
>  out:
>  	/* If we have gotten a failure, don't zero out status tree */
> -	if (!err) {
> -		ext4_zeroout_es(inode, &zero_ex1);
> -		ext4_zeroout_es(inode, &zero_ex2);
> -	}
> -	return err ? err : allocated;
> +	ext4_zeroout_es(inode, &zero_ex1);
> +	ext4_zeroout_es(inode, &zero_ex2);
> +	return path;
> +
> +errout:
> +	ext4_free_ext_path(path);
> +	return ERR_PTR(err);
>  }
>  
>  /*
> @@ -3896,7 +3893,6 @@ ext4_ext_handle_unwritten_extents(handle_t *handle, struct inode *inode,
>  			struct ext4_ext_path **ppath, int flags,
>  			unsigned int allocated, ext4_fsblk_t newblock)
>  {
> -	int ret = 0;
>  	int err = 0;
>  
>  	ext_debug(inode, "logical block %llu, max_blocks %u, flags 0x%x, allocated %u\n",
> @@ -3976,23 +3972,24 @@ ext4_ext_handle_unwritten_extents(handle_t *handle, struct inode *inode,
>  	 * For buffered writes, at writepage time, etc.  Convert a
>  	 * discovered unwritten extent to written.
>  	 */
> -	ret = ext4_ext_convert_to_initialized(handle, inode, map, ppath, flags);
> -	if (ret < 0) {
> -		err = ret;
> +	*ppath = ext4_ext_convert_to_initialized(handle, inode, map, *ppath,
> +						 flags, &allocated);
> +	if (IS_ERR(*ppath)) {
> +		err = PTR_ERR(*ppath);
> +		*ppath = NULL;
>  		goto out2;
>  	}
>  	ext4_update_inode_fsync_trans(handle, inode, 1);
>  	/*
> -	 * shouldn't get a 0 return when converting an unwritten extent
> +	 * shouldn't get a 0 allocated when converting an unwritten extent
>  	 * unless m_len is 0 (bug) or extent has been corrupted
>  	 */
> -	if (unlikely(ret == 0)) {
> -		EXT4_ERROR_INODE(inode, "unexpected ret == 0, m_len = %u",
> +	if (unlikely(allocated == 0)) {
> +		EXT4_ERROR_INODE(inode, "unexpected allocated == 0, m_len = %u",
>  				 map->m_len);
>  		err = -EFSCORRUPTED;
>  		goto out2;
>  	}
> -	allocated = ret;
>  
>  out:
>  	map->m_flags |= EXT4_MAP_NEW;
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ