* [PATCH] xfs: introduce CONFIG_XFS_WARN
@ 2013-04-23 6:38 Dave Chinner
2013-04-23 12:49 ` Brian Foster
0 siblings, 1 reply; 11+ messages in thread
From: Dave Chinner @ 2013-04-23 6:38 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
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@redhat.com>
---
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 | 2 +-
13 files changed, 62 insertions(+), 23 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..d3e0679 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 */
--
1.7.10.4
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] xfs: introduce CONFIG_XFS_WARN
2013-04-23 6:38 [PATCH] xfs: introduce CONFIG_XFS_WARN Dave Chinner
@ 2013-04-23 12:49 ` Brian Foster
2013-04-23 21:02 ` Dave Chinner
0 siblings, 1 reply; 11+ messages in thread
From: Brian Foster @ 2013-04-23 12:49 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On 04/23/2013 02:38 AM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
...
> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index cd29f61..d3e0679 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 */
>
I see some ASSERT() calls using these counters but the macros that
manage them appear to be defined against DEBUG only (further down in
xfs_trans.h). This looks like it would lead to spurious warnings..?
Brian
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xfs: introduce CONFIG_XFS_WARN
2013-04-23 12:49 ` Brian Foster
@ 2013-04-23 21:02 ` Dave Chinner
2013-04-24 8:55 ` [PATCH V2] " Dave Chinner
0 siblings, 1 reply; 11+ messages in thread
From: Dave Chinner @ 2013-04-23 21:02 UTC (permalink / raw)
To: Brian Foster; +Cc: xfs
On Tue, Apr 23, 2013 at 08:49:00AM -0400, Brian Foster wrote:
> On 04/23/2013 02:38 AM, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> >
> ...
> > diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> > index cd29f61..d3e0679 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 */
> >
>
> I see some ASSERT() calls using these counters but the macros that
> manage them appear to be defined against DEBUG only (further down in
> xfs_trans.h). This looks like it would lead to spurious warnings..?
Yes, you are right - it should lead to warnings being emitted, but I
didn't see any when running xfstests. I'll fix it up.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH V2] xfs: introduce CONFIG_XFS_WARN
2013-04-23 21:02 ` Dave Chinner
@ 2013-04-24 8:55 ` Dave Chinner
2013-04-24 18:39 ` Chandra Seetharaman
2013-04-25 14:48 ` Brian Foster
0 siblings, 2 replies; 11+ messages in thread
From: Dave Chinner @ 2013-04-24 8:55 UTC (permalink / raw)
To: Brian Foster; +Cc: xfs
From: Dave Chinner <dchinner@redhat.com>
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@redhat.com>
---
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@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH V2] xfs: introduce CONFIG_XFS_WARN
2013-04-24 8:55 ` [PATCH V2] " Dave Chinner
@ 2013-04-24 18:39 ` Chandra Seetharaman
2013-04-24 22:58 ` Dave Chinner
2013-04-25 14:48 ` Brian Foster
1 sibling, 1 reply; 11+ messages in thread
From: Chandra Seetharaman @ 2013-04-24 18:39 UTC (permalink / raw)
To: Dave Chinner; +Cc: Brian Foster, xfs
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@redhat.com>
>
> 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@redhat.com>
> ---
> 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@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V2] xfs: introduce CONFIG_XFS_WARN
2013-04-24 18:39 ` Chandra Seetharaman
@ 2013-04-24 22:58 ` Dave Chinner
2013-04-25 0:21 ` Eric Sandeen
2013-04-25 14:07 ` Chandra Seetharaman
0 siblings, 2 replies; 11+ messages in thread
From: Dave Chinner @ 2013-04-24 22:58 UTC (permalink / raw)
To: Chandra Seetharaman; +Cc: Brian Foster, xfs
On Wed, Apr 24, 2013 at 01:39:52PM -0500, Chandra Seetharaman wrote:
> 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 ?
The idea is that distros enable it on the debug kernel packages
rather than using CONFIG_XFS_DEBUG=y. Hence if someone has a
problem, they just install the debug kernel and they get all this
additional checking.
There are roughly 1700 ASSERT statements in the XFS code, so
compiling them in unconditionally is a lot of extra code. Adding a
sysctl to make them conditional adds as many branches into the code
than in 99.99999% of calls are never going to evaluate as true. The
convenience of a sysctl is more than outweighed by the additional
overhead for the majortiy of people that don't need to diagnose
problems on their system.
Hence I don't think the overhead of unconditionally compiling in
ASSERT checks is worth while for the majority of users, especially
as most distros ship a debug kernel for exactly this purpose....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V2] xfs: introduce CONFIG_XFS_WARN
2013-04-24 22:58 ` Dave Chinner
@ 2013-04-25 0:21 ` Eric Sandeen
2013-04-25 14:07 ` Chandra Seetharaman
1 sibling, 0 replies; 11+ messages in thread
From: Eric Sandeen @ 2013-04-25 0:21 UTC (permalink / raw)
To: Dave Chinner; +Cc: Brian Foster, Chandra Seetharaman, xfs
On 4/24/13 5:58 PM, Dave Chinner wrote:
> On Wed, Apr 24, 2013 at 01:39:52PM -0500, Chandra Seetharaman wrote:
>> 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 ?
>
> The idea is that distros enable it on the debug kernel packages
> rather than using CONFIG_XFS_DEBUG=y. Hence if someone has a
> problem, they just install the debug kernel and they get all this
> additional checking.
>
> There are roughly 1700 ASSERT statements in the XFS code, so
> compiling them in unconditionally is a lot of extra code. Adding a
> sysctl to make them conditional adds as many branches into the code
> than in 99.99999% of calls are never going to evaluate as true. The
> convenience of a sysctl is more than outweighed by the additional
> overhead for the majortiy of people that don't need to diagnose
> problems on their system.
>
> Hence I don't think the overhead of unconditionally compiling in
> ASSERT checks is worth while for the majority of users, especially
> as most distros ship a debug kernel for exactly this purpose....
I agree. kernel-debug, or whatever your distro does similarly,
is the right use for this code.
-Eric
> Cheers,
>
> Dave.
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V2] xfs: introduce CONFIG_XFS_WARN
2013-04-24 22:58 ` Dave Chinner
2013-04-25 0:21 ` Eric Sandeen
@ 2013-04-25 14:07 ` Chandra Seetharaman
2013-04-25 21:44 ` Dave Chinner
1 sibling, 1 reply; 11+ messages in thread
From: Chandra Seetharaman @ 2013-04-25 14:07 UTC (permalink / raw)
To: Dave Chinner; +Cc: Brian Foster, xfs
On Thu, 2013-04-25 at 08:58 +1000, Dave Chinner wrote:
> On Wed, Apr 24, 2013 at 01:39:52PM -0500, Chandra Seetharaman wrote:
> > 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 ?
>
> The idea is that distros enable it on the debug kernel packages
> rather than using CONFIG_XFS_DEBUG=y. Hence if someone has a
> problem, they just install the debug kernel and they get all this
> additional checking.
Thanks for the clarification.
I was thinking that CONFIG_XFS_DEBUG would be enabled in the distro's
debug kernel and this new option is in addition to that. Hence, my
question.
So, with this new config option, CONFIG_XFS_DEBUG will only be used in
development environment, correct ?
>
> There are roughly 1700 ASSERT statements in the XFS code, so
> compiling them in unconditionally is a lot of extra code. Adding a
> sysctl to make them conditional adds as many branches into the code
> than in 99.99999% of calls are never going to evaluate as true. The
> convenience of a sysctl is more than outweighed by the additional
> overhead for the majortiy of people that don't need to diagnose
> problems on their system.
>
> Hence I don't think the overhead of unconditionally compiling in
> ASSERT checks is worth while for the majority of users, especially
> as most distros ship a debug kernel for exactly this purpose....
>
> Cheers,
>
> Dave.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V2] xfs: introduce CONFIG_XFS_WARN
2013-04-24 8:55 ` [PATCH V2] " Dave Chinner
2013-04-24 18:39 ` Chandra Seetharaman
@ 2013-04-25 14:48 ` Brian Foster
1 sibling, 0 replies; 11+ messages in thread
From: Brian Foster @ 2013-04-25 14:48 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On 04/24/2013 04:55 AM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> 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@redhat.com>
> ---
> Version 2:
> - fix transaction accounting for superblock debug fields.
>
Looks good.
Reviewed-by: Brian Foster <bfoster@redhat.com>
> 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@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V2] xfs: introduce CONFIG_XFS_WARN
2013-04-25 14:07 ` Chandra Seetharaman
@ 2013-04-25 21:44 ` Dave Chinner
2013-04-25 22:29 ` Chandra Seetharaman
0 siblings, 1 reply; 11+ messages in thread
From: Dave Chinner @ 2013-04-25 21:44 UTC (permalink / raw)
To: Chandra Seetharaman; +Cc: Brian Foster, xfs
On Thu, Apr 25, 2013 at 09:07:49AM -0500, Chandra Seetharaman wrote:
> On Thu, 2013-04-25 at 08:58 +1000, Dave Chinner wrote:
> > On Wed, Apr 24, 2013 at 01:39:52PM -0500, Chandra Seetharaman wrote:
> > > 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 ?
> >
> > The idea is that distros enable it on the debug kernel packages
> > rather than using CONFIG_XFS_DEBUG=y. Hence if someone has a
> > problem, they just install the debug kernel and they get all this
> > additional checking.
>
> Thanks for the clarification.
>
> I was thinking that CONFIG_XFS_DEBUG would be enabled in the distro's
> debug kernel and this new option is in addition to that. Hence, my
> question.
If you are shipping a debug kernel with CONFIG_XFS_DEBUG, then you
don't want your customers running that debug kernel for very long.
CONFIG_XFS_DEBUG changes allocation algorithms to improve test
coverage and this causes accelerated filesystem aging....
CONFIG_XFS_WARN is designed to be used instead of CONFIG_XFS_DEBUG
in these situations - we get more verbose checking, but without the
down sides associated with all the algorithmic changes in
XFS_DEBUG...
> So, with this new config option, CONFIG_XFS_DEBUG will only be used in
> development environment, correct ?
That's what I has always been intended for.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V2] xfs: introduce CONFIG_XFS_WARN
2013-04-25 21:44 ` Dave Chinner
@ 2013-04-25 22:29 ` Chandra Seetharaman
0 siblings, 0 replies; 11+ messages in thread
From: Chandra Seetharaman @ 2013-04-25 22:29 UTC (permalink / raw)
To: Dave Chinner; +Cc: XFS mailing list
Thanks Dave.
On Fri, 2013-04-26 at 07:44 +1000, Dave Chinner wrote:
> On Thu, Apr 25, 2013 at 09:07:49AM -0500, Chandra Seetharaman wrote:
> > On Thu, 2013-04-25 at 08:58 +1000, Dave Chinner wrote:
> > > On Wed, Apr 24, 2013 at 01:39:52PM -0500, Chandra Seetharaman wrote:
> > > > 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 ?
> > >
> > > The idea is that distros enable it on the debug kernel packages
> > > rather than using CONFIG_XFS_DEBUG=y. Hence if someone has a
> > > problem, they just install the debug kernel and they get all this
> > > additional checking.
> >
> > Thanks for the clarification.
> >
> > I was thinking that CONFIG_XFS_DEBUG would be enabled in the distro's
> > debug kernel and this new option is in addition to that. Hence, my
> > question.
>
> If you are shipping a debug kernel with CONFIG_XFS_DEBUG, then you
> don't want your customers running that debug kernel for very long.
> CONFIG_XFS_DEBUG changes allocation algorithms to improve test
> coverage and this causes accelerated filesystem aging....
>
> CONFIG_XFS_WARN is designed to be used instead of CONFIG_XFS_DEBUG
> in these situations - we get more verbose checking, but without the
> down sides associated with all the algorithmic changes in
> XFS_DEBUG...
>
> > So, with this new config option, CONFIG_XFS_DEBUG will only be used in
> > development environment, correct ?
>
> That's what I has always been intended for.
>
> Cheers,
>
> Dave.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2013-04-25 22:30 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-23 6:38 [PATCH] xfs: introduce CONFIG_XFS_WARN Dave Chinner
2013-04-23 12:49 ` Brian Foster
2013-04-23 21:02 ` Dave Chinner
2013-04-24 8:55 ` [PATCH V2] " Dave Chinner
2013-04-24 18:39 ` Chandra Seetharaman
2013-04-24 22:58 ` Dave Chinner
2013-04-25 0:21 ` Eric Sandeen
2013-04-25 14:07 ` Chandra Seetharaman
2013-04-25 21:44 ` Dave Chinner
2013-04-25 22:29 ` Chandra Seetharaman
2013-04-25 14:48 ` Brian Foster
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox