Skip to content

Commit

Permalink
xfs: log shutdown triggers should only shut down the log
Browse files Browse the repository at this point in the history
We've got a mess on our hands.

1. xfs_trans_commit() cannot cancel transactions because the mount is
shut down - that causes dirty, aborted, unlogged log items to sit
unpinned in memory and potentially get written to disk before the
log is shut down. Hence xfs_trans_commit() can only abort
transactions when xlog_is_shutdown() is true.

2. xfs_force_shutdown() is used in places to cause the current
modification to be aborted via xfs_trans_commit() because it may be
impractical or impossible to cancel the transaction directly, and
hence xfs_trans_commit() must cancel transactions when
xfs_is_shutdown() is true in this situation. But we can't do that
because of #1.

3. Log IO errors cause log shutdowns by calling xfs_force_shutdown()
to shut down the mount and then the log from log IO completion.

4. xfs_force_shutdown() can result in a log force being issued,
which has to wait for log IO completion before it will mark the log
as shut down. If #3 races with some other shutdown trigger that runs
a log force, we rely on xfs_force_shutdown() silently ignoring #3
and avoiding shutting down the log until the failed log force
completes.

5. To ensure #2 always works, we have to ensure that
xfs_force_shutdown() does not return until the the log is shut down.
But in the case of #4, this will result in a deadlock because the
log Io completion will block waiting for a log force to complete
which is blocked waiting for log IO to complete....

So the very first thing we have to do here to untangle this mess is
dissociate log shutdown triggers from mount shutdowns. We already
have xlog_forced_shutdown, which will atomically transistion to the
log a shutdown state. Due to internal asserts it cannot be called
multiple times, but was done simply because the only place that
could call it was xfs_do_force_shutdown() (i.e. the mount shutdown!)
and that could only call it once and once only.  So the first thing
we do is remove the asserts.

We then convert all the internal log shutdown triggers to call
xlog_force_shutdown() directly instead of xfs_force_shutdown(). This
allows the log shutdown triggers to shut down the log without
needing to care about mount based shutdown constraints. This means
we shut down the log independently of the mount and the mount may
not notice this until it's next attempt to read or modify metadata.
At that point (e.g. xfs_trans_commit()) it will see that the log is
shutdown, error out and shutdown the mount.

To ensure that all the unmount behaviours and asserts track
correctly as a result of a log shutdown, propagate the shutdown up
to the mount if it is not already set. This keeps the mount and log
state in sync, and saves a huge amount of hassle where code fails
because of a log shutdown but only checks for mount shutdowns and
hence ends up doing the wrong thing. Cleaning up that mess is
an exercise for another day.

This enables us to address the other problems noted above in
followup patches.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
  • Loading branch information
Dave Chinner authored and Darrick J. Wong committed Mar 30, 2022
1 parent cd6f79d commit b5f17be
Show file tree
Hide file tree
Showing 5 changed files with 33 additions and 18 deletions.
32 changes: 23 additions & 9 deletions fs/xfs/xfs_log.c
Original file line number Diff line number Diff line change
Expand Up @@ -1374,7 +1374,7 @@ xlog_ioend_work(
*/
if (XFS_TEST_ERROR(error, log->l_mp, XFS_ERRTAG_IODONE_IOERR)) {
xfs_alert(log->l_mp, "log I/O error %d", error);
xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
xlog_force_shutdown(log, SHUTDOWN_LOG_IO_ERROR);
}

xlog_state_done_syncing(iclog);
Expand Down Expand Up @@ -1913,7 +1913,7 @@ xlog_write_iclog(
iclog->ic_flags &= ~(XLOG_ICL_NEED_FLUSH | XLOG_ICL_NEED_FUA);

if (xlog_map_iclog_data(&iclog->ic_bio, iclog->ic_data, count)) {
xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
xlog_force_shutdown(log, SHUTDOWN_LOG_IO_ERROR);
return;
}
if (is_vmalloc_addr(iclog->ic_data))
Expand Down Expand Up @@ -2488,7 +2488,7 @@ xlog_write(
xfs_alert_tag(log->l_mp, XFS_PTAG_LOGRES,
"ctx ticket reservation ran out. Need to up reservation");
xlog_print_tic_res(log->l_mp, ticket);
xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
xlog_force_shutdown(log, SHUTDOWN_LOG_IO_ERROR);
}

len = xlog_write_calc_vec_length(ticket, log_vector, optype);
Expand Down Expand Up @@ -3822,9 +3822,10 @@ xlog_verify_iclog(
#endif

/*
* Perform a forced shutdown on the log. This should be called once and once
* only by the high level filesystem shutdown code to shut the log subsystem
* down cleanly.
* Perform a forced shutdown on the log.
*
* This can be called from low level log code to trigger a shutdown, or from the
* high level mount shutdown code when the mount shuts down.
*
* Our main objectives here are to make sure that:
* a. if the shutdown was not due to a log IO error, flush the logs to
Expand All @@ -3833,6 +3834,8 @@ xlog_verify_iclog(
* parties to find out. Nothing new gets queued after this is done.
* c. Tasks sleeping on log reservations, pinned objects and
* other resources get woken up.
* d. The mount is also marked as shut down so that log triggered shutdowns
* still behave the same as if they called xfs_forced_shutdown().
*
* Return true if the shutdown cause was a log IO error and we actually shut the
* log down.
Expand All @@ -3851,8 +3854,6 @@ xlog_force_shutdown(
if (!log || xlog_in_recovery(log))
return false;

ASSERT(!xlog_is_shutdown(log));

/*
* Flush all the completed transactions to disk before marking the log
* being shut down. We need to do this first as shutting down the log
Expand All @@ -3879,11 +3880,24 @@ xlog_force_shutdown(
spin_lock(&log->l_icloglock);
if (test_and_set_bit(XLOG_IO_ERROR, &log->l_opstate)) {
spin_unlock(&log->l_icloglock);
ASSERT(0);
return false;
}
spin_unlock(&log->l_icloglock);

/*
* If this log shutdown also sets the mount shutdown state, issue a
* shutdown warning message.
*/
if (!test_and_set_bit(XFS_OPSTATE_SHUTDOWN, &log->l_mp->m_opstate)) {
xfs_alert_tag(log->l_mp, XFS_PTAG_SHUTDOWN_LOGERROR,
"Filesystem has been shut down due to log error (0x%x).",
shutdown_flags);
xfs_alert(log->l_mp,
"Please unmount the filesystem and rectify the problem(s).");
if (xfs_error_level >= XFS_ERRLEVEL_HIGH)
xfs_stack_trace();
}

/*
* We don't want anybody waiting for log reservations after this. That
* means we have to wake up everybody queued up on reserveq as well as
Expand Down
4 changes: 2 additions & 2 deletions fs/xfs/xfs_log_cil.c
Original file line number Diff line number Diff line change
Expand Up @@ -540,7 +540,7 @@ xlog_cil_insert_items(
spin_unlock(&cil->xc_cil_lock);

if (tp->t_ticket->t_curr_res < 0)
xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
xlog_force_shutdown(log, SHUTDOWN_LOG_IO_ERROR);
}

static void
Expand Down Expand Up @@ -854,7 +854,7 @@ xlog_cil_write_commit_record(

error = xlog_write(log, ctx, &vec, ctx->ticket, XLOG_COMMIT_TRANS);
if (error)
xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
xlog_force_shutdown(log, SHUTDOWN_LOG_IO_ERROR);
return error;
}

Expand Down
6 changes: 3 additions & 3 deletions fs/xfs/xfs_log_recover.c
Original file line number Diff line number Diff line change
Expand Up @@ -2485,7 +2485,7 @@ xlog_finish_defer_ops(
error = xfs_trans_alloc(mp, &resv, dfc->dfc_blkres,
dfc->dfc_rtxres, XFS_TRANS_RESERVE, &tp);
if (error) {
xfs_force_shutdown(mp, SHUTDOWN_LOG_IO_ERROR);
xlog_force_shutdown(mp->m_log, SHUTDOWN_LOG_IO_ERROR);
return error;
}

Expand Down Expand Up @@ -3454,7 +3454,7 @@ xlog_recover_finish(
*/
xlog_recover_cancel_intents(log);
xfs_alert(log->l_mp, "Failed to recover intents");
xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
xlog_force_shutdown(log, SHUTDOWN_LOG_IO_ERROR);
return error;
}

Expand Down Expand Up @@ -3501,7 +3501,7 @@ xlog_recover_finish(
* end of intents processing can be pushed through the CIL
* and AIL.
*/
xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
xlog_force_shutdown(log, SHUTDOWN_LOG_IO_ERROR);
}

return 0;
Expand Down
1 change: 1 addition & 0 deletions fs/xfs/xfs_mount.c
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include "xfs_trans.h"
#include "xfs_trans_priv.h"
#include "xfs_log.h"
#include "xfs_log_priv.h"
#include "xfs_error.h"
#include "xfs_quota.h"
#include "xfs_fsops.h"
Expand Down
8 changes: 4 additions & 4 deletions fs/xfs/xfs_trans_ail.c
Original file line number Diff line number Diff line change
Expand Up @@ -873,17 +873,17 @@ xfs_trans_ail_delete(
int shutdown_type)
{
struct xfs_ail *ailp = lip->li_ailp;
struct xfs_mount *mp = ailp->ail_log->l_mp;
struct xlog *log = ailp->ail_log;
xfs_lsn_t tail_lsn;

spin_lock(&ailp->ail_lock);
if (!test_bit(XFS_LI_IN_AIL, &lip->li_flags)) {
spin_unlock(&ailp->ail_lock);
if (shutdown_type && !xlog_is_shutdown(ailp->ail_log)) {
xfs_alert_tag(mp, XFS_PTAG_AILDELETE,
if (shutdown_type && !xlog_is_shutdown(log)) {
xfs_alert_tag(log->l_mp, XFS_PTAG_AILDELETE,
"%s: attempting to delete a log item that is not in the AIL",
__func__);
xfs_force_shutdown(mp, shutdown_type);
xlog_force_shutdown(log, shutdown_type);
}
return;
}
Expand Down

0 comments on commit b5f17be

Please sign in to comment.