xfs
[Top] [All Lists]

Re: [PATCH v2 2/3] xfs: fix infinite loop by detaching the group/project

To: Ben Myers <bpm@xxxxxxx>, Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH v2 2/3] xfs: fix infinite loop by detaching the group/project hints from user dquot
From: Jeff Liu <jeff.liu@xxxxxxxxxx>
Date: Sat, 07 Dec 2013 13:51:24 +0800
Cc: "xfs@xxxxxxxxxxx" <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20131206210137.GT1935@xxxxxxx>
References: <5294A469.1060000@xxxxxxxxxx> <20131128104336.GE26927@xxxxxxxxxxxxx> <52986001.9040101@xxxxxxxxxx> <20131206210137.GT1935@xxxxxxx>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.1.0
Hi Ben,

On 12/07 2013 05:01 AM, Ben Myers wrote:
> Hey Jeff,
> 
> On Fri, Nov 29, 2013 at 05:36:01PM +0800, Jeff Liu wrote:
>> On 11/28 2013 18:43 PM, Christoph Hellwig wrote:
>>> On Tue, Nov 26, 2013 at 09:38:49PM +0800, Jeff Liu wrote:
>>>> +  if (flags & XFS_QMOPT_UQUOTA)
>>>> +          return xfs_qm_dqpurge(dqp, NULL);
>>>
>>> To me it doesn't make any sense to overload this function for the user
>>> quotas that don't have hints.
>> To me it would like a silly compromise.
>>>
>>> I'd suggest dropping this hunk and keeping a separate walk for
>>> releasing the uquots.
>> I thought this over and yup, that is an overload if neither group nor project
>> are enabled, or we don't want to turn user quota off.
>>
>> But even so, we currently also have overloads by checking group/project hints
>> before releasing any type of quota in xfs_qm_purge().  In this point, this 
>> fix
>> can reduce a bit overloads by moving those checkups to xfs_qm_purge_hints() 
>> if
>> we want to turn group/project quotas off.
>>
>> If we considering to drop above hunk to release user quota separately, we 
>> finally
>> would have to walk through user quota to remove those hints again, i.e,
>>
>> /* Remove group/project hints from user dquot */
>> STATIC int
>> xfs_qm_dqpurge_hints(
>>      struct xfs_dquot        *dqp,
>>      void                    *data)
>> {
>>      uint                    flags = *((uint *)data);
>>      struct xfs_dquot        *gdqp;
>>      struct xfs_dquot        *pdqp;
>>
>>      xfs_dqlock(dqp);
>>      if (dqp->dq_flags & XFS_DQ_FREEING) {
>>              xfs_dqunlock(dqp);
>>              return EAGAIN;
>>      }
>>
>>      /* If this quota has a hint attached, prepare for releasing it now */
>>         gdqp = dqp->q_gdquot;
>>      if (gdqp)
>>              dqp->q_gdquot = NULL;
>>
>>      pdqp = dqp->q_pdquot;
>>      if (pdqp)
>>              dqp->q_pdquot = NULL;
>>
>>      xfs_dqunlock(dqp);
>>
>>      if (gdqp)
>>              xfs_qm_dqrele(gdqp);
>>      if (pdqp)
>>              xfs_qm_dqrele(pdqp);
>>
>>      return 0;
>> }
>>
>> void
>> xfs_qm_dqpurge_all()
>> {
>>      xfs_qm_dquot_walk(mp, XFS_DQ_USER, xfs_qm_dqpurge_hints, NULL);
>>
>>      if (flags & XFS_QMOPT_UQUOTA)
>>              xfs_qm_dquot_walk(mp, XFS_DQ_USER, xfs_qm_dqpurge, NULL);
>>      if (flags & XFS_QMOPT_GQUOTA)
>>              xfs_qm_dquot_walk(mp, XFS_DQ_GROUP, xfs_qm_dqpurge, NULL);
>>      if (flags & XFS_QMOPT_PQUOTA)
>>              xfs_qm_dquot_walk(mp, XFS_DQ_PROJ, xfs_qm_dqpurge, NULL);
>> }
>>
>> Above code is what I can figured out as per your suggestions for now, but it
>> would introduce overheads for walking through user dquots to release hints
>> separately if we want to turn user quota off.
>>
>> Any thoughts?
> 
> I was gonna pull in the single walk version, but now I realize that it is 
> still
> under discussion.  I'm happy with either implementation, with maybe a slight
> preference for a single user quota walk.  Can you and Christoph come to an
> agreement?
For now, I can not figure out a more optimized solution.  Well, I just realized
I don't need to initialize both gdqp and pdqp to NULL at xfs_qm_dqpurge_hints()
since they will be evaluated by dqp pointers dereference anyway.  As a minor 
fix,
the revised version was shown as follows.

Christoph, as I mentioned previously, keeping a separate walk to release the 
user
dquots would also have overloads in some cases, would you happy to have this fix
although it is not most optimized?

Also, Ben, would you please pull in another fix below? It is independent to the
current fix and it has already been reviewed by Christoph. :)
[PATCH v2 1/3] xfs: fix assertion failure at xfs_setattr_nonsize

Thanks,
-Jeff

From: Jie Liu <jeff.liu@xxxxxxxxxx>

xfs_quota(8) will hang up if trying to turn group/project quota off
before the user quota is off, this could be 100% reproduced by:
  # mount -ouquota,gquota /dev/sda7 /xfs
  # mkdir /xfs/test
  # xfs_quota -xc 'off -g' /xfs <-- hangs up
  # echo w > /proc/sysrq-trigger
  # dmesg

  SysRq : Show Blocked State
  task                        PC stack   pid father
  xfs_quota       D 0000000000000000     0 27574   2551 0x00000000
  [snip]
  Call Trace:
  [<ffffffff81aaa21d>] schedule+0xad/0xc0
  [<ffffffff81aa327e>] schedule_timeout+0x35e/0x3c0
  [<ffffffff8114b506>] ? mark_held_locks+0x176/0x1c0
  [<ffffffff810ad6c0>] ? call_timer_fn+0x2c0/0x2c0
  [<ffffffffa0c25380>] ? xfs_qm_shrink_count+0x30/0x30 [xfs]
  [<ffffffff81aa3306>] schedule_timeout_uninterruptible+0x26/0x30
  [<ffffffffa0c26155>] xfs_qm_dquot_walk+0x235/0x260 [xfs]
  [<ffffffffa0c059d8>] ? xfs_perag_get+0x1d8/0x2d0 [xfs]
  [<ffffffffa0c05805>] ? xfs_perag_get+0x5/0x2d0 [xfs]
  [<ffffffffa0b7707e>] ? xfs_inode_ag_iterator+0xae/0xf0 [xfs]
  [<ffffffffa0c22280>] ? xfs_trans_free_dqinfo+0x50/0x50 [xfs]
  [<ffffffffa0b7709f>] ? xfs_inode_ag_iterator+0xcf/0xf0 [xfs]
  [<ffffffffa0c261e6>] xfs_qm_dqpurge_all+0x66/0xb0 [xfs]
  [<ffffffffa0c2497a>] xfs_qm_scall_quotaoff+0x20a/0x5f0 [xfs]
  [<ffffffffa0c2b8f6>] xfs_fs_set_xstate+0x136/0x180 [xfs]
  [<ffffffff8136cf7a>] do_quotactl+0x53a/0x6b0
  [<ffffffff812fba4b>] ? iput+0x5b/0x90
  [<ffffffff8136d257>] SyS_quotactl+0x167/0x1d0
  [<ffffffff814cf2ee>] ? trace_hardirqs_on_thunk+0x3a/0x3f
  [<ffffffff81abcd19>] system_call_fastpath+0x16/0x1b

It's fine if we turn user quota off at first, then turn off other
kind of quotas if they are enabled since the group/project dquot
refcount is decreased to zero once the user quota if off. Otherwise,
those dquots refcount is non-zero due to the user dquot might refer
to them as hint(s).  Hence, above operation cause an infinite loop
at xfs_qm_dquot_walk() while trying to purge dquot cache.

This problem has been around since Linux 3.4, it was introduced by:
  [ b84a3a9675 xfs: remove the per-filesystem list of dquots ]

Originally we will release the group dquot pointers because the user
dquots maybe carrying around as a hint via xfs_qm_detach_gdquots().
However, with above change, there is no such work to be done before
purging group/project dquot cache.

In order to solve this problem, this patch introduces a special routine
xfs_qm_dqpurge_hints(), and it would release the group/project dquot
pointers the user dquots maybe carrying around as a hint, and then it
will proceed to purge the user dquot cache if requested.

Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Jie Liu <jeff.liu@xxxxxxxxxx>
---
 fs/xfs/xfs_qm.c | 71 ++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 50 insertions(+), 21 deletions(-)

diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 14a4996..424ef73 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -134,8 +134,6 @@ xfs_qm_dqpurge(
 {
        struct xfs_mount        *mp = dqp->q_mount;
        struct xfs_quotainfo    *qi = mp->m_quotainfo;
-       struct xfs_dquot        *gdqp = NULL;
-       struct xfs_dquot        *pdqp = NULL;
 
        xfs_dqlock(dqp);
        if ((dqp->dq_flags & XFS_DQ_FREEING) || dqp->q_nrefs != 0) {
@@ -143,21 +141,6 @@ xfs_qm_dqpurge(
                return EAGAIN;
        }
 
-       /*
-        * If this quota has a hint attached, prepare for releasing it now.
-        */
-       gdqp = dqp->q_gdquot;
-       if (gdqp) {
-               xfs_dqlock(gdqp);
-               dqp->q_gdquot = NULL;
-       }
-
-       pdqp = dqp->q_pdquot;
-       if (pdqp) {
-               xfs_dqlock(pdqp);
-               dqp->q_pdquot = NULL;
-       }
-
        dqp->dq_flags |= XFS_DQ_FREEING;
 
        xfs_dqflock(dqp);
@@ -206,11 +189,47 @@ xfs_qm_dqpurge(
        XFS_STATS_DEC(xs_qm_dquot_unused);
 
        xfs_qm_dqdestroy(dqp);
+       return 0;
+}
+
+/*
+ * Release the group or project dquot pointers the user dquots maybe carrying
+ * around as a hint, and proceed to purge the user dquot cache if requested.
+*/
+STATIC int
+xfs_qm_dqpurge_hints(
+       struct xfs_dquot        *dqp,
+       void                    *data)
+{
+       uint                    flags = *((uint *)data);
+       struct xfs_dquot        *gdqp;
+       struct xfs_dquot        *pdqp;
 
+       xfs_dqlock(dqp);
+       if (dqp->dq_flags & XFS_DQ_FREEING) {
+               xfs_dqunlock(dqp);
+               return EAGAIN;
+       }
+
+       /* If this quota has a hint attached, prepare for releasing it now */
+       gdqp = dqp->q_gdquot;
        if (gdqp)
-               xfs_qm_dqput(gdqp);
+               dqp->q_gdquot = NULL;
+
+       pdqp = dqp->q_pdquot;
        if (pdqp)
-               xfs_qm_dqput(pdqp);
+               dqp->q_pdquot = NULL;
+
+       xfs_dqunlock(dqp);
+
+       if (gdqp)
+               xfs_qm_dqrele(gdqp);
+       if (pdqp)
+               xfs_qm_dqrele(pdqp);
+
+       if (flags & XFS_QMOPT_UQUOTA)
+               return xfs_qm_dqpurge(dqp, NULL);
+
        return 0;
 }
 
@@ -222,8 +241,18 @@ xfs_qm_dqpurge_all(
        struct xfs_mount        *mp,
        uint                    flags)
 {
-       if (flags & XFS_QMOPT_UQUOTA)
-               xfs_qm_dquot_walk(mp, XFS_DQ_USER, xfs_qm_dqpurge, NULL);
+       /*
+        * We have to release group/project dquot hint(s) from the user dquot
+        * at first if they are there, otherwise we would run into an infinite
+        * loop while walking through radix tree to purge other type of dquots
+        * since their refcount is not zero if the user dquot refers to them
+        * as hint.
+        *
+        * Call the special xfs_qm_dqpurge_hints() will end up go through the
+        * general xfs_qm_dqpurge() against user dquot cache if requested.
+        */
+       xfs_qm_dquot_walk(mp, XFS_DQ_USER, xfs_qm_dqpurge_hints, &flags);
+
        if (flags & XFS_QMOPT_GQUOTA)
                xfs_qm_dquot_walk(mp, XFS_DQ_GROUP, xfs_qm_dqpurge, NULL);
        if (flags & XFS_QMOPT_PQUOTA)
-- 1.8.3.2

<Prev in Thread] Current Thread [Next in Thread>