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

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

[781735889]

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(-)

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;
-- 
2.34.1

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

[781735889]

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(-)

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;
-- 
2.34.1

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

[sashiko-bot]

> 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!

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ext4: fast commit mount failure due to broken idempotent directory replay
--

commit 4015b43a7d3a20549b784ebf5a4fb9ffc38d14b9
Author: lty <781735889@qq.com>
ext4: propagate errors from fast commit create replay

This commit updates the fast-commit replay for directory creation to
propagate errors from parent lookup, directory initialization, and inode
dirtying. Previously, some of these errors were either ignored or masked.

> diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
> index 0cac890cf3702..a6230f4411ed5 100644
> --- a/fs/ext4/fast_commit.c
> +++ b/fs/ext4/fast_commit.c
> @@ -2062,12 +2062,12 @@ static int ext4_fc_replay_create(struct super_block *sb,

[ ... ]

>  		ret = ext4_init_new_dir(NULL, dir, inode);
>  		iput(dir);
>  		if (ret) {
> -			ret = 0;

[Severity: High]
Does this removal break idempotent replays of directory creation?

Fast commit log replay must be idempotent because a directory's blocks might
have been flushed to disk by background writeback before the crash.

In ext4_fc_replay_create(), if the directory already exists on disk,
ext4_init_new_dir() forcefully sets its in-memory i_size to 0 and calls
ext4_append(). ext4_append() then computes logical block 0 and checks if
it is already allocated via ext4_map_blocks().

Since the directory was already fully persisted, the extent tree still maps
block 0, causing ext4_map_blocks() to return a positive value. ext4_append()
interprets this as a corruption ("Logical block already allocated") and
returns -EFSCORRUPTED.

Before this patch, this specific error was safely bypassed (ret = 0) since
the directory was already on disk. By propagating this error, it appears
journal recovery will abort with -EFSCORRUPTED whenever it encounters a
previously persisted directory without inline data, which might cause valid
mounts to fail.

>  			goto out;
>  		}
>  	}

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

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

[Jan Kara]

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
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR