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

Ben Myers
2011-12-13T17:30:22Z

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@lst.de>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>

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

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