Filesystem can be marked clear when it is not

Long time ago I noticed that forced reboot from multiuser (/ mounted rw long time ago) sometimes does not force filesystem check. It happened again today...

So I remounted r/o and fsck-ed. And hey, they are errors: [zero dtimes are ok, but bitmap differences really are not].

I *think* that we do not force ordering of "mark filesystem unclean" and writes to filesystem. And we really *should* force that ordering... Quick and dirty solution would be to sync just after we mark filesystem dirty...

Quoted 4 lines · Pavel

Long time ago I noticed that forced reboot from multiuser (/ mounted rw long time ago) sometimes does not force filesystem check. It happened again today...

Quoted 2 lines

So I remounted r/o and fsck-ed. And hey, they are errors: [zero dtimes are ok, but bitmap differences really are not].

It is funny you should mention this, as I made a patch last night for exactly this problem (Andrew Morton and Ted Ts'o have noticed this as well, and we discussed it previously on ext2-devel), and I only just got around to separating out the patch...

Quoted 4 lines

I *think* that we do not force ordering of "mark filesystem unclean" and writes to filesystem. And we really *should* force that ordering... Quick and dirty solution would be to sync just after we mark filesystem dirty...

Basically this is what the patch does. For the (very rare) cases where we change the VALID or ERROR flags on the filesystem, we write the super block to disk synchronously. This does not impact performance under normal operations, only a millisecond at mount/unmount and on error.

Note that I left ext2_panic() alone. It is not clear whether it should be doing sync I/O in this case, and it is just a cop-out. It is only called from a couple of impossible-to-reach places that should probably be eliminated anyways (i.e. NULL pointers that are always set at fs mount time).

Cheers, Andreas
============================ ext2-commit.diff ========================== diff -ru linux.orig/fs/ext2/super.c linux/fs/ext2/super.c

fs/ext2/super.c +4 -0

@@ -28,39 +28,54 @@
 #include <asm/uaccess.h>
 
 
+static void ext2_sync_super(struct super_block *sb,
+			    struct ext2_super_block *es);
 
 static char error_buf[1024];
 
+/*
+ * Deal with the reporting of failure conditions on a filesystem

such as + * inconsistencies detected in the on-disk structure or read IO failures. + * We set an error flag in the superblock and e2fsck will check this after + * each boot.
+ */
void ext2_error (struct super_block * sb, const char * function,

fs/ext2/super.c +1 -2

 		 const char * fmt, ...)
 {
 	va_list args;
+	struct ext2_super_block *es = EXT2_SB(sb)->s_es;
 
 	if (!(sb->s_flags & MS_RDONLY)) {
 		sb->u.ext2_sb.s_mount_state |= EXT2_ERROR_FS;
-		sb->u.ext2_sb.s_es->s_state =
-			cpu_to_le16(le16_to_cpu(sb->u.ext2_sb.s_es->s_state) |

EXT2_ERROR_FS); - mark_buffer_dirty(sb->u.ext2_sb.s_sbh);

fs/ext2/super.c +3 -2

-		sb->s_dirt = 1;
+		es->s_state =
+			cpu_to_le16(le16_to_cpu(es->s_state) | EXT2_ERROR_FS);
+		ext2_sync_super(sb, es);
 	}
 	va_start (args, fmt);
 	vsprintf (error_buf, fmt, args);
 	va_end (args);
 	if (test_opt (sb, ERRORS_PANIC) ||
-	    (le16_to_cpu(sb->u.ext2_sb.s_es->s_errors) == EXT2_ERRORS_PANIC

&& + (le16_to_cpu(es->s_errors) == EXT2_ERRORS_PANIC &&

fs/ext2/super.c +0 -2

 	     !test_opt (sb, ERRORS_CONT) && !test_opt (sb, ERRORS_RO)))
 		panic ("EXT2-fs panic (device %s): %s: %s\n",
 		       bdevname(sb->s_dev), function, error_buf);
 	printk (KERN_CRIT "EXT2-fs error (device %s): %s: %s\n",
 		bdevname(sb->s_dev), function, error_buf);
 	if (test_opt (sb, ERRORS_RO) ||
-	    (le16_to_cpu(sb->u.ext2_sb.s_es->s_errors) == EXT2_ERRORS_RO &&
-	     !test_opt (sb, ERRORS_CONT) && !test_opt (sb, ERRORS_PANIC)))

{ + (le16_to_cpu(es->s_errors) == EXT2_ERRORS_RO &&

fs/ext2/super.c +2 -0

+	     !test_opt (sb, ERRORS_CONT))) {
 		printk ("Remounting filesystem read-only\n");
 		sb->s_flags |= MS_RDONLY;
 	}
 }
 
+/* Deal with the reporting of failure conditions while running, such

as + * inconsistencies in operation or invalid system states where it is not + * possible to continue.
+ *
+ * Use ext2_error() for cases of invalid filesystem states, as that allows + * the administrator to control the error behaviour to meet their needs. + */
NORET_TYPE void ext2_panic (struct super_block * sb, const char *
function, const char * fmt, ...)
{

fs/ext2/super.c +4 -2

@@ -124,8 +139,10 @@
 	int i;
 
 	if (!(sb->s_flags & MS_RDONLY)) {
-		sb->u.ext2_sb.s_es->s_state =
 le16_to_cpu(sb->u.ext2_sb.s_mount_state);
-		mark_buffer_dirty(sb->u.ext2_sb.s_sbh);
+		struct ext2_super_block *es = EXT2_SB(sb)->s_es;
+
+		es->s_state = le16_to_cpu(EXT2_SB(sb)->s_mount_state);
+		ext2_sync_super(sb, es);
 	}
 	db_count = EXT2_SB(sb)->s_gdb_count;

fs/ext2/super.c +14 -9

 	for (i = 0; i < db_count; i++)

@@ -310,8 +327,7 @@
 		es->s_max_mnt_count = (__s16) cpu_to_le16(EXT2_DFL_MAX_MNT_COUNT);
 	es->s_mnt_count=cpu_to_le16(le16_to_cpu(es->s_mnt_count) + 1);
 	es->s_mtime = cpu_to_le32(CURRENT_TIME);
-	mark_buffer_dirty(sb->u.ext2_sb.s_sbh);
-	sb->s_dirt = 1;
+	ext2_sync_super(sb, es);
 	if (test_opt (sb, DEBUG))
 		printk ("[EXT II FS %s, %s, bs=%lu, fs=%lu, gc=%lu, "
 			"bpg=%lu, ipg=%lu, mo=%04lx]\n",

@@ -663,6 +679,15 @@
 	sb->s_dirt = 0;
 }
 
+static void ext2_sync_super(struct super_block *sb, struct
 ext2_super_block *es) +{
+	es->s_wtime = cpu_to_le32(CURRENT_TIME);
+	mark_buffer_dirty(EXT2_SB(sb)->s_sbh);
+	ll_rw_block(WRITE, 1, &EXT2_SB(sb)->s_sbh);
+	wait_on_buffer(EXT2_SB(sb)->s_sbh);
+	sb->s_dirt = 0;
+}
+
 /*
  * In the second extended file system, it is not necessary to
  * write the super block since we use a mapping of the

@@ -681,13 +706,13 @@
 	if (!(sb->s_flags & MS_RDONLY)) {
 		es = sb->u.ext2_sb.s_es;
 
-		ext2_debug ("setting valid to 0\n");
-
 		if (le16_to_cpu(es->s_state) & EXT2_VALID_FS) {
+			ext2_debug ("setting valid to 0\n");
 			es->s_state = cpu_to_le16(le16_to_cpu(es->s_state) &
 ~EXT2_VALID_FS); es->s_mtime = cpu_to_le32(CURRENT_TIME);
-		}
-		ext2_commit_super (sb, es);
+			ext2_sync_super(sb, es);
+		} else
+			ext2_commit_super (sb, es);
 	}
 	sb->s_dirt = 0;
 }

@@ -724,9 +749,7 @@
 		 */
 		es->s_state = cpu_to_le16(sb->u.ext2_sb.s_mount_state);
 		es->s_mtime = cpu_to_le32(CURRENT_TIME);
-		mark_buffer_dirty(sb->u.ext2_sb.s_sbh);
-		sb->s_dirt = 1;
-		ext2_commit_super (sb, es);
+		ext2_sync_super(sb, es);
 	}
 	else {
 		int ret;

I weighed in on that one too:

    http://marc.theaimsgroup.com/?l=ext2-devel&m=99090670900520&w=2

with a very simple sort-of patch, which I just made into a real patch.