Re: [RFC PATCH] xfs: consolidate local format inode fork verifiers

[Dave Chinner <david@fromorbit.com>]

On Sat, Jul 22, 2017 at 08:29:44AM -0400, Brian Foster wrote:
> On Sat, Jul 22, 2017 at 09:00:58AM +1000, Dave Chinner wrote:
> > 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:
> > > 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.
> > 
> 
> I agree. ;) But I disagree that using a macro somehow automatically
> negatively affects the readability of verifers. Anybody who can make
> sense of this:
> 
>         if (!xfs_sb_version_hascrc(&mp->m_sb))
>                 return __line__;
>         if (dsl->sl_magic != cpu_to_be32(XFS_SYMLINK_MAGIC))
>                 return __line__;
>         if (!uuid_equal(&dsl->sl_uuid, &mp->m_sb.sb_meta_uuid))
>                 return __line__;
> 
> ... could just as easily make sense of something like this:
> 
> 	xfs_verify_assert(xfs_sb_version_hascrc(&mp->m_sb));
> 	xfs_verify_assert(dsl->sl_magic == cpu_to_be32(XFS_SYMLINK_MAGIC));
> 	xfs_verify_assert(uuid_equal(&dsl->sl_uuid, &mp->m_sb.sb_meta_uuid));

Still needs branches to return on error so the verifier code
functions correctly. So it simply isn't as clean as your example
suggests. And, no hiding goto/return/errno variables inside a macro
is not an option - that makes for horrible code and we should not
repeat those past mistakes we inherited from the old Irix
codebase.

And we need to be really careful here - making the code more complex
for pretty errors blows out the size of the code. That means it runs
slower, even though we never execute most of the pretty error print
stuff.  This is an important consideration as verifiers are a
performance critical fast path, both in the kernel and in userspace.

> There is a simple solution to this problem: just include all of the
> readily accessible state information in the error report. If the
> messages above looked something like the following:
> 
>   XFS (...): Metadata corruption detected at xfs_symlink_read_verify [xfs], xfs_symlink block 0x58
>   XFS (...): Read verifier failed: xfs_log_check_lsn(mp, be64_to_cpu(dsl->sl_lsn)), xfs_symlink_remote.c:114
>
> ... we wouldn't have to decode what the error means for every possible
> kernel. The error report is self contained like most of the other
> errors/assert reports in XFS and throughout the kernel.

Just like an assert/warning, we've still got to go match it to the
exact kernel source tree to determine the context in which the
message was emitted and what it actually means.  Making it pretty
doesn't change the amount of work a developer needs to do to
classify or analyse errors in log files, all it does is change the
amount of overhead needed to generate the error message,

Really, verifiers are performance critical fast paths, so the code
that runs inside them needs to be implemented with this in mind. I'm
not against making messages pretty, but let's not do it based on the
on the premise that pretty error messages are more important than
runtime performance when there are no errors...

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com