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

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

[lty]

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>
---
 fs/ext4/extents.c     | 4 ++--
 fs/ext4/fast_commit.c | 7 +++++--
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 76038b6c3655..d67e30cf44ad 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;
@@ -6333,7 +6333,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);

base-commit: 704340f1cd0dcef829eb62f5b48ae95a2ce17bdf
-- 
2.34.1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The patch fails to fix error handling in the second loop of ext4_ext_replay_set_iblocks(), leading to partial block counts being applied to the inode and masked errors.
--

commit 561c6a78544da466ba6d1525d42b474f54d2d473
Author: lty <781735889@qq.com>

ext4: propagate errors from fast commit i_blocks replay

This patch stops ext4_ext_replay_set_iblocks() from calculating partial
block counts when ext4_map_blocks() fails. It leaves i_blocks unchanged
and aborts inode replay on errors.

> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 76038b6c36552..d67e30cf44adb 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
[ ... ]
> @@ -6329,11 +6329,11 @@ int ext4_ext_replay_set_iblocks(struct inode *inode)
>  out:
>  	inode->i_blocks = numblks << (inode->i_sb->s_blocksize_bits - 9);
>  	ext4_mark_inode_dirty(NULL, inode);

[Severity: High]
This isn't a bug introduced by this patch, but does the second loop in
ext4_ext_replay_set_iblocks() still suffer from the same issue?

While this patch correctly captures errors from ext4_map_blocks() in the
first loop, failures during the extent tree block counting phase still jump
to the out label:

	while (cur < end) {
		path = ext4_find_extent(inode, cur, path, 0);
		if (IS_ERR(path))
			break;
		ex = path[path->p_depth].p_ext;
		...
		ret = skip_hole(inode, &cur);
		if (ret < 0)
			break;

If any of these break conditions occur, we hit the out label shown in the
diff context above and blindly overwrite inode->i_blocks with an incomplete
numblks tally.

Additionally, if ext4_find_extent() fails, we don't propagate the error code
because ret might still be 0 from a previous skip_hole() success. This allows
fast commit replay to silently corrupt metadata while returning success.

Should the error paths in the second loop also jump to cleanup instead of
out, and ensure ret is updated to PTR_ERR(path) on lookup failures?

>  cleanup:
>  	ext4_free_ext_path(path);
>  	ext4_free_ext_path(path2);
> -	return 0;
> +	return ret;
>  }

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

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

[Jan Kara]

On Mon 14-09-26 15:16:45, lty wrote:
> 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>

I guess this is fair but please propagate all errors from
ext4_ext_replay_set_iblocks(). There are several other places in that
function doing things like:

	path = ext4_find_extent(inode, cur, path, 0);
        if (IS_ERR(path))
                goto out;

which can return errors we should be propagating up.

								Honza
> ---
>  fs/ext4/extents.c     | 4 ++--
>  fs/ext4/fast_commit.c | 7 +++++--
>  2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 76038b6c3655..d67e30cf44ad 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;
> @@ -6333,7 +6333,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);
> 
> base-commit: 704340f1cd0dcef829eb62f5b48ae95a2ce17bdf
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR