Re: [PATCH 1/3] xfs: add a if_xfs_meta_bad macro for testing and logging bad metadata

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Mon, Nov 11, 2019 at 08:51:19AM -0500, Brian Foster wrote:
> On Sun, Nov 10, 2019 at 05:17:58PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Add a new macro, if_xfs_meta_bad, which we will use to integrate some
> > corruption reporting when the corruption test expression is true.  This
> > will be used in the next patch to remove the ugly XFS_WANT_CORRUPT*
> > macros.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> 
> Ooh a new bikeshed... :)
> 
> >  fs/xfs/xfs_linux.h |   16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
> > index 2271db4e8d66..d0fb1e612c42 100644
> > --- a/fs/xfs/xfs_linux.h
> > +++ b/fs/xfs/xfs_linux.h
> > @@ -229,6 +229,10 @@ int xfs_rw_bdev(struct block_device *bdev, sector_t sector, unsigned int count,
> >  #define ASSERT(expr)	\
> >  	(likely(expr) ? (void)0 : assfail(NULL, #expr, __FILE__, __LINE__))
> >  
> > +#define xfs_meta_bad(mp, expr)	\
> > +	(unlikely(expr) ? assfail((mp), #expr, __FILE__, __LINE__), \
> > +			  true : false)
> > +
> >  #else	/* !DEBUG */
> >  
> >  #ifdef XFS_WARN
> > @@ -236,13 +240,23 @@ int xfs_rw_bdev(struct block_device *bdev, sector_t sector, unsigned int count,
> >  #define ASSERT(expr)	\
> >  	(likely(expr) ? (void)0 : asswarn(NULL, #expr, __FILE__, __LINE__))
> >  
> > +#define xfs_meta_bad(mp, expr)	\
> > +	(unlikely(expr) ? asswarn((mp), #expr, __FILE__, __LINE__), \
> > +			  true : false)
> > +
> >  #else	/* !DEBUG && !XFS_WARN */
> >  
> > -#define ASSERT(expr)	((void)0)
> > +#define ASSERT(expr)		((void)0)
> > +
> > +#define xfs_meta_bad(mp, expr)	\
> > +	(unlikely(expr) ? XFS_ERROR_REPORT(#expr, XFS_ERRLEVEL_LOW, (mp)), \
> > +			  true : false)
> >  
> >  #endif /* XFS_WARN */
> >  #endif /* DEBUG */
> >  
> > +#define if_xfs_meta_bad(mp, expr)	if (xfs_meta_bad((mp), (expr)))
> > +
> d
> FWIW, 'xfs_meta_bad' doesn't really tell me anything about what the
> macro is for, particularly since the logic that determines whether
> metadata is bad is fed into it. IOW, I read that and expect the macro to
> actually do something generic to determine whether metadata is bad.
> 
> Also having taken a quick look at the next patch, I agree with Christoph
> on embedding if logic into the macro itself, at least with respect to
> readability. It makes the code look like a typo/syntax error to me. :P

It's not just you. ;)

> I agree that the existing macros are ugly, but they at least express
> operational semantics reasonably well between [_RETURN|_GOTO]. If we
> really want to fix the latter bit, perhaps the best incremental step is
> to drop the branching logic and naming portion from the existing macros
> and leave everything else as is (from the commit logs, it sounds like
> this is more along the lines of your previous version, just without the
> name change). From there perhaps we can come up with better naming
> eventually. Just a thought.

<nod> I couldn't come up with much better than XFS_IS_CORRUPT, though I
see Dave's point about:

if (XFS_IS_CORRUPT(mp,
    xfs_measure_something() > BADNESS) {
	xfs_error(mp, "OHNO");
	return -EFSCORRUPTED;
}

Is a bit hard to read.

if (XFS_IS_CORRUPT(mp,
		   xfs_measure_something() > BADNESS) {
	xfs_error(mp, "OHNO");
	return -EFSCORRUPTED;
}

Isn't awesome either, but it at least works and is a bit more obvious.
:/

--D

> Brian
> 
> >  #define STATIC static noinline
> >  
> >  #ifdef CONFIG_XFS_RT
> > 
>