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

[Vyacheslav Dubeyko]

From: Vyacheslav Dubeyko <Vyacheslav.Dubeyko-***@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-***@public.gmane.org>
CC: Vyacheslav Dubeyko <slava-***@public.gmane.org>
CC: Ryusuke Konishi <konishi.ryusuke-***@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)
{

[Ryusuke Konishi]

Post by Vyacheslav Dubeyko
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.

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

Post by Vyacheslav Dubeyko
---
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
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-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Vyacheslav Dubeyko]

Post by Ryusuke Konishi

Post by Vyacheslav Dubeyko
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.

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-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[dE]

Post by Vyacheslav Dubeyko

Post by Ryusuke Konishi

Post by Vyacheslav Dubeyko
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.

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
[160281.690959] nilfs_btree_propagate: key = 10, level == 0
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
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
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-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Ryusuke Konishi]

Post by Vyacheslav Dubeyko

Post by Ryusuke Konishi

Post by Vyacheslav Dubeyko
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.

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
[160281.690959] nilfs_btree_propagate: key = 10, level == 0
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.

Post by Vyacheslav Dubeyko
How do you feel about enhancement of output for the case of errors? I
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-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html