* [PATCH] tune2fs: warn if the filesystem journal is dirty
@ 2014-09-12 20:39 Andreas Dilger
2014-09-12 21:18 ` Darrick J. Wong
0 siblings, 1 reply; 6+ messages in thread
From: Andreas Dilger @ 2014-09-12 20:39 UTC (permalink / raw)
To: tytso; +Cc: linux-ext4, Jim Garlick, Andreas Dilger
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 b65dab9..e5ec8a4 100644
--- a/misc/tune2fs.c
+++ b/misc/tune2fs.c
@@ -2395,6 +2395,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));
@@ -2404,6 +2405,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);
--
1.7.3.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] tune2fs: warn if the filesystem journal is dirty
2014-09-12 20:39 [PATCH] tune2fs: warn if the filesystem journal is dirty Andreas Dilger
@ 2014-09-12 21:18 ` Darrick J. Wong
0 siblings, 0 replies; 6+ messages in thread
From: Darrick J. Wong @ 2014-09-12 21:18 UTC (permalink / raw)
To: Andreas Dilger; +Cc: tytso, linux-ext4, Jim Garlick
On Fri, Sep 12, 2014 at 02:39:40PM -0600, 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 b65dab9..e5ec8a4 100644
> --- a/misc/tune2fs.c
> +++ b/misc/tune2fs.c
> @@ -2395,6 +2395,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));
> @@ -2404,6 +2405,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"));
At a bare minimum this ought to be "You may wish *to* replay...", but I
suggest:
"You should replay the journal and then rerun this command because it is
possible that your changes will be overwritten by the journal recovery."
Otherwise looks fine to me.
--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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] tune2fs: warn if the filesystem journal is dirty
@ 2015-11-24 22:57 Andreas Dilger
2015-11-24 23:26 ` Darrick J. Wong
0 siblings, 1 reply; 6+ messages in thread
From: Andreas Dilger @ 2015-11-24 22:57 UTC (permalink / raw)
To: tytso; +Cc: linux-ext4, Jim Garlick, Andreas Dilger
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)) {
+ 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);
--
1.7.3.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] tune2fs: warn if the filesystem journal is dirty
2015-11-24 22:57 Andreas Dilger
@ 2015-11-24 23:26 ` Darrick J. Wong
2015-11-24 23:42 ` Andreas Dilger
0 siblings, 1 reply; 6+ messages in thread
From: Darrick J. Wong @ 2015-11-24 23:26 UTC (permalink / raw)
To: Andreas Dilger; +Cc: tytso, linux-ext4, Jim Garlick, Andreas Dilger
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] tune2fs: warn if the filesystem journal is dirty
2015-11-24 23:26 ` Darrick J. Wong
@ 2015-11-24 23:42 ` Andreas Dilger
2015-11-25 0:18 ` Andreas Dilger
0 siblings, 1 reply; 6+ messages in thread
From: Andreas Dilger @ 2015-11-24 23:42 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Theodore Ts'o, linux-ext4, Jim Garlick
[-- Attachment #1: Type: text/plain, Size: 3732 bytes --]
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
[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] tune2fs: warn if the filesystem journal is dirty
2015-11-24 23:42 ` Andreas Dilger
@ 2015-11-25 0:18 ` Andreas Dilger
0 siblings, 0 replies; 6+ messages in thread
From: Andreas Dilger @ 2015-11-25 0:18 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Theodore Ts'o, linux-ext4
[-- Attachment #1: Type: text/plain, Size: 4442 bytes --]
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
[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-11-25 0:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-12 20:39 [PATCH] tune2fs: warn if the filesystem journal is dirty Andreas Dilger
2014-09-12 21:18 ` Darrick J. Wong
-- strict thread matches above, loose matches on Subject: below --
2015-11-24 22:57 Andreas Dilger
2015-11-24 23:26 ` Darrick J. Wong
2015-11-24 23:42 ` Andreas Dilger
2015-11-25 0:18 ` Andreas Dilger
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).