xfs
[Top] [All Lists]

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

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfs: prevent NMI timeouts in cmn_err
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Mon, 13 Dec 2010 11:30:43 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20101210132935.GA24210@xxxxxxxxxxxxx>
References: <1291341315-31338-1-git-send-email-david@xxxxxxxxxxxxx> <20101203043846.GB23339@dastard> <20101210132935.GA24210@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
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@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>