On Nov 24, 2015, at 4:26 PM, Darrick J. Wong <darrick.wong@xxxxxxxxxx> wrote:
>
> On Tue, Nov 24, 2015 at 03:57:14PM -0700, Andreas Dilger wrote:
>> From: Jim Garlick <garlick@xxxxxxxx>
>>
>> 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@xxxxxxxx>
>> Signed-off-by: Andreas Dilger <andreas.dilger@xxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
Cheers, Andreas
Attachment:
signature.asc
Description: Message signed with OpenPGP using GPGMail
