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: <Zqi6iEBlY6WOxQM/@li-bb2b2a4c-3307-11b2-a85c-8fa5c3a69313.ibm.com>
Date: Tue, 30 Jul 2024 15:33:52 +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 08/20] ext4: get rid of ppath in ext4_find_extent()

On Wed, Jul 10, 2024 at 12:06:42PM +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.
> 
> 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@...wei.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@...ux.ibm.com>

Regards,
ojaswin
> -- 
> 2.39.2

> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ