On Thu, Jul 08, 2021 at 09:40:20PM -0700, Darrick J. Wong wrote:
> On Wed, Jun 30, 2021 at 04:38:09PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> >
> > The running of a forced shutdown is a bit of a mess. It does racy
> > checks for XFS_MOUNT_SHUTDOWN in xfs_do_force_shutdown(), then
> > does more racy checks in xfs_log_force_unmount() before finally
> > setting XFS_MOUNT_SHUTDOWN and XLOG_IO_ERROR under the
> > log->icloglock.
> >
> > Move the checking and setting of XFS_MOUNT_SHUTDOWN into
> > xfs_do_force_shutdown() so we only process a shutdown once and once
> > only. Serialise this with the mp->m_sb_lock spinlock so that the
> > state change is atomic and won't race. Move all the mount specific
>
> Assuming you're working on cleaning /that/ up too, I'll let that go...
Yes, a forward ported patch set that does this will be posted soon.
> > + xfs_alert_tag(mp, tag,
> > +"%s (0x%x) detected at %pS (%s:%d). Shutting down filesystem.",
> > + why, flags, __return_address, fname, lnnum);
> > xfs_alert(mp,
> > "Please unmount the filesystem and rectify the problem(s)");
> > + if (xfs_error_level >= XFS_ERRLEVEL_HIGH)
> > + xfs_stack_trace();
>
> Doesn't xfs_alert already drop a stack trace for xfs_error_level >=
> XFS_ERRLEVEL_HIGH ?
It does? I've never seen it do that, and the existing code implies
it doesn't do this, either, and that's the logic was looking at
here:
if (flags & SHUTDOWN_CORRUPT_INCORE) {
xfs_alert_tag(mp, XFS_PTAG_SHUTDOWN_CORRUPT,
"Corruption of in-memory data (0x%x) detected at %pS (%s:%d). Shutting down filesystem",
flags, __return_address, fname, lnnum);
if (XFS_ERRLEVEL_HIGH <= xfs_error_level)
xfs_stack_trace();
} else if (....)
Yes, xfs_alert_tag() does not trigger a stack trace at all, but
there's an unconditional xfs_alert() call after this so if that
issues stack traces then we'd get a double stack trace on all
SHUTDOWN_CORRUPT_INCORE incidents. AFAICT, that doesn't actually
happen....
This pattern is repeated in several places - look at
xfs_inode_verifier_error(), xfs_buf_verifier_error(), and
xfs_buf_corruption_error(). They all have xfs_alert() calls, then
follow it up with a specific error level check for a stack dump.
Hmmm, it looks like xfs_alert() was intended to dump stacks, but I
don't think it works:
if (!kstrtoint(kern_level, 0, &level) && \
level <= LOGLEVEL_ERR && \
xfs_error_level >= XFS_ERRLEVEL_HIGH) \
xfs_stack_trace(); \
And kern_level is KERN_ALERT, which is:
#define KERN_SOH "\001"
....
#define KERN_ALERT KERN_SOH "1"
And:
#define LOGLEVEL_ERR 3 /* error conditions */
So what does kstrtoint() return when passed the string "\0011"? It's
not actually an integer string...
Hmmm, I think it returns -EINVAL, which means it then uses level
uninitialised, and the result is .... unpredictable it is likely
no stack trace is emitted....
Fixing this mess is out of scope for this patchset. The changes in
this patchset don't change the existing pattern of the function of
unconditionally calling xfs_alert() and conditionally and explicitly
dumping stack traces manually. I'll add it to my ever growing list
of cleanups that need to be done...
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
