On Fri, Jul 21, 2017 at 09:54:05AM -0400, Brian Foster wrote:

On Fri, Jul 21, 2017 at 08:49:34AM +1000, Dave Chinner wrote:

quoted

On Wed, Jul 19, 2017 at 10:50:57PM -0700, Darrick J. Wong wrote:

quoted

On Thu, Jul 20, 2017 at 12:26:02PM +1000, Dave Chinner wrote:

quoted

On Wed, Jul 19, 2017 at 09:03:18AM -0700, Darrick J. Wong wrote:

...

quoted

quoted

<rambling off topic now>

While we're on the subject of verifiers, Eric Sandeen has been wishing
that we could make it easier to figure out which buffer verifier test
failed, and it would seem that the XFS_CORRUPTION_ERROR macro is used to
highlight bad inode fork contents.  Perhaps we should create a similar
macro that we could use to log exactly which buffer verifier test
failed?

I don't want to put some shouty macro on every second line of a
verifier. Think differently - we currently return a true/false
from the internal verifier functions to trigger a call to
xfs_verifier_error(). How about they return __line__
on error and 0 on success and then pass that returned value into
xfs_verifier_error() and add that to the error output?

That tells us which check failed without adding more code to every
single verifier check - use the compiler to give us what we need
without any additional code, maintenance or runtime overhead.  All
we need to know is the kernel version so we can translate the line
number to a failed check...

I think the ideal situation is the verifier error prints the check that
failed, similar to an assert failure.

Well, that comes from a macro that feeds the assert failure message
__func__ and __line__.

I'm not aware of any way to do
that without a macro,

I just outlined how to do it above.

but I'm also not against crafting a new, verifier
specific one to accomplish that. Technically, it doesn't have to be
shouty :), but IMO, the diagnostic/usability benefit outweighs the
aesthetic cost.

I disagree - there are so many verifier checks (and we only grow
more as time goes on) so whatever we do needs them to be
easily maintainable and not compromise the readabilty of the verifer
code.

Beyond that, I'm not against dumping a line number but it would seem
kind of unusual to dump a line number without at least a filename. FWIW,

We don't really need the filename because we have the name of the
verifier that failed in the ops structure. Hence we can print a
{verfier name, line number} tuple which is effectively the same as
{filename, line number}.

the generic verifier error reporting function also dumps an instruction
address for where the report is generated:

 XFS (...): Metadata corruption detected at xfs_symlink_read_verify+0xcd/0x100 [xfs], xfs_symlink block 0x58

We obviously want to have information about which verifier failed, but
I'm not sure we need the actual address of the xfs_verifier_error()
caller. It would be nice if we could replace (the address, not
necessarily the function name) that with, or add to it, an address that
refers to the particular check that failed.

Yes, that's exactly what I'm proposing we do.  What we really need to know is

	1. the block that was corrupted (from bp)
	2. the verifier that detected the corruption (from bp)
	3. the IO type (read/write from bp)
	4. the verifier check that failed (returned from verifier)

We already have 1-3, but we don't have 4. We need to replace the
replace __return_address used in the error message with __line__ or
__THIS_IP__ that is returned from the if() branch that failed and
from that we can then easily track the cause of the failure back to
the source.

Returning __line__ or __THIS_IP__ from the verifier doesn't require
new macros, or really any significant code change as most verifiers
arelady return a true/false. All we need to do is plumb it into
xfs_verifier_error().

With that extra info we can output a slightly different message,
say:

XFS (...): Metadata corruption detected during read of block 0xx58 by xfs_symlink verifier, line 132

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com