| Message ID | 20200316144233.900390-15-hch@lst.de (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | [01/14] xfs: merge xlog_cil_push into xlog_cil_push_work | expand |
On Mon, Mar 16, 2020 at 03:42:33PM +0100, Christoph Hellwig wrote: > Just check the shutdown flag in struct xlog, instead of replicating the > information into each iclog and checking it there. > > As the iclog state is now not overload with the shut down information > various places that check for a specific state now don't need to account > for the fake IOERROR state. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/xfs_log.c | 68 +++++++++++-------------------------------- > fs/xfs/xfs_log_cil.c | 2 +- > fs/xfs/xfs_log_priv.h | 1 - > 3 files changed, 18 insertions(+), 53 deletions(-) > > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c > index 761b138d97ec..07023372ccbd 100644 > --- a/fs/xfs/xfs_log.c > +++ b/fs/xfs/xfs_log.c > @@ -578,7 +578,7 @@ xlog_state_release_iclog( > { > lockdep_assert_held(&log->l_icloglock); > > - if (iclog->ic_state == XLOG_STATE_IOERROR) > + if (XLOG_FORCED_SHUTDOWN(log)) > return -EIO; > > if (atomic_dec_and_test(&iclog->ic_refcnt) && > @@ -599,7 +599,7 @@ xfs_log_release_iclog( > bool sync = false; > > if (atomic_dec_and_lock(&iclog->ic_refcnt, &log->l_icloglock)) { > - if (iclog->ic_state != XLOG_STATE_IOERROR) > + if (!XLOG_FORCED_SHUTDOWN(log)) > sync = __xlog_state_release_iclog(log, iclog); > spin_unlock(&log->l_icloglock); > } > @@ -924,7 +924,7 @@ xfs_log_write_unmount_record( > error = xlog_write(log, &vec, tic, &lsn, NULL, flags); > /* > * At this point, we're umounting anyway, so there's no point in > - * transitioning log state to IOERROR. Just continue... > + * transitioning log state to IO_ERROR. Just continue... "...so there's no point in marking the log as shut down."? There's no IOERROR state anymore, right? --D > */ > out_err: > if (error) > @@ -936,8 +936,7 @@ xfs_log_write_unmount_record( > if (iclog->ic_state == XLOG_STATE_ACTIVE) > xlog_state_switch_iclogs(log, iclog, 0); > else > - ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC || > - iclog->ic_state == XLOG_STATE_IOERROR); > + ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC); > error = xlog_state_release_iclog(log, iclog); > xlog_wait_on_iclog(iclog); > > @@ -1740,7 +1739,7 @@ xlog_write_iclog( > * across the log IO to archieve that. > */ > down(&iclog->ic_sema); > - if (unlikely(iclog->ic_state == XLOG_STATE_IOERROR)) { > + if (XLOG_FORCED_SHUTDOWN(log)) { > /* > * It would seem logical to return EIO here, but we rely on > * the log state machine to propagate I/O errors instead of > @@ -2295,8 +2294,7 @@ xlog_write_copy_finish( > if (iclog->ic_state == XLOG_STATE_ACTIVE) > xlog_state_switch_iclogs(log, iclog, 0); > else > - ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC || > - iclog->ic_state == XLOG_STATE_IOERROR); > + ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC); > if (!commit_iclog) > goto release_iclog; > spin_unlock(&log->l_icloglock); > @@ -2817,8 +2815,7 @@ xlog_state_do_callback( > } > } while (cycled_icloglock); > > - if (log->l_iclog->ic_state == XLOG_STATE_ACTIVE || > - log->l_iclog->ic_state == XLOG_STATE_IOERROR) > + if (log->l_iclog->ic_state == XLOG_STATE_ACTIVE) > wake_up_all(&log->l_flush_wait); > > spin_unlock(&log->l_icloglock); > @@ -3167,7 +3164,7 @@ xfs_log_force( > > spin_lock(&log->l_icloglock); > iclog = log->l_iclog; > - if (iclog->ic_state == XLOG_STATE_IOERROR) > + if (XLOG_FORCED_SHUTDOWN(log)) > goto out_error; > > if (iclog->ic_state == XLOG_STATE_DIRTY || > @@ -3240,7 +3237,7 @@ __xfs_log_force_lsn( > > spin_lock(&log->l_icloglock); > iclog = log->l_iclog; > - if (iclog->ic_state == XLOG_STATE_IOERROR) > + if (XLOG_FORCED_SHUTDOWN(log)) > goto out_error; > > while (be64_to_cpu(iclog->ic_header.h_lsn) != lsn) { > @@ -3691,34 +3688,6 @@ xlog_verify_iclog( > } /* xlog_verify_iclog */ > #endif > > -/* > - * Mark all iclogs IOERROR. l_icloglock is held by the caller. > - */ > -STATIC int > -xlog_state_ioerror( > - struct xlog *log) > -{ > - xlog_in_core_t *iclog, *ic; > - > - iclog = log->l_iclog; > - if (iclog->ic_state != XLOG_STATE_IOERROR) { > - /* > - * Mark all the incore logs IOERROR. > - * From now on, no log flushes will result. > - */ > - ic = iclog; > - do { > - ic->ic_state = XLOG_STATE_IOERROR; > - ic = ic->ic_next; > - } while (ic != iclog); > - return 0; > - } > - /* > - * Return non-zero, if state transition has already happened. > - */ > - return 1; > -} > - > /* > * This is called from xfs_force_shutdown, when we're forcibly > * shutting down the filesystem, typically because of an IO error. > @@ -3740,10 +3709,8 @@ xfs_log_force_umount( > struct xfs_mount *mp, > int logerror) > { > - struct xlog *log; > - int retval; > - > - log = mp->m_log; > + struct xlog *log = mp->m_log; > + int retval = 0; > > /* > * If this happens during log recovery, don't worry about > @@ -3761,10 +3728,8 @@ xfs_log_force_umount( > * Somebody could've already done the hard work for us. > * No need to get locks for this. > */ > - if (logerror && log->l_iclog->ic_state == XLOG_STATE_IOERROR) { > - ASSERT(XLOG_FORCED_SHUTDOWN(log)); > + if (logerror && XLOG_FORCED_SHUTDOWN(log)) > return 1; > - } > > /* > * Flush all the completed transactions to disk before marking the log > @@ -3786,11 +3751,13 @@ xfs_log_force_umount( > mp->m_sb_bp->b_flags |= XBF_DONE; > > /* > - * Mark the log and the iclogs with IO error flags to prevent any > - * further log IO from being issued or completed. > + * Mark the log as shut down to prevent any further log IO from being > + * issued or completed. Return non-zero if log IO_ERROR transition had > + * already happened so that the caller can skip further processing. > */ > + if (XLOG_FORCED_SHUTDOWN(log)) > + retval = 1; > log->l_flags |= XLOG_IO_ERROR; > - retval = xlog_state_ioerror(log); > spin_unlock(&log->l_icloglock); > > /* > @@ -3814,7 +3781,6 @@ xfs_log_force_umount( > spin_unlock(&log->l_cilp->xc_push_lock); > xlog_state_do_callback(log); > > - /* return non-zero if log IOERROR transition had already happened */ > return retval; > } > > diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c > index adc4af336c97..a93097cf0990 100644 > --- a/fs/xfs/xfs_log_cil.c > +++ b/fs/xfs/xfs_log_cil.c > @@ -845,7 +845,7 @@ xlog_cil_push_work( > goto out_abort; > > spin_lock(&commit_iclog->ic_callback_lock); > - if (commit_iclog->ic_state == XLOG_STATE_IOERROR) { > + if (XLOG_FORCED_SHUTDOWN(log)) { > spin_unlock(&commit_iclog->ic_callback_lock); > goto out_abort; > } > diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h > index 2b0aec37e73e..f3f4a35ce153 100644 > --- a/fs/xfs/xfs_log_priv.h > +++ b/fs/xfs/xfs_log_priv.h > @@ -47,7 +47,6 @@ enum xlog_iclog_state { > XLOG_STATE_DONE_SYNC, /* Done syncing to disk */ > XLOG_STATE_CALLBACK, /* Callback functions now */ > XLOG_STATE_DIRTY, /* Dirty IC log, not ready for ACTIVE status */ > - XLOG_STATE_IOERROR, /* IO error happened in sync'ing log */ > }; > > /* > -- > 2.24.1 >
On Mon, Mar 16, 2020 at 02:25:39PM -0700, Darrick J. Wong wrote: > > * At this point, we're umounting anyway, so there's no point in > > - * transitioning log state to IOERROR. Just continue... > > + * transitioning log state to IO_ERROR. Just continue... > > "...so there's no point in marking the log as shut down."? > > There's no IOERROR state anymore, right? There is on the log itself: log->l_flags |= XLOG_IO_ERROR; but I think your version is nicer anyway.
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c index 761b138d97ec..07023372ccbd 100644 --- a/fs/xfs/xfs_log.c +++ b/fs/xfs/xfs_log.c @@ -578,7 +578,7 @@ xlog_state_release_iclog( { lockdep_assert_held(&log->l_icloglock); - if (iclog->ic_state == XLOG_STATE_IOERROR) + if (XLOG_FORCED_SHUTDOWN(log)) return -EIO; if (atomic_dec_and_test(&iclog->ic_refcnt) && @@ -599,7 +599,7 @@ xfs_log_release_iclog( bool sync = false; if (atomic_dec_and_lock(&iclog->ic_refcnt, &log->l_icloglock)) { - if (iclog->ic_state != XLOG_STATE_IOERROR) + if (!XLOG_FORCED_SHUTDOWN(log)) sync = __xlog_state_release_iclog(log, iclog); spin_unlock(&log->l_icloglock); } @@ -924,7 +924,7 @@ xfs_log_write_unmount_record( error = xlog_write(log, &vec, tic, &lsn, NULL, flags); /* * At this point, we're umounting anyway, so there's no point in - * transitioning log state to IOERROR. Just continue... + * transitioning log state to IO_ERROR. Just continue... */ out_err: if (error) @@ -936,8 +936,7 @@ xfs_log_write_unmount_record( if (iclog->ic_state == XLOG_STATE_ACTIVE) xlog_state_switch_iclogs(log, iclog, 0); else - ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC || - iclog->ic_state == XLOG_STATE_IOERROR); + ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC); error = xlog_state_release_iclog(log, iclog); xlog_wait_on_iclog(iclog); @@ -1740,7 +1739,7 @@ xlog_write_iclog( * across the log IO to archieve that. */ down(&iclog->ic_sema); - if (unlikely(iclog->ic_state == XLOG_STATE_IOERROR)) { + if (XLOG_FORCED_SHUTDOWN(log)) { /* * It would seem logical to return EIO here, but we rely on * the log state machine to propagate I/O errors instead of @@ -2295,8 +2294,7 @@ xlog_write_copy_finish( if (iclog->ic_state == XLOG_STATE_ACTIVE) xlog_state_switch_iclogs(log, iclog, 0); else - ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC || - iclog->ic_state == XLOG_STATE_IOERROR); + ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC); if (!commit_iclog) goto release_iclog; spin_unlock(&log->l_icloglock); @@ -2817,8 +2815,7 @@ xlog_state_do_callback( } } while (cycled_icloglock); - if (log->l_iclog->ic_state == XLOG_STATE_ACTIVE || - log->l_iclog->ic_state == XLOG_STATE_IOERROR) + if (log->l_iclog->ic_state == XLOG_STATE_ACTIVE) wake_up_all(&log->l_flush_wait); spin_unlock(&log->l_icloglock); @@ -3167,7 +3164,7 @@ xfs_log_force( spin_lock(&log->l_icloglock); iclog = log->l_iclog; - if (iclog->ic_state == XLOG_STATE_IOERROR) + if (XLOG_FORCED_SHUTDOWN(log)) goto out_error; if (iclog->ic_state == XLOG_STATE_DIRTY || @@ -3240,7 +3237,7 @@ __xfs_log_force_lsn( spin_lock(&log->l_icloglock); iclog = log->l_iclog; - if (iclog->ic_state == XLOG_STATE_IOERROR) + if (XLOG_FORCED_SHUTDOWN(log)) goto out_error; while (be64_to_cpu(iclog->ic_header.h_lsn) != lsn) { @@ -3691,34 +3688,6 @@ xlog_verify_iclog( } /* xlog_verify_iclog */ #endif -/* - * Mark all iclogs IOERROR. l_icloglock is held by the caller. - */ -STATIC int -xlog_state_ioerror( - struct xlog *log) -{ - xlog_in_core_t *iclog, *ic; - - iclog = log->l_iclog; - if (iclog->ic_state != XLOG_STATE_IOERROR) { - /* - * Mark all the incore logs IOERROR. - * From now on, no log flushes will result. - */ - ic = iclog; - do { - ic->ic_state = XLOG_STATE_IOERROR; - ic = ic->ic_next; - } while (ic != iclog); - return 0; - } - /* - * Return non-zero, if state transition has already happened. - */ - return 1; -} - /* * This is called from xfs_force_shutdown, when we're forcibly * shutting down the filesystem, typically because of an IO error. @@ -3740,10 +3709,8 @@ xfs_log_force_umount( struct xfs_mount *mp, int logerror) { - struct xlog *log; - int retval; - - log = mp->m_log; + struct xlog *log = mp->m_log; + int retval = 0; /* * If this happens during log recovery, don't worry about @@ -3761,10 +3728,8 @@ xfs_log_force_umount( * Somebody could've already done the hard work for us. * No need to get locks for this. */ - if (logerror && log->l_iclog->ic_state == XLOG_STATE_IOERROR) { - ASSERT(XLOG_FORCED_SHUTDOWN(log)); + if (logerror && XLOG_FORCED_SHUTDOWN(log)) return 1; - } /* * Flush all the completed transactions to disk before marking the log @@ -3786,11 +3751,13 @@ xfs_log_force_umount( mp->m_sb_bp->b_flags |= XBF_DONE; /* - * Mark the log and the iclogs with IO error flags to prevent any - * further log IO from being issued or completed. + * Mark the log as shut down to prevent any further log IO from being + * issued or completed. Return non-zero if log IO_ERROR transition had + * already happened so that the caller can skip further processing. */ + if (XLOG_FORCED_SHUTDOWN(log)) + retval = 1; log->l_flags |= XLOG_IO_ERROR; - retval = xlog_state_ioerror(log); spin_unlock(&log->l_icloglock); /* @@ -3814,7 +3781,6 @@ xfs_log_force_umount( spin_unlock(&log->l_cilp->xc_push_lock); xlog_state_do_callback(log); - /* return non-zero if log IOERROR transition had already happened */ return retval; } diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c index adc4af336c97..a93097cf0990 100644 --- a/fs/xfs/xfs_log_cil.c +++ b/fs/xfs/xfs_log_cil.c @@ -845,7 +845,7 @@ xlog_cil_push_work( goto out_abort; spin_lock(&commit_iclog->ic_callback_lock); - if (commit_iclog->ic_state == XLOG_STATE_IOERROR) { + if (XLOG_FORCED_SHUTDOWN(log)) { spin_unlock(&commit_iclog->ic_callback_lock); goto out_abort; } diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h index 2b0aec37e73e..f3f4a35ce153 100644 --- a/fs/xfs/xfs_log_priv.h +++ b/fs/xfs/xfs_log_priv.h @@ -47,7 +47,6 @@ enum xlog_iclog_state { XLOG_STATE_DONE_SYNC, /* Done syncing to disk */ XLOG_STATE_CALLBACK, /* Callback functions now */ XLOG_STATE_DIRTY, /* Dirty IC log, not ready for ACTIVE status */ - XLOG_STATE_IOERROR, /* IO error happened in sync'ing log */ }; /*
Just check the shutdown flag in struct xlog, instead of replicating the information into each iclog and checking it there. As the iclog state is now not overload with the shut down information various places that check for a specific state now don't need to account for the fake IOERROR state. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/xfs_log.c | 68 +++++++++++-------------------------------- fs/xfs/xfs_log_cil.c | 2 +- fs/xfs/xfs_log_priv.h | 1 - 3 files changed, 18 insertions(+), 53 deletions(-)