Re: [PATCH v2] ext4: don't fail journal recovery on an incomplete fast commit

[Jan Kara]

On Wed 07-10-26 12:08:29, Hsiu-Hsien Lee wrote:
> If the system crashes while the first fast commit after a full commit is
> being written, the fast commit area can hold the head of that fast
> commit without a valid tail.  ext4_fc_replay_scan() treats this as an
> error as long as no valid tail has been seen: an invalid tag length or
> an unknown tag returns -ECANCELED, a tail with a wrong tid or checksum
> returns -EFSBADCRC.  jbd2_journal_recover() then fails, the file system
> can be mounted neither read-write nor read-only, and the regular journal
> transactions committed before the fast commit are not replayed either:
> 
>   JBD2: journal recovery failed
>   EXT4-fs (sdb): error loading journal
> 
> The tail is written last and fsync() does not return before it has
> completed, so an incomplete fast commit was never reported as durable.
> Handle it like jbd2 handles a transaction without a valid commit block:
> stop the scan and replay what is valid.  This is already what happens
> when an earlier fast commit in the area has a valid tail.
> 
> Reproducer, with the power cut emulated by copying the device while it
> is mounted:
> 
>   mkfs.ext4 -O fast_commit /dev/sdb
>   mount /dev/sdb /mnt; mkdir /mnt/d
>   create 20 files in /mnt/d; sync
>   write a fragmented file and modify the 20 files, fsync one file
>       (one fast commit spanning several blocks)
>   copy /dev/sdb while still mounted, then zero the block holding the
>   fast commit tail in the copy
>   mount the copy
> 
> Without this patch the mount fails as above.  With it, the mount
> succeeds, the state after sync is recovered and e2fsck finds no errors.
> The same holds when a full commit precedes the torn fast commit, in
> which case the full commit is now replayed as well.
> 
> Fixes: 8016e29f4362 ("ext4: fast commit recovery path")
> Assisted-by: Claude Opus 5.5
> Signed-off-by: Hsiu-Hsien Lee <swinds24@xxxxxxxxx>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
> Changes in v2:
> - Drop the warning and the helper, use ext4_debug() and set the return
>   value inline (Jan Kara)
> - v1: https://lore.kernel.org/linux-ext4/20261006070005.1209234-1-swinds24@xxxxxxxxx/
> 
> Testing: re-tested on 6.6.y with the same crash images used for v1 and
> for the unpatched baseline (identical files).  Without the patch, all
> torn first-fast-commit variants fail to mount rw and ro with "JBD2:
> journal recovery failed".  With v2, all of them mount rw and ro, the
> state of the last full commit is recovered and e2fsck -fn is clean.
> Images with intact fast commits, or with a torn fast commit after a
> valid one, behave the same with and without the patch.  The tail was
> torn by zeroing whole blocks, so only the invalid tag length path was
> exercised.
> 
>  fs/ext4/fast_commit.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
> index 0cac890cf370..165950e2b7d9 100644
> --- a/fs/ext4/fast_commit.c
> +++ b/fs/ext4/fast_commit.c
> @@ -2443,6 +2443,12 @@ static bool ext4_fc_value_len_isvalid(struct ext4_sb_info *sbi,
>   * It returns a negative error to indicate that there was an error. At the end
>   * of a successful scan phase, sbi->s_fc_replay_state.fc_replay_num_tags is set
>   * to indicate the number of tags that need to replayed during the replay phase.
> + *
> + * An invalid tag or a tail that does not match ends the scan without an error.
> + * This is what a fast commit that was only partially written before a crash
> + * looks like.  Its tail is written last and fsync() does not return before
> + * the tail is on disk, so such a fast commit was never reported as durable and
> + * is simply dropped, like jbd2 drops a transaction without a commit block.
>   */
>  static int ext4_fc_replay_scan(journal_t *journal,
>  				struct buffer_head *bh, int off,
> @@ -2489,8 +2495,9 @@ static int ext4_fc_replay_scan(journal_t *journal,
>  		val = cur + EXT4_FC_TAG_BASE_LEN;
>  		if (tl.fc_len > end - val ||
>  		    !ext4_fc_value_len_isvalid(sbi, tl.fc_tag, tl.fc_len)) {
> -			ret = state->fc_replay_num_tags ?
> -				JBD2_FC_REPLAY_STOP : -ECANCELED;
> +			ext4_debug("Scan phase, invalid tag length, blk %lld\n",
> +				   bh->b_blocknr);
> +			ret = JBD2_FC_REPLAY_STOP;
>  			goto out_err;
>  		}
>  		ext4_debug("Scan phase, tag:%s, blk %lld\n",
> @@ -2530,8 +2537,9 @@ static int ext4_fc_replay_scan(journal_t *journal,
>  				state->fc_regions_valid =
>  					state->fc_regions_used;
>  			} else {
> -				ret = state->fc_replay_num_tags ?
> -					JBD2_FC_REPLAY_STOP : -EFSBADCRC;
> +				ext4_debug("Scan phase, invalid tail, blk %lld\n",
> +					   bh->b_blocknr);
> +				ret = JBD2_FC_REPLAY_STOP;
>  			}
>  			state->fc_crc = 0;
>  			break;
> @@ -2551,8 +2559,9 @@ static int ext4_fc_replay_scan(journal_t *journal,
>  				EXT4_FC_TAG_BASE_LEN + tl.fc_len);
>  			break;
>  		default:
> -			ret = state->fc_replay_num_tags ?
> -				JBD2_FC_REPLAY_STOP : -ECANCELED;
> +			ext4_debug("Scan phase, unknown tag %d, blk %lld\n",
> +				   tl.fc_tag, bh->b_blocknr);
> +			ret = JBD2_FC_REPLAY_STOP;
>  		}
>  		if (ret < 0 || ret == JBD2_FC_REPLAY_STOP)
>  			break;
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR