[PATCH 1/2] ext4: call WARN_ON after the debug message

[PATCH 1/2] ext4: call WARN_ON after the debug message

[Lukas Czerner]

In two places we call WARN_ON() before we print out the debug message,
however the custom is to print such messages before we call WARN_ON() so
change that.

Also use ext4_msg() instead of plain printk().

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 fs/ext4/extents.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 5ae1674..b23e1aa 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -4459,11 +4459,12 @@ retry:
 		ret = ext4_map_blocks(handle, inode, &map, flags);
 		if (ret <= 0) {
 #ifdef EXT4FS_DEBUG
-			WARN_ON(ret <= 0);
-			printk(KERN_ERR "%s: ext4_ext_map_blocks "
-				    "returned error inode#%lu, block=%u, "
-				    "max_blocks=%u", __func__,
-				    inode->i_ino, map.m_lblk, max_blocks);
+			ext4_msg(inode->i_sb, KERN_ERR,
+				 "%s:%d: inode #%lu: block %u: len %u: "
+				 "ext4_ext_map_blocks returned %d",
+				 __func__, __LINE__, inode->i_ino, map.m_lblk,
+				 map.m_len, ret);
+			WARN_ON(1);
 #endif
 			ext4_mark_inode_dirty(handle, inode);
 			ret2 = ext4_journal_stop(handle);
@@ -4537,12 +4538,12 @@ int ext4_convert_unwritten_extents(struct inode *inode, loff_t offset,
 		ret = ext4_map_blocks(handle, inode, &map,
 				      EXT4_GET_BLOCKS_IO_CONVERT_EXT);
 		if (ret <= 0) {
-			WARN_ON(ret <= 0);
 			ext4_msg(inode->i_sb, KERN_ERR,
 				 "%s:%d: inode #%lu: block %u: len %u: "
 				 "ext4_ext_map_blocks returned %d",
 				 __func__, __LINE__, inode->i_ino, map.m_lblk,
 				 map.m_len, ret);
+			WARN_ON(1);
 		}
 		ext4_mark_inode_dirty(handle, inode);
 		ret2 = ext4_journal_stop(handle);
-- 
1.7.7.6

[PATCH 2/2] ext4: remove unused variable flags

[Lukas Czerner]

Remove unused variable flags from dump_completed_IO(). The code is only
exercised when EXT4FS_DEBUG is defined.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 fs/ext4/page-io.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index 0016fbc..3331a76 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -119,7 +119,6 @@ static void dump_completed_IO(struct inode *inode)
 #ifdef	EXT4FS_DEBUG
 	struct list_head *cur, *before, *after;
 	ext4_io_end_t *io, *io0, *io1;
-	unsigned long flags;
 
 	if (list_empty(&EXT4_I(inode)->i_completed_io_list)) {
 		ext4_debug("inode %lu completed_io list is empty\n",
-- 
1.7.7.6

Re: [PATCH 2/2] ext4: remove unused variable flags

[gnehzuil.liu]

在 2013-1-25，上午12:30，Lukas Czerner <lczerner@redhat.com> 写道：

> Remove unused variable flags from dump_completed_IO(). The code is only
> exercised when EXT4FS_DEBUG is defined.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
Reviewed-by: Zheng Liu <wenqing.lz@taobao.com>
Regards,
                                                - Zheng
> ---
> fs/ext4/page-io.c |    1 -
> 1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
> index 0016fbc..3331a76 100644
> --- a/fs/ext4/page-io.c
> +++ b/fs/ext4/page-io.c
> @@ -119,7 +119,6 @@ static void dump_completed_IO(struct inode *inode)
> #ifdef    EXT4FS_DEBUG
>    struct list_head *cur, *before, *after;
>    ext4_io_end_t *io, *io0, *io1;
> -    unsigned long flags;
> 
>    if (list_empty(&EXT4_I(inode)->i_completed_io_list)) {
>        ext4_debug("inode %lu completed_io list is empty\n",
> -- 
> 1.7.7.6
> 
> --
> 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
--
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 1/2] ext4: call WARN_ON after the debug message

[gnehzuil.liu]

在 2013-1-25，上午12:30，Lukas Czerner <lczerner@redhat.com> 写道：

> In two places we call WARN_ON() before we print out the debug message,
> however the custom is to print such messages before we call WARN_ON() so
> change that.
> 
> Also use ext4_msg() instead of plain printk().
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
Reviewed-by: Zheng Liu <wenqing.lz@taobao.com>
Regards,
                                                - Zheng

> ---
> fs/ext4/extents.c |   13 +++++++------
> 1 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 5ae1674..b23e1aa 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -4459,11 +4459,12 @@ retry:
>        ret = ext4_map_blocks(handle, inode, &map, flags);
>        if (ret <= 0) {
> #ifdef EXT4FS_DEBUG
> -            WARN_ON(ret <= 0);
> -            printk(KERN_ERR "%s: ext4_ext_map_blocks "
> -                    "returned error inode#%lu, block=%u, "
> -                    "max_blocks=%u", __func__,
> -                    inode->i_ino, map.m_lblk, max_blocks);
> +            ext4_msg(inode->i_sb, KERN_ERR,
> +                 "%s:%d: inode #%lu: block %u: len %u: "
> +                 "ext4_ext_map_blocks returned %d",
> +                 __func__, __LINE__, inode->i_ino, map.m_lblk,
> +                 map.m_len, ret);
> +            WARN_ON(1);
> #endif
>            ext4_mark_inode_dirty(handle, inode);
>            ret2 = ext4_journal_stop(handle);
> @@ -4537,12 +4538,12 @@ int ext4_convert_unwritten_extents(struct inode *inode, loff_t offset,
>        ret = ext4_map_blocks(handle, inode, &map,
>                      EXT4_GET_BLOCKS_IO_CONVERT_EXT);
>        if (ret <= 0) {
> -            WARN_ON(ret <= 0);
>            ext4_msg(inode->i_sb, KERN_ERR,
>                 "%s:%d: inode #%lu: block %u: len %u: "
>                 "ext4_ext_map_blocks returned %d",
>                 __func__, __LINE__, inode->i_ino, map.m_lblk,
>                 map.m_len, ret);
> +            WARN_ON(1);
>        }
>        ext4_mark_inode_dirty(handle, inode);
>        ret2 = ext4_journal_stop(handle);
> -- 
> 1.7.7.6
> 
> --
> 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
--
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 1/2] ext4: call WARN_ON after the debug message

[Theodore Ts'o]

On Thu, Jan 24, 2013 at 05:30:43PM +0100, Lukas Czerner wrote:
> In two places we call WARN_ON() before we print out the debug message,
> however the custom is to print such messages before we call WARN_ON() so
> change that.
> 
> Also use ext4_msg() instead of plain printk().
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

I was actually thinking about removing the WARN_ON entirely, or at
least suppressing it when the error begin returned from
ext4_map_blocks is EIO.  The reason for that is the warning is causing
noise for automated log scanners, and if the problem is caused by a
hardware failure, there's no real point in dumping out a stack trace.
More generally, is there any reason why we need the stack trace at
all?

Also maybe we should use ext4_warning() instead of ext4_msg()?

     	      	     	 		- Ted

Re: [PATCH 1/2] ext4: call WARN_ON after the debug message

[Lukáš Czerner]

On Thu, 24 Jan 2013, Theodore Ts'o wrote:

> Date: Thu, 24 Jan 2013 14:54:58 -0500
> From: Theodore Ts'o <tytso@mit.edu>
> To: Lukas Czerner <lczerner@redhat.com>
> Cc: linux-ext4@vger.kernel.org
> Subject: Re: [PATCH 1/2] ext4: call WARN_ON after the debug message
> 
> On Thu, Jan 24, 2013 at 05:30:43PM +0100, Lukas Czerner wrote:
> > In two places we call WARN_ON() before we print out the debug message,
> > however the custom is to print such messages before we call WARN_ON() so
> > change that.
> > 
> > Also use ext4_msg() instead of plain printk().
> > 
> > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> 
> I was actually thinking about removing the WARN_ON entirely, or at
> least suppressing it when the error begin returned from
> ext4_map_blocks is EIO.  The reason for that is the warning is causing
> noise for automated log scanners, and if the problem is caused by a
> hardware failure, there's no real point in dumping out a stack trace.
> More generally, is there any reason why we need the stack trace at
> all?
> 
> Also maybe we should use ext4_warning() instead of ext4_msg()?
> 
>      	      	     	 		- Ted

Hi Ted,

we can get the EIO error from ext4_map_blocks not only in the case
of hardware error. The extent tree might not be in consistent state,
or we could even ask for blocks outside the file system itself (I
believe I've seen this before) and I think that in those cases it
might be worth to all WARN_ON.

Also people from ABRT are working on kerneloops.org replacement (so
far it only works for Fedora)
https://retrace.fedoraproject.org/faf/problems/hot/ so we can notice
such problems, yes they catch even kernel WARNs because it usually
means that we have bug somewhere else.

I agree that it would be better to use ext4_warning(), so I'll
resend the patch.

Thanks!
-Lukas

Re: [PATCH 1/2] ext4: call WARN_ON after the debug message

[Theodore Ts'o]

On Fri, Jan 25, 2013 at 10:22:44AM +0100, Lukáš Czerner wrote:
> 
> we can get the EIO error from ext4_map_blocks not only in the case
> of hardware error. The extent tree might not be in consistent state,
> or we could even ask for blocks outside the file system itself (I
> believe I've seen this before) and I think that in those cases it
> might be worth to all WARN_ON.

Sure, but in those cases, the file system is corrupt, and we should
have thrown an ext4_error() in ext4_map_blocks().  The point is that a
WARN_ON is only useful if there is a potential programming bug.  If we
know for sure that it's caused by a file system corruption, then we
don't want to throw a WARN_ON.

Even if there is a kerneloops.org replacement --- in fact, especially
if there is kerneloops.org replacement --- we only want to throw
WARN_ON's in cases where it's just a pedestrian file system
corruption.  Otherwise we'll end up wasting a lot of time chasing down
something which was caused by a hardware error, and needing to calm
down users (and breathless, spectacularizing, irresponsible journalism
from web sites such as Phoronix).

					- Ted
--
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 1/2] ext4: call WARN_ON after the debug message

[Lukáš Czerner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1553 bytes --]

On Fri, 25 Jan 2013, Theodore Ts'o wrote:

> Date: Fri, 25 Jan 2013 10:09:48 -0500
> From: Theodore Ts'o <tytso@mit.edu>
> To: Lukáš Czerner <lczerner@redhat.com>
> Cc: linux-ext4@vger.kernel.org
> Subject: Re: [PATCH 1/2] ext4: call WARN_ON after the debug message
> 
> On Fri, Jan 25, 2013 at 10:22:44AM +0100, Lukáš Czerner wrote:
> > 
> > we can get the EIO error from ext4_map_blocks not only in the case
> > of hardware error. The extent tree might not be in consistent state,
> > or we could even ask for blocks outside the file system itself (I
> > believe I've seen this before) and I think that in those cases it
> > might be worth to all WARN_ON.
> 
> Sure, but in those cases, the file system is corrupt, and we should
> have thrown an ext4_error() in ext4_map_blocks().  The point is that a
> WARN_ON is only useful if there is a potential programming bug.  If we
> know for sure that it's caused by a file system corruption, then we
> don't want to throw a WARN_ON.
> 
> Even if there is a kerneloops.org replacement --- in fact, especially
> if there is kerneloops.org replacement --- we only want to throw
> WARN_ON's in cases where it's just a pedestrian file system
> corruption.  Otherwise we'll end up wasting a lot of time chasing down
> something which was caused by a hardware error, and needing to calm
> down users (and breathless, spectacularizing, irresponsible journalism
> from web sites such as Phoronix).
> 
> 					- Ted

Fair enough, I'll remove the WARN_ON and use ext4_warning() instead
of ext4_msg.

Thanks!
-Lukas

Re: [PATCH 1/2] ext4: call WARN_ON after the debug message

[Dave Chinner]

On Fri, Jan 25, 2013 at 10:09:48AM -0500, Theodore Ts'o wrote:
> On Fri, Jan 25, 2013 at 10:22:44AM +0100, Lukáš Czerner wrote:
> > 
> > we can get the EIO error from ext4_map_blocks not only in the case
> > of hardware error. The extent tree might not be in consistent state,
> > or we could even ask for blocks outside the file system itself (I
> > believe I've seen this before) and I think that in those cases it
> > might be worth to all WARN_ON.
> 
> Sure, but in those cases, the file system is corrupt, and we should
> have thrown an ext4_error() in ext4_map_blocks().  The point is that a
> WARN_ON is only useful if there is a potential programming bug.  If we
> know for sure that it's caused by a file system corruption, then we
> don't want to throw a WARN_ON.

FWIW, XFS handles this problem with an error level sysctl. For
situations like this, the default level doesn't throw stack traces
(just the error message), but there are situations where diagnosis
requires emitting the stack trace.

Hence if it is necessary for bug triage, users can then turn the
sysctl up to 11 and the stack trace will be emitted after the
warning message.  CONFIG_XFS_DEBUG sets the default log level to 11,
because developers always want to know where an error came from ;)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com
--
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 1/2] ext4: call WARN_ON after the debug message

[Lukáš Czerner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1711 bytes --]

On Sat, 26 Jan 2013, Dave Chinner wrote:

> Date: Sat, 26 Jan 2013 12:00:55 +1100
> From: Dave Chinner <david@fromorbit.com>
> To: Theodore Ts'o <tytso@mit.edu>
> Cc: Lukáš Czerner <lczerner@redhat.com>, linux-ext4@vger.kernel.org
> Subject: Re: [PATCH 1/2] ext4: call WARN_ON after the debug message
> 
> On Fri, Jan 25, 2013 at 10:09:48AM -0500, Theodore Ts'o wrote:
> > On Fri, Jan 25, 2013 at 10:22:44AM +0100, Lukáš Czerner wrote:
> > > 
> > > we can get the EIO error from ext4_map_blocks not only in the case
> > > of hardware error. The extent tree might not be in consistent state,
> > > or we could even ask for blocks outside the file system itself (I
> > > believe I've seen this before) and I think that in those cases it
> > > might be worth to all WARN_ON.
> > 
> > Sure, but in those cases, the file system is corrupt, and we should
> > have thrown an ext4_error() in ext4_map_blocks().  The point is that a
> > WARN_ON is only useful if there is a potential programming bug.  If we
> > know for sure that it's caused by a file system corruption, then we
> > don't want to throw a WARN_ON.
> 
> FWIW, XFS handles this problem with an error level sysctl. For
> situations like this, the default level doesn't throw stack traces
> (just the error message), but there are situations where diagnosis
> requires emitting the stack trace.
> 
> Hence if it is necessary for bug triage, users can then turn the
> sysctl up to 11 and the stack trace will be emitted after the
> warning message.  CONFIG_XFS_DEBUG sets the default log level to 11,
> because developers always want to know where an error came from ;)
> 
> Cheers,
> 
> Dave.
> 

That might actually be very useful. Thanks Dave!

-Lukas