Skip to contentSkip to conversation

ext3: bump mount count on journal replay

PATCH Pavel Machek 3 participants 7 messages Jul 14, 2004 – Jul 16, 2004

lore
Share & tools
lore

Pavel Machek

author
#0 lore raw

Hi!

Currently, you get fsck "just to be sure" once every ~30 clean mounts or ~30 hard shutdowns. I believe that hard shutdown is way more likely to cause some disk corruption, so it would make sense to fsck more often when system is hit by hard shutdown.

What about this patch?
Pavel

fs/ext3/super.c +10 -8
@@ -919,7 +919,7 @@
 }
 
 static int ext3_setup_super(struct super_block *sb, struct ext3_super_block *es,
-			    int read_only)
+			    int read_only, int mount_cost)
 {
 	struct ext3_sb_info *sbi = EXT3_SB(sb);
 	int res = 0;

@@ -960,7 +961,7 @@
 	if (!(__s16) le16_to_cpu(es->s_max_mnt_count))
 		es->s_max_mnt_count =
 			(__s16) cpu_to_le16(EXT3_DFL_MAX_MNT_COUNT);
-	es->s_mnt_count=cpu_to_le16(le16_to_cpu(es->s_mnt_count) + 1);
+	es->s_mnt_count=cpu_to_le16(le16_to_cpu(es->s_mnt_count) + mount_cost);
 	es->s_mtime = cpu_to_le32(get_seconds());
 	ext3_update_dynamic_rev(sb);
 	EXT3_SET_INCOMPAT_FEATURE(sb, EXT3_FEATURE_INCOMPAT_RECOVER);

@@ -1214,7 +1215,7 @@
 	int hblock;
 	int db_count;
 	int i;
-	int needs_recovery;
+	int needs_recovery, mount_cost = 1;
 
 	sbi = kmalloc(sizeof(*sbi), GFP_KERNEL);
 	if (!sbi)

@@ -1484,9 +1485,11 @@
 	 * root first: it may be modified in the journal!
 	 */
 	if (!test_opt(sb, NOLOAD) &&
-	    EXT3_HAS_COMPAT_FEATURE(sb, EXT3_FEATURE_COMPAT_HAS_JOURNAL)) {
-		if (ext3_load_journal(sb, es))
-			goto failed_mount2;
+	    EXT3_HAS_COMPAT_FEATURE(sb, EXT3_FEATURE_COMPAT_HAS_JOURNAL)) { {
+		    mount_cost = 5;
+		    if (ext3_load_journal(sb, es))
+			    goto failed_mount2;
+	    }
 	} else if (journal_inum) {
 		if (ext3_create_journal(sb, es, journal_inum))
 			goto failed_mount2;

@@ -1543,7 +1546,7 @@
 		goto failed_mount3;
 	}
 
-	ext3_setup_super (sb, es, sb->s_flags & MS_RDONLY);
+	ext3_setup_super (sb, es, sb->s_flags & MS_RDONLY, mount_cost);
 	/*
 	 * akpm: core read_super() calls in here with the superblock locked.
 	 * That deadlocks, because orphan cleanup needs to lock the superblock

@@ -2069,7 +2072,7 @@
 			 */
 			ext3_clear_journal_err(sb, es);
 			sbi->s_mount_state = le16_to_cpu(es->s_state);
-			if (!ext3_setup_super (sb, es, 0))
+			if (!ext3_setup_super (sb, es, 0, 1))
 				sb->s_flags &= ~MS_RDONLY;
 		}
 	}
Signature · 2 lines
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!
#1 lore raw
Quoted 6 lines · Pavel Machek

Currently, you get fsck "just to be sure" once every ~30 clean mounts or ~30 hard shutdowns. I believe that hard shutdown is way more likely to cause some disk corruption, so it would make sense to fsck more often when system is hit by hard shutdown.

At least in theory an unclean shutdown is not going to cause any problems, unless the hardware is screwy, in which case even a single hard shutdown is going to cause problems. I'm not sure that it really makes sense to arbitrarily state that a hard shutdown is 5 times more likely to cause problems. We could make it be configurable, I suppose, but I'm not sure it's worth it to add all that extra complexity. (Heck, we could also argue using a similar reasoning that software suspends also increases the chances of filesystem corruption "if something bad happens". :-)

- Ted

author
#2 lore raw

Hi!

Quoted 10 lines
Quoted 5 lines

Currently, you get fsck "just to be sure" once every ~30 clean mounts or ~30 hard shutdowns. I believe that hard shutdown is way more likely to cause some disk corruption, so it would make sense to fsck more often when system is hit by hard shutdown.

At least in theory an unclean shutdown is not going to cause any problems, unless the hardware is screwy, in which case even a single hard shutdown is going to cause problems. I'm not sure that it really

I'd say "unclean shutdown is not going to cause any problems *if it was due to powerfail". If unclean shutdown is caused by software problem journaling is not guaranteed to help.

Quoted 6 lines

makes sense to arbitrarily state that a hard shutdown is 5 times more likely to cause problems. We could make it be configurable, I suppose, but I'm not sure it's worth it to add all that extra complexity. (Heck, we could also argue using a similar reasoning that software suspends also increases the chances of filesystem corruption "if something bad happens". :-)

Well, if you suspend, resume and then your machine crashes, you should better run fsck, or it is not going to be pretty. Problem is that during bootup, its hard to tell if machine failed due to powerfail or if software problem caused shutdown.

Of course, "5" is very wrong number in any case. Do you see any non-ugly way to make it configurable?

Pavel

Signature · 2 lines
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!
#3 lore raw
In reply to Pavel Machek
Quoted 9 lines · Pavel Machek

Currently, you get fsck "just to be sure" once every ~30 clean mounts or ~30 hard shutdowns. I believe that hard shutdown is way more likely to cause some disk corruption, so it would make sense to fsck more often when system is hit by hard shutdown.

What about this patch?

fs/ext3/super.c +0 -0
@@ -1484,9 +1485,11 @@
 	 * root first: it may be modified in the journal!

fs/ext3/super.c line 1492

Quoted 10 lines
fs/ext3/super.c +5 -3
 	 */
 	if (!test_opt(sb, NOLOAD) &&
-	    EXT3_HAS_COMPAT_FEATURE(sb, EXT3_FEATURE_COMPAT_HAS_JOURNAL)) {
-		if (ext3_load_journal(sb, es))
-			goto failed_mount2;
+	    EXT3_HAS_COMPAT_FEATURE(sb, EXT3_FEATURE_COMPAT_HAS_JOURNAL)) { {
+		    mount_cost = 5;
+		    if (ext3_load_journal(sb, es))
+			    goto failed_mount2;
+	    }

AFAICS, this just means that if you have an ext3 filesystem (i.e. has_journal) that you will fsck 5x as often, not so great. You should instead check for INCOMPAT_RECOVER instead of HAS_JOURNAL.

Instead, you could change this to only increment the mount count after a clean unmount 20% of the time (randomly). Since most people bitch about the full fsck anyways this is probably the better choice than increasing the frequency of checks and forcing the users to change the check interval to get the old behaviour.

Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://members.shaw.ca/adilger/ http://members.shaw.ca/golinux/

author
#4 lore raw

Hi!

Quoted 7 lines
Quoted 7 lines

Currently, you get fsck "just to be sure" once every ~30 clean mounts or ~30 hard shutdowns. I believe that hard shutdown is way more likely to cause some disk corruption, so it would make sense to fsck more often when system is hit by hard shutdown.

What about this patch?

Quoted 16 lines

fs/ext3/super.c line 1492

Quoted 12 lines
fs/ext3/super.c +5 -3
@@ -1484,9 +1485,11 @@
 	 * root first: it may be modified in the journal!
 	 */
 	if (!test_opt(sb, NOLOAD) &&
-	    EXT3_HAS_COMPAT_FEATURE(sb, EXT3_FEATURE_COMPAT_HAS_JOURNAL)) {
-		if (ext3_load_journal(sb, es))
-			goto failed_mount2;
+	    EXT3_HAS_COMPAT_FEATURE(sb, EXT3_FEATURE_COMPAT_HAS_JOURNAL)) { {
+		    mount_cost = 5;
+		    if (ext3_load_journal(sb, es))
+			    goto failed_mount2;
+	    }

AFAICS, this just means that if you have an ext3 filesystem (i.e. has_journal) that you will fsck 5x as often, not so great. You should instead check for INCOMPAT_RECOVER instead of HAS_JOURNAL.

Oops, you are right. Updated patch is attached.

Quoted 5 lines

Instead, you could change this to only increment the mount count after a clean unmount 20% of the time (randomly). Since most people bitch about the full fsck anyways this is probably the better choice than increasing the frequency of checks and forcing the users to change the check interval to get the old behaviour.

Nice hack.... would that be acceptable?
Pavel

Signature · 2 lines
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!
#5 lore raw
Quoted 5 lines · Pavel Machek
Quoted 3 lines

AFAICS, this just means that if you have an ext3 filesystem (i.e. has_journal) that you will fsck 5x as often, not so great. You should instead check for INCOMPAT_RECOVER instead of HAS_JOURNAL.

Oops, you are right. Updated patch is attached.

No patch was attached.

Quoted 7 lines
Quoted 5 lines

Instead, you could change this to only increment the mount count after a clean unmount 20% of the time (randomly). Since most people bitch about the full fsck anyways this is probably the better choice than increasing the frequency of checks and forcing the users to change the check interval to get the old behaviour.

Nice hack.... would that be acceptable?

It's OK by me. I don't think you'll get complaints from users if it is checked less often (there is still the time-based check).

Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://members.shaw.ca/adilger/ http://members.shaw.ca/golinux/

author
#6 lore raw

Hi!

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

AFAICS, this just means that if you have an ext3 filesystem (i.e. has_journal) that you will fsck 5x as often, not so great. You should instead check for INCOMPAT_RECOVER instead of HAS_JOURNAL.

Oops, you are right. Updated patch is attached.

No patch was attached.

Sorry, here it is:

fs/ext3/super.c +9 -7
@@ -919,7 +919,7 @@
 }
 
 static int ext3_setup_super(struct super_block *sb, struct ext3_super_block *es,
-			    int read_only)
+			    int read_only, int mount_cost)
 {
 	struct ext3_sb_info *sbi = EXT3_SB(sb);
 	int res = 0;

@@ -960,7 +960,7 @@
 	if (!(__s16) le16_to_cpu(es->s_max_mnt_count))
 		es->s_max_mnt_count =
 			(__s16) cpu_to_le16(EXT3_DFL_MAX_MNT_COUNT);
-	es->s_mnt_count=cpu_to_le16(le16_to_cpu(es->s_mnt_count) + 1);
+	es->s_mnt_count=cpu_to_le16(le16_to_cpu(es->s_mnt_count) + mount_cost);
 	es->s_mtime = cpu_to_le32(get_seconds());
 	ext3_update_dynamic_rev(sb);
 	EXT3_SET_INCOMPAT_FEATURE(sb, EXT3_FEATURE_INCOMPAT_RECOVER);

@@ -1214,7 +1214,7 @@
 	int hblock;
 	int db_count;
 	int i;
-	int needs_recovery;
+	int needs_recovery, mount_cost = 1;
 
 	sbi = kmalloc(sizeof(*sbi), GFP_KERNEL);
 	if (!sbi)

@@ -1478,6 +1478,8 @@
 	needs_recovery = (es->s_last_orphan != 0 ||
 			  EXT3_HAS_INCOMPAT_FEATURE(sb,
 				    EXT3_FEATURE_INCOMPAT_RECOVER));
+	if (needs_recovery)
+		    mount_cost = 5;
 
 	/*
 	 * The first inode we look at is the journal inode.  Don't try

@@ -1485,8 +1487,8 @@
 	 */
 	if (!test_opt(sb, NOLOAD) &&
 	    EXT3_HAS_COMPAT_FEATURE(sb, EXT3_FEATURE_COMPAT_HAS_JOURNAL)) {
-		if (ext3_load_journal(sb, es))
-			goto failed_mount2;
+		    if (ext3_load_journal(sb, es))
+			    goto failed_mount2;
 	} else if (journal_inum) {
 		if (ext3_create_journal(sb, es, journal_inum))
 			goto failed_mount2;

@@ -1543,7 +1545,7 @@
 		goto failed_mount3;
 	}
 
-	ext3_setup_super (sb, es, sb->s_flags & MS_RDONLY);
+	ext3_setup_super (sb, es, sb->s_flags & MS_RDONLY, mount_cost);
 	/*
 	 * akpm: core read_super() calls in here with the superblock locked.
 	 * That deadlocks, because orphan cleanup needs to lock the superblock

@@ -2069,7 +2071,7 @@
 			 */
 			ext3_clear_journal_err(sb, es);
 			sbi->s_mount_state = le16_to_cpu(es->s_state);
-			if (!ext3_setup_super (sb, es, 0))
+			if (!ext3_setup_super (sb, es, 0, 1))
 				sb->s_flags &= ~MS_RDONLY;
 		}
 	}
Quoted 10 lines
Quoted 7 lines
Quoted 5 lines

Instead, you could change this to only increment the mount count after a clean unmount 20% of the time (randomly). Since most people bitch about the full fsck anyways this is probably the better choice than increasing the frequency of checks and forcing the users to change the check interval to get the old behaviour.

Nice hack.... would that be acceptable?

It's OK by me. I don't think you'll get complaints from users if it is checked less often (there is still the time-based check).

Hmmm... I guess that using get_random_bytes is pretty easy. Completely untested diff (have to sleep now):

fs/ext3/super.c +13 -7
@@ -919,7 +919,7 @@
 }
 
 static int ext3_setup_super(struct super_block *sb, struct ext3_super_block *es,
-			    int read_only)
+			    int read_only, int mount_cost)
 {
 	struct ext3_sb_info *sbi = EXT3_SB(sb);
 	int res = 0;

@@ -960,7 +960,7 @@
 	if (!(__s16) le16_to_cpu(es->s_max_mnt_count))
 		es->s_max_mnt_count =
 			(__s16) cpu_to_le16(EXT3_DFL_MAX_MNT_COUNT);
-	es->s_mnt_count=cpu_to_le16(le16_to_cpu(es->s_mnt_count) + 1);
+	es->s_mnt_count=cpu_to_le16(le16_to_cpu(es->s_mnt_count) + mount_cost);
 	es->s_mtime = cpu_to_le32(get_seconds());
 	ext3_update_dynamic_rev(sb);
 	EXT3_SET_INCOMPAT_FEATURE(sb, EXT3_FEATURE_INCOMPAT_RECOVER);

@@ -1214,7 +1214,11 @@
 	int hblock;
 	int db_count;
 	int i;
-	int needs_recovery;
+	int needs_recovery, mount_cost;
+	unsigned char random;
+
+	get_random_bytes(&random, 1);
+	mount_cost = (random < 60);
 
 	sbi = kmalloc(sizeof(*sbi), GFP_KERNEL);
 	if (!sbi)

@@ -1478,6 +1482,8 @@
 	needs_recovery = (es->s_last_orphan != 0 ||
 			  EXT3_HAS_INCOMPAT_FEATURE(sb,
 				    EXT3_FEATURE_INCOMPAT_RECOVER));
+	if (needs_recovery)
+		    mount_cost = 1;
 
 	/*
 	 * The first inode we look at is the journal inode.  Don't try

@@ -1485,8 +1491,8 @@
 	 */
 	if (!test_opt(sb, NOLOAD) &&
 	    EXT3_HAS_COMPAT_FEATURE(sb, EXT3_FEATURE_COMPAT_HAS_JOURNAL)) {
-		if (ext3_load_journal(sb, es))
-			goto failed_mount2;
+		    if (ext3_load_journal(sb, es))
+			    goto failed_mount2;
 	} else if (journal_inum) {
 		if (ext3_create_journal(sb, es, journal_inum))
 			goto failed_mount2;

@@ -1543,7 +1549,7 @@
 		goto failed_mount3;
 	}
 
-	ext3_setup_super (sb, es, sb->s_flags & MS_RDONLY);
+	ext3_setup_super (sb, es, sb->s_flags & MS_RDONLY, mount_cost);
 	/*
 	 * akpm: core read_super() calls in here with the superblock locked.
 	 * That deadlocks, because orphan cleanup needs to lock the superblock

@@ -2069,7 +2075,7 @@
 			 */
 			ext3_clear_journal_err(sb, es);
 			sbi->s_mount_state = le16_to_cpu(es->s_state);
-			if (!ext3_setup_super (sb, es, 0))
+			if (!ext3_setup_super (sb, es, 0, 1))
 				sb->s_flags &= ~MS_RDONLY;
 		}
 	}

Pavel

Signature · 2 lines
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!