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

[Hsiu-Hsien Lee]

Thanks for the review. It makes sense, and I'll drop the warning and the
helper and use ext4_debug() in v2.

- Jerry


Jan Kara <jack@xxxxxxx> 於 2026年10月7日週三 上午12:08寫道：
>
> On Tue 06-10-26 15:00:05, 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.  Log a
> > warning so that the dropped fast commit is visible.
> >
> > 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>
> > ---
> >  fs/ext4/fast_commit.c | 36 +++++++++++++++++++++++++++++-------
> >  1 file changed, 29 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
> > index 0cac890cf370..5649447b9524 100644
> > --- a/fs/ext4/fast_commit.c
> > +++ b/fs/ext4/fast_commit.c
> > @@ -2427,6 +2427,26 @@ static bool ext4_fc_value_len_isvalid(struct ext4_sb_info *sbi,
> >       return false;
> >  }
> >
> > +/*
> > + * The fast commit area ends with an invalid tag or a tail that does not
> > + * match.  This is how a fast commit that was only partially written before
> > + * a crash looks like: its tail is written last and fsync() only returns
> > + * after the tail write has completed, so nobody was told that this fast
> > + * commit is durable and it can simply be dropped, the same way jbd2 drops
> > + * a transaction without a valid commit block.  Everything up to the last
> > + * valid tail is still replayed.
> > + */
> > +static int ext4_fc_replay_scan_end(struct super_block *sb,
> > +                                struct ext4_fc_replay_state *state,
> > +                                int off, const char *reason)
> > +{
> > +     if (!state->fc_replay_num_tags)
> > +             ext4_msg(sb, KERN_WARNING,
> > +                      "ignoring incomplete fast commit at block %d: %s",
> > +                      off, reason);
>
> I don't think there's need to print any message when discovering incomplete
> fast commit. As you mention, it is a perfectly normal situation (similarly
> as incomplete normal transaction we spot in a journal). I'd perhaps just
> print ext4_debug() message (and I don't think conditioning on
> fc_replay_num_tags makes sense in that case. So probably just drop the
> helper function and print the debug message and set the return value
> inline...
>
>                                                                 Honza
>
> > +     return JBD2_FC_REPLAY_STOP;
> > +}
> > +
> >  /*
> >   * Recovery Scan phase handler
> >   *
> > @@ -2440,7 +2460,9 @@ static bool ext4_fc_value_len_isvalid(struct ext4_sb_info *sbi,
> >   * This function returns JBD2_FC_REPLAY_CONTINUE to indicate that SCAN is
> >   * incomplete and JBD2 should send more blocks. It returns JBD2_FC_REPLAY_STOP
> >   * to indicate that scan has finished and JBD2 can now start replay phase.
> > - * It returns a negative error to indicate that there was an error. At the end
> > + * It returns a negative error to indicate that there was an error. An invalid
> > + * or incomplete fast commit at the end of the area is not an error, see
> > + * ext4_fc_replay_scan_end(). 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.
> >   */
> > @@ -2489,8 +2511,8 @@ 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;
> > +                     ret = ext4_fc_replay_scan_end(sb, state, off,
> > +                                                   "invalid tag length");
> >                       goto out_err;
> >               }
> >               ext4_debug("Scan phase, tag:%s, blk %lld\n",
> > @@ -2530,8 +2552,8 @@ 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;
> > +                             ret = ext4_fc_replay_scan_end(sb, state, off,
> > +                                                           "invalid tail");
> >                       }
> >                       state->fc_crc = 0;
> >                       break;
> > @@ -2551,8 +2573,8 @@ 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;
> > +                     ret = ext4_fc_replay_scan_end(sb, state, off,
> > +                                                   "unknown tag");
> >               }
> >               if (ret < 0 || ret == JBD2_FC_REPLAY_STOP)
> >                       break;
> > --
> > 2.43.0
> >
> --
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR