[PATCH] ext4: fix fast commit replay failing on a read-only mount

[PATCH] ext4: fix fast commit replay failing on a read-only mount

[Hsiu-Hsien Lee]

A filesystem with fast_commit that needs recovery cannot be mounted
read-only:

  EXT4-fs (dm-0): INFO: recovery required on readonly filesystem
  EXT4-fs (dm-0): write access will be enabled during recovery
  WARNING: CPU: 22 PID: 5544 at fs/ext4/ext4_jbd2.c:73
   ext4_journal_check_start
   __ext4_journal_start_sb
   __ext4_unlink
   ext4_fc_replay
   do_one_pass
   jbd2_journal_recover
   jbd2_journal_load
   __ext4_fill_super
  JBD2: journal recovery failed
  EXT4-fs (dm-0): error loading journal

Fast commit replay runs ext4 metadata operations instead of writing
blocks through the buffer cache: ext4_fc_replay_{unlink,link,create}()
reach __ext4_unlink() and __ext4_link(), which start a handle.
ext4_journal_check_start() returns -EROFS on a read-only sb, and as
that is not -ENOENT it propagates out of jbd2_journal_recover() and
kills the whole recovery.  The EXT4_FC_REPLAY check that would hand
out a no-journal handle sits after the sb_rdonly() test, so replay can
never complete read-only.

ext4_load_journal() has already promised that write access will be
enabled during recovery, so make that true for the superblock as well:
clear SB_RDONLY across jbd2_journal_load() when recovery is needed on a
read-only mount and the devices are writable, as ext4_orphan_cleanup()
does.  Unlike ext4_handle_error(), which avoids SB_RDONLY because it
would need s_umount, the sb here is still inside ext4_fill_super() and
not published, so nothing can observe it.

The failure is not clean either: the replay handlers passing a NULL
handle (ext4_fc_replay_inode(), _add_range(), _del_range()) never hit
ext4_journal_check_start() and do write, leaving a partially applied
fast commit behind.

Reproducer, where the unlink only ever reaches the fast commit area:

  mke2fs -q -F -t ext4 -O fast_commit -b 4096 /dev/sdb3 262144
  mount /dev/sdb3 /mnt
  dd if=/dev/zero of=/mnt/victim bs=4k count=1 conv=fsync
  sync                          # victim now in a full commit
  rm /mnt/victim
  dd if=/dev/zero of=/mnt/trigger bs=4k count=1 conv=fsync
  <crash, or snapshot the device while mounted and write it back>
  mount -o ro /dev/sdb3 /mnt

Without this patch that mount fails; with it recovery completes and
victim is gone, i.e. the UNLINK record was really replayed.

Fixes: 8016e29f4362 ("ext4: fast commit recovery path")
Cc: stable@vger.kernel.org
Signed-off-by: Hsiu-Hsien Lee <swinds24@gmail.com>
---
 fs/ext4/super.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 245f67d10ded..6c2b275a9cf3 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -6096,6 +6096,7 @@ static int ext4_load_journal(struct super_block *sb,
 	int err = 0;
 	int really_read_only;
 	int journal_dev_ro;
+	bool enable_write = false;
 
 	if (WARN_ON_ONCE(!ext4_has_feature_journal(sb)))
 		return -EFSCORRUPTED;
@@ -6152,6 +6153,7 @@ static int ext4_load_journal(struct super_block *sb,
 			}
 			ext4_msg(sb, KERN_INFO, "write access will "
 			       "be enabled during recovery");
+			enable_write = true;
 		}
 	}
 
@@ -6168,7 +6170,19 @@ static int ext4_load_journal(struct super_block *sb,
 		if (save)
 			memcpy(save, ((char *) es) +
 			       EXT4_S_ERR_START, EXT4_S_ERR_LEN);
+		/*
+		 * Fast commit replay performs regular ext4 metadata updates
+		 * (see ext4_fc_replay()) which refuse to run on a read-only
+		 * superblock.  We promised write access above, so make that
+		 * true for the duration of the recovery, the same way
+		 * ext4_orphan_cleanup() does.  The superblock is not published
+		 * yet, so nothing can observe the transient state.
+		 */
+		if (enable_write)
+			sb->s_flags &= ~SB_RDONLY;
 		err = jbd2_journal_load(journal);
+		if (enable_write)
+			sb->s_flags |= SB_RDONLY;
 		if (save && memcmp(((char *) es) + EXT4_S_ERR_START,
 				   save, EXT4_S_ERR_LEN)) {
 			memcpy(((char *) es) + EXT4_S_ERR_START,
-- 
2.43.0

Re: [PATCH] ext4: fix fast commit replay failing on a read-only mount

[sashiko-bot]

> A filesystem with fast_commit that needs recovery cannot be mounted
> read-only:
> 
>   EXT4-fs (dm-0): INFO: recovery required on readonly filesystem
>   EXT4-fs (dm-0): write access will be enabled during recovery
>   WARNING: CPU: 22 PID: 5544 at fs/ext4/ext4_jbd2.c:73
>    ext4_journal_check_start
>    __ext4_journal_start_sb
>    __ext4_unlink
>    ext4_fc_replay
>    do_one_pass
>    jbd2_journal_recover
>    jbd2_journal_load
>    __ext4_fill_super
>   JBD2: journal recovery failed
> [ ... ]
> victim is gone, i.e. the UNLINK record was really replayed.
> 
> Fixes: 8016e29f4362 ("ext4: fast commit recovery path")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hsiu-Hsien Lee <swinds24@gmail.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260812044353.1018268-1-swinds24@gmail.com?part=1

Re: [PATCH] ext4: fix fast commit replay failing on a read-only mount

[Jan Kara]

On Wed 12-08-26 12:43:53, Hsiu-Hsien Lee wrote:
> A filesystem with fast_commit that needs recovery cannot be mounted
> read-only:
> 
>   EXT4-fs (dm-0): INFO: recovery required on readonly filesystem
>   EXT4-fs (dm-0): write access will be enabled during recovery
>   WARNING: CPU: 22 PID: 5544 at fs/ext4/ext4_jbd2.c:73
>    ext4_journal_check_start
>    __ext4_journal_start_sb
>    __ext4_unlink
>    ext4_fc_replay
>    do_one_pass
>    jbd2_journal_recover
>    jbd2_journal_load
>    __ext4_fill_super
>   JBD2: journal recovery failed
>   EXT4-fs (dm-0): error loading journal
> 
> Fast commit replay runs ext4 metadata operations instead of writing
> blocks through the buffer cache: ext4_fc_replay_{unlink,link,create}()
> reach __ext4_unlink() and __ext4_link(), which start a handle.
> ext4_journal_check_start() returns -EROFS on a read-only sb, and as
> that is not -ENOENT it propagates out of jbd2_journal_recover() and
> kills the whole recovery.  The EXT4_FC_REPLAY check that would hand
> out a no-journal handle sits after the sb_rdonly() test, so replay can
> never complete read-only.
> 
> ext4_load_journal() has already promised that write access will be
> enabled during recovery, so make that true for the superblock as well:
> clear SB_RDONLY across jbd2_journal_load() when recovery is needed on a
> read-only mount and the devices are writable, as ext4_orphan_cleanup()
> does.  Unlike ext4_handle_error(), which avoids SB_RDONLY because it
> would need s_umount, the sb here is still inside ext4_fill_super() and
> not published, so nothing can observe it.
> 
> The failure is not clean either: the replay handlers passing a NULL
> handle (ext4_fc_replay_inode(), _add_range(), _del_range()) never hit
> ext4_journal_check_start() and do write, leaving a partially applied
> fast commit behind.
> 
> Reproducer, where the unlink only ever reaches the fast commit area:
> 
>   mke2fs -q -F -t ext4 -O fast_commit -b 4096 /dev/sdb3 262144
>   mount /dev/sdb3 /mnt
>   dd if=/dev/zero of=/mnt/victim bs=4k count=1 conv=fsync
>   sync                          # victim now in a full commit
>   rm /mnt/victim
>   dd if=/dev/zero of=/mnt/trigger bs=4k count=1 conv=fsync
>   <crash, or snapshot the device while mounted and write it back>
>   mount -o ro /dev/sdb3 /mnt
> 
> Without this patch that mount fails; with it recovery completes and
> victim is gone, i.e. the UNLINK record was really replayed.
> 
> Fixes: 8016e29f4362 ("ext4: fast commit recovery path")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hsiu-Hsien Lee <swinds24@gmail.com>

Looks sensible to me and I'm surprised nobody tripped over this earlier.
Feel free to add:

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

								Honza

> ---
>  fs/ext4/super.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 245f67d10ded..6c2b275a9cf3 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -6096,6 +6096,7 @@ static int ext4_load_journal(struct super_block *sb,
>  	int err = 0;
>  	int really_read_only;
>  	int journal_dev_ro;
> +	bool enable_write = false;
>  
>  	if (WARN_ON_ONCE(!ext4_has_feature_journal(sb)))
>  		return -EFSCORRUPTED;
> @@ -6152,6 +6153,7 @@ static int ext4_load_journal(struct super_block *sb,
>  			}
>  			ext4_msg(sb, KERN_INFO, "write access will "
>  			       "be enabled during recovery");
> +			enable_write = true;
>  		}
>  	}
>  
> @@ -6168,7 +6170,19 @@ static int ext4_load_journal(struct super_block *sb,
>  		if (save)
>  			memcpy(save, ((char *) es) +
>  			       EXT4_S_ERR_START, EXT4_S_ERR_LEN);
> +		/*
> +		 * Fast commit replay performs regular ext4 metadata updates
> +		 * (see ext4_fc_replay()) which refuse to run on a read-only
> +		 * superblock.  We promised write access above, so make that
> +		 * true for the duration of the recovery, the same way
> +		 * ext4_orphan_cleanup() does.  The superblock is not published
> +		 * yet, so nothing can observe the transient state.
> +		 */
> +		if (enable_write)
> +			sb->s_flags &= ~SB_RDONLY;
>  		err = jbd2_journal_load(journal);
> +		if (enable_write)
> +			sb->s_flags |= SB_RDONLY;
>  		if (save && memcmp(((char *) es) + EXT4_S_ERR_START,
>  				   save, EXT4_S_ERR_LEN)) {
>  			memcpy(((char *) es) + EXT4_S_ERR_START,
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR