xfs
[Top] [All Lists]

Re: [PATCH 3/3] xfs: xfs_trans_dqresv() can be made lockless

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 3/3] xfs: xfs_trans_dqresv() can be made lockless
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Fri, 13 Dec 2013 05:37:45 -0800
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1386841258-22183-4-git-send-email-david@xxxxxxxxxxxxx>
References: <1386841258-22183-1-git-send-email-david@xxxxxxxxxxxxx> <1386841258-22183-4-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Thu, Dec 12, 2013 at 08:40:58PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> xfs_trans_dqresv() serialises dquot modifications by taking the
> dquot lock while it is doing reservations. The thing is, nothing it
> does really requires exclusive access to the dquot except for the
> reservation accounting. We can do that locklessly with cmpxchg.

Can you split the various refactorings into separate patches to make
this more readable?

> +do_ninos:
> +     if (ninos == 0)
> +             goto do_trans;
> +
> +     smp_mb();
> +     timer = be32_to_cpu(dqp->q_core.d_itimer);
> +     warns = be16_to_cpu(dqp->q_core.d_iwarns);
> +     warnlimit = dqp->q_mount->m_quotainfo->qi_iwarnlimit;
> +     hardlimit = be64_to_cpu(dqp->q_core.d_ino_hardlimit);
> +     if (!hardlimit)
> +             hardlimit = q->qi_ihardlimit;
> +     softlimit = be64_to_cpu(dqp->q_core.d_ino_softlimit);
> +     if (!softlimit)
> +             softlimit = q->qi_isoftlimit;
> +     resbcountp = &dqp->q_res_icount;
> +
> +     oldcnt = xfs_dqresv_cmpxchg(mp, dqp, resbcountp, ninos, false, enforce,
> +                                 hardlimit, softlimit, timer, warns,
> +                                 warnlimit);
> +     if (oldcnt == (xfs_qcnt_t)-1ULL)
> +             goto error_undo_nblks;
> +
> +do_trans:

Instead of having all these goto labels maye this should be factored
into helpers for each of the stages?

>       if (udqp) {
> +             enforce = !(flags & XFS_QMOPT_FORCE_RES) &&
> +                       udqp->q_core.d_id && XFS_IS_UQUOTA_ENFORCED(mp);
>               error = xfs_trans_dqresv(tp, mp, udqp, nblks, ninos,
> -                                     (flags & ~XFS_QMOPT_ENOSPC));
> +                                     (flags & ~XFS_QMOPT_ENOSPC), enforce);

I have to say I'd much prefer having the enforcement decision hidden
inside xfs_trans_dqresv.

>               if (error)
>                       return error;
>       }
>  
>       if (gdqp) {
> -             error = xfs_trans_dqresv(tp, mp, gdqp, nblks, ninos, flags);
> +             enforce = !(flags & XFS_QMOPT_FORCE_RES) &&
> +                       gdqp->q_core.d_id && XFS_IS_GQUOTA_ENFORCED(mp);
> +             error = xfs_trans_dqresv(tp, mp, gdqp, nblks, ninos, flags,
> +                                      enforce);
>               if (error)

Unrelated to the patch: why do we clear XFS_QMOPT_ENOSPC for user
quotas, but not for project quotas here?

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