Thanks for your review. New v3 patch will be resent. 在 2021/3/16 0:53, harshad shirwadkar 写道: > Thanks for the updated patch. Just have some a couple of minor nits > but other than that this looks good. > > Reviewed-by: Harshad Shirwadkar <harshadshirwadkar@xxxxxxxxx> > > On Thu, Mar 11, 2021 at 3:50 AM Zhiqiang Liu <liuzhiqiang26@xxxxxxxxxx> wrote: >> >> friendly ping.. >> >> On 2021/3/6 15:27, Haotian Li wrote: >>> jbd2_journal_recover() may fail when some error occers such >>> as ENOMEM and EIO. However, jsb->s_start is still cleared >>> by func e2fsck_journal_release(). This may break consistency >>> between metadata and data in disk. Sometimes, failure in >>> jbd2_journal_recover() is temporary but retry e2fsck will >>> skip the journal recovery when the temporary problem is fixed. >>> >>> Following harshad shirwadkar's suggestion,we add an option >>> "recovery_error_behavior" with default value "continue" to >>> e2fsck.conf. User may set it to "retry" or "exit" to adopt >>> different behavior when such journal recovery errors occur. >>> >>> Reported-by: Liangyun <liangyun2@xxxxxxxxxx> >>> Signed-off-by: Haotian Li <lihaotian9@xxxxxxxxxx> >>> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@xxxxxxxxxx> >>> --- >>> e2fsck/e2fsck.h | 11 +++++++++++ >>> e2fsck/journal.c | 33 +++++++++++++++++++++++++++++++-- >>> e2fsck/unix.c | 13 ++++++++++++- >>> 3 files changed, 54 insertions(+), 3 deletions(-) >>> >>> diff --git a/e2fsck/e2fsck.h b/e2fsck/e2fsck.h >>> index 15d043ee..22f9ad11 100644 >>> --- a/e2fsck/e2fsck.h >>> +++ b/e2fsck/e2fsck.h >>> @@ -451,6 +451,9 @@ struct e2fsck_struct { >>> >>> /* Fast commit replay state */ >>> struct e2fsck_fc_replay_state fc_replay_state; >>> + >>> + /* Behavior when journal recovery fails */ >>> + int recovery_error_behavior; >>> }; >>> >>> /* Data structures to evaluate whether an extent tree needs rebuilding. */ >>> @@ -474,6 +477,14 @@ typedef struct region_struct *region_t; >>> extern int e2fsck_strnlen(const char * s, int count); >>> #endif >>> >>> +/* Different behaviors when journal recovery fails */ >>> +#define RECOVERY_ERROR_CONTINUE 0 >>> +#define RECOVERY_ERROR_RETRY 1 >>> +#define RECOVERY_ERROR_EXIT 2 >>> + >>> +/* Journal retry times if RECOVERY_ERROR_RETRY is set*/ >>> +#define RECOVERY_TIMES_LIMIT 3 >>> + >>> /* >>> * Procedure declarations >>> */ >>> diff --git a/e2fsck/journal.c b/e2fsck/journal.c >>> index a425bbd1..c1c6f6ee 100644 >>> --- a/e2fsck/journal.c >>> +++ b/e2fsck/journal.c >>> @@ -1600,11 +1600,26 @@ no_has_journal: >>> return retval; >>> } >>> >>> +static void set_recovery_error_behavior(e2fsck_t ctx, const char *recovery_behavior) >>> +{ >>> + if (!recovery_behavior) { >>> + ctx->recovery_error_behavior = RECOVERY_ERROR_CONTINUE; >>> + return; >>> + } >>> + if (strcmp(recovery_behavior, "retry") == 0) >>> + ctx->recovery_error_behavior = RECOVERY_ERROR_RETRY; >>> + else if (strcmp(recovery_behavior, "exit") == 0) >>> + ctx->recovery_error_behavior = RECOVERY_ERROR_EXIT; >>> + else >>> + ctx->recovery_error_behavior = RECOVERY_ERROR_CONTINUE; >>> +} >>> + >>> static errcode_t recover_ext3_journal(e2fsck_t ctx) >>> { >>> struct problem_context pctx; >>> journal_t *journal; >>> errcode_t retval; >>> + char *recovery_behavior = 0; >>> >>> clear_problem_context(&pctx); >>> >>> @@ -1629,8 +1644,12 @@ static errcode_t recover_ext3_journal(e2fsck_t ctx) >>> goto errout; >>> >>> retval = -jbd2_journal_recover(journal); >>> - if (retval) >>> + if (retval) { >>> + profile_get_string(ctx->profile, "options", "recovery_error_behavior", >>> + 0, "continue", &recovery_behavior); >>> + set_recovery_error_behavior(ctx, recovery_behavior); >>> goto errout; >>> + } >>> >>> if (journal->j_failed_commit) { >>> pctx.ino = journal->j_failed_commit; >>> @@ -1645,7 +1664,15 @@ errout: >>> jbd2_journal_destroy_revoke(journal); >>> jbd2_journal_destroy_revoke_record_cache(); >>> jbd2_journal_destroy_revoke_table_cache(); >>> - e2fsck_journal_release(ctx, journal, 1, 0); >>> + if (retval == 0 || ctx->recovery_error_behavior == RECOVERY_ERROR_CONTINUE) >>> + e2fsck_journal_release(ctx, journal, 1, 0); >>> + if (retval && ctx->recovery_error_behavior == RECOVERY_ERROR_EXIT) { >>> + ctx->fs->flags &= ~EXT2_FLAG_VALID; >>> + com_err(ctx->program_name, 0, >>> + _("Journal recovery failed " >>> + "on %s\n"), ctx->device_name); >>> + fatal_error(ctx, 0); >>> + } >>> return retval; >>> } >>> >>> @@ -1697,6 +1724,8 @@ errcode_t e2fsck_run_ext3_journal(e2fsck_t ctx) >>> >>> /* Set the superblock flags */ >>> e2fsck_clear_recover(ctx, recover_retval != 0); >>> + if (recover_retval != 0 && ctx->recovery_error_behavior == RECOVERY_ERROR_RETRY) >>> + ext2fs_set_feature_journal_needs_recovery(ctx->fs->super); >>> >>> /* >>> * Do one last sanity check, and propagate journal->s_errno to >>> diff --git a/e2fsck/unix.c b/e2fsck/unix.c >>> index c5f9e441..25978471 100644 >>> --- a/e2fsck/unix.c >>> +++ b/e2fsck/unix.c >>> @@ -1068,6 +1068,8 @@ static errcode_t PRS(int argc, char *argv[], e2fsck_t *ret_ctx) >>> if (c) >>> ctx->options |= E2F_OPT_ICOUNT_FULLMAP; >>> >>> + ctx->recovery_error_behavior = RECOVERY_ERROR_CONTINUE; >>> + >>> if (ctx->readahead_kb == ~0ULL) { >>> profile_get_integer(ctx->profile, "options", >>> "readahead_mem_pct", 0, -1, &c); >>> @@ -1776,6 +1778,7 @@ failure: >>> "doing a read-only filesystem check.\n")); >>> io_channel_flush(ctx->fs->io); >>> } else { >>> + int recovery_retry_times = 0; >>> if (ctx->flags & E2F_FLAG_RESTARTED) { >>> /* >>> * Whoops, we attempted to run the >>> @@ -1788,7 +1791,15 @@ failure: >>> "on %s\n"), ctx->device_name); >>> fatal_error(ctx, 0); >>> } >>> - retval = e2fsck_run_ext3_journal(ctx); >>> + while (recovery_retry_times++ < RECOVERY_TIMES_LIMIT) { >>> + retval = e2fsck_run_ext3_journal(ctx); >>> + if (retval && ctx->recovery_error_behavior == RECOVERY_ERROR_RETRY) { >>> + log_out(ctx, _("Try to recovery Journal " >>> + "again in %s\n"), > (nit) I think there's no reason to break the string into 2 lines. This > will make this string searchable. >>> + ctx->device_name); >>> + } else >>> + break; > (style) Since you have {} brackets for if condition, please add it for > else too (or remove it for if condition too) > > Thanks, > Harshad >>> + } >>> if (retval == EFSBADCRC) { >>> log_out(ctx, _("Journal checksum error " >>> "found in %s\n"), >> > . >