Re: [PATCH] e2fsck: flush out the superblock and bitmaps before printing final messages

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxx>
> Cc: Dan Jacobson <jidanni@xxxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
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




[Index of Archives]     [Reiser Filesystem Development]     [Ceph FS]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite National Park]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Device Mapper]     [Linux Media]

  Powered by Linux