diff mbox series

ext4: propagate errors from fast commit create replay

Message ID tencent_46A95D4281B70962D6EC1D523A85C448DC09@qq.com
State New
Headers show
Series ext4: propagate errors from fast commit create replay | expand

Commit Message

lty Sept. 17, 2026, 3:28 a.m. UTC 
From: lty <781735889@qq.com>

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@vger.kernel.org
Signed-off-by: lty <781735889@qq.com>
---
 fs/ext4/fast_commit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

sashiko-bot@kernel.org Sept. 17, 2026, 3:37 a.m. UTC | #1 
> From: lty <781735889@qq.com>
> 
> 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@vger.kernel.org
> Signed-off-by: lty <781735889@qq.com>

Sashiko has reviewed this patch and found no issues. It looks great!
Jan Kara Sept. 24, 2026, 4:25 p.m. UTC | #2 
On Thu 17-09-26 11:28:33, 781735889@qq.com wrote:
> From: lty <781735889@qq.com>
> 
> 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@vger.kernel.org

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

> Signed-off-by: lty <781735889@qq.com>

Otherwise looks mostly good. Feel free to add:

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

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
diff mbox series

Patch

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;
 		}
 	}
@@ -1669,7 +1669,7 @@  static int ext4_fc_replay_create(struct super_block *sb,
 	if (ret)
 		goto out;
 	set_nlink(inode, 1);
-	ext4_mark_inode_dirty(NULL, inode);
+	ret = ext4_mark_inode_dirty(NULL, inode);
 out:
 	iput(inode);
 	return ret;