Patchwork [RFC,1/3] ext4: refactor code to read the extent tree block

login
register
mail settings
Submitter Theodore Ts'o
Date July 11, 2013, 4:39 a.m.
Message ID <1373517542-21234-1-git-send-email-tytso@mit.edu>
Download mbox | patch
Permalink /patch/258299/
State Superseded
Headers show

Comments

Theodore Ts'o - July 11, 2013, 4:39 a.m.
Refactor out the code needed to read the extent tree block into a
single read_extent_tree_block() function.  In addition to simplifying
the code, it also makes sure that we call the ext4_ext_load_extent
tracepoint whenever we need to read an extent tree block from disk.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/ext4/extents.c | 100 ++++++++++++++++++++++++------------------------------
 1 file changed, 45 insertions(+), 55 deletions(-)
Zheng Liu - July 11, 2013, 12:41 p.m.
On Thu, Jul 11, 2013 at 12:39:00AM -0400, Theodore Ts'o wrote:
> Refactor out the code needed to read the extent tree block into a
> single read_extent_tree_block() function.  In addition to simplifying
> the code, it also makes sure that we call the ext4_ext_load_extent
> tracepoint whenever we need to read an extent tree block from disk.
> 
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

The patch looks good to me.
Reviewed-by: Zheng Liu <wenqing.lz@taobao.com>

                                                - Zheng

> ---
>  fs/ext4/extents.c | 100 ++++++++++++++++++++++++------------------------------
>  1 file changed, 45 insertions(+), 55 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 7097b0f..e174814 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -464,25 +464,41 @@ int ext4_ext_check_inode(struct inode *inode)
>  	return ext4_ext_check(inode, ext_inode_hdr(inode), ext_depth(inode));
>  }
>  
> -static int __ext4_ext_check_block(const char *function, unsigned int line,
> -				  struct inode *inode,
> -				  struct ext4_extent_header *eh,
> -				  int depth,
> -				  struct buffer_head *bh)
> +static struct buffer_head *
> +__read_extent_tree_block(const char *function, unsigned int line,
> +			 struct inode *inode, ext4_fsblk_t pblk, int depth)
>  {
> -	int ret;
> +	struct buffer_head		*bh;
> +	int				err;
> +
> +	BUG_ON(!rwsem_is_locked(&EXT4_I(inode)->i_data_sem));
> +
> +	bh = sb_getblk(inode->i_sb, pblk);
> +	if (unlikely(!bh))
> +		return ERR_PTR(-ENOMEM);
>  
> +	if (!bh_uptodate_or_lock(bh)) {
> +		trace_ext4_ext_load_extent(inode, pblk, _RET_IP_);
> +		err = bh_submit_read(bh);
> +		if (err < 0)
> +			goto errout;
> +	}
>  	if (buffer_verified(bh))
> -		return 0;
> -	ret = ext4_ext_check(inode, eh, depth);
> -	if (ret)
> -		return ret;
> +		return bh;
> +	err = __ext4_ext_check(function, line, inode,
> +			       ext_block_hdr(bh), depth);
> +	if (err)
> +		goto errout;
>  	set_buffer_verified(bh);
> -	return ret;
> +	return bh;
> +errout:
> +	put_bh(bh);
> +	return ERR_PTR(err);
> +
>  }
>  
> -#define ext4_ext_check_block(inode, eh, depth, bh)	\
> -	__ext4_ext_check_block(__func__, __LINE__, inode, eh, depth, bh)
> +#define read_extent_tree_block(inode, pblk, depth)		\
> +	__read_extent_tree_block(__func__, __LINE__, (inode), (pblk), (depth))
>  
>  #ifdef EXT_DEBUG
>  static void ext4_ext_show_path(struct inode *inode, struct ext4_ext_path *path)
> @@ -748,20 +764,12 @@ ext4_ext_find_extent(struct inode *inode, ext4_lblk_t block,
>  		path[ppos].p_depth = i;
>  		path[ppos].p_ext = NULL;
>  
> -		bh = sb_getblk(inode->i_sb, path[ppos].p_block);
> -		if (unlikely(!bh)) {
> -			ret = -ENOMEM;
> +		bh = read_extent_tree_block(inode, path[ppos].p_block, --i);
> +		if (IS_ERR(bh)) {
> +			ret = PTR_ERR(bh);
>  			goto err;
>  		}
> -		if (!bh_uptodate_or_lock(bh)) {
> -			trace_ext4_ext_load_extent(inode, block,
> -						path[ppos].p_block);
> -			ret = bh_submit_read(bh);
> -			if (ret < 0) {
> -				put_bh(bh);
> -				goto err;
> -			}
> -		}
> +
>  		eh = ext_block_hdr(bh);
>  		ppos++;
>  		if (unlikely(ppos > depth)) {
> @@ -773,11 +781,6 @@ ext4_ext_find_extent(struct inode *inode, ext4_lblk_t block,
>  		}
>  		path[ppos].p_bh = bh;
>  		path[ppos].p_hdr = eh;
> -		i--;
> -
> -		ret = ext4_ext_check_block(inode, eh, i, bh);
> -		if (ret < 0)
> -			goto err;
>  	}
>  
>  	path[ppos].p_depth = i;
> @@ -1412,29 +1415,21 @@ got_index:
>  	ix++;
>  	block = ext4_idx_pblock(ix);
>  	while (++depth < path->p_depth) {
> -		bh = sb_bread(inode->i_sb, block);
> -		if (bh == NULL)
> -			return -EIO;
> -		eh = ext_block_hdr(bh);
>  		/* subtract from p_depth to get proper eh_depth */
> -		if (ext4_ext_check_block(inode, eh,
> -					 path->p_depth - depth, bh)) {
> -			put_bh(bh);
> -			return -EIO;
> -		}
> +		bh = read_extent_tree_block(inode, *logical,
> +					    path->p_depth - depth);
> +		if (IS_ERR(bh))
> +			return PTR_ERR(bh);
> +		eh = ext_block_hdr(bh);
>  		ix = EXT_FIRST_INDEX(eh);
>  		block = ext4_idx_pblock(ix);
>  		put_bh(bh);
>  	}
>  
> -	bh = sb_bread(inode->i_sb, block);
> -	if (bh == NULL)
> -		return -EIO;
> +	bh = read_extent_tree_block(inode, *logical, path->p_depth - depth);
> +	if (IS_ERR(bh))
> +		return PTR_ERR(bh);
>  	eh = ext_block_hdr(bh);
> -	if (ext4_ext_check_block(inode, eh, path->p_depth - depth, bh)) {
> -		put_bh(bh);
> -		return -EIO;
> -	}
>  	ex = EXT_FIRST_EXTENT(eh);
>  found_extent:
>  	*logical = le32_to_cpu(ex->ee_block);
> @@ -2829,21 +2824,16 @@ again:
>  			ext_debug("move to level %d (block %llu)\n",
>  				  i + 1, ext4_idx_pblock(path[i].p_idx));
>  			memset(path + i + 1, 0, sizeof(*path));
> -			bh = sb_bread(sb, ext4_idx_pblock(path[i].p_idx));
> -			if (!bh) {
> -				/* should we reset i_size? */
> -				err = -EIO;
> +			bh = read_extent_tree_block(inode,
> +				ext4_idx_pblock(path[i].p_idx), depth - i - 1);
> +			if (IS_ERR(bh)) {
> +				err = PTR_ERR(bh);
>  				break;
>  			}
>  			if (WARN_ON(i + 1 > depth)) {
>  				err = -EIO;
>  				break;
>  			}
> -			if (ext4_ext_check_block(inode, ext_block_hdr(bh),
> -							depth - i - 1, bh)) {
> -				err = -EIO;
> -				break;
> -			}
>  			path[i + 1].p_bh = bh;
>  
>  			/* save actual number of indexes since this
> -- 
> 1.7.12.rc0.22.gcdd159b
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 7097b0f..e174814 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -464,25 +464,41 @@  int ext4_ext_check_inode(struct inode *inode)
 	return ext4_ext_check(inode, ext_inode_hdr(inode), ext_depth(inode));
 }
 
-static int __ext4_ext_check_block(const char *function, unsigned int line,
-				  struct inode *inode,
-				  struct ext4_extent_header *eh,
-				  int depth,
-				  struct buffer_head *bh)
+static struct buffer_head *
+__read_extent_tree_block(const char *function, unsigned int line,
+			 struct inode *inode, ext4_fsblk_t pblk, int depth)
 {
-	int ret;
+	struct buffer_head		*bh;
+	int				err;
+
+	BUG_ON(!rwsem_is_locked(&EXT4_I(inode)->i_data_sem));
+
+	bh = sb_getblk(inode->i_sb, pblk);
+	if (unlikely(!bh))
+		return ERR_PTR(-ENOMEM);
 
+	if (!bh_uptodate_or_lock(bh)) {
+		trace_ext4_ext_load_extent(inode, pblk, _RET_IP_);
+		err = bh_submit_read(bh);
+		if (err < 0)
+			goto errout;
+	}
 	if (buffer_verified(bh))
-		return 0;
-	ret = ext4_ext_check(inode, eh, depth);
-	if (ret)
-		return ret;
+		return bh;
+	err = __ext4_ext_check(function, line, inode,
+			       ext_block_hdr(bh), depth);
+	if (err)
+		goto errout;
 	set_buffer_verified(bh);
-	return ret;
+	return bh;
+errout:
+	put_bh(bh);
+	return ERR_PTR(err);
+
 }
 
-#define ext4_ext_check_block(inode, eh, depth, bh)	\
-	__ext4_ext_check_block(__func__, __LINE__, inode, eh, depth, bh)
+#define read_extent_tree_block(inode, pblk, depth)		\
+	__read_extent_tree_block(__func__, __LINE__, (inode), (pblk), (depth))
 
 #ifdef EXT_DEBUG
 static void ext4_ext_show_path(struct inode *inode, struct ext4_ext_path *path)
@@ -748,20 +764,12 @@  ext4_ext_find_extent(struct inode *inode, ext4_lblk_t block,
 		path[ppos].p_depth = i;
 		path[ppos].p_ext = NULL;
 
-		bh = sb_getblk(inode->i_sb, path[ppos].p_block);
-		if (unlikely(!bh)) {
-			ret = -ENOMEM;
+		bh = read_extent_tree_block(inode, path[ppos].p_block, --i);
+		if (IS_ERR(bh)) {
+			ret = PTR_ERR(bh);
 			goto err;
 		}
-		if (!bh_uptodate_or_lock(bh)) {
-			trace_ext4_ext_load_extent(inode, block,
-						path[ppos].p_block);
-			ret = bh_submit_read(bh);
-			if (ret < 0) {
-				put_bh(bh);
-				goto err;
-			}
-		}
+
 		eh = ext_block_hdr(bh);
 		ppos++;
 		if (unlikely(ppos > depth)) {
@@ -773,11 +781,6 @@  ext4_ext_find_extent(struct inode *inode, ext4_lblk_t block,
 		}
 		path[ppos].p_bh = bh;
 		path[ppos].p_hdr = eh;
-		i--;
-
-		ret = ext4_ext_check_block(inode, eh, i, bh);
-		if (ret < 0)
-			goto err;
 	}
 
 	path[ppos].p_depth = i;
@@ -1412,29 +1415,21 @@  got_index:
 	ix++;
 	block = ext4_idx_pblock(ix);
 	while (++depth < path->p_depth) {
-		bh = sb_bread(inode->i_sb, block);
-		if (bh == NULL)
-			return -EIO;
-		eh = ext_block_hdr(bh);
 		/* subtract from p_depth to get proper eh_depth */
-		if (ext4_ext_check_block(inode, eh,
-					 path->p_depth - depth, bh)) {
-			put_bh(bh);
-			return -EIO;
-		}
+		bh = read_extent_tree_block(inode, *logical,
+					    path->p_depth - depth);
+		if (IS_ERR(bh))
+			return PTR_ERR(bh);
+		eh = ext_block_hdr(bh);
 		ix = EXT_FIRST_INDEX(eh);
 		block = ext4_idx_pblock(ix);
 		put_bh(bh);
 	}
 
-	bh = sb_bread(inode->i_sb, block);
-	if (bh == NULL)
-		return -EIO;
+	bh = read_extent_tree_block(inode, *logical, path->p_depth - depth);
+	if (IS_ERR(bh))
+		return PTR_ERR(bh);
 	eh = ext_block_hdr(bh);
-	if (ext4_ext_check_block(inode, eh, path->p_depth - depth, bh)) {
-		put_bh(bh);
-		return -EIO;
-	}
 	ex = EXT_FIRST_EXTENT(eh);
 found_extent:
 	*logical = le32_to_cpu(ex->ee_block);
@@ -2829,21 +2824,16 @@  again:
 			ext_debug("move to level %d (block %llu)\n",
 				  i + 1, ext4_idx_pblock(path[i].p_idx));
 			memset(path + i + 1, 0, sizeof(*path));
-			bh = sb_bread(sb, ext4_idx_pblock(path[i].p_idx));
-			if (!bh) {
-				/* should we reset i_size? */
-				err = -EIO;
+			bh = read_extent_tree_block(inode,
+				ext4_idx_pblock(path[i].p_idx), depth - i - 1);
+			if (IS_ERR(bh)) {
+				err = PTR_ERR(bh);
 				break;
 			}
 			if (WARN_ON(i + 1 > depth)) {
 				err = -EIO;
 				break;
 			}
-			if (ext4_ext_check_block(inode, ext_block_hdr(bh),
-							depth - i - 1, bh)) {
-				err = -EIO;
-				break;
-			}
 			path[i + 1].p_bh = bh;
 
 			/* save actual number of indexes since this