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

From: Dave Chinner <david@fromorbit.com>
To: Christoph Hellwig <hch@infradead.org>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 5/9] xfs: make forced shutdown processing atomic
Date: Mon, 5 Jul 2021 11:28:35 +1000	[thread overview]
Message-ID: <20210705012835.GI664593@dread.disaster.area> (raw)
In-Reply-To: <YN7NO5tAn6tVnyIb@infradead.org>

On Fri, Jul 02, 2021 at 09:24:27AM +0100, Christoph Hellwig wrote:
> > +	spin_lock(&mp->m_sb_lock);
> > +	if (XFS_FORCED_SHUTDOWN(mp)) {
> > +		spin_unlock(&mp->m_sb_lock);
> >  		return;
> >  	}
> > +	mp->m_flags |= XFS_MOUNT_FS_SHUTDOWN;
> > +	if (mp->m_sb_bp)
> > +		mp->m_sb_bp->b_flags |= XBF_DONE;
> > +	spin_unlock(&mp->m_sb_lock);
> 
> Any particular reason for picking m_sb_lock which so far doesn't
> seem to be related to mp->m_flags at all? (On which we probably
> have a few other races, most notably remount).

I was just reusing an existing lock rather than having to add yet
another global scope spinlock just for the shutdown. I can add
another lock but...

... as you point out, m_flags is a mess when it comes to races. I've
got a series somewhere that addresses this... Yeah:

https://lore.kernel.org/linux-xfs/20180820044851.414-1-david@fromorbit.com/

And that does similar things bit making state changes atomic as this
patchset does, in which case the lock around this shutdown state
change goes away...

I need to rebase that series and get it moving again, because we
really do need to split m_flags up into features and atomic
operational state, too. In the mean time, I considered using the
m_sb_lock largely harmless...

> > +	if (xfs_error_level >= XFS_ERRLEVEL_HIGH)
> > +		xfs_stack_trace();
> 
> This seems new and unrelated?

It's run on the majority of shutdowns already, but not all. It makes
no sense to have an error level triggered stack dump on shutdown and
not actually use it multiple shutdown vectors - that has been
problematic in the past when trying to diagnose shutdown causes in
the field. I'll add a comment to the commit description.

> > +
> 
> Spurious empty line at the end of the function.

Removed.

-Dave.
-- 
Dave Chinner
david@fromorbit.com