xfs
[Top] [All Lists]

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

To: Lachlan McIlroy <lmcilroy@xxxxxxxxxx>
Subject: Re: [PATCH] xfs: prevent NMI timeouts in cmn_err
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Fri, 3 Dec 2010 19:36:59 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <275948151.359301291348292101.JavaMail.root@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
References: <121488966.359171291347997519.JavaMail.root@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> <275948151.359301291348292101.JavaMail.root@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
On Thu, Dec 02, 2010 at 10:51:32PM -0500, Lachlan McIlroy wrote:
> Dave, overall it looks good - just a few minor points below.
> Thanks for doing this.
> 
> ----- "Dave Chinner" <david@xxxxxxxxxxxxx> wrote:

[snip]

> > -void
> > -cmn_err(register int level, char *fmt, ...)
> > -{
> > -   char    *fp = fmt;
> > -   int     len;
> > -   ulong   flags;
> > -   va_list ap;
> > -
> > -   level &= XFS_ERR_MASK;
> > -   if (level > XFS_MAX_ERR_LEVEL)
> > -           level = XFS_MAX_ERR_LEVEL;
> > -   spin_lock_irqsave(&xfs_err_lock,flags);
> > -   va_start(ap, fmt);
> > -   if (*fmt == '!') fp++;
> > -   len = vsnprintf(message, sizeof(message), fp, ap);
> > -   if (len >= sizeof(message))
> > -           len = sizeof(message) - 1;
> > -   if (message[len-1] == '\n')
> > -           message[len-1] = 0;
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^

> > -   printk("%s%s\n", err_level[level], message);
> > -   va_end(ap);
> > -   spin_unlock_irqrestore(&xfs_err_lock,flags);
> > -   BUG_ON(level == CE_PANIC);
> > -}

[snip]

> > +#define CE_DEBUG        KERN_DEBUG
> > +#define CE_CONT         KERN_INFO
> > +#define CE_NOTE         KERN_NOTICE
> > +#define CE_WARN         KERN_WARNING
> > +#define CE_ALERT        KERN_ALERT
> > +#define CE_PANIC        KERN_EMERG
> > +
> > +#define cmn_err(lvl, fmt, args...) \
> > +   do { \
> > +           printk(lvl fmt, ## args); \
> 
> The old cmn_err() routine would append a newline if one was not supplied.
> As far as I know printk() will not do the same so either we need to fix
> all calls to cmn_err() to supply a '\n' or add it here (at the risk of
> having two newlines) - maybe:

See above - I think you have it the wrong way around - it looks to
me like the old cmn_err() stripped the newline character if it
existed, then added one itself.

> printk(lvl fmt "\n", ## args);

printk() is actually pretty tolerant of missing newlines - if it
detects the previous printk() was not newline terminated and the
next one starts with a log level that is not KERN_CONT, it will
print the new message on a new line automatically. This is the code
in vprintk() that handles it:

        /* Do we have a loglevel in the string? */
        if (p[0] == '<') {
                unsigned char c = p[1];
                if (c && p[2] == '>') {
                        switch (c) {
                        case '0' ... '7': /* loglevel */
                                current_log_level = c - '0';
                        /* Fallthrough - make sure we're on a new line */
                        case 'd': /* KERN_DEFAULT */
                                if (!new_text_line) {
                                        emit_log_char('\n');
                                        new_text_line = 1;
                                }
                        /* Fallthrough - skip the loglevel */
                        case 'c': /* KERN_CONT */
                                p += 3;
                                break;
                        }
                }
        }

So we are probably OK not to need additional newlines. Indeed, I
didn't notice the output being screwed up without them. ;)

I can add them if you want, though.

> 
> > +           BUG_ON(strncmp(lvl, KERN_EMERG, strlen(KERN_EMERG)) == 0); \
> > +   } while (0)
> > +
> > +#define xfs_fs_cmn_err(lvl, mp, fmt, args...)      \
> > +   do { \
> > +           printk(lvl "Filesystem %s: " fmt, (mp)->m_fsname, ## args); \
> 
> printk(lvl "Filesystem %s: " fmt "\n", (mp)->m_fsname, ## args);
> 
> > +           BUG_ON(strncmp(lvl, KERN_EMERG, strlen(KERN_EMERG)) == 0); \
> > +   } while (0)
> > +
> > +/* All callers to xfs_cmn_err use CE_ALERT, so don't bother testing
> > lvl */
> > +#define xfs_cmn_err(panic_tag, lvl, mp, fmt, args...)      \
> > +   do { \
> > +           int     panic = 0; \
> > +           if (xfs_panic_mask && (xfs_panic_mask & panic_tag)) { \
> > +                   printk(KERN_ALERT "XFS: Transforming an alert into a 
> > BUG."); \
> > +                   panic = 1; \
> > +           } \
> > +           printk(KERN_ALERT "Filesystem %s: " fmt, (mp)->m_fsname, ## 
> > args);
> > \
> > +           BUG_ON(panic); \
> > +   } while (0)
> 
> I think we can simplify this case a bit and remove the panic variable,
> like this:
> 
>       do { \
>               printk(KERN_ALERT "Filesystem %s: " fmt "\n", (mp)->m_fsname, 
> ## args);
>               if (xfs_panic_mask && (xfs_panic_mask & panic_tag)) { \
>                       printk(KERN_ALERT "XFS: Transforming an alert into a 
> BUG.\n"); \
>                       BUG_ON(1); \
>               } \
>       } while (0)
> 
> This also reorders the messages which I think makes more sense.

Definitely. That's a vast improvement. ;)

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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