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

[Allison Collins <allison.henderson@oracle.com>]

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;
>