On Tue, Feb 18, 2020 at 02:36:44PM -0800, Christoph Hellwig wrote:
> 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?
I _think_ it was originally intended to prevent log shutdown
recursion when shutdowns trigger log IO errors and try to shut down
again.
> 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?
I'd suggest that XLOG_FORCED_SHUTDOWN() is more appropriate in code
that has no reason to know anything about the xfs_mount state e.g.
the code in xlog_state_release_iclog() has a log and iclog context
and introducing a xfs-mount context to check for shutdown is a
fairly significant layering violation deep inside the internal log
implementation...
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
