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

[Lachlan McIlroy <lmcilroy@redhat.com>]

----- "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.

> 
> 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?  ie most of the above functions
will just be wrappers that convert the function name to a log level.

> 
> 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

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