| Message ID | 1447463429-5966-2-git-send-email-andreas.dilger@intel.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Nov 13, 2015, at 6:10 PM, Andreas Dilger <andreas.dilger@intel.com> wrote: > There is a bug in how e2fsck handles being interrupted by CTRL-C. > If CTRL-C is pressed to kill e2fsck rather than e.g. kill -9, then > the interrupt handler sets E2F_FLAG_CANCEL in the context but doesn't > actually kill the process. Instead, e2fsck_pass1() checks this flag > before processing the next inode. Sigh, please remove the "LU-7368" label from the summary line, as that is for internal tracking and I thought I'd done that for the patches before sending them out. If desired, this patch can be linked back to our bug tracker to get a full discussion of the bug as below: Lustre-change: http://review.whamcloud.com/17150 Lustre-bug-id: https://jira.hpdd.intel.com/browse/LU-7368 For "libext2fs: fix block-mapped file punch" it would be: Lustre-change: http://review.whamcloud.com/17152 Lustre-bug-id: https://jira.hpdd.intel.com/browse/LU-7381 and for "e2fsck: fix e2fsck -fD directory truncation" it is: Lustre-change: http://review.whamcloud.com/17153 Lustre-bug-id: https://jira.hpdd.intel.com/browse/LU-7381 Cheers, Andreas > If a filesystem is running in fix mode (e2fsck -fy) is interrupted, > and the quota feature is enabled, then the quota file will still be > written to disk even though the inode scan was not complete and the > quota information is totally inaccurate. Even worse, if the Pass 1 > inode and block scan was not finished, then the in-memory block > bitmaps (which are used for block allocation during e2fsck) are also > invalid, so any blocks allocated to the quota files may corrupt other > files if those blocks were actually used. > > e2fsck 1.42.13.wc3 (28-Aug-2015) > Pass 1: Checking inodes, blocks, and sizes > ^C[QUOTA WARNING] Usage inconsistent for ID 0: > actual (6455296, 168) != expected (8568832, 231) > [QUOTA WARNING] Usage inconsistent for ID 695: > actual (614932320256, 63981) != expected (2102405386240, 176432) > Update quota info for quota type 0? yes > > [QUOTA WARNING] Usage inconsistent for ID 0: > actual (6455296, 168) != expected (8568832, 231) > [QUOTA WARNING] Usage inconsistent for ID 538: > actual (614932320256, 63981) != expected (2102405386240, 176432) > Update quota info for quota type 1? yes > > myth-OST0001: e2fsck canceled. > myth-OST0001: ***** FILE SYSTEM WAS MODIFIED ***** > > There may be a desire to flush out modified inodes and such that have > been repaired, so that restarting an interrupted e2fsck will make > progress, but the quota file update is plain wrong unless at least > pass1 has finished, and the journal recreation is also dangerous if > the block bitmaps have not been fully updated. > > Signed-off-by: Andreas Dilger <andreas.dilger@intel.com> > --- > e2fsck/e2fsck.c | 2 -- > e2fsck/e2fsck.h | 4 ++-- > e2fsck/pass1.c | 4 +++- > e2fsck/pass2.c | 10 +++++----- > e2fsck/unix.c | 18 ++++++++++-------- > 5 files changed, 20 insertions(+), 18 deletions(-) > > diff --git a/e2fsck/e2fsck.c b/e2fsck/e2fsck.c > index 0ec1540..2002dc0 100644 > --- a/e2fsck/e2fsck.c > +++ b/e2fsck/e2fsck.c > @@ -203,8 +203,6 @@ static pass_t e2fsck_passes[] = { > e2fsck_pass1, e2fsck_pass2, e2fsck_pass3, e2fsck_pass4, > e2fsck_pass5, 0 }; > > -#define E2F_FLAG_RUN_RETURN (E2F_FLAG_SIGNAL_MASK|E2F_FLAG_RESTART) > - > int e2fsck_run(e2fsck_t ctx) > { > int i; > diff --git a/e2fsck/e2fsck.h b/e2fsck/e2fsck.h > index f904026..810030e 100644 > --- a/e2fsck/e2fsck.h > +++ b/e2fsck/e2fsck.h > @@ -173,10 +173,10 @@ struct resource_track { > */ > #define E2F_FLAG_ABORT 0x0001 /* Abort signaled */ > #define E2F_FLAG_CANCEL 0x0002 /* Cancel signaled */ > -#define E2F_FLAG_SIGNAL_MASK 0x0003 > +#define E2F_FLAG_SIGNAL_MASK (E2F_FLAG_ABORT | E2F_FLAG_CANCEL) > #define E2F_FLAG_RESTART 0x0004 /* Restart signaled */ > +#define E2F_FLAG_RUN_RETURN (E2F_FLAG_SIGNAL_MASK | E2F_FLAG_RESTART) > #define E2F_FLAG_RESTART_LATER 0x0008 /* Restart after all iterations done */ > - > #define E2F_FLAG_SETJMP_OK 0x0010 /* Setjmp valid for abort */ > > #define E2F_FLAG_PROG_BAR 0x0020 /* Progress bar on screen */ > diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c > index 50a8b99..1c2b8fd 100644 > --- a/e2fsck/pass1.c > +++ b/e2fsck/pass1.c > @@ -766,7 +766,7 @@ void e2fsck_pass1(e2fsck_t ctx) > inode, inode_size); > ehandler_operation(old_op); > if (ctx->flags & E2F_FLAG_SIGNAL_MASK) > - return; > + goto endit; > if (pctx.errcode == EXT2_ET_BAD_BLOCK_IN_INODE_TABLE) { > if (!ctx->inode_bb_map) > alloc_bb_map(ctx); > @@ -1276,6 +1276,8 @@ endit: > > if ((ctx->flags & E2F_FLAG_SIGNAL_MASK) == 0) > print_resource_track(ctx, _("Pass 1"), &rtrack, ctx->fs->io); > + else > + ctx->invalid_bitmaps++; > } > > /* > diff --git a/e2fsck/pass2.c b/e2fsck/pass2.c > index 2b7bff4..822eee4 100644 > --- a/e2fsck/pass2.c > +++ b/e2fsck/pass2.c > @@ -148,14 +148,14 @@ void e2fsck_pass2(e2fsck_t ctx) > > cd.pctx.errcode = ext2fs_dblist_iterate2(fs->dblist, check_dir_block, > &cd); > - if (ctx->flags & E2F_FLAG_SIGNAL_MASK || ctx->flags & E2F_FLAG_RESTART) > - return; > - > if (ctx->flags & E2F_FLAG_RESTART_LATER) { > ctx->flags |= E2F_FLAG_RESTART; > - return; > + ctx->flags &= ~E2F_FLAG_RESTART_LATER; > } > > + if (ctx->flags & E2F_FLAG_RUN_RETURN) > + return; > + > if (cd.pctx.errcode) { > fix_problem(ctx, PR_2_DBLIST_ITERATE, &cd.pctx); > ctx->flags |= E2F_FLAG_ABORT; > @@ -739,7 +739,7 @@ static int check_dir_block(ext2_filsys fs, > buf = cd->buf; > ctx = cd->ctx; > > - if (ctx->flags & E2F_FLAG_SIGNAL_MASK || ctx->flags & E2F_FLAG_RESTART) > + if (ctx->flags & E2F_FLAG_RUN_RETURN) > return DIRENT_ABORT; > > if (ctx->progress && (ctx->progress)(ctx, 2, cd->count++, cd->max)) > diff --git a/e2fsck/unix.c b/e2fsck/unix.c > index 66debcd..6d87f50 100644 > --- a/e2fsck/unix.c > +++ b/e2fsck/unix.c > @@ -1667,8 +1667,15 @@ print_unsupp_features: > } > no_journal: > > - if (ctx->qctx) { > + if (run_result & E2F_FLAG_ABORT) { > + fatal_error(ctx, _("aborted")); > + } else if (run_result & E2F_FLAG_CANCEL) { > + log_out(ctx, _("%s: e2fsck canceled.\n"), ctx->device_name ? > + ctx->device_name : ctx->filesystem_name); > + exit_value |= FSCK_CANCELED; > + } else if (ctx->qctx && !ctx->invalid_bitmaps) { > int i, needs_writeout; > + > for (i = 0; i < MAXQUOTAS; i++) { > if (qtype != -1 && qtype != i) > continue; > @@ -1695,18 +1702,13 @@ no_journal: > ext2fs_close_free(&fs); > goto restart; > } > - if (run_result & E2F_FLAG_ABORT) > - fatal_error(ctx, _("aborted")); > > #ifdef MTRACE > mtrace_print("Cleanup"); > #endif > was_changed = ext2fs_test_changed(fs); > - if (run_result & E2F_FLAG_CANCEL) { > - log_out(ctx, _("%s: e2fsck canceled.\n"), ctx->device_name ? > - ctx->device_name : ctx->filesystem_name); > - exit_value |= FSCK_CANCELED; > - } else if (!(ctx->options & E2F_OPT_READONLY)) { > + if (!(ctx->flags & E2F_FLAG_RUN_RETURN) && > + !(ctx->options & E2F_OPT_READONLY)) { > if (ext2fs_test_valid(fs)) { > if (!(sb->s_state & EXT2_VALID_FS)) > exit_value |= FSCK_NONDESTRUCT; > -- > 1.7.3.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Cheers, Andreas
Thanks, applied. - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/e2fsck/e2fsck.c b/e2fsck/e2fsck.c index 0ec1540..2002dc0 100644 --- a/e2fsck/e2fsck.c +++ b/e2fsck/e2fsck.c @@ -203,8 +203,6 @@ static pass_t e2fsck_passes[] = { e2fsck_pass1, e2fsck_pass2, e2fsck_pass3, e2fsck_pass4, e2fsck_pass5, 0 }; -#define E2F_FLAG_RUN_RETURN (E2F_FLAG_SIGNAL_MASK|E2F_FLAG_RESTART) - int e2fsck_run(e2fsck_t ctx) { int i; diff --git a/e2fsck/e2fsck.h b/e2fsck/e2fsck.h index f904026..810030e 100644 --- a/e2fsck/e2fsck.h +++ b/e2fsck/e2fsck.h @@ -173,10 +173,10 @@ struct resource_track { */ #define E2F_FLAG_ABORT 0x0001 /* Abort signaled */ #define E2F_FLAG_CANCEL 0x0002 /* Cancel signaled */ -#define E2F_FLAG_SIGNAL_MASK 0x0003 +#define E2F_FLAG_SIGNAL_MASK (E2F_FLAG_ABORT | E2F_FLAG_CANCEL) #define E2F_FLAG_RESTART 0x0004 /* Restart signaled */ +#define E2F_FLAG_RUN_RETURN (E2F_FLAG_SIGNAL_MASK | E2F_FLAG_RESTART) #define E2F_FLAG_RESTART_LATER 0x0008 /* Restart after all iterations done */ - #define E2F_FLAG_SETJMP_OK 0x0010 /* Setjmp valid for abort */ #define E2F_FLAG_PROG_BAR 0x0020 /* Progress bar on screen */ diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c index 50a8b99..1c2b8fd 100644 --- a/e2fsck/pass1.c +++ b/e2fsck/pass1.c @@ -766,7 +766,7 @@ void e2fsck_pass1(e2fsck_t ctx) inode, inode_size); ehandler_operation(old_op); if (ctx->flags & E2F_FLAG_SIGNAL_MASK) - return; + goto endit; if (pctx.errcode == EXT2_ET_BAD_BLOCK_IN_INODE_TABLE) { if (!ctx->inode_bb_map) alloc_bb_map(ctx); @@ -1276,6 +1276,8 @@ endit: if ((ctx->flags & E2F_FLAG_SIGNAL_MASK) == 0) print_resource_track(ctx, _("Pass 1"), &rtrack, ctx->fs->io); + else + ctx->invalid_bitmaps++; } /* diff --git a/e2fsck/pass2.c b/e2fsck/pass2.c index 2b7bff4..822eee4 100644 --- a/e2fsck/pass2.c +++ b/e2fsck/pass2.c @@ -148,14 +148,14 @@ void e2fsck_pass2(e2fsck_t ctx) cd.pctx.errcode = ext2fs_dblist_iterate2(fs->dblist, check_dir_block, &cd); - if (ctx->flags & E2F_FLAG_SIGNAL_MASK || ctx->flags & E2F_FLAG_RESTART) - return; - if (ctx->flags & E2F_FLAG_RESTART_LATER) { ctx->flags |= E2F_FLAG_RESTART; - return; + ctx->flags &= ~E2F_FLAG_RESTART_LATER; } + if (ctx->flags & E2F_FLAG_RUN_RETURN) + return; + if (cd.pctx.errcode) { fix_problem(ctx, PR_2_DBLIST_ITERATE, &cd.pctx); ctx->flags |= E2F_FLAG_ABORT; @@ -739,7 +739,7 @@ static int check_dir_block(ext2_filsys fs, buf = cd->buf; ctx = cd->ctx; - if (ctx->flags & E2F_FLAG_SIGNAL_MASK || ctx->flags & E2F_FLAG_RESTART) + if (ctx->flags & E2F_FLAG_RUN_RETURN) return DIRENT_ABORT; if (ctx->progress && (ctx->progress)(ctx, 2, cd->count++, cd->max)) diff --git a/e2fsck/unix.c b/e2fsck/unix.c index 66debcd..6d87f50 100644 --- a/e2fsck/unix.c +++ b/e2fsck/unix.c @@ -1667,8 +1667,15 @@ print_unsupp_features: } no_journal: - if (ctx->qctx) { + if (run_result & E2F_FLAG_ABORT) { + fatal_error(ctx, _("aborted")); + } else if (run_result & E2F_FLAG_CANCEL) { + log_out(ctx, _("%s: e2fsck canceled.\n"), ctx->device_name ? + ctx->device_name : ctx->filesystem_name); + exit_value |= FSCK_CANCELED; + } else if (ctx->qctx && !ctx->invalid_bitmaps) { int i, needs_writeout; + for (i = 0; i < MAXQUOTAS; i++) { if (qtype != -1 && qtype != i) continue; @@ -1695,18 +1702,13 @@ no_journal: ext2fs_close_free(&fs); goto restart; } - if (run_result & E2F_FLAG_ABORT) - fatal_error(ctx, _("aborted")); #ifdef MTRACE mtrace_print("Cleanup"); #endif was_changed = ext2fs_test_changed(fs); - if (run_result & E2F_FLAG_CANCEL) { - log_out(ctx, _("%s: e2fsck canceled.\n"), ctx->device_name ? - ctx->device_name : ctx->filesystem_name); - exit_value |= FSCK_CANCELED; - } else if (!(ctx->options & E2F_OPT_READONLY)) { + if (!(ctx->flags & E2F_FLAG_RUN_RETURN) && + !(ctx->options & E2F_OPT_READONLY)) { if (ext2fs_test_valid(fs)) { if (!(sb->s_state & EXT2_VALID_FS)) exit_value |= FSCK_NONDESTRUCT;
There is a bug in how e2fsck handles being interrupted by CTRL-C. If CTRL-C is pressed to kill e2fsck rather than e.g. kill -9, then the interrupt handler sets E2F_FLAG_CANCEL in the context but doesn't actually kill the process. Instead, e2fsck_pass1() checks this flag before processing the next inode. If a filesystem is running in fix mode (e2fsck -fy) is interrupted, and the quota feature is enabled, then the quota file will still be written to disk even though the inode scan was not complete and the quota information is totally inaccurate. Even worse, if the Pass 1 inode and block scan was not finished, then the in-memory block bitmaps (which are used for block allocation during e2fsck) are also invalid, so any blocks allocated to the quota files may corrupt other files if those blocks were actually used. e2fsck 1.42.13.wc3 (28-Aug-2015) Pass 1: Checking inodes, blocks, and sizes ^C[QUOTA WARNING] Usage inconsistent for ID 0: actual (6455296, 168) != expected (8568832, 231) [QUOTA WARNING] Usage inconsistent for ID 695: actual (614932320256, 63981) != expected (2102405386240, 176432) Update quota info for quota type 0? yes [QUOTA WARNING] Usage inconsistent for ID 0: actual (6455296, 168) != expected (8568832, 231) [QUOTA WARNING] Usage inconsistent for ID 538: actual (614932320256, 63981) != expected (2102405386240, 176432) Update quota info for quota type 1? yes myth-OST0001: e2fsck canceled. myth-OST0001: ***** FILE SYSTEM WAS MODIFIED ***** There may be a desire to flush out modified inodes and such that have been repaired, so that restarting an interrupted e2fsck will make progress, but the quota file update is plain wrong unless at least pass1 has finished, and the journal recreation is also dangerous if the block bitmaps have not been fully updated. Signed-off-by: Andreas Dilger <andreas.dilger@intel.com> --- e2fsck/e2fsck.c | 2 -- e2fsck/e2fsck.h | 4 ++-- e2fsck/pass1.c | 4 +++- e2fsck/pass2.c | 10 +++++----- e2fsck/unix.c | 18 ++++++++++-------- 5 files changed, 20 insertions(+), 18 deletions(-)