| Message ID | 20220324002103.710477-6-david@fromorbit.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfs: more shutdown/recovery fixes | expand |
On Thu, Mar 24, 2022 at 11:21:02AM +1100, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > When we call xfs_forced_shutdown(), the caller often expects the > filesystem to be completely shut down when it returns. However, > if we have racing xfs_forced_shutdown() calls, the first caller sets > the mount shutdown flag then goes to shutdown the log. The second > caller sees the mount shutdown flag and returns immediately - it > does not wait for the log to be shut down. > > Unfortunately, xfs_forced_shutdown() is used in some places that > expect it to completely shut down the filesystem before it returns > (e.g. xfs_trans_log_inode()). As such, returning before the log has > been shut down leaves us in a place where the transaction failed to > complete correctly but we still call xfs_trans_commit(). This > situation arises because xfs_trans_log_inode() does not return an > error and instead calls xfs_force_shutdown() to ensure that the > transaction being committed is aborted. > > Unfortunately, we have a race condition where xfs_trans_commit() > needs to check xlog_is_shutdown() because it can't abort log items > before the log is shut down, but it needs to use xfs_is_shutdown() > because xfs_forced_shutdown() does not block waiting for the log to > shut down. > > To fix this conundrum, first we make all calls to > xfs_forced_shutdown() block until the log is also shut down. This > means we can then safely use xfs_forced_shutdown() as a mechanism > that ensures the currently running transaction will be aborted by > xfs_trans_commit() regardless of the shutdown check it uses. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> I always feel dirty looking at what __var_waitqueue does, but this does make sense that everything must be shut down by the time xfs_force_shutdown returns. Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > fs/xfs/xfs_fsops.c | 6 +++++- > fs/xfs/xfs_log.c | 1 + > fs/xfs/xfs_log_priv.h | 11 +++++++++++ > 3 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c > index 33e26690a8c4..093a44aecec0 100644 > --- a/fs/xfs/xfs_fsops.c > +++ b/fs/xfs/xfs_fsops.c > @@ -17,6 +17,7 @@ > #include "xfs_fsops.h" > #include "xfs_trans_space.h" > #include "xfs_log.h" > +#include "xfs_log_priv.h" > #include "xfs_ag.h" > #include "xfs_ag_resv.h" > #include "xfs_trace.h" > @@ -528,8 +529,11 @@ xfs_do_force_shutdown( > int tag; > const char *why; > > - if (test_and_set_bit(XFS_OPSTATE_SHUTDOWN, &mp->m_opstate)) > + > + if (test_and_set_bit(XFS_OPSTATE_SHUTDOWN, &mp->m_opstate)) { > + xlog_shutdown_wait(mp->m_log); > return; > + } > if (mp->m_sb_bp) > mp->m_sb_bp->b_flags |= XBF_DONE; > > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c > index 6879d6d19d68..4336eb804a4a 100644 > --- a/fs/xfs/xfs_log.c > +++ b/fs/xfs/xfs_log.c > @@ -3909,6 +3909,7 @@ xlog_force_shutdown( > xlog_state_shutdown_callbacks(log); > spin_unlock(&log->l_icloglock); > > + wake_up_var(&log->l_opstate); > return log_error; > } > > diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h > index 80d77aac6fe4..401cdc400980 100644 > --- a/fs/xfs/xfs_log_priv.h > +++ b/fs/xfs/xfs_log_priv.h > @@ -484,6 +484,17 @@ xlog_is_shutdown(struct xlog *log) > return test_bit(XLOG_IO_ERROR, &log->l_opstate); > } > > +/* > + * Wait until the xlog_force_shutdown() has marked the log as shut down > + * so xlog_is_shutdown() will always return true. > + */ > +static inline void > +xlog_shutdown_wait( > + struct xlog *log) > +{ > + wait_var_event(&log->l_opstate, xlog_is_shutdown(log)); > +} > + > /* common routines */ > extern int > xlog_recover( > -- > 2.35.1 >
diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c index 33e26690a8c4..093a44aecec0 100644 --- a/fs/xfs/xfs_fsops.c +++ b/fs/xfs/xfs_fsops.c @@ -17,6 +17,7 @@ #include "xfs_fsops.h" #include "xfs_trans_space.h" #include "xfs_log.h" +#include "xfs_log_priv.h" #include "xfs_ag.h" #include "xfs_ag_resv.h" #include "xfs_trace.h" @@ -528,8 +529,11 @@ xfs_do_force_shutdown( int tag; const char *why; - if (test_and_set_bit(XFS_OPSTATE_SHUTDOWN, &mp->m_opstate)) + + if (test_and_set_bit(XFS_OPSTATE_SHUTDOWN, &mp->m_opstate)) { + xlog_shutdown_wait(mp->m_log); return; + } if (mp->m_sb_bp) mp->m_sb_bp->b_flags |= XBF_DONE; diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c index 6879d6d19d68..4336eb804a4a 100644 --- a/fs/xfs/xfs_log.c +++ b/fs/xfs/xfs_log.c @@ -3909,6 +3909,7 @@ xlog_force_shutdown( xlog_state_shutdown_callbacks(log); spin_unlock(&log->l_icloglock); + wake_up_var(&log->l_opstate); return log_error; } diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h index 80d77aac6fe4..401cdc400980 100644 --- a/fs/xfs/xfs_log_priv.h +++ b/fs/xfs/xfs_log_priv.h @@ -484,6 +484,17 @@ xlog_is_shutdown(struct xlog *log) return test_bit(XLOG_IO_ERROR, &log->l_opstate); } +/* + * Wait until the xlog_force_shutdown() has marked the log as shut down + * so xlog_is_shutdown() will always return true. + */ +static inline void +xlog_shutdown_wait( + struct xlog *log) +{ + wait_var_event(&log->l_opstate, xlog_is_shutdown(log)); +} + /* common routines */ extern int xlog_recover(