ext3: bump mount count on journal replay

ext3: bump mount count on journal replay

[Pavel Machek]

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

--- clean.amd/fs/ext3/super.c	2004-06-22 12:38:25.000000000 +0200
+++ linux/fs/ext3/super.c	2004-07-07 22:50:26.000000000 +0200
@@ -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;
 		}
 	}
 

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

Re: ext3: bump mount count on journal replay

[Theodore Ts'o]

On Wed, Jul 14, 2004 at 03:15:25PM +0200, Pavel Machek wrote:
> 
> 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

Re: ext3: bump mount count on journal replay

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 1483 bytes --]

On Jul 14, 2004  15:15 +0200, Pavel Machek wrote:
> 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?
>
> @@ -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.

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/


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: ext3: bump mount count on journal replay

[Pavel Machek]

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.
> > 
> 
> 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.

> 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
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: ext3: bump mount count on journal replay

[Pavel Machek]

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?
> >
> > @@ -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.

> 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

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

Re: [Ext2-devel] Re: ext3: bump mount count on journal replay

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 1034 bytes --]

On Jul 14, 2004  22:32 +0200, Pavel Machek wrote:
> > 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.

> > 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/


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [Ext2-devel] Re: ext3: bump mount count on journal replay

[Pavel Machek]

Hi!

> > > 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:

--- clean/fs/ext3/super.c	2004-06-22 12:36:30.000000000 +0200
+++ linux/fs/ext3/super.c	2004-07-14 22:32:20.000000000 +0200
@@ -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;
 		}
 	}


> > > 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):

--- clean/fs/ext3/super.c	2004-06-22 12:36:30.000000000 +0200
+++ linux/fs/ext3/super.c	2004-07-16 23:05:30.000000000 +0200
@@ -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



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