Re: [PATCH v2] xfs: fix iclog release error check race with shutdown

From: Christoph Hellwig <hch@infradead.org>
To: Dave Chinner <david@fromorbit.com>
Cc: Brian Foster <bfoster@redhat.com>, linux-xfs@vger.kernel.org
Subject: Re: [PATCH v2] xfs: fix iclog release error check race with shutdown
Date: Tue, 18 Feb 2020 14:36:44 -0800	[thread overview]
Message-ID: <20200218223644.GA24053@infradead.org> (raw)
In-Reply-To: <20200218215243.GS10776@dread.disaster.area>

On Wed, Feb 19, 2020 at 08:52:43AM +1100, Dave Chinner wrote:
> xfs_force_shutdown() will does nothing if the iclog at the head of
> the log->iclog list is marked with XLOG_STATE_IOERROR before IO is
> submitted. In general, that is the case here as the head iclog is
> what xlog_state_get_iclog_space() returns.
> 
> i.e. XLOG_STATE_IOERROR here implies the log has already been shut
> down because the state is IOERROR rather than ACTIVE or WANT_SYNC as
> was set when the iclog was obtained from
> xlog_state_get_iclog_space().
> 
> > +	if (iclog->ic_state == XLOG_STATE_IOERROR)
> > +		goto error;
> >  
> >  	if (atomic_dec_and_lock(&iclog->ic_refcnt, &log->l_icloglock)) {
> > +		if (iclog->ic_state == XLOG_STATE_IOERROR) {
> > +			spin_unlock(&log->l_icloglock);
> > +			goto error;
> > +		}
> >  		sync = __xlog_state_release_iclog(log, iclog);
> >  		spin_unlock(&log->l_icloglock);
> >  		if (sync)
> >  			xlog_sync(log, iclog);
> >  	}
> >  	return 0;
> > +error:
> > +	xfs_force_shutdown(mp, SHUTDOWN_LOG_IO_ERROR);
> > +	return -EIO;
> 
> Hence I suspect that this should be ASSERT(XLOG_FORCED_SHUTDOWN(log))
> just like is in xfs_log_force_umount() when this pre-existing log
> IO error condition is tripped over...

Indeed, the xfs_force_shutdown is a no-op both for the old and this
new check.

Now the real question, which is a bit out of scope for this patch is
why we even have XLOG_STATE_IOERROR?  Wouldn't it make more sense
to just user the shutdown flag in the mount structure and avoid the
extra state complexity and thus clean up this whole mess?