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

Ben Myers
2013-12-06T21:01:41Z

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?

Thanks,
Ben