xfs
[Top] [All Lists]

Re: [patch 09/19] xfs: cleanup xfs_qm_dqlookup

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [patch 09/19] xfs: cleanup xfs_qm_dqlookup
From: Ben Myers <bpm@xxxxxxx>
Date: Tue, 13 Dec 2011 11:30:46 -0600
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20111206215854.196332791@xxxxxxxxxxxxxxxxxxxxxx>
References: <20111206215806.844405397@xxxxxxxxxxxxxxxxxxxxxx> <20111206215854.196332791@xxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.18 (2008-05-17)
On Tue, Dec 06, 2011 at 04:58:15PM -0500, Christoph Hellwig wrote:
> Rearrange the code to avoid the conditional locking around the flist_locked
> variable.  This means we lose a (rather pointless) assert, and hold the
> freelist lock a bit longer for one corner case.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>

Looks good.
Reviewed-by: Ben Myers <bpm@xxxxxxx>

> ---
>  fs/xfs/xfs_dquot.c |   25 +++++--------------------
>  1 file changed, 5 insertions(+), 20 deletions(-)
> 
> Index: xfs/fs/xfs/xfs_dquot.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_dquot.c       2011-10-27 22:40:02.250173174 +0200
> +++ xfs/fs/xfs/xfs_dquot.c    2011-10-27 22:40:02.770171231 +0200
> @@ -710,12 +710,9 @@ xfs_qm_dqlookup(
>       xfs_dquot_t             **O_dqpp)
>  {
>       xfs_dquot_t             *dqp;
> -     uint                    flist_locked;
>  
>       ASSERT(mutex_is_locked(&qh->qh_lock));
>  
> -     flist_locked = B_FALSE;
> -
>       /*
>        * Traverse the hashchain looking for a match
>        */
> @@ -750,31 +747,19 @@ xfs_qm_dqlookup(
>                                       xfs_dqlock(dqp);
>                                       dqp->dq_flags &= ~(XFS_DQ_WANT);
>                               }
> -                             flist_locked = B_TRUE;
> -                     }
> -
> -                     /*
> -                      * id couldn't have changed; we had the hashlock all
> -                      * along
> -                      */
> -                     ASSERT(be32_to_cpu(dqp->q_core.d_id) == id);
>  
> -                     if (flist_locked) {
> -                             if (dqp->q_nrefs != 0) {
> -                                     
> mutex_unlock(&xfs_Gqm->qm_dqfrlist_lock);
> -                                     flist_locked = B_FALSE;
> -                             } else {
> +                             if (dqp->q_nrefs == 0) {

Ah.  We have to check this again because we would have unlocked and
reaquired the dqlock when we went for the free list lock in the
contended case.

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