[PATCH v3] ext4: propagate errors from fast commit i_blocks replay

[PATCH v3] ext4: propagate errors from fast commit i_blocks replay

[781735889]

From: lty <781735889@qq.com>

ext4_ext_replay_set_iblocks() breaks out of its mapping loop when
ext4_map_blocks() fails, but then continues with a partial block count
and unconditionally returns success. The fast commit replay caller also
ignores the helper return value.

Stop the count immediately on mapping errors, leave i_blocks unchanged,
and propagate the failure to abort inode replay.

Fixes: 8016e29f4362 ("ext4: fast commit recovery path")
Signed-off-by: lty <781735889@qq.com>
---
Changes in v3:
- Move the version history below the separator.

Changes in v2:
- Propagate errors from all ext4_find_extent() and skip_hole() calls in
  ext4_ext_replay_set_iblocks(), as requested by Jan Kara.
- Keep i_blocks unchanged when any replay lookup fails.

 fs/ext4/extents.c     | 26 ++++++++++++++++----------
 fs/ext4/fast_commit.c |  7 +++++--
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 76038b6c3655..f8c805d80c33 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -6275,7 +6275,7 @@ int ext4_ext_replay_set_iblocks(struct inode *inode)
 		map.m_len = end - cur;
 		ret = ext4_map_blocks(NULL, inode, &map, 0);
 		if (ret < 0)
-			break;
+			goto cleanup;
 		if (ret > 0)
 			numblks += ret;
 		cur = cur + map.m_len;
@@ -6291,15 +6291,19 @@ int ext4_ext_replay_set_iblocks(struct inode *inode)
 	cur = 0;
 	ret = skip_hole(inode, &cur);
 	if (ret < 0)
-		goto out;
+		goto cleanup;
 	path = ext4_find_extent(inode, cur, path, 0);
-	if (IS_ERR(path))
-		goto out;
+	if (IS_ERR(path)) {
+		ret = PTR_ERR(path);
+		goto cleanup;
+	}
 	numblks += path->p_depth;
 	while (cur < end) {
 		path = ext4_find_extent(inode, cur, path, 0);
-		if (IS_ERR(path))
-			break;
+		if (IS_ERR(path)) {
+			ret = PTR_ERR(path);
+			goto cleanup;
+		}
 		ex = path[path->p_depth].p_ext;
 		if (!ex)
 			goto cleanup;
@@ -6308,11 +6312,13 @@ int ext4_ext_replay_set_iblocks(struct inode *inode)
 					ext4_ext_get_actual_len(ex));
 		ret = skip_hole(inode, &cur);
 		if (ret < 0)
-			break;
+			goto cleanup;
 
 		path2 = ext4_find_extent(inode, cur, path2, 0);
-		if (IS_ERR(path2))
-			break;
+		if (IS_ERR(path2)) {
+			ret = PTR_ERR(path2);
+			goto cleanup;
+		}
 
 		for (i = 0; i <= max(path->p_depth, path2->p_depth); i++) {
 			cmp1 = cmp2 = 0;
@@ -6333,7 +6339,7 @@ int ext4_ext_replay_set_iblocks(struct inode *inode)
 cleanup:
 	ext4_free_ext_path(path);
 	ext4_free_ext_path(path2);
-	return 0;
+	return ret;
 }
 
 int ext4_ext_clear_bb(struct inode *inode)
diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
index 0cac890cf370..8110b01e36c1 100644
--- a/fs/ext4/fast_commit.c
+++ b/fs/ext4/fast_commit.c
@@ -2002,8 +2002,11 @@ static int ext4_fc_replay_inode(struct super_block *sb,
 	 * crashing. This should be fixed but until then, we calculate
 	 * the number of blocks the inode.
 	 */
-	if (!ext4_test_inode_flag(inode, EXT4_INODE_INLINE_DATA))
-		ext4_ext_replay_set_iblocks(inode);
+	if (!ext4_test_inode_flag(inode, EXT4_INODE_INLINE_DATA)) {
+		ret = ext4_ext_replay_set_iblocks(inode);
+		if (ret)
+			goto out_brelse;
+	}
 
 	inode->i_generation = le32_to_cpu(ext4_raw_inode(&iloc)->i_generation);
 	ext4_reset_inode_seed(inode);

-- 
2.34.1

Re: [PATCH v3] ext4: propagate errors from fast commit i_blocks replay

[sashiko-bot]

> From: lty <781735889@qq.com>
> 
> ext4_ext_replay_set_iblocks() breaks out of its mapping loop when
> ext4_map_blocks() fails, but then continues with a partial block count
> and unconditionally returns success. The fast commit replay caller also
> ignores the helper return value.
> 
> Stop the count immediately on mapping errors, leave i_blocks unchanged,
> and propagate the failure to abort inode replay.
> 
> Fixes: 8016e29f4362 ("ext4: fast commit recovery path")
> Signed-off-by: lty <781735889@qq.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_ECF4DA918377A7A0D8863B860B0F3025C106@qq.com?part=1

Re: [PATCH v3] ext4: propagate errors from fast commit i_blocks replay

[Jan Kara]

On Tue 15-09-26 09:57:46, 781735889@qq.com wrote:
> From: lty <781735889@qq.com>
> 
> ext4_ext_replay_set_iblocks() breaks out of its mapping loop when
> ext4_map_blocks() fails, but then continues with a partial block count
> and unconditionally returns success. The fast commit replay caller also
> ignores the helper return value.
> 
> Stop the count immediately on mapping errors, leave i_blocks unchanged,
> and propagate the failure to abort inode replay.
> 
> Fixes: 8016e29f4362 ("ext4: fast commit recovery path")
> Signed-off-by: lty <781735889@qq.com>

You could have kept my Reviewed-by tag just for this changelog change.
Anyway feel free to add:

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

								Honza

> ---
> Changes in v3:
> - Move the version history below the separator.
> 
> Changes in v2:
> - Propagate errors from all ext4_find_extent() and skip_hole() calls in
>   ext4_ext_replay_set_iblocks(), as requested by Jan Kara.
> - Keep i_blocks unchanged when any replay lookup fails.
> 
>  fs/ext4/extents.c     | 26 ++++++++++++++++----------
>  fs/ext4/fast_commit.c |  7 +++++--
>  2 files changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 76038b6c3655..f8c805d80c33 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -6275,7 +6275,7 @@ int ext4_ext_replay_set_iblocks(struct inode *inode)
>  		map.m_len = end - cur;
>  		ret = ext4_map_blocks(NULL, inode, &map, 0);
>  		if (ret < 0)
> -			break;
> +			goto cleanup;
>  		if (ret > 0)
>  			numblks += ret;
>  		cur = cur + map.m_len;
> @@ -6291,15 +6291,19 @@ int ext4_ext_replay_set_iblocks(struct inode *inode)
>  	cur = 0;
>  	ret = skip_hole(inode, &cur);
>  	if (ret < 0)
> -		goto out;
> +		goto cleanup;
>  	path = ext4_find_extent(inode, cur, path, 0);
> -	if (IS_ERR(path))
> -		goto out;
> +	if (IS_ERR(path)) {
> +		ret = PTR_ERR(path);
> +		goto cleanup;
> +	}
>  	numblks += path->p_depth;
>  	while (cur < end) {
>  		path = ext4_find_extent(inode, cur, path, 0);
> -		if (IS_ERR(path))
> -			break;
> +		if (IS_ERR(path)) {
> +			ret = PTR_ERR(path);
> +			goto cleanup;
> +		}
>  		ex = path[path->p_depth].p_ext;
>  		if (!ex)
>  			goto cleanup;
> @@ -6308,11 +6312,13 @@ int ext4_ext_replay_set_iblocks(struct inode *inode)
>  					ext4_ext_get_actual_len(ex));
>  		ret = skip_hole(inode, &cur);
>  		if (ret < 0)
> -			break;
> +			goto cleanup;
>  
>  		path2 = ext4_find_extent(inode, cur, path2, 0);
> -		if (IS_ERR(path2))
> -			break;
> +		if (IS_ERR(path2)) {
> +			ret = PTR_ERR(path2);
> +			goto cleanup;
> +		}
>  
>  		for (i = 0; i <= max(path->p_depth, path2->p_depth); i++) {
>  			cmp1 = cmp2 = 0;
> @@ -6333,7 +6339,7 @@ int ext4_ext_replay_set_iblocks(struct inode *inode)
>  cleanup:
>  	ext4_free_ext_path(path);
>  	ext4_free_ext_path(path2);
> -	return 0;
> +	return ret;
>  }
>  
>  int ext4_ext_clear_bb(struct inode *inode)
> diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
> index 0cac890cf370..8110b01e36c1 100644
> --- a/fs/ext4/fast_commit.c
> +++ b/fs/ext4/fast_commit.c
> @@ -2002,8 +2002,11 @@ static int ext4_fc_replay_inode(struct super_block *sb,
>  	 * crashing. This should be fixed but until then, we calculate
>  	 * the number of blocks the inode.
>  	 */
> -	if (!ext4_test_inode_flag(inode, EXT4_INODE_INLINE_DATA))
> -		ext4_ext_replay_set_iblocks(inode);
> +	if (!ext4_test_inode_flag(inode, EXT4_INODE_INLINE_DATA)) {
> +		ret = ext4_ext_replay_set_iblocks(inode);
> +		if (ret)
> +			goto out_brelse;
> +	}
>  
>  	inode->i_generation = le32_to_cpu(ext4_raw_inode(&iloc)->i_generation);
>  	ext4_reset_inode_seed(inode);
> 
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR