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@xxxxxxxx>
>> > 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@xxxxxxxx>
>> > CC: Vyacheslav Dubeyko <slava@xxxxxxxxxxx>
>> > CC: Ryusuke Konishi <konishi.ryusuke@xxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
