[PATCH] Make some ext3 kernel messages useful by showing device

[PATCH] Make some ext3 kernel messages useful by showing device

[Kasper Sandberg]

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

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 <lkml@metanurb.dk>


--- super.c.orig	2008-06-08 20:49:26.153047364 +0200
+++ super.c	2008-06-08 20:45:20.812047463 +0200
@@ -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;
 	}
 


[-- Attachment #2: super.c.diff --]
[-- Type: text/x-patch, Size: 2159 bytes --]

--- super.c.orig	2008-06-08 20:49:26.153047364 +0200
+++ super.c	2008-06-08 20:45:20.812047463 +0200
@@ -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;
 	}
 

Re: [PATCH] Make some ext3 kernel messages useful by showing device

[Andrew Morton]

On Sun, 08 Jun 2008 21:02:41 +0200 Kasper Sandberg <lkml@metanurb.dk> 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.

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

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

> Oh, and i dont really know if this is nessecary, but:
> Signed-off-by: Kasper Sandberg <lkml@metanurb.dk>

It is very much preferred, thanks.

(patch retained for linux-ext4 review)

> 
> --- super.c.orig	2008-06-08 20:49:26.153047364 +0200
> +++ super.c	2008-06-08 20:45:20.812047463 +0200
> @@ -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;
>  	}
>  
> 
> 

Re: [PATCH] Make some ext3 kernel messages useful by showing device

[Kasper Sandberg]

On Mon, 2008-06-09 at 02:59 -0700, Andrew Morton wrote:
> On Sun, 08 Jun 2008 21:02:41 +0200 Kasper Sandberg <lkml@metanurb.dk> 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.
> 
> > 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.

> 
> > 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.
> 
> > Oh, and i dont really know if this is nessecary, but:
> > Signed-off-by: Kasper Sandberg <lkml@metanurb.dk>
> 
> It is very much preferred, thanks.
> 
> (patch retained for linux-ext4 review)
> 
> > 
> > --- super.c.orig	2008-06-08 20:49:26.153047364 +0200
> > +++ super.c	2008-06-08 20:45:20.812047463 +0200
> > @@ -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;
> >  	}
> >  
> > 
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Make some ext3 kernel messages useful by showing device

[Andrew Morton]

On Mon, 09 Jun 2008 15:38:10 +0200
Kasper Sandberg <lkml@metanurb.dk> wrote:

> On Mon, 2008-06-09 at 02:59 -0700, Andrew Morton wrote:
> > On Sun, 08 Jun 2008 21:02:41 +0200 Kasper Sandberg <lkml@metanurb.dk> 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.
> > 
> > > 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.

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.

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

> > 
> > > Oh, and i dont really know if this is nessecary, but:
> > > Signed-off-by: Kasper Sandberg <lkml@metanurb.dk>
> > 
> > It is very much preferred, thanks.
> > 
> > (patch retained for linux-ext4 review)
> > 
> > > 
> > > --- super.c.orig	2008-06-08 20:49:26.153047364 +0200
> > > +++ super.c	2008-06-08 20:45:20.812047463 +0200

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.

Re: [PATCH] Make some ext3 kernel messages useful by showing device

[Eric Sandeen]

Andrew Morton wrote:
> On Sun, 08 Jun 2008 21:02:41 +0200 Kasper Sandberg <lkml@metanurb.dk> 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.
> 
>> 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.
> 
>> 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.
> 
>> Oh, and i dont really know if this is nessecary, but:
>> Signed-off-by: Kasper Sandberg <lkml@metanurb.dk>
> 
> 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

>> --- super.c.orig	2008-06-08 20:49:26.153047364 +0200
>> +++ super.c	2008-06-08 20:45:20.812047463 +0200
>> @@ -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;
>>  	}
>>  
>>
>>
> --
> 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

Re: [PATCH] Make some ext3 kernel messages useful by showing device

[Andrew Morton]

On Mon, 09 Jun 2008 08:43:20 -0500
Eric Sandeen <sandeen@redhat.com> wrote:

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

Good point, thanks.

Re: [PATCH] Make some ext3 kernel messages useful by showing device

[Andreas Dilger]

Eric Sandeen <sandeen@redhat.com> wrote:
> 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.