On Nov 13, 2015, at 6:10 PM, Andreas Dilger <andreas.dilger@xxxxxxxxx> 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@xxxxxxxxx> > --- > 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@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html Cheers, Andreas
Attachment:
signature.asc
Description: Message signed with OpenPGP using GPGMail
