[PATCH] nilfs2: suggest to use dump_stack() in nilfs_error() and nilfs_warning()

[PATCH] nilfs2: suggest to use dump_stack() in nilfs_error() and nilfs_warning()

[Vyacheslav Dubeyko]

From: Vyacheslav Dubeyko <Vyacheslav.Dubeyko-XckBA8eALwE@public.gmane.org>
Subject: [PATCH] nilfs2: suggest to use dump_stack() in nilfs_error() and nilfs_warning()

This patch suggests to use dump_stack() call in nilfs_error() and
nilfs_warning() for more clear understanding of different type of
issues. As a result, end-users can report more informative
descriptions of issues.

Signed-off-by: Vyacheslav Dubeyko <Vyacheslav.Dubeyko-XckBA8eALwE@public.gmane.org>
CC: Vyacheslav Dubeyko <slava-yeENwD64cLxBDgjK7y7TUQ@public.gmane.org>
CC: Ryusuke Konishi <konishi.ryusuke-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org>
---
 fs/nilfs2/inode.c |    1 -
 fs/nilfs2/super.c |    5 ++++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/nilfs2/inode.c b/fs/nilfs2/inode.c
index 6252b17..745e96d 100644
--- a/fs/nilfs2/inode.c
+++ b/fs/nilfs2/inode.c
@@ -970,7 +970,6 @@ void nilfs_dirty_inode(struct inode *inode, int flags)
 	if (is_bad_inode(inode)) {
 		nilfs_warning(inode->i_sb, __func__,
 			      "tried to mark bad_inode dirty. ignored.\n");
-		dump_stack();
 		return;
 	}
 	if (mdi) {
diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
index 8c532b2..c6cd607 100644
--- a/fs/nilfs2/super.c
+++ b/fs/nilfs2/super.c
@@ -123,6 +123,8 @@ void nilfs_error(struct super_block *sb, const char *function,
 
 	va_end(args);
 
+	dump_stack();
+
 	if (!(sb->s_flags & MS_RDONLY)) {
 		nilfs_set_error(sb);
 
@@ -152,8 +154,9 @@ void nilfs_warning(struct super_block *sb, const char *function,
 	       sb->s_id, function, &vaf);
 
 	va_end(args);
-}
 
+	dump_stack();
+}
 
 struct inode *nilfs_alloc_inode(struct super_block *sb)
 {
-- 
1.7.9.5


--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] nilfs2: suggest to use dump_stack() in nilfs_error() and nilfs_warning()

[Ryusuke Konishi]

On Sun, 29 Jun 2014 18:03:41 +0400, Vyacheslav Dubeyko wrote:
> From: Vyacheslav Dubeyko <Vyacheslav.Dubeyko-XckBA8eALwE@public.gmane.org>
> Subject: [PATCH] nilfs2: suggest to use dump_stack() in nilfs_error() and nilfs_warning()
> 
> This patch suggests to use dump_stack() call in nilfs_error() and
> nilfs_warning() for more clear understanding of different type of
> issues. As a result, end-users can report more informative
> descriptions of issues.
> 
> Signed-off-by: Vyacheslav Dubeyko <Vyacheslav.Dubeyko-XckBA8eALwE@public.gmane.org>
> CC: Vyacheslav Dubeyko <slava-yeENwD64cLxBDgjK7y7TUQ@public.gmane.org>
> CC: Ryusuke Konishi <konishi.ryusuke-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org>

NAK.

This patch makes error/warning routine of nilfs so verbose.

dump_stack() should be used for some critical situations or internal
inconsisent situtations, or for cases in which the identifying call
path of trouble is not easy.

I think it's not used for regular errors or warnings because they are
mostly identifiable.

In addition, "errors=panic" mount option is available for nilfs_error
to force stack dump.

Regards,
Ryusuke Konishi

> ---
>  fs/nilfs2/inode.c |    1 -
>  fs/nilfs2/super.c |    5 ++++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nilfs2/inode.c b/fs/nilfs2/inode.c
> index 6252b17..745e96d 100644
> --- a/fs/nilfs2/inode.c
> +++ b/fs/nilfs2/inode.c
> @@ -970,7 +970,6 @@ void nilfs_dirty_inode(struct inode *inode, int flags)
>  	if (is_bad_inode(inode)) {
>  		nilfs_warning(inode->i_sb, __func__,
>  			      "tried to mark bad_inode dirty. ignored.\n");
> -		dump_stack();
>  		return;
>  	}
>  	if (mdi) {
> diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
> index 8c532b2..c6cd607 100644
> --- a/fs/nilfs2/super.c
> +++ b/fs/nilfs2/super.c
> @@ -123,6 +123,8 @@ void nilfs_error(struct super_block *sb, const char *function,
>  
>  	va_end(args);
>  
> +	dump_stack();
> +
>  	if (!(sb->s_flags & MS_RDONLY)) {
>  		nilfs_set_error(sb);
>  
> @@ -152,8 +154,9 @@ void nilfs_warning(struct super_block *sb, const char *function,
>  	       sb->s_id, function, &vaf);
>  
>  	va_end(args);
> -}
>  
> +	dump_stack();
> +}
>  
>  struct inode *nilfs_alloc_inode(struct super_block *sb)
>  {
> -- 
> 1.7.9.5
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] nilfs2: suggest to use dump_stack() in nilfs_error() and nilfs_warning()

[Vyacheslav Dubeyko]

On Mon, 2014-06-30 at 04:01 +0900, Ryusuke Konishi wrote:
> On Sun, 29 Jun 2014 18:03:41 +0400, Vyacheslav Dubeyko wrote:
> > From: Vyacheslav Dubeyko <Vyacheslav.Dubeyko-XckBA8eALwE@public.gmane.org>
> > Subject: [PATCH] nilfs2: suggest to use dump_stack() in nilfs_error() and nilfs_warning()
> > 
> > This patch suggests to use dump_stack() call in nilfs_error() and
> > nilfs_warning() for more clear understanding of different type of
> > issues. As a result, end-users can report more informative
> > descriptions of issues.
> > 
> > Signed-off-by: Vyacheslav Dubeyko <Vyacheslav.Dubeyko-XckBA8eALwE@public.gmane.org>
> > CC: Vyacheslav Dubeyko <slava-yeENwD64cLxBDgjK7y7TUQ@public.gmane.org>
> > CC: Ryusuke Konishi <konishi.ryusuke-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org>
> 
> NAK.
> 
> This patch makes error/warning routine of nilfs so verbose.
> 
> dump_stack() should be used for some critical situations or internal
> inconsisent situtations, or for cases in which the identifying call
> path of trouble is not easy.
> 
> I think it's not used for regular errors or warnings because they are
> mostly identifiable.
> 
> In addition, "errors=panic" mount option is available for nilfs_error
> to force stack dump.
> 

I worry about end-user side. Yes, if developer investigates the issue
then he has many opportunities for the investigation. But we have very
frequently end-users' reports with not very informative details. And it
is not so easy to identify the reason of an issue very frequently. We've
spent about a year on trying to identify the reason of the issue with
race condition of competition between segments for dirty blocks. And the
reason of such situation was impossibility to reproduce the issue easily
and to understand situation on end-users' side. So, from my point of
view, it makes sense to show dump_stack() for the case of remount in RO
mode or any warnings.

I think that NILFS2 driver contains many places without necessary output
about errors. For example:

[160281.690959] nilfs_btree_propagate: key = 10, level == 0
[160281.690967] NILFS warning (device dm-0): nilfs_clean_segments: 
segment construction failed. (err=-2)

Yes, it is possible to understand that we have some error situation on
segctor side. And somewhere inside of segctor thread took place
situation with -ENOENT error. But it is really hard to understand the
place and environment of such situation. I suppose that if we will have
more detailed error messages output then it will be more easily to
understand an issue's environment and the reason of it.

How do you feel about enhancement of output for the case of errors? I
mean to add messages for such places:

err = some_func();
if (err) {
	/* add message output here with details */
	return err;
}

Thanks,
Vyacheslav Dubeyko.


--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] nilfs2: suggest to use dump_stack() in nilfs_error() and nilfs_warning()

[dE]

On 06/30/14 13:54, Vyacheslav Dubeyko wrote:
> On Mon, 2014-06-30 at 04:01 +0900, Ryusuke Konishi wrote:
>> On Sun, 29 Jun 2014 18:03:41 +0400, Vyacheslav Dubeyko wrote:
>>> From: Vyacheslav Dubeyko <Vyacheslav.Dubeyko-XckBA8eALwE@public.gmane.org>
>>> Subject: [PATCH] nilfs2: suggest to use dump_stack() in nilfs_error() and nilfs_warning()
>>>
>>> This patch suggests to use dump_stack() call in nilfs_error() and
>>> nilfs_warning() for more clear understanding of different type of
>>> issues. As a result, end-users can report more informative
>>> descriptions of issues.
>>>
>>> Signed-off-by: Vyacheslav Dubeyko <Vyacheslav.Dubeyko-XckBA8eALwE@public.gmane.org>
>>> CC: Vyacheslav Dubeyko <slava-yeENwD64cLxBDgjK7y7TUQ@public.gmane.org>
>>> CC: Ryusuke Konishi <konishi.ryusuke-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org>
>> NAK.
>>
>> This patch makes error/warning routine of nilfs so verbose.
>>
>> dump_stack() should be used for some critical situations or internal
>> inconsisent situtations, or for cases in which the identifying call
>> path of trouble is not easy.
>>
>> I think it's not used for regular errors or warnings because they are
>> mostly identifiable.
>>
>> In addition, "errors=panic" mount option is available for nilfs_error
>> to force stack dump.
>>
> I worry about end-user side. Yes, if developer investigates the issue
> then he has many opportunities for the investigation. But we have very
> frequently end-users' reports with not very informative details. And it
> is not so easy to identify the reason of an issue very frequently. We've
> spent about a year on trying to identify the reason of the issue with
> race condition of competition between segments for dirty blocks. And the
> reason of such situation was impossibility to reproduce the issue easily
> and to understand situation on end-users' side. So, from my point of
> view, it makes sense to show dump_stack() for the case of remount in RO
> mode or any warnings.
>
> I think that NILFS2 driver contains many places without necessary output
> about errors. For example:
>
> [160281.690959] nilfs_btree_propagate: key = 10, level == 0
> [160281.690967] NILFS warning (device dm-0): nilfs_clean_segments:
> segment construction failed. (err=-2)
>
> Yes, it is possible to understand that we have some error situation on
> segctor side. And somewhere inside of segctor thread took place
> situation with -ENOENT error. But it is really hard to understand the
> place and environment of such situation. I suppose that if we will have
> more detailed error messages output then it will be more easily to
> understand an issue's environment and the reason of it.
>
> How do you feel about enhancement of output for the case of errors? I
> mean to add messages for such places:
>
> err = some_func();
> if (err) {
> 	/* add message output here with details */
> 	return err;
> }
>
> Thanks,
> Vyacheslav Dubeyko.
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

When will this be merged in the mainline kernel?
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] nilfs2: suggest to use dump_stack() in nilfs_error() and nilfs_warning()

[Ryusuke Konishi]

On Mon, 30 Jun 2014 12:24:43 +0400, Vyacheslav Dubeyko wrote:
> On Mon, 2014-06-30 at 04:01 +0900, Ryusuke Konishi wrote:
>> On Sun, 29 Jun 2014 18:03:41 +0400, Vyacheslav Dubeyko wrote:
>> > From: Vyacheslav Dubeyko <Vyacheslav.Dubeyko-XckBA8eALwE@public.gmane.org>
>> > Subject: [PATCH] nilfs2: suggest to use dump_stack() in nilfs_error() and nilfs_warning()
>> > 
>> > This patch suggests to use dump_stack() call in nilfs_error() and
>> > nilfs_warning() for more clear understanding of different type of
>> > issues. As a result, end-users can report more informative
>> > descriptions of issues.
>> > 
>> > Signed-off-by: Vyacheslav Dubeyko <Vyacheslav.Dubeyko-XckBA8eALwE@public.gmane.org>
>> > CC: Vyacheslav Dubeyko <slava-yeENwD64cLxBDgjK7y7TUQ@public.gmane.org>
>> > CC: Ryusuke Konishi <konishi.ryusuke-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org>
>> 
>> NAK.
>> 
>> This patch makes error/warning routine of nilfs so verbose.
>> 
>> dump_stack() should be used for some critical situations or internal
>> inconsisent situtations, or for cases in which the identifying call
>> path of trouble is not easy.
>> 
>> I think it's not used for regular errors or warnings because they are
>> mostly identifiable.
>> 
>> In addition, "errors=panic" mount option is available for nilfs_error
>> to force stack dump.
>> 
> 
> I worry about end-user side. Yes, if developer investigates the issue
> then he has many opportunities for the investigation. But we have very
> frequently end-users' reports with not very informative details. And it
> is not so easy to identify the reason of an issue very frequently. We've
> spent about a year on trying to identify the reason of the issue with
> race condition of competition between segments for dirty blocks. And the
> reason of such situation was impossibility to reproduce the issue easily
> and to understand situation on end-users' side. So, from my point of
> view, it makes sense to show dump_stack() for the case of remount in RO
> mode or any warnings.
> 
> I think that NILFS2 driver contains many places without necessary output
> about errors. For example:
> 
> [160281.690959] nilfs_btree_propagate: key = 10, level == 0
> [160281.690967] NILFS warning (device dm-0): nilfs_clean_segments: 
> segment construction failed. (err=-2)
> 
> Yes, it is possible to understand that we have some error situation on
> segctor side. And somewhere inside of segctor thread took place
> situation with -ENOENT error. But it is really hard to understand the
> place and environment of such situation. I suppose that if we will have
> more detailed error messages output then it will be more easily to
> understand an issue's environment and the reason of it.

The above warning shows that nilfs_clean_segments() function failed
due to -ENOENT error, but it is unclear how and where the error is
created.

Note that inserting dump_stack() in nilfs_warning() doesn't help to
clear this situation.  It will show how nilfs_clean_segments() was
called, but it is almost clear and can easily come to superfluous
information.

Error messages should be inserted or enhanced effectively and
thoughtfully.  Verbose (and useless) information causes overflow of
kernel ring buffer, and can lead to poor or corrupted log messages.

For the above example, adding error messages in error paths of segment
constructor's functions seems better.

> How do you feel about enhancement of output for the case of errors? I
> mean to add messages for such places:
> 
> err = some_func();
> if (err) {
> 	/* add message output here with details */
> 	return err;
> }
> 

It's on a case by case basis whether the enhancement is meaningful or
not.

Regards,
Ryusuke Konishi
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html