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

[Jan Kara]

On Mon 14-09-26 20:05:56, 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>
> 
> 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.

The 'Changes' paragraph belongs after the --- separator below so that it
doesn't get into the pernament changelog. Otherwise the fix looks good to
me. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

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