Re: [PATCH RFC] xfs: support debug mode with assert warnings

["Darrick J. Wong" <darrick.wong@oracle.com>]

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