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

[Jan Kara]

On Thu 17-09-26 11:28:33, 781735889@xxxxxx wrote:
> From: lty <781735889@xxxxxx>
> 
> During fast-commit replay, ext4_fc_replay_create() can fail while
> looking up the parent directory, initializing a new directory, or
> dirtying the replayed inode.  The parent lookup error jumps to the
> cleanup path while ret is still zero.  Errors from ext4_init_new_dir()
> are explicitly converted to zero, and the return value from
> ext4_mark_inode_dirty() is ignored.
> 
> Consequently, replay can report success and mount the filesystem even
> though the directory creation was not completely replayed.  A failed
> parent lookup, for example, leaves an unconnected directory inode.
> 
> Propagate these errors so journal recovery aborts rather than completing
> with inconsistent metadata.
> 
> Tested with QEMU fast-commit directory replay and injected failures in
> the parent lookup, directory initialization, and inode dirtying paths.
> Before the change, recovery completed and e2fsck reported an unconnected
> directory inode or inconsistent inode metadata.  With the change, JBD2
> reports recovery failure and the mount fails in all three cases.
> 
> Fixes: 8016e29f4362 ("ext4: fast commit recovery path")
> Cc: stable@xxxxxxxxxxxxxxx

I don't think this is really stable material. Mostly a cosmetic bugfix...

> Signed-off-by: lty <781735889@xxxxxx>

Otherwise looks mostly good. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

but please fix one nit below:

> diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
> index b3c22636251d..e656abd938fb 100644
> --- a/fs/ext4/fast_commit.c
> +++ b/fs/ext4/fast_commit.c
> @@ -1656,12 +1656,12 @@ static int ext4_fc_replay_create(struct super_block *sb,
>  		dir = ext4_iget(sb, darg.parent_ino, EXT4_IGET_NORMAL);
>  		if (IS_ERR(dir)) {
>  			ext4_debug("Dir %d not found.", darg.ino);
> +			ret = PTR_ERR(dir);
>  			goto out;
>  		}
>  		ret = ext4_init_new_dir(NULL, dir, inode);
>  		iput(dir);
>  		if (ret) {
> -			ret = 0;
>  			goto out;
>  		}

Please remove the now superfluous braces.

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR