Re: [PATCH 5/6] xfs: xfs_do_force_shutdown needs to block racing shutdowns

["Darrick J. Wong" <djwong@kernel.org>]

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
>