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

[Brian Foster <bfoster@redhat.com>]

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)))
> +

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 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.

Brian

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