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@xxxxxx>

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@xxxxxxxx>
SUSE Labs, CR