* [PATCH] e2fsck: flush out the superblock and bitmaps before printing final messages
@ 2014-08-10 21:33 Theodore Ts'o
2014-08-11 17:09 ` Darrick J. Wong
0 siblings, 1 reply; 3+ messages in thread
From: Theodore Ts'o @ 2014-08-10 21:33 UTC (permalink / raw)
To: Ext4 Developers List; +Cc: Theodore Ts'o, Dan Jacobson
A user who sees the message
***** REBOOT LINUX *****
or
***** FILE SYSTEM WAS MODIFIED *****
might think that e2fsck was complete even though we haven't finished
writing out the superblock or bitmap blocks, and then either forcibly
reboot or power cycle the box, or yank the USB key out while the
storage device is still being written (before e2fsck exits).
So rearrange the exit path of e2fsck so that we flush out the dirty
superblock/bg descriptors/bitmaps before we print the final message.
Also clean up this code so that the flow of control is easier to
understand, and add error checking to catch any errors (normally
caused by I/O errors writing to the disk) for these final writebacks.
Addresses-Debian-Bugs: #757543, #757544
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Cc: Dan Jacobson <jidanni@jidanni.org>
---
e2fsck/problem.c | 15 ++++++++++++
e2fsck/problem.h | 9 +++++++
e2fsck/unix.c | 73 ++++++++++++++++++++++++++------------------------------
3 files changed, 58 insertions(+), 39 deletions(-)
diff --git a/e2fsck/problem.c b/e2fsck/problem.c
index 57c2e39..be4bd0c 100644
--- a/e2fsck/problem.c
+++ b/e2fsck/problem.c
@@ -1737,6 +1737,21 @@ static struct e2fsck_problem problem_table[] = {
N_("Update quota info for quota type %N"),
PROMPT_NULL, PR_PREEN_OK },
+ /* Error setting block group checksum info */
+ { PR_6_SET_BG_CHECKSUM,
+ N_("Error setting @b @g checksum info: %m\n"),
+ PROMPT_NULL, PR_FATAL },
+
+ /* Error writing file system info */
+ { PR_6_FLUSH_FILESYSTEM,
+ N_("Error writing file system info: %m\n"),
+ PROMPT_NULL, PR_FATAL },
+
+ /* Error flushing writes to storage device */
+ { PR_6_IO_FLUSH,
+ N_("Error flushing writes to strage device: %m\n"),
+ PROMPT_NULL, PR_FATAL },
+
{ 0 }
};
diff --git a/e2fsck/problem.h b/e2fsck/problem.h
index 3426a22..212ed35 100644
--- a/e2fsck/problem.h
+++ b/e2fsck/problem.h
@@ -1059,6 +1059,15 @@ struct problem_context {
/* Update quota information if it is inconsistent */
#define PR_6_UPDATE_QUOTAS 0x060002
+/* Error setting block group checksum info */
+#define PR_6_SET_BG_CHECKSUM 0x060003
+
+/* Error writing file system info */
+#define PR_6_FLUSH_FILESYSTEM 0x060004
+
+/* Error flushing writes to storage device */
+#define PR_6_IO_FLUSH 0x060005
+
/*
* Function declarations
*/
diff --git a/e2fsck/unix.c b/e2fsck/unix.c
index fc05bde..628faeb 100644
--- a/e2fsck/unix.c
+++ b/e2fsck/unix.c
@@ -1177,7 +1177,7 @@ int main (int argc, char *argv[])
e2fsck_t ctx;
blk64_t orig_superblock;
struct problem_context pctx;
- int flags, run_result;
+ int flags, run_result, was_changed;
int journal_size;
int sysval, sys_page_size = 4096;
int old_bitmaps;
@@ -1695,22 +1695,45 @@ 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;
- }
- if (run_result & E2F_FLAG_ABORT)
- fatal_error(ctx, _("aborted"));
- if (check_backup_super_block(ctx)) {
- fs->flags &= ~EXT2_FLAG_MASTER_SB_ONLY;
+ } else if (!(ctx->options & E2F_OPT_READONLY)) {
+ if (ext2fs_test_valid(fs)) {
+ if (!(sb->s_state & EXT2_VALID_FS))
+ exit_value |= FSCK_NONDESTRUCT;
+ sb->s_state = EXT2_VALID_FS;
+ if (check_backup_super_block(ctx))
+ fs->flags &= ~EXT2_FLAG_MASTER_SB_ONLY;
+ } else
+ sb->s_state &= ~EXT2_VALID_FS;
+ if (!(ctx->flags & E2F_FLAG_TIME_INSANE))
+ sb->s_lastcheck = ctx->now;
+ sb->s_mnt_count = 0;
+ memset(((char *) sb) + EXT4_S_ERR_START, 0, EXT4_S_ERR_LEN);
+ pctx.errcode = ext2fs_set_gdt_csum(ctx->fs);
+ if (pctx.errcode)
+ fix_problem(ctx, PR_6_SET_BG_CHECKSUM, &pctx);
ext2fs_mark_super_dirty(fs);
}
-#ifdef MTRACE
- mtrace_print("Cleanup");
-#endif
- if (ext2fs_test_changed(fs)) {
+ e2fsck_write_bitmaps(ctx);
+ pctx.errcode = ext2fs_flush(ctx->fs);
+ if (pctx.errcode)
+ fix_problem(ctx, PR_6_FLUSH_FILESYSTEM, &pctx);
+ pctx.errcode = io_channel_flush(ctx->fs->io);
+ if (pctx.errcode)
+ fix_problem(ctx, PR_6_IO_FLUSH, &pctx);
+
+ if (was_changed) {
exit_value |= FSCK_NONDESTRUCT;
if (!(ctx->options & E2F_OPT_PREEN))
log_out(ctx, _("\n%s: ***** FILE SYSTEM WAS "
@@ -1741,37 +1764,9 @@ no_journal:
(sb->s_state & EXT2_VALID_FS) &&
!(sb->s_state & EXT2_ERROR_FS))
exit_value = 0;
- } else {
+ } else
show_stats(ctx);
- if (!(ctx->options & E2F_OPT_READONLY)) {
- if (ext2fs_test_valid(fs)) {
- if (!(sb->s_state & EXT2_VALID_FS))
- exit_value |= FSCK_NONDESTRUCT;
- sb->s_state = EXT2_VALID_FS;
- } else
- sb->s_state &= ~EXT2_VALID_FS;
- sb->s_mnt_count = 0;
- if (!(ctx->flags & E2F_FLAG_TIME_INSANE))
- sb->s_lastcheck = ctx->now;
- memset(((char *) sb) + EXT4_S_ERR_START, 0,
- EXT4_S_ERR_LEN);
- ext2fs_mark_super_dirty(fs);
- }
- }
- if ((run_result & E2F_FLAG_CANCEL) == 0 &&
- sb->s_feature_ro_compat & EXT4_FEATURE_RO_COMPAT_GDT_CSUM &&
- !(ctx->options & E2F_OPT_READONLY)) {
- retval = ext2fs_set_gdt_csum(ctx->fs);
- if (retval) {
- com_err(ctx->program_name, retval, "%s",
- _("while setting block group checksum info"));
- fatal_error(ctx, 0);
- }
- }
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] e2fsck: flush out the superblock and bitmaps before printing final messages
2014-08-10 21:33 [PATCH] e2fsck: flush out the superblock and bitmaps before printing final messages Theodore Ts'o
@ 2014-08-11 17:09 ` Darrick J. Wong
2014-08-11 17:27 ` Theodore Ts'o
0 siblings, 1 reply; 3+ messages in thread
From: Darrick J. Wong @ 2014-08-11 17:09 UTC (permalink / raw)
To: Theodore Ts'o; +Cc: Ext4 Developers List, Dan Jacobson
On Sun, Aug 10, 2014 at 05:33:44PM -0400, Theodore Ts'o wrote:
> A user who sees the message
>
> ***** REBOOT LINUX *****
>
> or
>
> ***** FILE SYSTEM WAS MODIFIED *****
>
> might think that e2fsck was complete even though we haven't finished
> writing out the superblock or bitmap blocks, and then either forcibly
> reboot or power cycle the box, or yank the USB key out while the
> storage device is still being written (before e2fsck exits).
>
> So rearrange the exit path of e2fsck so that we flush out the dirty
> superblock/bg descriptors/bitmaps before we print the final message.
> Also clean up this code so that the flow of control is easier to
> understand, and add error checking to catch any errors (normally
> caused by I/O errors writing to the disk) for these final writebacks.
Uh... if I run a readonly fsck, I see this:
# e2fsck -fn /tmp/a
e2fsck 1.43-WIP (09-Jul-2014)
Pass 1: Checking inodes, blocks, and sizes
Pass 2: Checking directory structure
Pass 3: Checking directory connectivity
Pass 4: Checking reference counts
Pass 5: Checking group summary information
Error writing block 2 (Attempt to write block to filesystem resulted in short write). Ignore error? no
Error writing block 2 (Attempt to write block to filesystem resulted in short write). Ignore error? no
Error writing file system info: Attempt to write block to filesystem resulted in short write
Looking at the strace output, it looks like a bunch of pwrite/write calls are
returning -EBADF when it tries to write the group descriptors to a O_RDONLY
file descriptor.
The call to ext2fs_flush() needs to be guarded by a "if (fs->flags &
EXT2_FLAG_DIRTY)" because ext2fs_flush() unconditionally writes out the group
descriptors. We don't want to do that unless the FS is actually dirty, and we
certainly don't want to do that for a RO check.
Will send patch shortly.
--D
>
> Addresses-Debian-Bugs: #757543, #757544
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> Cc: Dan Jacobson <jidanni@jidanni.org>
> ---
> e2fsck/problem.c | 15 ++++++++++++
> e2fsck/problem.h | 9 +++++++
> e2fsck/unix.c | 73 ++++++++++++++++++++++++++------------------------------
> 3 files changed, 58 insertions(+), 39 deletions(-)
>
> diff --git a/e2fsck/problem.c b/e2fsck/problem.c
> index 57c2e39..be4bd0c 100644
> --- a/e2fsck/problem.c
> +++ b/e2fsck/problem.c
> @@ -1737,6 +1737,21 @@ static struct e2fsck_problem problem_table[] = {
> N_("Update quota info for quota type %N"),
> PROMPT_NULL, PR_PREEN_OK },
>
> + /* Error setting block group checksum info */
> + { PR_6_SET_BG_CHECKSUM,
> + N_("Error setting @b @g checksum info: %m\n"),
> + PROMPT_NULL, PR_FATAL },
> +
> + /* Error writing file system info */
> + { PR_6_FLUSH_FILESYSTEM,
> + N_("Error writing file system info: %m\n"),
> + PROMPT_NULL, PR_FATAL },
> +
> + /* Error flushing writes to storage device */
> + { PR_6_IO_FLUSH,
> + N_("Error flushing writes to strage device: %m\n"),
> + PROMPT_NULL, PR_FATAL },
> +
> { 0 }
> };
>
> diff --git a/e2fsck/problem.h b/e2fsck/problem.h
> index 3426a22..212ed35 100644
> --- a/e2fsck/problem.h
> +++ b/e2fsck/problem.h
> @@ -1059,6 +1059,15 @@ struct problem_context {
> /* Update quota information if it is inconsistent */
> #define PR_6_UPDATE_QUOTAS 0x060002
>
> +/* Error setting block group checksum info */
> +#define PR_6_SET_BG_CHECKSUM 0x060003
> +
> +/* Error writing file system info */
> +#define PR_6_FLUSH_FILESYSTEM 0x060004
> +
> +/* Error flushing writes to storage device */
> +#define PR_6_IO_FLUSH 0x060005
> +
> /*
> * Function declarations
> */
> diff --git a/e2fsck/unix.c b/e2fsck/unix.c
> index fc05bde..628faeb 100644
> --- a/e2fsck/unix.c
> +++ b/e2fsck/unix.c
> @@ -1177,7 +1177,7 @@ int main (int argc, char *argv[])
> e2fsck_t ctx;
> blk64_t orig_superblock;
> struct problem_context pctx;
> - int flags, run_result;
> + int flags, run_result, was_changed;
> int journal_size;
> int sysval, sys_page_size = 4096;
> int old_bitmaps;
> @@ -1695,22 +1695,45 @@ 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;
> - }
> - if (run_result & E2F_FLAG_ABORT)
> - fatal_error(ctx, _("aborted"));
> - if (check_backup_super_block(ctx)) {
> - fs->flags &= ~EXT2_FLAG_MASTER_SB_ONLY;
> + } else if (!(ctx->options & E2F_OPT_READONLY)) {
> + if (ext2fs_test_valid(fs)) {
> + if (!(sb->s_state & EXT2_VALID_FS))
> + exit_value |= FSCK_NONDESTRUCT;
> + sb->s_state = EXT2_VALID_FS;
> + if (check_backup_super_block(ctx))
> + fs->flags &= ~EXT2_FLAG_MASTER_SB_ONLY;
> + } else
> + sb->s_state &= ~EXT2_VALID_FS;
> + if (!(ctx->flags & E2F_FLAG_TIME_INSANE))
> + sb->s_lastcheck = ctx->now;
> + sb->s_mnt_count = 0;
> + memset(((char *) sb) + EXT4_S_ERR_START, 0, EXT4_S_ERR_LEN);
> + pctx.errcode = ext2fs_set_gdt_csum(ctx->fs);
> + if (pctx.errcode)
> + fix_problem(ctx, PR_6_SET_BG_CHECKSUM, &pctx);
> ext2fs_mark_super_dirty(fs);
> }
>
> -#ifdef MTRACE
> - mtrace_print("Cleanup");
> -#endif
> - if (ext2fs_test_changed(fs)) {
> + e2fsck_write_bitmaps(ctx);
> + pctx.errcode = ext2fs_flush(ctx->fs);
> + if (pctx.errcode)
> + fix_problem(ctx, PR_6_FLUSH_FILESYSTEM, &pctx);
> + pctx.errcode = io_channel_flush(ctx->fs->io);
> + if (pctx.errcode)
> + fix_problem(ctx, PR_6_IO_FLUSH, &pctx);
> +
> + if (was_changed) {
> exit_value |= FSCK_NONDESTRUCT;
> if (!(ctx->options & E2F_OPT_PREEN))
> log_out(ctx, _("\n%s: ***** FILE SYSTEM WAS "
> @@ -1741,37 +1764,9 @@ no_journal:
> (sb->s_state & EXT2_VALID_FS) &&
> !(sb->s_state & EXT2_ERROR_FS))
> exit_value = 0;
> - } else {
> + } else
> show_stats(ctx);
> - if (!(ctx->options & E2F_OPT_READONLY)) {
> - if (ext2fs_test_valid(fs)) {
> - if (!(sb->s_state & EXT2_VALID_FS))
> - exit_value |= FSCK_NONDESTRUCT;
> - sb->s_state = EXT2_VALID_FS;
> - } else
> - sb->s_state &= ~EXT2_VALID_FS;
> - sb->s_mnt_count = 0;
> - if (!(ctx->flags & E2F_FLAG_TIME_INSANE))
> - sb->s_lastcheck = ctx->now;
> - memset(((char *) sb) + EXT4_S_ERR_START, 0,
> - EXT4_S_ERR_LEN);
> - ext2fs_mark_super_dirty(fs);
> - }
> - }
>
> - if ((run_result & E2F_FLAG_CANCEL) == 0 &&
> - sb->s_feature_ro_compat & EXT4_FEATURE_RO_COMPAT_GDT_CSUM &&
> - !(ctx->options & E2F_OPT_READONLY)) {
> - retval = ext2fs_set_gdt_csum(ctx->fs);
> - if (retval) {
> - com_err(ctx->program_name, retval, "%s",
> - _("while setting block group checksum info"));
> - fatal_error(ctx, 0);
> - }
> - }
> -
> - e2fsck_write_bitmaps(ctx);
> - io_channel_flush(ctx->fs->io);
> print_resource_track(ctx, NULL, &ctx->global_rtrack, ctx->fs->io);
>
> ext2fs_close_free(&ctx->fs);
> --
> 2.0.0
>
> --
> 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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] e2fsck: flush out the superblock and bitmaps before printing final messages
2014-08-11 17:09 ` Darrick J. Wong
@ 2014-08-11 17:27 ` Theodore Ts'o
0 siblings, 0 replies; 3+ messages in thread
From: Theodore Ts'o @ 2014-08-11 17:27 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Ext4 Developers List, Dan Jacobson
On Mon, Aug 11, 2014 at 10:09:35AM -0700, Darrick J. Wong wrote:
>
> Looking at the strace output, it looks like a bunch of pwrite/write calls are
> returning -EBADF when it tries to write the group descriptors to a O_RDONLY
> file descriptor.
>
> The call to ext2fs_flush() needs to be guarded by a "if (fs->flags &
> EXT2_FLAG_DIRTY)" because ext2fs_flush() unconditionally writes out the group
> descriptors. We don't want to do that unless the FS is actually dirty, and we
> certainly don't want to do that for a RO check.
Oops, I had forgotten that we didn't have such a check inside
ext2fs_flush().
Thanks for catching this!
- Ted
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-08-11 17:27 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-08-10 21:33 [PATCH] e2fsck: flush out the superblock and bitmaps before printing final messages Theodore Ts'o
2014-08-11 17:09 ` Darrick J. Wong
2014-08-11 17:27 ` Theodore Ts'o
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).