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

[Dave Chinner <david@fromorbit.com>]

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?

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.

I'm open to other ideas on the best way to rationalise all the
logging we currently have - the above is just my current thoughts on
what to do. I'm pushing this out as a proposal for discussion before
I change anything as I don't relish the idea of iterating different
logging APIs through patches...

Cheers,

Dave.

> 
> 

-- 
Dave Chinner
david@fromorbit.com

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