diff mbox series

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

Message ID 157343507829.1945685.13191379852906635565.stgit@magnolia (mailing list archive)
State New, archived
Headers show
Series xfs: refactor corruption returns | expand

Commit Message

Darrick J. Wong Nov. 11, 2019, 1:17 a.m. UTC 
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>
---
 fs/xfs/xfs_linux.h |   16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

Comments

Brian Foster Nov. 11, 2019, 1:51 p.m. UTC | #1 
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
>
Darrick J. Wong Nov. 13, 2019, 12:01 a.m. UTC | #2 
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
> > 
>
diff mbox series

Patch

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)))
+
 #define STATIC static noinline
 
 #ifdef CONFIG_XFS_RT