Re: [PATCH 5/9] xfs: make forced shutdown processing atomic

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Jul 14, 2021 at 01:15:24PM +1000, Dave Chinner wrote:
> 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@redhat.com>
> > > 
> > > 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.

Ok.

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

AHA!  That's why that's never seemed to work right for me.

Well, at least the good news is that we each have enough outstanding
patchsets to keep the other busy reviewing until 2028 or so. ;)

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com