* [PATCH RFC] xfs: support debug mode with assert warnings
@ 2017-04-28 17:29 Brian Foster
2017-05-02 7:37 ` Christoph Hellwig
0 siblings, 1 reply; 6+ messages in thread
From: Brian Foster @ 2017-04-28 17:29 UTC (permalink / raw)
To: linux-xfs
Signed-off-by: Brian Foster <bfoster@redhat.com>
---
Hi all,
Every now and then I find myself wanting to enable DEBUG mode code
without having to BUG the kernel every time an assert fails. Currently,
I end up just commenting out the BUG() call from assfail(). Any thoughts
on something like the below to update our configuration to support the
ability to enable debug mode with assert warnings?
While this appears as a new option in Kconfig, it just reuses the
existing XFS_WARN definition to convert asserts into warnings regardless
of whether debug mode is enabled or not. There are probably multiple
other ways to do something like this (e.g., a Kconfig 'choice' selection
for various XFS debug modes, dropping the BUG() entirely, etc.).
Thoughts?
Brian
fs/xfs/Kconfig | 9 +++++++++
fs/xfs/xfs_linux.h | 32 +++++++++++++++++++-------------
2 files changed, 28 insertions(+), 13 deletions(-)
diff --git a/fs/xfs/Kconfig b/fs/xfs/Kconfig
index 35faf12..db6d830 100644
--- a/fs/xfs/Kconfig
+++ b/fs/xfs/Kconfig
@@ -96,3 +96,12 @@ config XFS_DEBUG
not useful unless you are debugging a particular problem.
Say N unless you are an XFS developer, or you play one on TV.
+
+config XFS_WARN
+ bool "Non-fatal Asserts"
+ default n
+ depends on XFS_FS && XFS_DEBUG
+ help
+ Say Y here to convert DEBUG mode ASSERT failures into warnings.
+ Otherwise, ASSERT failures are considered fatal errors and BUG the
+ kernel.
diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
index 044fb0e..3432863 100644
--- a/fs/xfs/xfs_linux.h
+++ b/fs/xfs/xfs_linux.h
@@ -245,37 +245,43 @@ static inline __uint64_t howmany_64(__uint64_t x, __uint32_t y)
return x;
}
+/*
+ * ASSERT() definitions
+ */
#define ASSERT_ALWAYS(expr) \
(likely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__))
-#ifdef DEBUG
+#if defined(DEBUG) && !defined(XFS_WARN)
+
#define ASSERT(expr) \
(likely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__))
-#ifndef STATIC
-# define STATIC noinline
-#endif
-
-#else /* !DEBUG */
-
-#ifdef XFS_WARN
+#elif defined(XFS_WARN)
#define ASSERT(expr) \
(likely(expr) ? (void)0 : asswarn(#expr, __FILE__, __LINE__))
-#ifndef STATIC
-# define STATIC static noinline
+#else
+
+#define ASSERT(expr) ((void)0)
+
#endif
-#else /* !DEBUG && !XFS_WARN */
+/*
+ * STATIC definitions
+ */
+#ifdef DEBUG
+
+#ifndef STATIC
+# define STATIC noinline
+#endif
-#define ASSERT(expr) ((void)0)
+#else /* !DEBUG */
#ifndef STATIC
# define STATIC static noinline
#endif
-#endif /* XFS_WARN */
#endif /* DEBUG */
#ifdef CONFIG_XFS_RT
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH RFC] xfs: support debug mode with assert warnings
2017-04-28 17:29 [PATCH RFC] xfs: support debug mode with assert warnings Brian Foster
@ 2017-05-02 7:37 ` Christoph Hellwig
2017-05-02 12:14 ` Brian Foster
0 siblings, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2017-05-02 7:37 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On Fri, Apr 28, 2017 at 01:29:01PM -0400, Brian Foster wrote:
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>
> Hi all,
>
> Every now and then I find myself wanting to enable DEBUG mode code
> without having to BUG the kernel every time an assert fails. Currently,
> I end up just commenting out the BUG() call from assfail(). Any thoughts
> on something like the below to update our configuration to support the
> ability to enable debug mode with assert warnings?
>
> While this appears as a new option in Kconfig, it just reuses the
> existing XFS_WARN definition to convert asserts into warnings regardless
> of whether debug mode is enabled or not. There are probably multiple
> other ways to do something like this (e.g., a Kconfig 'choice' selection
> for various XFS debug modes, dropping the BUG() entirely, etc.).
> Thoughts?
>
> Brian
>
> fs/xfs/Kconfig | 9 +++++++++
> fs/xfs/xfs_linux.h | 32 +++++++++++++++++++-------------
> 2 files changed, 28 insertions(+), 13 deletions(-)
>
> diff --git a/fs/xfs/Kconfig b/fs/xfs/Kconfig
> index 35faf12..db6d830 100644
> --- a/fs/xfs/Kconfig
> +++ b/fs/xfs/Kconfig
> @@ -96,3 +96,12 @@ config XFS_DEBUG
> not useful unless you are debugging a particular problem.
>
> Say N unless you are an XFS developer, or you play one on TV.
> +
> +config XFS_WARN
> + bool "Non-fatal Asserts"
> + default n
> + depends on XFS_FS && XFS_DEBUG
> + help
> + Say Y here to convert DEBUG mode ASSERT failures into warnings.
> + Otherwise, ASSERT failures are considered fatal errors and BUG the
> + kernel.
I'm very confused by this patch. At least my fs/xfs/Kconfig already
has a "config XFS_WARN" line. What am I missing?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC] xfs: support debug mode with assert warnings
2017-05-02 7:37 ` Christoph Hellwig
@ 2017-05-02 12:14 ` Brian Foster
2017-05-04 11:57 ` Christoph Hellwig
0 siblings, 1 reply; 6+ messages in thread
From: Brian Foster @ 2017-05-02 12:14 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs
On Tue, May 02, 2017 at 12:37:30AM -0700, Christoph Hellwig wrote:
> On Fri, Apr 28, 2017 at 01:29:01PM -0400, Brian Foster wrote:
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> >
> > Hi all,
> >
> > Every now and then I find myself wanting to enable DEBUG mode code
> > without having to BUG the kernel every time an assert fails. Currently,
> > I end up just commenting out the BUG() call from assfail(). Any thoughts
> > on something like the below to update our configuration to support the
> > ability to enable debug mode with assert warnings?
> >
> > While this appears as a new option in Kconfig, it just reuses the
> > existing XFS_WARN definition to convert asserts into warnings regardless
> > of whether debug mode is enabled or not. There are probably multiple
> > other ways to do something like this (e.g., a Kconfig 'choice' selection
> > for various XFS debug modes, dropping the BUG() entirely, etc.).
> > Thoughts?
> >
> > Brian
> >
> > fs/xfs/Kconfig | 9 +++++++++
> > fs/xfs/xfs_linux.h | 32 +++++++++++++++++++-------------
> > 2 files changed, 28 insertions(+), 13 deletions(-)
> >
> > diff --git a/fs/xfs/Kconfig b/fs/xfs/Kconfig
> > index 35faf12..db6d830 100644
> > --- a/fs/xfs/Kconfig
> > +++ b/fs/xfs/Kconfig
> > @@ -96,3 +96,12 @@ config XFS_DEBUG
> > not useful unless you are debugging a particular problem.
> >
> > Say N unless you are an XFS developer, or you play one on TV.
> > +
> > +config XFS_WARN
> > + bool "Non-fatal Asserts"
> > + default n
> > + depends on XFS_FS && XFS_DEBUG
> > + help
> > + Say Y here to convert DEBUG mode ASSERT failures into warnings.
> > + Otherwise, ASSERT failures are considered fatal errors and BUG the
> > + kernel.
>
> I'm very confused by this patch. At least my fs/xfs/Kconfig already
> has a "config XFS_WARN" line. What am I missing?
As noted above, this is just a reuse of the flag. XFS_WARN and XFS_DEBUG
are currently mutually exclusive. The former enables warnings on assert
failures. The latter enables BUG()'s on assert failures and the
additional, typical debug mode code. The Kconfig hack above simply pops
up a conditional option when debug mode is enabled that effectively
allows setting both XFS_WARN and XFS_DEBUG at the same time. The header
files interpret this as debug mode enabled with an override for the
assert failures to warn rather than BUG().
Note that this is just a hack and we can organize the Kconfig options
however we want. For example, we could call this XFS_DEBUG_WARN and
continue to consider it a debug mode sub-flag, or we could turn the
debug mode option into a multi-mode selector (i.e., Debug modes: "None,"
"Warn only," "Debug mode," "Debug mode w/ non-fatal asserts"). I played
around a bit with the latter but it seems like a bit of overkill to me.
Brian
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC] xfs: support debug mode with assert warnings
2017-05-02 12:14 ` Brian Foster
@ 2017-05-04 11:57 ` Christoph Hellwig
2017-05-04 12:44 ` Brian Foster
0 siblings, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2017-05-04 11:57 UTC (permalink / raw)
To: Brian Foster; +Cc: Christoph Hellwig, linux-xfs
On Tue, May 02, 2017 at 08:14:25AM -0400, Brian Foster wrote:
> As noted above, this is just a reuse of the flag. XFS_WARN and XFS_DEBUG
> are currently mutually exclusive. The former enables warnings on assert
> failures. The latter enables BUG()'s on assert failures and the
> additional, typical debug mode code. The Kconfig hack above simply pops
> up a conditional option when debug mode is enabled that effectively
> allows setting both XFS_WARN and XFS_DEBUG at the same time. The header
> files interpret this as debug mode enabled with an override for the
> assert failures to warn rather than BUG().
Oh, I didn't know Kconfig allows the same symbol to be define twice.
But even if that's ok I'd say it's rather odd.
> Note that this is just a hack and we can organize the Kconfig options
> however we want. For example, we could call this XFS_DEBUG_WARN and
> continue to consider it a debug mode sub-flag, or we could turn the
> debug mode option into a multi-mode selector (i.e., Debug modes: "None,"
> "Warn only," "Debug mode," "Debug mode w/ non-fatal asserts"). I played
> around a bit with the latter but it seems like a bit of overkill to me.
Maybe we need something like:
XFS_WARN (as-is)
XFS_WARN_BUG (XFS_WARN + BUG_ON on assert)
XFS_DEBUG (everyhing under XFS_DEBUG currently that's not related to
ASSERT)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC] xfs: support debug mode with assert warnings
2017-05-04 11:57 ` Christoph Hellwig
@ 2017-05-04 12:44 ` Brian Foster
2017-05-04 16:41 ` Darrick J. Wong
0 siblings, 1 reply; 6+ messages in thread
From: Brian Foster @ 2017-05-04 12:44 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs
On Thu, May 04, 2017 at 04:57:25AM -0700, Christoph Hellwig wrote:
> On Tue, May 02, 2017 at 08:14:25AM -0400, Brian Foster wrote:
> > As noted above, this is just a reuse of the flag. XFS_WARN and XFS_DEBUG
> > are currently mutually exclusive. The former enables warnings on assert
> > failures. The latter enables BUG()'s on assert failures and the
> > additional, typical debug mode code. The Kconfig hack above simply pops
> > up a conditional option when debug mode is enabled that effectively
> > allows setting both XFS_WARN and XFS_DEBUG at the same time. The header
> > files interpret this as debug mode enabled with an override for the
> > assert failures to warn rather than BUG().
>
> Oh, I didn't know Kconfig allows the same symbol to be define twice.
> But even if that's ok I'd say it's rather odd.
>
Yes, I just slapped it together quickly to float the idea.
> > Note that this is just a hack and we can organize the Kconfig options
> > however we want. For example, we could call this XFS_DEBUG_WARN and
> > continue to consider it a debug mode sub-flag, or we could turn the
> > debug mode option into a multi-mode selector (i.e., Debug modes: "None,"
> > "Warn only," "Debug mode," "Debug mode w/ non-fatal asserts"). I played
> > around a bit with the latter but it seems like a bit of overkill to me.
>
> Maybe we need something like:
>
> XFS_WARN (as-is)
> XFS_WARN_BUG (XFS_WARN + BUG_ON on assert)
> XFS_DEBUG (everyhing under XFS_DEBUG currently that's not related to
> ASSERT)
That allows asserts to BUG() on an XFS_WARN kernel, which is not quite
what I want to accomplish here (and I don't think that's really needed
for an XFS_WARN kernel). I'm not quite following if/how that allows to
disable assert BUG()s on an XFS_DEBUG kernel. Would XFS_DEBUG now never
BUG() unless XFS_WARN_BUG is defined as well?
If so, that sounds reasonable to me. I may just suggest tweaking it to
something like this:
XFS_WARN (as-is)
XFS_DEBUG (DEBUG code modified to warn on assert failure by default)
XFS_ASSERT_BUG (depends on XFS_DEBUG, enables BUG() on assert failure)
WARN and DEBUG remain mutually exclusive, the default assert behavior
for DEBUG changes to a warning rather than BUG(), and the latter is
enabled by a new conditional XFS_ASSERT_BUG config option. My only
slight concern is that changes default behavior for distros that might
create debug builds/packages, but I can see whether we can mitigate that
by setting 'default y' for XFS_ASSERT_BUG so long as it is only
available in XFS_DEBUG mode. Thoughts?
Brian
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC] xfs: support debug mode with assert warnings
2017-05-04 12:44 ` Brian Foster
@ 2017-05-04 16:41 ` Darrick J. Wong
0 siblings, 0 replies; 6+ messages in thread
From: Darrick J. Wong @ 2017-05-04 16:41 UTC (permalink / raw)
To: Brian Foster; +Cc: Christoph Hellwig, linux-xfs
On Thu, May 04, 2017 at 08:44:26AM -0400, Brian Foster wrote:
> On Thu, May 04, 2017 at 04:57:25AM -0700, Christoph Hellwig wrote:
> > On Tue, May 02, 2017 at 08:14:25AM -0400, Brian Foster wrote:
> > > As noted above, this is just a reuse of the flag. XFS_WARN and XFS_DEBUG
> > > are currently mutually exclusive. The former enables warnings on assert
> > > failures. The latter enables BUG()'s on assert failures and the
> > > additional, typical debug mode code. The Kconfig hack above simply pops
> > > up a conditional option when debug mode is enabled that effectively
> > > allows setting both XFS_WARN and XFS_DEBUG at the same time. The header
> > > files interpret this as debug mode enabled with an override for the
> > > assert failures to warn rather than BUG().
> >
> > Oh, I didn't know Kconfig allows the same symbol to be define twice.
> > But even if that's ok I'd say it's rather odd.
> >
>
> Yes, I just slapped it together quickly to float the idea.
>
> > > Note that this is just a hack and we can organize the Kconfig options
> > > however we want. For example, we could call this XFS_DEBUG_WARN and
> > > continue to consider it a debug mode sub-flag, or we could turn the
> > > debug mode option into a multi-mode selector (i.e., Debug modes: "None,"
> > > "Warn only," "Debug mode," "Debug mode w/ non-fatal asserts"). I played
> > > around a bit with the latter but it seems like a bit of overkill to me.
> >
> > Maybe we need something like:
> >
> > XFS_WARN (as-is)
> > XFS_WARN_BUG (XFS_WARN + BUG_ON on assert)
> > XFS_DEBUG (everyhing under XFS_DEBUG currently that's not related to
> > ASSERT)
>
> That allows asserts to BUG() on an XFS_WARN kernel, which is not quite
> what I want to accomplish here (and I don't think that's really needed
> for an XFS_WARN kernel). I'm not quite following if/how that allows to
> disable assert BUG()s on an XFS_DEBUG kernel. Would XFS_DEBUG now never
> BUG() unless XFS_WARN_BUG is defined as well?
>
> If so, that sounds reasonable to me. I may just suggest tweaking it to
> something like this:
>
> XFS_WARN (as-is)
> XFS_DEBUG (DEBUG code modified to warn on assert failure by default)
> XFS_ASSERT_BUG (depends on XFS_DEBUG, enables BUG() on assert failure)
>
> WARN and DEBUG remain mutually exclusive, the default assert behavior
> for DEBUG changes to a warning rather than BUG(), and the latter is
> enabled by a new conditional XFS_ASSERT_BUG config option. My only
> slight concern is that changes default behavior for distros that might
> create debug builds/packages, but I can see whether we can mitigate that
> by setting 'default y' for XFS_ASSERT_BUG so long as it is only
> available in XFS_DEBUG mode. Thoughts?
Sounds reasonable. For years I've been banging around a silly patch
that removes the BUG() call so that I can perform more forensic analysis
after something goes wrong.
(Granted it helps immensely that BUG reports now dump the ftrace
buffer though sometimes that just leads to a flood of out of date
crap coming over the serial line...)
--D
>
> Brian
>
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-05-04 16:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-04-28 17:29 [PATCH RFC] xfs: support debug mode with assert warnings Brian Foster
2017-05-02 7:37 ` Christoph Hellwig
2017-05-02 12:14 ` Brian Foster
2017-05-04 11:57 ` Christoph Hellwig
2017-05-04 12:44 ` Brian Foster
2017-05-04 16:41 ` Darrick J. Wong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).