Make some ext3 kernel messages useful by showing device

PATCH In review Kasper Sandberg 4 participants 7 messages Jun 8, 2008 – Jun 9, 2008

lore

Share & tools

lore

Kasper Sandberg

author

2008-06-08 19:03 UTC #0 lore raw

PATCH 1/1 Make some ext3 kernel messages useful by showing device

Hello.

Some of the ext3 warnings in super.c are not really as useful as they can be, for instance the "EXT3-fs warning: maximal mount count reached, running e2fsck is recommended" message, does not tell which device it actually is.

Below is patch(both inlined, and in attached form, since i dont trust my mailer), to a patch which fixes that particular message, and a few more. I could look at the rest if anyones interrested?

Oh, and i dont really know if this is nessecary, but:

Signed-off-by Kasper Sandberg <[email protected]>

super.c renamed from super.c.orig +12 ~~-12~~

@@ -1188,31 +1188,31 @@
 	int res = 0;
 
 	if (le32_to_cpu(es->s_rev_level) > EXT3_MAX_SUPP_REV) {
-		printk (KERN_ERR "EXT3-fs warning: revision level too high, "
-			"forcing read-only mode\n");
+		printk (KERN_ERR "EXT3-fs on %s warning: revision level too high, "
+			"forcing read-only mode\n", sb->s_id);
 		res = MS_RDONLY;
 	}
 	if (read_only)
 		return res;
 	if (!(sbi->s_mount_state & EXT3_VALID_FS))
-		printk (KERN_WARNING "EXT3-fs warning: mounting unchecked fs, "
-			"running e2fsck is recommended\n");
+		printk (KERN_WARNING "EXT3-fs on %s warning: mounting unchecked fs, "
+			"running e2fsck is recommended\n", sb->s_id);
 	else if ((sbi->s_mount_state & EXT3_ERROR_FS))
 		printk (KERN_WARNING
-			"EXT3-fs warning: mounting fs with errors, "
-			"running e2fsck is recommended\n");
+			"EXT3-fs on %s warning: mounting fs with errors, "
+			"running e2fsck is recommended\n", sb->s_id);
 	else if ((__s16) le16_to_cpu(es->s_max_mnt_count) >= 0 &&
 		 le16_to_cpu(es->s_mnt_count) >=
 		 (unsigned short) (__s16) le16_to_cpu(es->s_max_mnt_count))
 		printk (KERN_WARNING
-			"EXT3-fs warning: maximal mount count reached, "
-			"running e2fsck is recommended\n");
+			"EXT3-fs on %s warning: maximal mount count reached, "
+			"running e2fsck is recommended\n", sb->s_id);
 	else if (le32_to_cpu(es->s_checkinterval) &&
 		(le32_to_cpu(es->s_lastcheck) +
 			le32_to_cpu(es->s_checkinterval) <= get_seconds()))
 		printk (KERN_WARNING
-			"EXT3-fs warning: checktime reached, "
-			"running e2fsck is recommended\n");
+			"EXT3-fs on %s warning: checktime reached, "
+			"running e2fsck is recommended\n", sb->s_id);
 #if 0
 		/* @@@ We _will_ want to clear the valid bit if we find
                    inconsistencies, to force a fsck at reboot.  But for

@@ -1339,8 +1339,8 @@
 	}
 
 	if (bdev_read_only(sb->s_bdev)) {
-		printk(KERN_ERR "EXT3-fs: write access "
-			"unavailable, skipping orphan cleanup.\n");
+		printk(KERN_ERR "EXT3-fs on %s: write access "
+			"unavailable, skipping orphan cleanup.\n", sb->s_id);
 		return;
 	}

super.c renamed from super.c.orig +12 ~~-12~~

@@ -1188,31 +1188,31 @@
 	int res = 0;
 
 	if (le32_to_cpu(es->s_rev_level) > EXT3_MAX_SUPP_REV) {
-		printk (KERN_ERR "EXT3-fs warning: revision level too high, "
-			"forcing read-only mode\n");
+		printk (KERN_ERR "EXT3-fs on %s warning: revision level too high, "
+			"forcing read-only mode\n", sb->s_id);
 		res = MS_RDONLY;
 	}
 	if (read_only)
 		return res;
 	if (!(sbi->s_mount_state & EXT3_VALID_FS))
-		printk (KERN_WARNING "EXT3-fs warning: mounting unchecked fs, "
-			"running e2fsck is recommended\n");
+		printk (KERN_WARNING "EXT3-fs on %s warning: mounting unchecked fs, "
+			"running e2fsck is recommended\n", sb->s_id);
 	else if ((sbi->s_mount_state & EXT3_ERROR_FS))
 		printk (KERN_WARNING
-			"EXT3-fs warning: mounting fs with errors, "
-			"running e2fsck is recommended\n");
+			"EXT3-fs on %s warning: mounting fs with errors, "
+			"running e2fsck is recommended\n", sb->s_id);
 	else if ((__s16) le16_to_cpu(es->s_max_mnt_count) >= 0 &&
 		 le16_to_cpu(es->s_mnt_count) >=
 		 (unsigned short) (__s16) le16_to_cpu(es->s_max_mnt_count))
 		printk (KERN_WARNING
-			"EXT3-fs warning: maximal mount count reached, "
-			"running e2fsck is recommended\n");
+			"EXT3-fs on %s warning: maximal mount count reached, "
+			"running e2fsck is recommended\n", sb->s_id);
 	else if (le32_to_cpu(es->s_checkinterval) &&
 		(le32_to_cpu(es->s_lastcheck) +
 			le32_to_cpu(es->s_checkinterval) <= get_seconds()))
 		printk (KERN_WARNING
-			"EXT3-fs warning: checktime reached, "
-			"running e2fsck is recommended\n");
+			"EXT3-fs on %s warning: checktime reached, "
+			"running e2fsck is recommended\n", sb->s_id);
 #if 0
 		/* @@@ We _will_ want to clear the valid bit if we find
                    inconsistencies, to force a fsck at reboot.  But for

@@ -1339,8 +1339,8 @@
 	}
 
 	if (bdev_read_only(sb->s_bdev)) {
-		printk(KERN_ERR "EXT3-fs: write access "
-			"unavailable, skipping orphan cleanup.\n");
+		printk(KERN_ERR "EXT3-fs on %s: write access "
+			"unavailable, skipping orphan cleanup.\n", sb->s_id);
 		return;
 	}

Andrew Morton

2008-06-09 10:00 UTC #1 lore raw

Quoted 6 lines · On Sun, 08 Jun 2008 21:02:41 +0200 Kasper Sandberg <[email protected]> wrote:

Hello.
Some of the ext3 warnings in super.c are not really as useful as they can be, for instance the "EXT3-fs warning: maximal mount count reached, running e2fsck is recommended" message, does not tell which device it actually is.

Seems sensible.

Quoted 2 lines

Below is patch(both inlined, and in attached form, since i dont trust my mailer),

That really tricks me. Because the resulting file applies nicely with patch --dry-run' but doesn't apply with plain old patch'. Inlined is preferred, attached is grumpily accepted, but please avoid duplicating the patch.

Quoted 2 lines

to a patch which fixes that particular message, and a few more. I could look at the rest if anyones interrested?

We like to keep ext3 and ext4 in sync as much as poss, please.

Quoted 2 lines

Oh, and i dont really know if this is nessecary, but:
Signed-off-by Kasper Sandberg <[email protected]>

It is very much preferred, thanks.

(patch retained for linux-ext4 review)

Quoted 58 lines

super.c renamed from super.c.orig +12 ~~-12~~

@@ -1188,31 +1188,31 @@
 	int res = 0;
 
 	if (le32_to_cpu(es->s_rev_level) > EXT3_MAX_SUPP_REV) {
-		printk (KERN_ERR "EXT3-fs warning: revision level too high, "
-			"forcing read-only mode\n");
+		printk (KERN_ERR "EXT3-fs on %s warning: revision level too high, "
+			"forcing read-only mode\n", sb->s_id);
 		res = MS_RDONLY;
 	}
 	if (read_only)
 		return res;
 	if (!(sbi->s_mount_state & EXT3_VALID_FS))
-		printk (KERN_WARNING "EXT3-fs warning: mounting unchecked fs, "
-			"running e2fsck is recommended\n");
+		printk (KERN_WARNING "EXT3-fs on %s warning: mounting unchecked fs, "
+			"running e2fsck is recommended\n", sb->s_id);
 	else if ((sbi->s_mount_state & EXT3_ERROR_FS))
 		printk (KERN_WARNING
-			"EXT3-fs warning: mounting fs with errors, "
-			"running e2fsck is recommended\n");
+			"EXT3-fs on %s warning: mounting fs with errors, "
+			"running e2fsck is recommended\n", sb->s_id);
 	else if ((__s16) le16_to_cpu(es->s_max_mnt_count) >= 0 &&
 		 le16_to_cpu(es->s_mnt_count) >=
 		 (unsigned short) (__s16) le16_to_cpu(es->s_max_mnt_count))
 		printk (KERN_WARNING
-			"EXT3-fs warning: maximal mount count reached, "
-			"running e2fsck is recommended\n");
+			"EXT3-fs on %s warning: maximal mount count reached, "
+			"running e2fsck is recommended\n", sb->s_id);
 	else if (le32_to_cpu(es->s_checkinterval) &&
 		(le32_to_cpu(es->s_lastcheck) +
 			le32_to_cpu(es->s_checkinterval) <= get_seconds()))
 		printk (KERN_WARNING
-			"EXT3-fs warning: checktime reached, "
-			"running e2fsck is recommended\n");
+			"EXT3-fs on %s warning: checktime reached, "
+			"running e2fsck is recommended\n", sb->s_id);
 #if 0
 		/* @@@ We _will_ want to clear the valid bit if we find
                    inconsistencies, to force a fsck at reboot.  But for

@@ -1339,8 +1339,8 @@
 	}
 
 	if (bdev_read_only(sb->s_bdev)) {
-		printk(KERN_ERR "EXT3-fs: write access "
-			"unavailable, skipping orphan cleanup.\n");
+		printk(KERN_ERR "EXT3-fs on %s: write access "
+			"unavailable, skipping orphan cleanup.\n", sb->s_id);
 		return;
 	}

Kasper Sandberg

author

2008-06-09 13:39 UTC #2 lore raw

Quoted 11 lines · Andrew Morton

Quoted 6 lines · On Sun, 08 Jun 2008 21:02:41 +0200 Kasper Sandberg <[email protected]> wrote:
Hello.
Some of the ext3 warnings in super.c are not really as useful as they can be, for instance the "EXT3-fs warning: maximal mount count reached, running e2fsck is recommended" message, does not tell which device it actually is.
Seems sensible.

Quoted 7 lines

Quoted 2 lines
Below is patch(both inlined, and in attached form, since i dont trust my mailer),
That really tricks me. Because the resulting file applies nicely with patch --dry-run' but doesn't apply with plain old patch'. Inlined is preferred, attached is grumpily accepted, but please avoid duplicating the patch.

I do not know what happened, i did with diff -Naur. I attached because of the fact that i _KNOW_ evolution is not to be trusted with inlined, but i also know inlined is easier for review, even if it is not properly accepted as input for patch.

Quoted 5 lines

Quoted 3 lines
to a patch which fixes that particular message, and a few more. I could look at the rest if anyones interrested?
We like to keep ext3 and ext4 in sync as much as poss, please.

Yes, i will do this soon, i have a few things to do first though.

Quoted 71 lines

Quoted 3 lines

Oh, and i dont really know if this is nessecary, but:
Signed-off-by Kasper Sandberg <[email protected]>

It is very much preferred, thanks.

(patch retained for linux-ext4 review)

Quoted 48 lines

super.c renamed from super.c.orig +10 ~~-10~~

@@ -1188,31 +1188,31 @@
 	int res = 0;
 
 	if (le32_to_cpu(es->s_rev_level) > EXT3_MAX_SUPP_REV) {
-		printk (KERN_ERR "EXT3-fs warning: revision level too high, "
-			"forcing read-only mode\n");
+		printk (KERN_ERR "EXT3-fs on %s warning: revision level too high, "
+			"forcing read-only mode\n", sb->s_id);
 		res = MS_RDONLY;
 	}
 	if (read_only)
 		return res;
 	if (!(sbi->s_mount_state & EXT3_VALID_FS))
-		printk (KERN_WARNING "EXT3-fs warning: mounting unchecked fs, "
-			"running e2fsck is recommended\n");
+		printk (KERN_WARNING "EXT3-fs on %s warning: mounting unchecked fs, "
+			"running e2fsck is recommended\n", sb->s_id);
 	else if ((sbi->s_mount_state & EXT3_ERROR_FS))
 		printk (KERN_WARNING
-			"EXT3-fs warning: mounting fs with errors, "
-			"running e2fsck is recommended\n");
+			"EXT3-fs on %s warning: mounting fs with errors, "
+			"running e2fsck is recommended\n", sb->s_id);
 	else if ((__s16) le16_to_cpu(es->s_max_mnt_count) >= 0 &&
 		 le16_to_cpu(es->s_mnt_count) >=
 		 (unsigned short) (__s16) le16_to_cpu(es->s_max_mnt_count))
 		printk (KERN_WARNING
-			"EXT3-fs warning: maximal mount count reached, "
-			"running e2fsck is recommended\n");
+			"EXT3-fs on %s warning: maximal mount count reached, "
+			"running e2fsck is recommended\n", sb->s_id);
 	else if (le32_to_cpu(es->s_checkinterval) &&
 		(le32_to_cpu(es->s_lastcheck) +
 			le32_to_cpu(es->s_checkinterval) <= get_seconds()))
 		printk (KERN_WARNING
-			"EXT3-fs warning: checktime reached, "
-			"running e2fsck is recommended\n");
+			"EXT3-fs on %s warning: checktime reached, "
+			"running e2fsck is recommended\n", sb->s_id);
 #if 0
 		/* @@@ We _will_ want to clear the valid bit if we find
                    inconsistencies, to force a fsck at reboot.  But for

super.c line 1346

Quoted 10 lines

super.c renamed from super.c.orig +2 -2

@@ -1339,8 +1339,8 @@
 	}
 
 	if (bdev_read_only(sb->s_bdev)) {
-		printk(KERN_ERR "EXT3-fs: write access "
-			"unavailable, skipping orphan cleanup.\n");
+		printk(KERN_ERR "EXT3-fs on %s: write access "
+			"unavailable, skipping orphan cleanup.\n", sb->s_id);
 		return;
 	}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Eric Sandeen

2008-06-09 13:43 UTC #4 lore raw

In reply to Andrew Morton

Quoted 22 lines · Andrew Morton

Quoted 6 lines · On Sun, 08 Jun 2008 21:02:41 +0200 Kasper Sandberg <[email protected]> wrote:
Hello.
Some of the ext3 warnings in super.c are not really as useful as they can be, for instance the "EXT3-fs warning: maximal mount count reached, running e2fsck is recommended" message, does not tell which device it actually is.
Seems sensible.
Quoted 2 lines
Below is patch(both inlined, and in attached form, since i dont trust my mailer),
That really tricks me. Because the resulting file applies nicely with patch --dry-run' but doesn't apply with plain old patch'. Inlined is preferred, attached is grumpily accepted, but please avoid duplicating the patch.
Quoted 2 lines
to a patch which fixes that particular message, and a few more. I could look at the rest if anyones interrested?

Quoted 8 lines

We like to keep ext3 and ext4 in sync as much as poss, please.
Quoted 2 lines
Oh, and i dont really know if this is nessecary, but:
Signed-off-by Kasper Sandberg <[email protected]>
It is very much preferred, thanks.
(patch retained for linux-ext4 review)

The patch itself looks fine to me, and is a good idea. I recently saw a confused user on another fs where successful mount messages were followed by failure mount messages with no indication of which fs had the trouble...

There probably are more spots in extX which could use this treatment... should we maybe just be using ext3_warning in more places, which automatically prints the sb, function name, etc?

-Eric

Quoted 61 lines

Quoted 47 lines

super.c renamed from super.c.orig +10 ~~-10~~

@@ -1188,31 +1188,31 @@
 	int res = 0;
 
 	if (le32_to_cpu(es->s_rev_level) > EXT3_MAX_SUPP_REV) {
-		printk (KERN_ERR "EXT3-fs warning: revision level too high, "
-			"forcing read-only mode\n");
+		printk (KERN_ERR "EXT3-fs on %s warning: revision level too high, "
+			"forcing read-only mode\n", sb->s_id);
 		res = MS_RDONLY;
 	}
 	if (read_only)
 		return res;
 	if (!(sbi->s_mount_state & EXT3_VALID_FS))
-		printk (KERN_WARNING "EXT3-fs warning: mounting unchecked fs, "
-			"running e2fsck is recommended\n");
+		printk (KERN_WARNING "EXT3-fs on %s warning: mounting unchecked fs, "
+			"running e2fsck is recommended\n", sb->s_id);
 	else if ((sbi->s_mount_state & EXT3_ERROR_FS))
 		printk (KERN_WARNING
-			"EXT3-fs warning: mounting fs with errors, "
-			"running e2fsck is recommended\n");
+			"EXT3-fs on %s warning: mounting fs with errors, "
+			"running e2fsck is recommended\n", sb->s_id);
 	else if ((__s16) le16_to_cpu(es->s_max_mnt_count) >= 0 &&
 		 le16_to_cpu(es->s_mnt_count) >=
 		 (unsigned short) (__s16) le16_to_cpu(es->s_max_mnt_count))
 		printk (KERN_WARNING
-			"EXT3-fs warning: maximal mount count reached, "
-			"running e2fsck is recommended\n");
+			"EXT3-fs on %s warning: maximal mount count reached, "
+			"running e2fsck is recommended\n", sb->s_id);
 	else if (le32_to_cpu(es->s_checkinterval) &&
 		(le32_to_cpu(es->s_lastcheck) +
 			le32_to_cpu(es->s_checkinterval) <= get_seconds()))
 		printk (KERN_WARNING
-			"EXT3-fs warning: checktime reached, "
-			"running e2fsck is recommended\n");
+			"EXT3-fs on %s warning: checktime reached, "
+			"running e2fsck is recommended\n", sb->s_id);
 #if 0
 		/* @@@ We _will_ want to clear the valid bit if we find
                    inconsistencies, to force a fsck at reboot.  But for

super.c line 1346

Quoted 10 lines

super.c renamed from super.c.orig +2 -2

@@ -1339,8 +1339,8 @@
 	}
 
 	if (bdev_read_only(sb->s_bdev)) {
-		printk(KERN_ERR "EXT3-fs: write access "
-			"unavailable, skipping orphan cleanup.\n");
+		printk(KERN_ERR "EXT3-fs on %s: write access "
+			"unavailable, skipping orphan cleanup.\n", sb->s_id);
 		return;
 	}

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Andrew Morton

2008-06-09 20:48 UTC #3 lore raw

In reply to Kasper Sandberg

Quoted 20 lines · On Mon, 09 Jun 2008 15:38:10 +0200 Kasper Sandberg <[email protected]> wrote:

Quoted 11 lines · Andrew Morton
Quoted 6 lines · On Sun, 08 Jun 2008 21:02:41 +0200 Kasper Sandberg <[email protected]> wrote:
Hello.
Some of the ext3 warnings in super.c are not really as useful as they can be, for instance the "EXT3-fs warning: maximal mount count reached, running e2fsck is recommended" message, does not tell which device it actually is.
Seems sensible.
Quoted 7 lines
Quoted 2 lines
Below is patch(both inlined, and in attached form, since i dont trust my mailer),
That really tricks me. Because the resulting file applies nicely with patch --dry-run' but doesn't apply with plain old patch'. Inlined is preferred, attached is grumpily accepted, but please avoid duplicating the patch.

Quoted 4 lines

I do not know what happened, i did with diff -Naur. I attached because of the fact that i _KNOW_ evolution is not to be trusted with inlined, but i also know inlined is easier for review, even if it is not properly accepted as input for patch.

What happened was that you inlined the patch and also attached it. So when my email client does save-to-file, I get two copies of the same patch in the one local file. This causes me problems.

I suggest that you try to inline the patches. Email yourself a patch, check that it still applies. If this ends up being too hard then sure, use attachments, but please don't also inline the patch.

Quoted 6 lines

Quoted 5 lines
Quoted 3 lines
to a patch which fixes that particular message, and a few more. I could look at the rest if anyones interrested?
We like to keep ext3 and ext4 in sync as much as poss, please.
Yes, i will do this soon, i have a few things to do first though.

No probs, thanks.

Quoted 11 lines

Quoted 11 lines
Quoted 3 lines
Oh, and i dont really know if this is nessecary, but:
Signed-off-by Kasper Sandberg <[email protected]>
It is very much preferred, thanks.
(patch retained for linux-ext4 review)
super.c
Quoted 3 lines
super.c renamed from super.c.orig +0 -0

btw, please prepare patches in `patch -p1' form. The headers here should have been

--- a/fs/ext3/super.c
+++ a/fs/ext3/super.c

So the patch will apply with `patch -p1' when run from the root of the kernel tree.

Andrew Morton

2008-06-09 20:49 UTC #5 lore raw

In reply to Eric Sandeen

Quoted 5 lines · On Mon, 09 Jun 2008 08:43:20 -0500 Eric Sandeen <[email protected]> wrote:

Quoted 4 lines
It is very much preferred, thanks.
(patch retained for linux-ext4 review)

Quoted 8 lines

The patch itself looks fine to me, and is a good idea. I recently saw a confused user on another fs where successful mount messages were followed by failure mount messages with no indication of which fs had the trouble...
There probably are more spots in extX which could use this treatment... should we maybe just be using ext3_warning in more places, which automatically prints the sb, function name, etc?

Good point, thanks.

Andreas Dilger

2008-06-09 22:08 UTC #6 lore raw

Quoted 8 lines · Eric Sandeen

The patch itself looks fine to me, and is a good idea. I recently saw a confused user on another fs where successful mount messages were followed by failure mount messages with no indication of which fs had the trouble...
There probably are more spots in extX which could use this treatment... should we maybe just be using ext3_warning in more places, which automatically prints the sb, function name, etc?

Most of the messages in ext[234]_fill_super() should really be ext3_error(), but because the superblock isn't properly set up calling ext3_error() is a way to oops the kernel.

Probably the cleanest solution is to split ext3_error() into a helper function ext3_error_msg() that can be called from ext3_fill_super(), and the rest of the code that now lives in ext3_handle_error().

Alternately, a flag could be set in the superblock ("EXT3_SETUP_DONE") once everything is initialized, and then ext3_error() calls ext3_handle_error(), and otherwise just prints the message and the mount will fail by itself. This has the benefit that we can use ext3_error() everywhere there is a fatal problem, but the drawback that it may become confusing due to the added "errors=*" semantics that will not behave as they do in other functions.

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.