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.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;
}
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.
> 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 <[email protected]>
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;
> }
>
>
>
On Mon, 2008-06-09 at 02:59 -0700, Andrew Morton wrote:
> 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.
>
> > 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 <[email protected]>
>
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
Andrew Morton wrote:
> 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.
>
>> 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 <[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
>> --- 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 09 Jun 2008 15:38:10 +0200
Kasper Sandberg <[email protected]> wrote:
> On Mon, 2008-06-09 at 02:59 -0700, Andrew Morton wrote:
> > 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.
> >
> > > 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 <[email protected]>
> >
> > 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.
On Mon, 09 Jun 2008 08:43:20 -0500
Eric Sandeen <[email protected]> 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.
Eric Sandeen <[email protected]> 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.