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

Allison Collins
2020-02-28T18:55:57Z

On 2/28/20 7:03 AM, Brian Foster wrote:
> On Thu, Feb 27, 2020 at 04:19:42PM -0700, Allison Collins wrote:
>>
>>
>> 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.
>>
>
> Permanent transactions have a separate default log count (2 instead of
> 1) because they are intended to be rolled (at least once). This
> basically means the initial allocation will acquire enough log
> reservation for the initial transaction and a subsequent roll, similar
> to how all other permanant transactions are initialized in this file.
> This is required for quotaoff because the transaction now uses
> XFS_TRANS_RELOG, which requires a roll up front.
>
> Brian

Ok, I see. Thank you for the explanation!
Allison

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