Re: [PATCH] xfs: prevent NMI timeouts in cmn_err

[Dave Chinner <david@fromorbit.com>]

On Sun, Dec 12, 2010 at 08:43:11PM -0500, Lachlan McIlroy wrote:
> 
> ----- "Dave Chinner" <david@fromorbit.com> wrote:
> 
> > On Fri, Dec 10, 2010 at 08:29:35AM -0500, Christoph Hellwig wrote:
> > > On Fri, Dec 03, 2010 at 03:38:46PM +1100, Dave Chinner wrote:
> > > > FWIW, while these macros are the best way to make a simple
> > backport
> > > > is possible, I just discovered that mainline has a %pV format
> > > > operator that allows an implementation like:
> > > > 
> > > > void
> > > > xfs_fs_cmn_err(
> > > > 	const char              *lvl,
> > > > 	struct xfs_mount        *mp,
> > > > 	const char              *fmt,
> > > > 	...)
> > > > {
> > > > 	struct va_format        vaf;
> > > > 	va_list                 args;
> > > > 
> > > > 	va_start(args, fmt);
> > > > 	vaf.fmt = fmt;
> > > > 	vaf.va = &args;
> > > > 
> > > > 	printk("%sFilesystem %s: %pV", lvl, mp->m_fsname, &vaf);
> > > > 	va_end(args);
> > > > 
> > > > 	BUG_ON(strncmp(lvl, KERN_EMERG, strlen(KERN_EMERG)) == 0);
> > > > }
> > > 
> > > With this we can also keep the existing integer-based CE_ values
> > > and do trivial array lookup.  That also avoids having to do a strcmp
> > for
> > > every message printed.
> > 
> > Very true.
> > 
> > Ok, so how do we want to process for this? I'm happy to drop the
> > macro-ised patch and only use it as a backportable fix for old
> > kernels - mainline appears to have a lot more functionality in this
> > area than even recent distro kernels. Lachlan - is that an
> > acceptable approach for you?
> 
> Yes, that works for me.  We don't have much choice for RHEL4/5 and
> there's no reason that should compromise a better solution upstream.

Ok, I'll get that ball moving...

> > It seems to me that the above is a much better way to start cleaning
> > up the mainline code base, in the following way:
> > 
> > 	1. the above patch to remove the local message buffer +
> > 	   lock.
> > 	2. rationalise all the differences in error reporting down
> > 	   to a common interface
> > 	3. convert the common interface to use kernel log levels
> > 	   directly.
> > 
> > The current trend seems to be to move towards logging interfaces
> > with the following template:
> > 
> > 	{sub_sys}_pr_{level}(priv, fmt, ...)
> > 
> > Which in this case would give us:
> > 
> > 	xfs_pr_debug(struct xfs_mount *mp, char *fmt, ...);
> > 	xfs_pr_note(struct xfs_mount *mp, char *fmt, ...);
> > 	....
> > 	xfs_pr_alert(struct xfs_mount *mp, char *fmt, ...);
> > 	xfs_pr_emergency(struct xfs_mount *mp, char *fmt, ...);
> > 
> > And a variant for the panic mask tagged version of xfs_pr_alert()
> > of:
> > 
> > 	xfs_pr_alert_tag(struct xfs_mount *mp, int tag, char *fmt, ...);
> > 
> > I can't see any particular reason for needing to keep the separate
> > parameter for the log level, nor for keeping all the different
> > logging variants we currently have.
> 
> Wont you need to reintroduce the log level parameter when each of these
> new functions calls the common interface?

Ah, what I meant was that the above xfs_pr_*() functions form the
common logging interface, instead of cmn_err, xfs_cmn_err,
xfs_fs_cmn_err, etc.  So the second step above would be to convert
all the existing logging calls to use this, but still using the
current cmn_err() back end for the custom XFS log messages. i.e. the
callers change API, but they still use the same CE_* levels.

> ie most of the above functions
> will just be wrappers that convert the function name to a log level.

The third step is to change the implementation to something like
the dev_<level>() printk implementation, or possibly even using the 
pr_<level> macros directly via creative abuse of the pr_fmt() macro.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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