extX: convert prink(KERN_WARNING) to extX_warning()

PATCH In review FD Cami 4 participants 7 messages Jun 22, 2008 – Jun 24, 2008 ext4 file system

lore

Share & tools

lore

FD Cami

author

2008-06-22 18:18 UTC #0 lore raw

PATCH 1/1 extX: convert prink(KERN_WARNING) to extX_warning()

Hello,

This patch takes a shot at replacing the direct use of printk(KERN_WARNING)
in extX by extX_warning. We now get the device number in the warning message :
"EXT3-fs warning (device hda1): ext3_setup_super: maximal mount count reached, running e2fsck is recommended" It also adds the device information on a pair of printk(KERN_ERR) (this was
first suggested by Kasper two weeks ago).
It also adds the device information on the "mounted filesystem" message :
"EXT3-fs: hda1 is now mounted with ordered data mode."
It also replaces some whitespaces by tabs to increase consistency within
super.c .
One unused variable was removed in ext4/super.c .

I will work on replacing all printks in ext3/ext4 in the coming days if no one / beats / already has beaten / me to it.

Compile tested on ext4, compiled and booted on ext3, however not all string changes have been tested.

Signed-off-by Francois Cami <[email protected]>

fs/ext3/super.c +41 ~~-36~~

diff -uprN -X linux-2.6.26-rc7/Documentation/dontdiff linux-2.6.26-rc7-vanilla/fs/ext3/super.c linux-2.6.26-rc7/fs/ext3/super.c
@@ -305,9 +305,9 @@ void ext3_update_dynamic_rev(struct supe
 		return;
 
 	ext3_warning(sb, __func__,
-		     "updating to rev %d because of new feature flag, "
-		     "running e2fsck is recommended",
-		     EXT3_DYNAMIC_REV);
+			"updating to rev %d because of new feature flag, "
+			"running e2fsck is recommended",
+			EXT3_DYNAMIC_REV);
 
 	es->s_first_ino = cpu_to_le32(EXT3_GOOD_OLD_FIRST_INO);
 	es->s_inode_size = cpu_to_le16(EXT3_GOOD_OLD_INODE_SIZE);

@@ -1192,31 +1192,33 @@ static int ext3_setup_super(struct super
 	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");
+		ext3_warning(sb, __func__,
+				"mounting unchecked fs, running e2fsck is "
+				"recommended");
 	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_warning(sb, __func__,
+				"mounting fs with errors, running e2fsck is "
+				"recommended");
 	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_warning(sb, __func__,
+				"maximal mount count reached, running e2fsck "
+				"is recommended");
 	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_warning(sb, __func__,
+				"checktime reached, running e2fsck is "
+				"recommended");
+
 #if 0
 		/* @@@ We _will_ want to clear the valid bit if we find
                    inconsistencies, to force a fsck at reboot.  But for

@@ -1342,8 +1344,8 @@ static void ext3_orphan_cleanup (struct 
 	}
 
 	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;
 	}

@@ -1601,9 +1603,9 @@ static int ext3_fill_super (struct super
 	    (EXT3_HAS_COMPAT_FEATURE(sb, ~0U) ||
 	     EXT3_HAS_RO_COMPAT_FEATURE(sb, ~0U) ||
 	     EXT3_HAS_INCOMPAT_FEATURE(sb, ~0U)))
-		printk(KERN_WARNING
-		       "EXT3-fs warning: feature flags set on rev 0 fs, "
-		       "running e2fsck is recommended\n");
+		ext3_warning(sb, __func__,
+				"feature flags set on rev 0 fs, running "
+				"e2fsck is recommended");
 	/*
 	 * Check feature flags regardless of the revision level, since we
 	 * previously didn't change the revision level when setting the flags,

@@ -1737,8 +1739,7 @@ static int ext3_fill_super (struct super
 		printk(KERN_ERR "EXT3-fs: filesystem on %s:"
 			" too large to mount safely\n", sb->s_id);
 		if (sizeof(sector_t) < 8)
-			printk(KERN_WARNING "EXT3-fs: CONFIG_LBD not "
-					"enabled\n");
+			ext3_warning(sb, __func__, "CONFIG_LBD not enabled");
 		goto failed_mount;
 	}

@@ -1869,8 +1870,9 @@ static int ext3_fill_super (struct super
 
 	if (test_opt(sb, NOBH)) {
 		if (!(test_opt(sb, DATA_FLAGS) == EXT3_MOUNT_WRITEBACK_DATA)) {
-			printk(KERN_WARNING "EXT3-fs: Ignoring nobh option - "
-				"its supported only with writeback mode\n");
+			ext3_warning(sb, __func__,
+					"Ignoring nobh option - it is only "
+					"supported with writeback mode");
 			clear_opt(sbi->s_mount_opt, NOBH);
 		}
 	}

@@ -1913,7 +1915,8 @@ static int ext3_fill_super (struct super
 	if (needs_recovery)
 		printk (KERN_INFO "EXT3-fs: recovery complete.\n");
 	ext3_mark_recovery_complete(sb, es);
-	printk (KERN_INFO "EXT3-fs: mounted filesystem with %s data mode.\n",
+	printk (KERN_INFO "EXT3-fs: %s is now mounted with %s data mode.\n",
+		sb->s_id,
 		test_opt(sb,DATA_FLAGS) == EXT3_MOUNT_JOURNAL_DATA ? "journal":
 		test_opt(sb,DATA_FLAGS) == EXT3_MOUNT_ORDERED_DATA ? "ordered":
 		"writeback");

@@ -2301,11 +2304,12 @@ static void ext3_clear_journal_err(struc
 		char nbuf[16];
 
 		errstr = ext3_decode_error(sb, j_errno, nbuf);
-		ext3_warning(sb, __func__, "Filesystem error recorded "
-			     "from previous mount: %s", errstr);
-		ext3_warning(sb, __func__, "Marking fs in need of "
-			     "filesystem check.");
-
+		ext3_warning(sb, __func__,
+			"Filesystem error recorded from previous mount: %s",
+			errstr);
+		ext3_warning(sb, __func__,
+			"Marking fs in need of filesystem check.");
+					
 		EXT3_SB(sb)->s_mount_state |= EXT3_ERROR_FS;
 		es->s_state |= cpu_to_le16(EXT3_ERROR_FS);
 		ext3_commit_super (sb, es, 1);

@@ -2773,9 +2777,9 @@ static int ext3_quota_on(struct super_bl
 	}
 	/* Quotafile not in fs root? */
 	if (nd.path.dentry->d_parent->d_inode != sb->s_root->d_inode)
-		printk(KERN_WARNING
-			"EXT3-fs: Quota file not on filesystem root. "
-			"Journalled quota will not work.\n");
+		ext3_warning(sb, __func__,
+ 			"Quota file not on filesystem root. "
+ 			"Journalled quota will not work.");
 	path_put(&nd.path);
 	return vfs_quota_on(sb, type, format_id, path, remount);
 }

@@ -2836,8 +2840,9 @@ static ssize_t ext3_quota_write(struct s
 	handle_t *handle = journal_current_handle();
 
 	if (!handle) {
-		printk(KERN_WARNING "EXT3-fs: Quota write (off=%Lu, len=%Lu)"
-			" cancelled because transaction is not started.\n",
+		ext3_warning(sb, __func__,
+			"Quota write (off=%Lu, len=%Lu) cancelled because "
+			"transaction is not started.",
 			(unsigned long long)off, (unsigned long long)len);
 		return -EIO;
 	}

fs/ext4/super.c +49 ~~-45~~

192 lines not on this page · raw message

Andreas Dilger

2008-06-23 20:31 UTC #1 lore raw

Quoted 5 lines · FD Cami

This patch takes a shot at replacing the direct use of printk(KERN_WARNING)
in extX by extX_warning. We now get the device number in the warning message :
"EXT3-fs warning (device hda1): ext3_setup_super: maximal mount count reached, running e2fsck is recommended" It also adds the device information on a pair of printk(KERN_ERR) (this was
first suggested by Kasper two weeks ago).

The one issue is that KERN_ERR != KERN_WARNING, so these errors might not be visible on the console, or may not be saved to the syslog. The other minor difference is that the function name is also printed, and this makes the error message very long.

One suggestion is to create a separate macro that passes the KERN_* flag and __func__ to ext3_console_msg(), and call that from ext3_warning() and a new ext3_start_error() function. I always found it annoying to have to specify __func__ as a parameter for every call.

fs/ext3/super.c void ext3_update_dynamic_rev(struct supe · line 310

Quoted 7 lines

fs/ext3/super.c +3 -3

 	ext3_warning(sb, __func__,
-		     "updating to rev %d because of new feature flag, "
-		     "running e2fsck is recommended",
-		     EXT3_DYNAMIC_REV);
+			"updating to rev %d because of new feature flag, "
+			"running e2fsck is recommended",
+			EXT3_DYNAMIC_REV);

Please don't change all of the indenting. The old indending is proper linux coding style (aligned with previous '('), the new one is not.

Note that you need to split up the patches for ext2, ext3, ext4 into separate emails. I'd suggest just sending one of them until we agree on what is right, then submitting the rest afterward.

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

Eric Sandeen

2008-06-23 20:34 UTC #2 lore raw

Quoted 12 lines · Andreas Dilger

Quoted 5 lines · FD Cami
This patch takes a shot at replacing the direct use of printk(KERN_WARNING)
in extX by extX_warning. We now get the device number in the warning message :
"EXT3-fs warning (device hda1): ext3_setup_super: maximal mount count reached, running e2fsck is recommended" It also adds the device information on a pair of printk(KERN_ERR) (this was
first suggested by Kasper two weeks ago).
The one issue is that KERN_ERR != KERN_WARNING, so these errors might not be visible on the console, or may not be saved to the syslog. The other minor difference is that the function name is also printed, and this makes the error message very long.

Quoted 4 lines

One suggestion is to create a separate macro that passes the KERN_* flag and __func__ to ext3_console_msg(), and call that from ext3_warning() and a new ext3_start_error() function. I always found it annoying to have to specify __func__ as a parameter for every call.

I agree and meant to suggest that as well.

-Eric

Randy Dunlap

2008-06-23 20:55 UTC #3 lore raw

In reply to Andreas Dilger

Quoted 17 lines · On Mon, 23 Jun 2008 14:31:07 -0600 Andreas Dilger wrote:

Quoted 5 lines · FD Cami
This patch takes a shot at replacing the direct use of printk(KERN_WARNING)
in extX by extX_warning. We now get the device number in the warning message :
"EXT3-fs warning (device hda1): ext3_setup_super: maximal mount count reached, running e2fsck is recommended" It also adds the device information on a pair of printk(KERN_ERR) (this was
first suggested by Kasper two weeks ago).
The one issue is that KERN_ERR != KERN_WARNING, so these errors might not be visible on the console, or may not be saved to the syslog. The other minor difference is that the function name is also printed, and this makes the error message very long.
One suggestion is to create a separate macro that passes the KERN_* flag and __func__ to ext3_console_msg(), and call that from ext3_warning() and a new ext3_start_error() function. I always found it annoying to have to specify __func__ as a parameter for every call.

Quoted 10 lines

fs/ext3/super.c void ext3_update_dynamic_rev(struct supe · line 310
Quoted 7 lines
fs/ext3/super.c +3 -3
 	ext3_warning(sb, __func__,
-		     "updating to rev %d because of new feature flag, "
-		     "running e2fsck is recommended",
-		     EXT3_DYNAMIC_REV);
+			"updating to rev %d because of new feature flag, "
+			"running e2fsck is recommended",
+			EXT3_DYNAMIC_REV);
Please don't change all of the indenting. The old indending is proper linux coding style (aligned with previous '('), the new one is not.

Hi,
I don't mind which way it's done, but I'm curious: are you saying that aligning with '(' is codified (e.g., in CodingStyle) or just that it's dominant?

Quoted 3 lines

Note that you need to split up the patches for ext2, ext3, ext4 into separate emails. I'd suggest just sending one of them until we agree on what is right, then submitting the rest afterward.

Thanks,
---
~Randy
Linux Plumbers Conference, 17-19 September 2008, Portland, Oregon USA
http://linuxplumbersconf.org/

FD Cami

author

2008-06-23 22:20 UTC #5 lore raw

In reply to Andreas Dilger

Hi,

(Eric and Randy added as CC to keep track of this)

Quoted 9 lines · On Mon, 23 Jun 2008 14:31:07 -0600 Andreas Dilger <[email protected]> wrote:

Quoted 5 lines · FD Cami
This patch takes a shot at replacing the direct use of printk(KERN_WARNING)
in extX by extX_warning. We now get the device number in the warning message :
"EXT3-fs warning (device hda1): ext3_setup_super: maximal mount count reached, running e2fsck is recommended" It also adds the device information on a pair of printk(KERN_ERR) (this was
first suggested by Kasper two weeks ago).
The one issue is that KERN_ERR != KERN_WARNING, so these errors might not be visible on the console, or may not be saved to the syslog.

Hmmm, I did not replace any KERN_ERR by ext3_warning / KERN_WARNING, but made those KERN_ERR calls more verbose. Does that change anything ?

For the record, I wanted to replace KERN_ERR calls by calls to a wrapper much like ext3_warning (but obviously calling KERN_ERR), later on.

Quoted 2 lines

The other minor difference is that the function name is also printed, and this makes the error message very long.

Ack, not pretty. Will fix that in next version.

Quoted 4 lines

One suggestion is to create a separate macro that passes the KERN_* flag and __func__ to ext3_console_msg(), and call that from ext3_warning() and a new ext3_start_error() function. I always found it annoying to have to specify __func__ as a parameter for every call.

OK, will do. Thanks for the suggestion.

Quoted 10 lines

fs/ext3/super.c void ext3_update_dynamic_rev(struct supe · line 310
Quoted 7 lines
fs/ext3/super.c +3 -3
 	ext3_warning(sb, __func__,
-		     "updating to rev %d because of new feature flag, "
-		     "running e2fsck is recommended",
-		     EXT3_DYNAMIC_REV);
+			"updating to rev %d because of new feature flag, "
+			"running e2fsck is recommended",
+			EXT3_DYNAMIC_REV);
Please don't change all of the indenting. The old indending is proper linux coding style (aligned with previous '('), the new one is not.

Then this (old) coding style is not consistent within super.c , and not consistent with the example given in Documentation/CodingStyle either. I will "fix" the indentation in a separate patch for the whole file, to be merged or not.

Quoted 3 lines

Note that you need to split up the patches for ext2, ext3, ext4 into separate emails. I'd suggest just sending one of them until we agree on what is right, then submitting the rest afterward.

OK. I did ext3/ext4 at once because of what Andrew replied to Kasper :
"We like to keep ext3 and ext4 in sync as much as poss, please."
http://lkml.org/lkml/2008/6/9/79
But I can do ext3 first and then back / forward port to ext2 and ext4 as well, as you suggest.

Thank you very much for replying.

Best,

Francois

Andreas Dilger

2008-06-24 06:47 UTC #4 lore raw

In reply to Randy Dunlap

Quoted 12 lines · Randy Dunlap

Quoted 10 lines · On Mon, 23 Jun 2008 14:31:07 -0600 Andreas Dilger wrote:
fs/ext3/super.c void ext3_update_dynamic_rev(struct supe · line 310
Quoted 7 lines
fs/ext3/super.c +3 -3
 	ext3_warning(sb, __func__,
-		     "updating to rev %d because of new feature flag, "
-		     "running e2fsck is recommended",
-		     EXT3_DYNAMIC_REV);
+			"updating to rev %d because of new feature flag, "
+			"running e2fsck is recommended",
+			EXT3_DYNAMIC_REV);
Please don't change all of the indenting. The old indending is proper linux coding style (aligned with previous '('), the new one is not.

Quoted 4 lines

Hi,
I don't mind which way it's done, but I'm curious: are you saying that aligning with '(' is codified (e.g., in CodingStyle) or just that it's dominant?

You're right - it isn't in the Linux CodingStyle... It is the style that we use for Lustre that explicitly requires aligning with '('. The common stype definitely IS to align with the previous '(' if a line is a continuation.

That said, it is IMHO bad form to go and change all of the indenting of existing code with little reason to do so.

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

Andreas Dilger

2008-06-24 06:53 UTC #6 lore raw

In reply to FD Cami

Quoted 7 lines · FD Cami

Quoted 2 lines · On Mon, 23 Jun 2008 14:31:07 -0600 Andreas Dilger <[email protected]> wrote:
The one issue is that KERN_ERR != KERN_WARNING, so these errors might not be visible on the console, or may not be saved to the syslog.
Hmmm, I did not replace any KERN_ERR by ext3_warning / KERN_WARNING, but made those KERN_ERR calls more verbose. Does that change anything ?

My apologies, when I started reading the patch I was thinking it replaced all of the printk's in ext3_fill_super(), which it in fact does not.

Quoted 2 lines

For the record, I wanted to replace KERN_ERR calls by calls to a wrapper much like ext3_warning (but obviously calling KERN_ERR), later on.

OK, that's in line with what I was suggesting. Carry on :-).

Quoted 9 lines

Quoted 3 lines
Note that you need to split up the patches for ext2, ext3, ext4 into separate emails. I'd suggest just sending one of them until we agree on what is right, then submitting the rest afterward.
OK. I did ext3/ext4 at once because of what Andrew replied to Kasper :
"We like to keep ext3 and ext4 in sync as much as poss, please."
http://lkml.org/lkml/2008/6/9/79
But I can do ext3 first and then back / forward port to ext2 and ext4 as well, as you suggest.

Yes, it is definitely desirable to have patches for all of them, but always in separate emails. If you feel the patches are ready to go, then go ahead and send all 3 at once. I just thought it might save some work and list traffic if we agree on one of the patches and then produce the other two to match.

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