| Message ID | 1448405834-25328-1-git-send-email-adilger@dilger.ca |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue, Nov 24, 2015 at 03:57:14PM -0700, Andreas Dilger wrote: > From: Jim Garlick <garlick@llnl.gov> > > Running tune2fs on a filesystem with an unrecovered journal can > cause the tune2fs settings changes in the superblock to be reverted > when the journal is replayed if it contains an uncommitted copy of > the superblock. Print a warning if this is detected so that the > user isn't surprised if it happens. > > Signed-off-by: Jim Garlick <garlick@llnl.gov> > Signed-off-by: Andreas Dilger <andreas.dilger@intel.com> > --- > misc/tune2fs.c | 12 ++++++++++++ > 1 files changed, 12 insertions(+), 0 deletions(-) > > diff --git a/misc/tune2fs.c b/misc/tune2fs.c > index cd1d17f..fcb963a 100644 > --- a/misc/tune2fs.c > +++ b/misc/tune2fs.c > @@ -2397,6 +2397,7 @@ retry_open: > ext2fs_mark_super_dirty(fs); > printf(_("Setting stripe width to %d\n"), stripe_width); > } > + > if (ext_mount_opts) { > strncpy((char *)(fs->super->s_mount_opts), ext_mount_opts, > sizeof(fs->super->s_mount_opts)); > @@ -2406,6 +2407,17 @@ retry_open: > ext_mount_opts); > free(ext_mount_opts); > } > + > + /* Warn if file system needs recovery and it is opened for writing. */ > + if ((open_flag & EXT2_FLAG_RW) && !(mount_flags & EXT2_MF_MOUNTED) && > + (sb->s_feature_compat & EXT3_FEATURE_COMPAT_HAS_JOURNAL) && > + (sb->s_feature_incompat & EXT3_FEATURE_INCOMPAT_RECOVER)) { ext2fs_has_feature_journal(sb) && ext2fs_has_feature_journal_needs_recovery(sb) > + fprintf(stderr, > + _("Warning: needs_recovery flag is set. You may wish\n" > + "replay the journal then rerun this command, or any\n" "You may wish to replay the journal..." > + "changes may be overwritten by journal recovery.\n")); I wonder if we ought simply to replay the journal in this situation, since debugfs/fuse2fs can do it. ...or at least tell the user how to replay? ("e2fsck -E journal_only"/mount) --D > + } > + > free(device_name); > remove_error_table(&et_ext2_error_table); > > -- > 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 -- 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
On Nov 24, 2015, at 4:26 PM, Darrick J. Wong <darrick.wong@oracle.com> wrote: > > On Tue, Nov 24, 2015 at 03:57:14PM -0700, Andreas Dilger wrote: >> From: Jim Garlick <garlick@llnl.gov> >> >> Running tune2fs on a filesystem with an unrecovered journal can >> cause the tune2fs settings changes in the superblock to be reverted >> when the journal is replayed if it contains an uncommitted copy of >> the superblock. Print a warning if this is detected so that the >> user isn't surprised if it happens. >> >> Signed-off-by: Jim Garlick <garlick@llnl.gov> >> Signed-off-by: Andreas Dilger <andreas.dilger@intel.com> >> --- >> misc/tune2fs.c | 12 ++++++++++++ >> 1 files changed, 12 insertions(+), 0 deletions(-) >> >> diff --git a/misc/tune2fs.c b/misc/tune2fs.c >> index cd1d17f..fcb963a 100644 >> --- a/misc/tune2fs.c >> +++ b/misc/tune2fs.c >> @@ -2397,6 +2397,7 @@ retry_open: >> ext2fs_mark_super_dirty(fs); >> printf(_("Setting stripe width to %d\n"), stripe_width); >> } >> + >> if (ext_mount_opts) { >> strncpy((char *)(fs->super->s_mount_opts), ext_mount_opts, >> sizeof(fs->super->s_mount_opts)); >> @@ -2406,6 +2407,17 @@ retry_open: >> ext_mount_opts); >> free(ext_mount_opts); >> } >> + >> + /* Warn if file system needs recovery and it is opened for writing. */ >> + if ((open_flag & EXT2_FLAG_RW) && !(mount_flags & EXT2_MF_MOUNTED) && >> + (sb->s_feature_compat & EXT3_FEATURE_COMPAT_HAS_JOURNAL) && >> + (sb->s_feature_incompat & EXT3_FEATURE_INCOMPAT_RECOVER)) { > > ext2fs_has_feature_journal(sb) && > ext2fs_has_feature_journal_needs_recovery(sb) Those patches don't exist on maint or master... Maybe Ted will consider to cherry-pick 86f3b6cf98 and dependent patches to those branches? >> + fprintf(stderr, >> + _("Warning: needs_recovery flag is set. You may wish\n" >> + "replay the journal then rerun this command, or any\n" > > "You may wish to replay the journal..." > >> + "changes may be overwritten by journal recovery.\n")); > > I wonder if we ought simply to replay the journal in this situation, since > debugfs/fuse2fs can do it. > > ...or at least tell the user how to replay? ("e2fsck -E journal_only"/mount) I'm not sure if there is some reason they may _not_ want the journal to be replayed in this case? I agree at least with telling them how to do it. I'll send a new patch for this update. We had a bad time with "tune2fs" doing too much stuff to the filesystem unexpectedly a few weekends ago (related to the "quota update" bug). The more automagic added to tune2fs (e.g. huge reorganization of the filesystem when setting a feature), the more likely it is that there will be a bad outcome for some user that isn't expecting tune2fs to make major changes. At a minimum, I think anything in tune2fs that is changing more than a single flag or field in the superblock (e.g. change inode size, recompute checksums, etc. that used to require a separate e2fsck run) should pause/prompt like: This may take several minutes/hours and cannot be interrupted. Are you sure? for interactive users ala proceed_question(). That should be done before the 1.43 release I think, since the number of such actions taken by tune2fs has increased significantly in that release. Cheers, Andreas > --D > >> + } >> + >> free(device_name); >> remove_error_table(&et_ext2_error_table); >> >> -- >> 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
On Nov 24, 2015, at 4:42 PM, Andreas Dilger <adilger@dilger.ca> wrote: > > On Nov 24, 2015, at 4:26 PM, Darrick J. Wong <darrick.wong@oracle.com> wrote: >> >> On Tue, Nov 24, 2015 at 03:57:14PM -0700, Andreas Dilger wrote: >>> From: Jim Garlick <garlick@llnl.gov> >>> >>> Running tune2fs on a filesystem with an unrecovered journal can >>> cause the tune2fs settings changes in the superblock to be reverted >>> when the journal is replayed if it contains an uncommitted copy of >>> the superblock. Print a warning if this is detected so that the >>> user isn't surprised if it happens. >>> >>> Signed-off-by: Jim Garlick <garlick@llnl.gov> >>> Signed-off-by: Andreas Dilger <andreas.dilger@intel.com> >>> --- >>> misc/tune2fs.c | 12 ++++++++++++ >>> 1 files changed, 12 insertions(+), 0 deletions(-) >>> >>> diff --git a/misc/tune2fs.c b/misc/tune2fs.c >>> index cd1d17f..fcb963a 100644 >>> --- a/misc/tune2fs.c >>> +++ b/misc/tune2fs.c >>> @@ -2397,6 +2397,7 @@ retry_open: >>> ext2fs_mark_super_dirty(fs); >>> printf(_("Setting stripe width to %d\n"), stripe_width); >>> } >>> + >>> if (ext_mount_opts) { >>> strncpy((char *)(fs->super->s_mount_opts), ext_mount_opts, >>> sizeof(fs->super->s_mount_opts)); >>> @@ -2406,6 +2407,17 @@ retry_open: >>> ext_mount_opts); >>> free(ext_mount_opts); >>> } >>> + >>> + /* Warn if file system needs recovery and it is opened for writing. */ >>> + if ((open_flag & EXT2_FLAG_RW) && !(mount_flags & EXT2_MF_MOUNTED) && >>> + (sb->s_feature_compat & EXT3_FEATURE_COMPAT_HAS_JOURNAL) && >>> + (sb->s_feature_incompat & EXT3_FEATURE_INCOMPAT_RECOVER)) { >> >> ext2fs_has_feature_journal(sb) && >> ext2fs_has_feature_journal_needs_recovery(sb) > > Those patches don't exist on maint or master... Maybe Ted will consider to > cherry-pick 86f3b6cf98 and dependent patches to those branches? > >>> + fprintf(stderr, >>> + _("Warning: needs_recovery flag is set. You may wish\n" >>> + "replay the journal then rerun this command, or any\n" >> >> "You may wish to replay the journal..." >> >>> + "changes may be overwritten by journal recovery.\n")); >> >> I wonder if we ought simply to replay the journal in this situation, since >> debugfs/fuse2fs can do it. >> >> ...or at least tell the user how to replay? ("e2fsck -E journal_only"/mount) > > I'm not sure if there is some reason they may _not_ want the journal to be > replayed in this case? I agree at least with telling them how to do it. > I'll send a new patch for this update. > > > We had a bad time with "tune2fs" doing too much stuff to the filesystem > unexpectedly a few weekends ago (related to the "quota update" bug). The > more automagic added to tune2fs (e.g. huge reorganization of the filesystem > when setting a feature), the more likely it is that there will be a bad > outcome for some user that isn't expecting tune2fs to make major changes. > > At a minimum, I think anything in tune2fs that is changing more than a single > flag or field in the superblock (e.g. change inode size, recompute checksums, > etc. that used to require a separate e2fsck run) should pause/prompt like: > > This may take several minutes/hours and cannot be interrupted. Are you sure? > > for interactive users ala proceed_question(). > > That should be done before the 1.43 release I think, since the number of such > actions taken by tune2fs has increased significantly in that release. Actually, it looks like tune2fs doesn't have any other checks before "dangerous" changes whether the journal needs to be replayed, which could potentially lead to significant corruption if e.g. the inode size is changed and then the journal is replayed afterward. Some functions call check_fsck_needed(), but that doesn't check if the journal needs to be replayed. Definitely something for 1.43 before these features in tune2fs become generally available. Cheers, Andreas >> --D >> >>> + } >>> + >>> free(device_name); >>> remove_error_table(&et_ext2_error_table); >>> >>> -- >>> 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 > > > > > Cheers, Andreas
diff --git a/misc/tune2fs.c b/misc/tune2fs.c index cd1d17f..fcb963a 100644 --- a/misc/tune2fs.c +++ b/misc/tune2fs.c @@ -2397,6 +2397,7 @@ retry_open: ext2fs_mark_super_dirty(fs); printf(_("Setting stripe width to %d\n"), stripe_width); } + if (ext_mount_opts) { strncpy((char *)(fs->super->s_mount_opts), ext_mount_opts, sizeof(fs->super->s_mount_opts)); @@ -2406,6 +2407,17 @@ retry_open: ext_mount_opts); free(ext_mount_opts); } + + /* Warn if file system needs recovery and it is opened for writing. */ + if ((open_flag & EXT2_FLAG_RW) && !(mount_flags & EXT2_MF_MOUNTED) && + (sb->s_feature_compat & EXT3_FEATURE_COMPAT_HAS_JOURNAL) && + (sb->s_feature_incompat & EXT3_FEATURE_INCOMPAT_RECOVER)) { + fprintf(stderr, + _("Warning: needs_recovery flag is set. You may wish\n" + "replay the journal then rerun this command, or any\n" + "changes may be overwritten by journal recovery.\n")); + } + free(device_name); remove_error_table(&et_ext2_error_table);