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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
>
> 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
> >
>
