Re: [PATCH V2] xfs: introduce CONFIG_XFS_WARN

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Dave,

Since this solution is for production environment, would it be valuable
to have a sysctl variable to allow enabling/disabling XFS_WARN, as
opposed to needing to recompile the module afresh ?

regards,

Chandra
On Wed, 2013-04-24 at 18:55 +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> Running a CONFIG_XFS_DEBUG kernel in production environments is not
> the best idea as it introduces significant overhead, can change
> the behaviour of algorithms (such as allocation) to improve test
> coverage, and (most importantly) panic the machine on non-fatal
> errors.
> 
> There are many cases where all we want to do is run a
> kernel with more bounds checking enabled, such as is provided by the
> ASSERT() statements throughout the code, but without all the
> potential overhead and drawbacks.
> 
> This patch converts all the ASSERT statements to evaluate as
> WARN_ON(1) statements and hence if they fail dump a warning and a
> stack trace to the log. This has minimal overhead and does not
> change any algorithms, and will allow us to find strange "out of
> bounds" problems more easily on production machines.
> 
> There are a few places where assert statements contain debug only
> code. These are converted to be debug-or-warn only code so that we
> still get all the assert checks in the code.
> 
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
> Version 2:
> - fix transaction accounting for superblock debug fields.
> 
>  fs/xfs/Kconfig            |   13 +++++++++++++
>  fs/xfs/mrlock.h           |   12 ++++++------
>  fs/xfs/xfs.h              |    5 +++++
>  fs/xfs/xfs_alloc_btree.c  |    4 ++--
>  fs/xfs/xfs_bmap_btree.c   |    4 ++--
>  fs/xfs/xfs_btree.h        |    2 +-
>  fs/xfs/xfs_dir2_node.c    |    4 ++--
>  fs/xfs/xfs_ialloc_btree.c |    4 ++--
>  fs/xfs/xfs_inode.c        |    2 +-
>  fs/xfs/xfs_linux.h        |   24 ++++++++++++++++++------
>  fs/xfs/xfs_message.c      |    8 ++++++++
>  fs/xfs/xfs_message.h      |    1 +
>  fs/xfs/xfs_trans.h        |    4 ++--
>  13 files changed, 63 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/xfs/Kconfig b/fs/xfs/Kconfig
> index cc33aaf..399e8ce 100644
> --- a/fs/xfs/Kconfig
> +++ b/fs/xfs/Kconfig
> @@ -69,6 +69,19 @@ config XFS_RT
> 
>  	  If unsure, say N.
> 
> +config XFS_WARN
> +	bool "XFS Verbose Warnings"
> +	depends on XFS_FS && !XFS_DEBUG
> +	help
> +	  Say Y here to get an XFS build with many additional warnings.
> +	  It converts ASSERT checks to WARN, so will log any out-of-bounds
> +	  conditions that occur that would otherwise be missed. It is much
> +	  lighter weight than XFS_DEBUG and does not modify algorithms and will
> +	  not cause the kernel to panic on non-fatal errors.
> +
> +	  However, similar to XFS_DEBUG, it is only advisable to use this if you
> +	  are debugging a particular problem.
> +
>  config XFS_DEBUG
>  	bool "XFS Debugging support"
>  	depends on XFS_FS
> diff --git a/fs/xfs/mrlock.h b/fs/xfs/mrlock.h
> index ff6a198..e3c92d1 100644
> --- a/fs/xfs/mrlock.h
> +++ b/fs/xfs/mrlock.h
> @@ -22,12 +22,12 @@
> 
>  typedef struct {
>  	struct rw_semaphore	mr_lock;
> -#ifdef DEBUG
> +#if defined(DEBUG) || defined(XFS_WARN)
>  	int			mr_writer;
>  #endif
>  } mrlock_t;
> 
> -#ifdef DEBUG
> +#if defined(DEBUG) || defined(XFS_WARN)
>  #define mrinit(mrp, name)	\
>  	do { (mrp)->mr_writer = 0; init_rwsem(&(mrp)->mr_lock); } while (0)
>  #else
> @@ -46,7 +46,7 @@ static inline void mraccess_nested(mrlock_t *mrp, int subclass)
>  static inline void mrupdate_nested(mrlock_t *mrp, int subclass)
>  {
>  	down_write_nested(&mrp->mr_lock, subclass);
> -#ifdef DEBUG
> +#if defined(DEBUG) || defined(XFS_WARN)
>  	mrp->mr_writer = 1;
>  #endif
>  }
> @@ -60,7 +60,7 @@ static inline int mrtryupdate(mrlock_t *mrp)
>  {
>  	if (!down_write_trylock(&mrp->mr_lock))
>  		return 0;
> -#ifdef DEBUG
> +#if defined(DEBUG) || defined(XFS_WARN)
>  	mrp->mr_writer = 1;
>  #endif
>  	return 1;
> @@ -68,7 +68,7 @@ static inline int mrtryupdate(mrlock_t *mrp)
> 
>  static inline void mrunlock_excl(mrlock_t *mrp)
>  {
> -#ifdef DEBUG
> +#if defined(DEBUG) || defined(XFS_WARN)
>  	mrp->mr_writer = 0;
>  #endif
>  	up_write(&mrp->mr_lock);
> @@ -81,7 +81,7 @@ static inline void mrunlock_shared(mrlock_t *mrp)
> 
>  static inline void mrdemote(mrlock_t *mrp)
>  {
> -#ifdef DEBUG
> +#if defined(DEBUG) || defined(XFS_WARN)
>  	mrp->mr_writer = 0;
>  #endif
>  	downgrade_write(&mrp->mr_lock);
> diff --git a/fs/xfs/xfs.h b/fs/xfs/xfs.h
> index d8b11b7..a742c47 100644
> --- a/fs/xfs/xfs.h
> +++ b/fs/xfs/xfs.h
> @@ -24,6 +24,11 @@
>  #define XFS_BUF_LOCK_TRACKING 1
>  #endif
> 
> +#ifdef CONFIG_XFS_WARN
> +#define XFS_WARN 1
> +#endif
> +
> +
>  #include "xfs_linux.h"
> 
>  #endif	/* __XFS_H__ */
> diff --git a/fs/xfs/xfs_alloc_btree.c b/fs/xfs/xfs_alloc_btree.c
> index b1ddef6..cbcd34b 100644
> --- a/fs/xfs/xfs_alloc_btree.c
> +++ b/fs/xfs/xfs_alloc_btree.c
> @@ -348,7 +348,7 @@ const struct xfs_buf_ops xfs_allocbt_buf_ops = {
>  };
> 
> 
> -#ifdef DEBUG
> +#if defined(DEBUG) || defined(XFS_WARN)
>  STATIC int
>  xfs_allocbt_keys_inorder(
>  	struct xfs_btree_cur	*cur,
> @@ -404,7 +404,7 @@ static const struct xfs_btree_ops xfs_allocbt_ops = {
>  	.init_ptr_from_cur	= xfs_allocbt_init_ptr_from_cur,
>  	.key_diff		= xfs_allocbt_key_diff,
>  	.buf_ops		= &xfs_allocbt_buf_ops,
> -#ifdef DEBUG
> +#if defined(DEBUG) || defined(XFS_WARN)
>  	.keys_inorder		= xfs_allocbt_keys_inorder,
>  	.recs_inorder		= xfs_allocbt_recs_inorder,
>  #endif
> diff --git a/fs/xfs/xfs_bmap_btree.c b/fs/xfs/xfs_bmap_btree.c
> index 061b45c..5eaaa4b 100644
> --- a/fs/xfs/xfs_bmap_btree.c
> +++ b/fs/xfs/xfs_bmap_btree.c
> @@ -769,7 +769,7 @@ const struct xfs_buf_ops xfs_bmbt_buf_ops = {
>  };
> 
> 
> -#ifdef DEBUG
> +#if defined(DEBUG) || defined(XFS_WARN)
>  STATIC int
>  xfs_bmbt_keys_inorder(
>  	struct xfs_btree_cur	*cur,
> @@ -809,7 +809,7 @@ static const struct xfs_btree_ops xfs_bmbt_ops = {
>  	.init_ptr_from_cur	= xfs_bmbt_init_ptr_from_cur,
>  	.key_diff		= xfs_bmbt_key_diff,
>  	.buf_ops		= &xfs_bmbt_buf_ops,
> -#ifdef DEBUG
> +#if defined(DEBUG) || defined(XFS_WARN)
>  	.keys_inorder		= xfs_bmbt_keys_inorder,
>  	.recs_inorder		= xfs_bmbt_recs_inorder,
>  #endif
> diff --git a/fs/xfs/xfs_btree.h b/fs/xfs/xfs_btree.h
> index f932897..6302e9a 100644
> --- a/fs/xfs/xfs_btree.h
> +++ b/fs/xfs/xfs_btree.h
> @@ -190,7 +190,7 @@ struct xfs_btree_ops {
> 
>  	const struct xfs_buf_ops	*buf_ops;
> 
> -#ifdef DEBUG
> +#if defined(DEBUG) || defined(XFS_WARN)
>  	/* check that k1 is lower than k2 */
>  	int	(*keys_inorder)(struct xfs_btree_cur *cur,
>  				union xfs_btree_key *k1,
> diff --git a/fs/xfs/xfs_dir2_node.c b/fs/xfs/xfs_dir2_node.c
> index 5980f9b..33177c3 100644
> --- a/fs/xfs/xfs_dir2_node.c
> +++ b/fs/xfs/xfs_dir2_node.c
> @@ -811,7 +811,7 @@ xfs_dir2_leafn_rebalance(
>  	xfs_dir2_leaf_t		*leaf1;		/* first leaf structure */
>  	xfs_dir2_leaf_t		*leaf2;		/* second leaf structure */
>  	int			mid;		/* midpoint leaf index */
> -#ifdef DEBUG
> +#if defined(DEBUG) || defined(XFS_WARN)
>  	int			oldstale;	/* old count of stale leaves */
>  #endif
>  	int			oldsum;		/* old total leaf count */
> @@ -831,7 +831,7 @@ xfs_dir2_leafn_rebalance(
>  	leaf1 = blk1->bp->b_addr;
>  	leaf2 = blk2->bp->b_addr;
>  	oldsum = be16_to_cpu(leaf1->hdr.count) + be16_to_cpu(leaf2->hdr.count);
> -#ifdef DEBUG
> +#if defined(DEBUG) || defined(XFS_WARN)
>  	oldstale = be16_to_cpu(leaf1->hdr.stale) + be16_to_cpu(leaf2->hdr.stale);
>  #endif
>  	mid = oldsum >> 1;
> diff --git a/fs/xfs/xfs_ialloc_btree.c b/fs/xfs/xfs_ialloc_btree.c
> index bec344b..7b9c7be 100644
> --- a/fs/xfs/xfs_ialloc_btree.c
> +++ b/fs/xfs/xfs_ialloc_btree.c
> @@ -235,7 +235,7 @@ const struct xfs_buf_ops xfs_inobt_buf_ops = {
>  	.verify_write = xfs_inobt_write_verify,
>  };
> 
> -#ifdef DEBUG
> +#if defined(DEBUG) || defined(XFS_WARN)
>  STATIC int
>  xfs_inobt_keys_inorder(
>  	struct xfs_btree_cur	*cur,
> @@ -273,7 +273,7 @@ static const struct xfs_btree_ops xfs_inobt_ops = {
>  	.init_ptr_from_cur	= xfs_inobt_init_ptr_from_cur,
>  	.key_diff		= xfs_inobt_key_diff,
>  	.buf_ops		= &xfs_inobt_buf_ops,
> -#ifdef DEBUG
> +#if defined(DEBUG) || defined(XFS_WARN)
>  	.keys_inorder		= xfs_inobt_keys_inorder,
>  	.recs_inorder		= xfs_inobt_recs_inorder,
>  #endif
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 4f20165..4630d47 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -286,7 +286,7 @@ xfs_ilock_demote(
>  	trace_xfs_ilock_demote(ip, lock_flags, _RET_IP_);
>  }
> 
> -#ifdef DEBUG
> +#if defined(DEBUG) || defined(XFS_WARN)
>  int
>  xfs_isilocked(
>  	xfs_inode_t		*ip,
> diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
> index 14e59d9..800f896 100644
> --- a/fs/xfs/xfs_linux.h
> +++ b/fs/xfs/xfs_linux.h
> @@ -293,22 +293,34 @@ static inline __uint64_t howmany_64(__uint64_t x, __uint32_t y)
>  #define ASSERT_ALWAYS(expr)	\
>  	(unlikely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__))
> 
> -#ifndef DEBUG
> -#define ASSERT(expr)	((void)0)
> +#ifdef DEBUG
> +#define ASSERT(expr)	\
> +	(unlikely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__))
> 
>  #ifndef STATIC
> -# define STATIC static noinline
> +# define STATIC noinline
>  #endif
> 
> -#else /* DEBUG */
> +#else	/* !DEBUG */
> +
> +#ifdef XFS_WARN
> 
>  #define ASSERT(expr)	\
> -	(unlikely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__))
> +	(unlikely(expr) ? (void)0 : asswarn(#expr, __FILE__, __LINE__))
> 
>  #ifndef STATIC
> -# define STATIC noinline
> +# define STATIC static noinline
> +#endif
> +
> +#else	/* !DEBUG && !XFS_WARN */
> +
> +#define ASSERT(expr)	((void)0)
> +
> +#ifndef STATIC
> +# define STATIC static noinline
>  #endif
> 
> +#endif /* XFS_WARN */
>  #endif /* DEBUG */
> 
>  #endif /* __XFS_LINUX__ */
> diff --git a/fs/xfs/xfs_message.c b/fs/xfs/xfs_message.c
> index 331cd9f..9163dc1 100644
> --- a/fs/xfs/xfs_message.c
> +++ b/fs/xfs/xfs_message.c
> @@ -93,6 +93,14 @@ xfs_alert_tag(
>  }
> 
>  void
> +asswarn(char *expr, char *file, int line)
> +{
> +	xfs_warn(NULL, "Assertion failed: %s, file: %s, line: %d",
> +		expr, file, line);
> +	WARN_ON(1);
> +}
> +
> +void
>  assfail(char *expr, char *file, int line)
>  {
>  	xfs_emerg(NULL, "Assertion failed: %s, file: %s, line: %d",
> diff --git a/fs/xfs/xfs_message.h b/fs/xfs/xfs_message.h
> index 76c8198..8540115 100644
> --- a/fs/xfs/xfs_message.h
> +++ b/fs/xfs/xfs_message.h
> @@ -57,6 +57,7 @@ do {									\
>  	xfs_printk_ratelimited(xfs_debug, dev, fmt, ##__VA_ARGS__)
> 
>  extern void assfail(char *expr, char *f, int l);
> +extern void asswarn(char *expr, char *f, int l);
> 
>  extern void xfs_hex_dump(void *p, int length);
> 
> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index cd29f61..a44dba5 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -405,7 +405,7 @@ typedef struct xfs_trans {
>  	int64_t			t_res_fdblocks_delta; /* on-disk only chg */
>  	int64_t			t_frextents_delta;/* superblock freextents chg*/
>  	int64_t			t_res_frextents_delta; /* on-disk only chg */
> -#ifdef DEBUG
> +#if defined(DEBUG) || defined(XFS_WARN)
>  	int64_t			t_ag_freeblks_delta; /* debugging counter */
>  	int64_t			t_ag_flist_delta; /* debugging counter */
>  	int64_t			t_ag_btree_delta; /* debugging counter */
> @@ -433,7 +433,7 @@ typedef struct xfs_trans {
>  #define	xfs_trans_get_block_res(tp)	((tp)->t_blk_res)
>  #define	xfs_trans_set_sync(tp)		((tp)->t_flags |= XFS_TRANS_SYNC)
> 
> -#ifdef DEBUG
> +#if defined(DEBUG) || defined(XFS_WARN)
>  #define	xfs_trans_agblocks_delta(tp, d)	((tp)->t_ag_freeblks_delta += (int64_t)d)
>  #define	xfs_trans_agflist_delta(tp, d)	((tp)->t_ag_flist_delta += (int64_t)d)
>  #define	xfs_trans_agbtree_delta(tp, d)	((tp)->t_ag_btree_delta += (int64_t)d)
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
> 


_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs




[Index of Archives]     [Linux XFS Devel]     [Linux Filesystem Development]     [Filesystem Testing]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux