| Message ID | 20200306143137.236478-8-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/7] xfs: remove the unused return value from xfs_log_unmount_write | expand |
On Fri, Mar 06, 2020 at 07:31:37AM -0700, Christoph Hellwig wrote: > Just check the shutdown flag in struct xlog, instead of replicating the > information into each iclog and checking it there. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- Most of the code seems Ok to me, but as I work through this series I'm starting to have a harder time seeing the value of removing the error state in the current code. Of course there's obvious value of less code and whatnot, but the state code that's being removed is mostly busted code or very simple (i.e. the shutdown helper). In contrast, all of these checks that remain sprinkled throughout the log code change specific iclog checks into broader state checks where we now have to consider what new racing might or might not occur, etc. In the end the fs is busted and we're shutting down, but at the same time shutdown has consistently been riddled with subtle race/locking/state bugs that are low impact but quite annoying to track down. I do see value in simplifying the log error handling overall as the previous patches start to do, but ISTM that should be the primary goal here. IOW, to simplify the bigger picture logic first to the point where this patch should be much, much smaller than it is. If we were to first significantly reduce the number of error state checks required throughout this code (i.e. reduced to the minimum critical points necessary that ensure we don't do more log I/O or other "bad things"), _then_ I see the value of a patch to kill off the error state. Until we get to that point, this kind of strikes me as rejiggering complexity around. For example, things like how xlog_state_do_callback() passes ioerror to xlog_state_iodone_process_iclog(), which assigns it based on shutdown state, only for the caller to also check the shutdown state again are indication that more cleanup is in order before killing off the state. Brian > fs/xfs/xfs_log.c | 95 +++++++++++++++---------------------------- > fs/xfs/xfs_log_cil.c | 2 +- > fs/xfs/xfs_log_priv.h | 1 - > 3 files changed, 34 insertions(+), 64 deletions(-) > > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c > index fae5107099b1..1bcd5c735d6b 100644 > --- a/fs/xfs/xfs_log.c > +++ b/fs/xfs/xfs_log.c > @@ -583,7 +583,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) && > @@ -604,11 +604,11 @@ xfs_log_release_iclog( > struct xlog *log = mp->m_log; > bool sync; > > - if (iclog->ic_state == XLOG_STATE_IOERROR) > + if (XLOG_FORCED_SHUTDOWN(log)) > goto error; > > if (atomic_dec_and_lock(&iclog->ic_refcnt, &log->l_icloglock)) { > - if (iclog->ic_state == XLOG_STATE_IOERROR) { > + if (XLOG_FORCED_SHUTDOWN(log)) { > spin_unlock(&log->l_icloglock); > goto error; > } > @@ -914,7 +914,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) > @@ -1737,7 +1737,7 @@ xlog_write_iclog( > * across the log IO to archieve that. > */ > down(&iclog->ic_sema); > - if (unlikely(iclog->ic_state == XLOG_STATE_IOERROR)) { > + if (unlikely(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 > @@ -2721,6 +2721,17 @@ xlog_state_iodone_process_iclog( > xfs_lsn_t lowest_lsn; > xfs_lsn_t header_lsn; > > + /* > + * Between marking a filesystem SHUTDOWN and stopping the log, we do > + * flush all iclogs to disk (if there wasn't a log I/O error). So, we > + * do want things to go smoothly in case of just a SHUTDOWN w/o a > + * LOG_IO_ERROR. > + */ > + if (XLOG_FORCED_SHUTDOWN(log)) { > + *ioerror = true; > + return false; > + } > + > switch (iclog->ic_state) { > case XLOG_STATE_ACTIVE: > case XLOG_STATE_DIRTY: > @@ -2728,15 +2739,6 @@ xlog_state_iodone_process_iclog( > * Skip all iclogs in the ACTIVE & DIRTY states: > */ > return false; > - case XLOG_STATE_IOERROR: > - /* > - * Between marking a filesystem SHUTDOWN and stopping the log, > - * we do flush all iclogs to disk (if there wasn't a log I/O > - * error). So, we do want things to go smoothly in case of just > - * a SHUTDOWN w/o a LOG_IO_ERROR. > - */ > - *ioerror = true; > - return false; > case XLOG_STATE_DONE_SYNC: > /* > * Now that we have an iclog that is in the DONE_SYNC state, do > @@ -2830,7 +2832,7 @@ xlog_state_do_callback( > break; > > if (iclog->ic_state != XLOG_STATE_CALLBACK && > - iclog->ic_state != XLOG_STATE_IOERROR) { > + !XLOG_FORCED_SHUTDOWN(log)) { > iclog = iclog->ic_next; > continue; > } > @@ -2856,7 +2858,7 @@ xlog_state_do_callback( > } while (!ioerror && cycled_icloglock); > > if (log->l_iclog->ic_state == XLOG_STATE_ACTIVE || > - log->l_iclog->ic_state == XLOG_STATE_IOERROR) > + XLOG_FORCED_SHUTDOWN(log)) > wake_up_all(&log->l_flush_wait); > > spin_unlock(&log->l_icloglock); > @@ -3202,7 +3204,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 || > @@ -3259,11 +3261,11 @@ xfs_log_force( > if (!(flags & XFS_LOG_SYNC)) > goto out_unlock; > > - if (iclog->ic_state == XLOG_STATE_IOERROR) > + if (XLOG_FORCED_SHUTDOWN(log)) > goto out_error; > XFS_STATS_INC(mp, xs_log_force_sleep); > xlog_wait(&iclog->ic_force_wait, &log->l_icloglock); > - if (iclog->ic_state == XLOG_STATE_IOERROR) > + if (XLOG_FORCED_SHUTDOWN(log)) > return -EIO; > return 0; > > @@ -3288,7 +3290,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) { > @@ -3338,12 +3340,12 @@ __xfs_log_force_lsn( > iclog->ic_state == XLOG_STATE_DIRTY)) > goto out_unlock; > > - if (iclog->ic_state == XLOG_STATE_IOERROR) > + if (XLOG_FORCED_SHUTDOWN(log)) > goto out_error; > > XFS_STATS_INC(mp, xs_log_force_sleep); > xlog_wait(&iclog->ic_force_wait, &log->l_icloglock); > - if (iclog->ic_state == XLOG_STATE_IOERROR) > + if (XLOG_FORCED_SHUTDOWN(log)) > return -EIO; > return 0; > > @@ -3407,7 +3409,7 @@ xlog_state_want_sync( > xlog_state_switch_iclogs(log, iclog, 0); > } else { > ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC || > - iclog->ic_state == XLOG_STATE_IOERROR); > + XLOG_FORCED_SHUTDOWN(log)); > } > } > > @@ -3774,34 +3776,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. > @@ -3823,10 +3797,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 > @@ -3844,10 +3816,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 > @@ -3869,11 +3839,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); > > /* > @@ -3897,7 +3869,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 b5c4a45c208c..41a45d75a2d0 100644 > --- a/fs/xfs/xfs_log_cil.c > +++ b/fs/xfs/xfs_log_cil.c > @@ -846,7 +846,7 @@ xlog_cil_push( > 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 b192c5a9f9fd..fd4c913ee7e6 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 Fri, Mar 06, 2020 at 12:15:45PM -0500, Brian Foster wrote: > If we were to first significantly reduce the number of error state > checks required throughout this code (i.e. reduced to the minimum > critical points necessary that ensure we don't do more log I/O or other > "bad things"), _then_ I see the value of a patch to kill off the error > state. Until we get to that point, this kind of strikes me as > rejiggering complexity around. For example, things like how > xlog_state_do_callback() passes ioerror to > xlog_state_iodone_process_iclog(), which assigns it based on shutdown > state, only for the caller to also check the shutdown state again are > indication that more cleanup is in order before killing off the state. I've added a few more patches fixing up some low hanging fruit and completely redoing the code structure around xlog_state_iodone_process_iclog. Although that means the series keeps growing, so I might split it into multiple series with some easier prep patches and then the bigger changes in a second round.
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c index fae5107099b1..1bcd5c735d6b 100644 --- a/fs/xfs/xfs_log.c +++ b/fs/xfs/xfs_log.c @@ -583,7 +583,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) && @@ -604,11 +604,11 @@ xfs_log_release_iclog( struct xlog *log = mp->m_log; bool sync; - if (iclog->ic_state == XLOG_STATE_IOERROR) + if (XLOG_FORCED_SHUTDOWN(log)) goto error; if (atomic_dec_and_lock(&iclog->ic_refcnt, &log->l_icloglock)) { - if (iclog->ic_state == XLOG_STATE_IOERROR) { + if (XLOG_FORCED_SHUTDOWN(log)) { spin_unlock(&log->l_icloglock); goto error; } @@ -914,7 +914,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) @@ -1737,7 +1737,7 @@ xlog_write_iclog( * across the log IO to archieve that. */ down(&iclog->ic_sema); - if (unlikely(iclog->ic_state == XLOG_STATE_IOERROR)) { + if (unlikely(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 @@ -2721,6 +2721,17 @@ xlog_state_iodone_process_iclog( xfs_lsn_t lowest_lsn; xfs_lsn_t header_lsn; + /* + * Between marking a filesystem SHUTDOWN and stopping the log, we do + * flush all iclogs to disk (if there wasn't a log I/O error). So, we + * do want things to go smoothly in case of just a SHUTDOWN w/o a + * LOG_IO_ERROR. + */ + if (XLOG_FORCED_SHUTDOWN(log)) { + *ioerror = true; + return false; + } + switch (iclog->ic_state) { case XLOG_STATE_ACTIVE: case XLOG_STATE_DIRTY: @@ -2728,15 +2739,6 @@ xlog_state_iodone_process_iclog( * Skip all iclogs in the ACTIVE & DIRTY states: */ return false; - case XLOG_STATE_IOERROR: - /* - * Between marking a filesystem SHUTDOWN and stopping the log, - * we do flush all iclogs to disk (if there wasn't a log I/O - * error). So, we do want things to go smoothly in case of just - * a SHUTDOWN w/o a LOG_IO_ERROR. - */ - *ioerror = true; - return false; case XLOG_STATE_DONE_SYNC: /* * Now that we have an iclog that is in the DONE_SYNC state, do @@ -2830,7 +2832,7 @@ xlog_state_do_callback( break; if (iclog->ic_state != XLOG_STATE_CALLBACK && - iclog->ic_state != XLOG_STATE_IOERROR) { + !XLOG_FORCED_SHUTDOWN(log)) { iclog = iclog->ic_next; continue; } @@ -2856,7 +2858,7 @@ xlog_state_do_callback( } while (!ioerror && cycled_icloglock); if (log->l_iclog->ic_state == XLOG_STATE_ACTIVE || - log->l_iclog->ic_state == XLOG_STATE_IOERROR) + XLOG_FORCED_SHUTDOWN(log)) wake_up_all(&log->l_flush_wait); spin_unlock(&log->l_icloglock); @@ -3202,7 +3204,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 || @@ -3259,11 +3261,11 @@ xfs_log_force( if (!(flags & XFS_LOG_SYNC)) goto out_unlock; - if (iclog->ic_state == XLOG_STATE_IOERROR) + if (XLOG_FORCED_SHUTDOWN(log)) goto out_error; XFS_STATS_INC(mp, xs_log_force_sleep); xlog_wait(&iclog->ic_force_wait, &log->l_icloglock); - if (iclog->ic_state == XLOG_STATE_IOERROR) + if (XLOG_FORCED_SHUTDOWN(log)) return -EIO; return 0; @@ -3288,7 +3290,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) { @@ -3338,12 +3340,12 @@ __xfs_log_force_lsn( iclog->ic_state == XLOG_STATE_DIRTY)) goto out_unlock; - if (iclog->ic_state == XLOG_STATE_IOERROR) + if (XLOG_FORCED_SHUTDOWN(log)) goto out_error; XFS_STATS_INC(mp, xs_log_force_sleep); xlog_wait(&iclog->ic_force_wait, &log->l_icloglock); - if (iclog->ic_state == XLOG_STATE_IOERROR) + if (XLOG_FORCED_SHUTDOWN(log)) return -EIO; return 0; @@ -3407,7 +3409,7 @@ xlog_state_want_sync( xlog_state_switch_iclogs(log, iclog, 0); } else { ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC || - iclog->ic_state == XLOG_STATE_IOERROR); + XLOG_FORCED_SHUTDOWN(log)); } } @@ -3774,34 +3776,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. @@ -3823,10 +3797,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 @@ -3844,10 +3816,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 @@ -3869,11 +3839,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); /* @@ -3897,7 +3869,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 b5c4a45c208c..41a45d75a2d0 100644 --- a/fs/xfs/xfs_log_cil.c +++ b/fs/xfs/xfs_log_cil.c @@ -846,7 +846,7 @@ xlog_cil_push( 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 b192c5a9f9fd..fd4c913ee7e6 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. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/xfs_log.c | 95 +++++++++++++++---------------------------- fs/xfs/xfs_log_cil.c | 2 +- fs/xfs/xfs_log_priv.h | 1 - 3 files changed, 34 insertions(+), 64 deletions(-)