Re: [RFC v5 PATCH 6/9] xfs: automatically relog the quotaoff start intent

Allison Collins
2020-02-27T23:19:50Z

On 2/27/20 6:43 AM, Brian Foster wrote:
> The quotaoff operation has a rare but longstanding deadlock vector
> in terms of how the operation is logged. A quotaoff start intent is
> logged (synchronously) at the onset to ensure recovery can handle
> the operation if interrupted before in-core changes are made. This
> quotaoff intent pins the log tail while the quotaoff sequence scans
> and purges dquots from all in-core inodes. While this operation
> generally doesn't generate much log traffic on its own, it can be
> time consuming. If unrelated, concurrent filesystem activity
> consumes remaining log space before quotaoff is able to acquire log
> reservation for the quotaoff end intent, the filesystem locks up
> indefinitely.
>
> quotaoff cannot allocate the end intent before the scan because the
> latter can result in transaction allocation itself in certain
> indirect cases (releasing an inode, for example). Further, rolling
> the original transaction is difficult because the scanning work
> occurs multiple layers down where caller context is lost and not
> much information is available to determine how often to roll the
> transaction.
>
> To address this problem, enable automatic relogging of the quotaoff
> start intent. This automatically relogs the intent whenever AIL
> pushing finds the item at the tail of the log. When quotaoff
> completes, wait for relogging to complete as the end intent expects
> to be able to permanently remove the start intent from the log
> subsystem. This ensures that the log tail is kept moving during a
> particularly long quotaoff operation and avoids the log reservation
> deadlock.
>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
> fs/xfs/libxfs/xfs_trans_resv.c | 3 ++-
> fs/xfs/xfs_dquot_item.c | 7 +++++++
> fs/xfs/xfs_qm_syscalls.c | 12 +++++++++++-
> 3 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> index 1f5c9e6e1afc..f49b20c9ca33 100644
> --- a/fs/xfs/libxfs/xfs_trans_resv.c
> +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> @@ -935,7 +935,8 @@ xfs_trans_resv_calc(
> resp->tr_qm_setqlim.tr_logcount = XFS_DEFAULT_LOG_COUNT;
>
> resp->tr_qm_quotaoff.tr_logres = xfs_calc_qm_quotaoff_reservation(mp);
> - resp->tr_qm_quotaoff.tr_logcount = XFS_DEFAULT_LOG_COUNT;
> + resp->tr_qm_quotaoff.tr_logcount = XFS_DEFAULT_PERM_LOG_COUNT;
> + resp->tr_qm_quotaoff.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
What's the reason for the log count change here? Otherwise looks ok.

Allison
>
> resp->tr_qm_equotaoff.tr_logres =
> xfs_calc_qm_quotaoff_end_reservation();
> diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
> index d60647d7197b..ea5123678466 100644
> --- a/fs/xfs/xfs_dquot_item.c
> +++ b/fs/xfs/xfs_dquot_item.c
> @@ -297,6 +297,13 @@ xfs_qm_qoff_logitem_push(
> struct xfs_log_item *lip,
> struct list_head *buffer_list)
> {
> + struct xfs_log_item *mlip = xfs_ail_min(lip->li_ailp);
> +
> + if (test_bit(XFS_LI_RELOG, &lip->li_flags) &&
> + !test_bit(XFS_LI_RELOGGED, &lip->li_flags) &&
> + !XFS_LSN_CMP(lip->li_lsn, mlip->li_lsn))
> + return XFS_ITEM_RELOG;
> +
> return XFS_ITEM_LOCKED;
> }
>
> diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
> index 1ea82764bf89..7b48d34da0f4 100644
> --- a/fs/xfs/xfs_qm_syscalls.c
> +++ b/fs/xfs/xfs_qm_syscalls.c
> @@ -18,6 +18,7 @@
> #include "xfs_quota.h"
> #include "xfs_qm.h"
> #include "xfs_icache.h"
> +#include "xfs_trans_priv.h"
>
> STATIC int
> xfs_qm_log_quotaoff(
> @@ -31,12 +32,14 @@ xfs_qm_log_quotaoff(
>
> *qoffstartp = NULL;
>
> - 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_RELOG, &tp);
> if (error)
> goto out;
>
> qoffi = xfs_trans_get_qoff_item(tp, NULL, flags & XFS_ALL_QUOTA_ACCT);
> xfs_trans_log_quotaoff_item(tp, qoffi);
> + xfs_trans_relog_item(&qoffi->qql_item);
>
> spin_lock(&mp->m_sb_lock);
> mp->m_sb.sb_qflags = (mp->m_qflags & ~(flags)) & XFS_MOUNT_QUOTA_ALL;
> @@ -69,6 +72,13 @@ xfs_qm_log_quotaoff_end(
> int error;
> struct xfs_qoff_logitem *qoffi;
>
> + /*
> + * startqoff must be in the AIL and not the CIL when the end intent
> + * commits to ensure it is not readded to the AIL out of order. Wait on
> + * relog activity to drain to isolate startqoff to the AIL.
> + */
> + xfs_trans_relog_item_cancel(&startqoff->qql_item, true);
> +
> error = xfs_trans_alloc(mp, &M_RES(mp)->tr_qm_equotaoff, 0, 0, 0, &tp);
> if (error)
> return error;
>