On Tue, Sep 29, 2020 at 10:12:28AM -0400, Brian Foster wrote:
> The quotaoff operation logs two log items. The start item is
> committed first, followed by the bulk of the in-core quotaoff
> processing, and then the quotaoff end item is committed to release
> the start item from the log. The problem with this mechanism is that
> quite a bit of processing can be required to release dquots from all
> in-core inodes and subsequently flush/purge all dquots in the
> system. This processing work doesn't generally generate much log
> traffic itself, but the start item pins the tail of the log. If an
> external workload consumes the remaining log space before the
> transaction for the end item is allocated, a log deadlock can occur.
>
> The purpose of the separate start and end log items is primarily to
> ensure that log recovery does not incorrectly recover dquot data
> after an fs crash where a quotaoff was in progress. If we only
> logged a single quotaoff item, for example, it could fall behind the
> tail of the log before the last dquot modification was made and
> incorrectly replay dquot changes that might have occurred after the
> start item committed but before quotaoff deactivated the quota.
>
> With that context, we can make some small changes to the quotaoff
> algorithm to provide the same general log ordering guarantee without
> such a large window to create a log deadlock vector. Rather than
> place a start item in the log for the duration of quotaoff
> processing, we can quiesce the transaction subsystem up front to
> guarantee that no further dquots are logged from that point forward.
> IOW, we freeze the transaction subsystem, commit the start item in a
> synchronous transaction that forces the log, deactivate the
> associated quota such that subsequent transactions no longer modify
> associated dquots, unfreeze the transaction subsystem and finally
> commit the quotaoff end item. The transaction freeze is somewhat of
> a heavy weight operation, but quotaoff is already a rare, slow and
> performance disruptive operation.
>
> Altogether, this means that the dquot rele/purge sequence occurs
> after the quotaoff end item has committed and thus can technically
> fall off the active range of the log. This is safe because the
> remaining processing is all in-core work that doesn't involve
> logging dquots and we've guaranteed that no further dquots can be
> modified by external transactions. This allows quotaoff to complete
> without risking log deadlock regardless of how much dquot processing
> is required.
>
> Suggested-by: Dave Chinner <david@xxxxxxxxxxxxx>
> Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
> ---
> fs/xfs/xfs_qm_syscalls.c | 36 ++++++++++++++++--------------------
> fs/xfs/xfs_trans_dquot.c | 2 ++
> 2 files changed, 18 insertions(+), 20 deletions(-)
>
> diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
> index ca1b57d291dc..97844f33f70f 100644
> --- a/fs/xfs/xfs_qm_syscalls.c
> +++ b/fs/xfs/xfs_qm_syscalls.c
> @@ -29,7 +29,8 @@ xfs_qm_log_quotaoff(
> int error;
> struct xfs_qoff_logitem *qoffi;
>
> - error = xfs_trans_alloc(mp, &M_RES(mp)->tr_qm_quotaoff, 0, 0, 0, &tp);
> + error = xfs_trans_alloc(mp, &M_RES(mp)->tr_qm_quotaoff, 0, 0,
> + XFS_TRANS_NO_WRITECOUNT, &tp);
> if (error)
> goto out;
>
> @@ -169,14 +170,18 @@ xfs_qm_scall_quotaoff(
> if ((mp->m_qflags & flags) == 0)
> goto out_unlock;
>
> + xfs_trans_freeze(mp);
> +
> /*
> * Write the LI_QUOTAOFF log record, and do SB changes atomically,
> * and synchronously. If we fail to write, we should abort the
> * operation as it cannot be recovered safely if we crash.
> */
> error = xfs_qm_log_quotaoff(mp, &qoffstart, flags);
> - if (error)
> + if (error) {
> + xfs_trans_unfreeze(mp);
> goto out_unlock;
> + }
>
> /*
> * Next we clear the XFS_MOUNT_*DQ_ACTIVE bit(s) in the mount struct
> @@ -191,6 +196,15 @@ xfs_qm_scall_quotaoff(
> */
> mp->m_qflags &= ~inactivate_flags;
>
> + xfs_trans_unfreeze(mp);
> +
> + error = xfs_qm_log_quotaoff_end(mp, &qoffstart, flags);
> + if (error) {
> + /* We're screwed now. Shutdown is the only option. */
> + xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> + goto out_unlock;
> + }
> +
The m_qflags update is racy. There's no memory barriers or locks
here to order the mp->m_qflags update with other CPUs, so there's no
guarantee that an incoming transaction will see the m_qflags change
that disables quotas.
Also, we can now race with other transaction reservations to log the
quota off end item, which means that if there are enough pending
transactions reservations queued before the quotaoff_end transaction
is started, the quotaoff item can pin the tail of the log and we
deadlock.
That was the reason why I logged both quota off items in the same
transaction in the prototype code I send out - we have to log both
quota-off items while the transaction subsystem is quiesced
otherwise we don't fix the original problem (that the quotaoff
item can pin the tail of the log and deadlock).....
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
