On Mon 26-06-23 15:33:22, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@xxxxxxxxxx>
>
> We got a filesystem inconsistency issue below while running generic/475
> I/O failure pressure test with fast_commit feature enabled.
>
> Symlink /p3/d3/d1c/d6c/dd6/dce/l101 (inode #132605) is invalid.
>
> If fast_commit feature is enabled, a special fast_commit journal area is
> appended to the end of the normal journal area. The journal->j_last
> point to the first unused block behind the normal journal area instead
> of the whole log area, and the journal->j_fc_last point to the first
> unused block behind the fast_commit journal area. While doing journal
> recovery, do_one_pass(PASS_SCAN) should first scan the normal journal
> area and turn around to the first block once it meet journal->j_last,
> but the wrap() macro misuse the journal->j_fc_last, so the recovering
> could not read the next magic block (commit block perhaps) and would end
> early mistakenly and missing tN and every transaction after it in the
> following example. Finally, it could lead to filesystem inconsistency.
>
> | normal journal area | fast commit area |
> +-------------------------------------------------+------------------+
> | tN(rere) | tN+1 |~| tN-x |...| tN-1 | tN(front) | .... |
> +-------------------------------------------------+------------------+
> / / /
> start journal->j_last journal->j_fc_last
>
> This patch fix it by use the correct ending journal->j_last.
>
> Fixes: 5b849b5f96b4 ("jbd2: fast commit recovery path")
> Reported-by: Theodore Ts'o <tytso@xxxxxxx>
> Link: https://lore.kernel.org/linux-ext4/20230613043120.GB1584772@xxxxxxx/
> Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx>
Ah, great catch! The patch looks good to me. Feel free to add:
Reviewed-by: Jan Kara <jack@xxxxxxx>
Honza
> ---
> fs/jbd2/recovery.c | 12 +++---------
> 1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
> index 0184931d47f7..c269a7d29a46 100644
> --- a/fs/jbd2/recovery.c
> +++ b/fs/jbd2/recovery.c
> @@ -230,12 +230,8 @@ static int count_tags(journal_t *journal, struct buffer_head *bh)
> /* Make sure we wrap around the log correctly! */
> #define wrap(journal, var) \
> do { \
> - unsigned long _wrap_last = \
> - jbd2_has_feature_fast_commit(journal) ? \
> - (journal)->j_fc_last : (journal)->j_last; \
> - \
> - if (var >= _wrap_last) \
> - var -= (_wrap_last - (journal)->j_first); \
> + if (var >= (journal)->j_last) \
> + var -= ((journal)->j_last - (journal)->j_first); \
> } while (0)
>
> static int fc_do_one_pass(journal_t *journal,
> @@ -524,9 +520,7 @@ static int do_one_pass(journal_t *journal,
> break;
>
> jbd2_debug(2, "Scanning for sequence ID %u at %lu/%lu\n",
> - next_commit_ID, next_log_block,
> - jbd2_has_feature_fast_commit(journal) ?
> - journal->j_fc_last : journal->j_last);
> + next_commit_ID, next_log_block, journal->j_last);
>
> /* Skip over each chunk of the transaction looking
> * either the next descriptor block or the final commit
> --
> 2.31.1
>
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
