Re: [patch 15/19] xfs: simplify xfs_qm_detach_gdquots

From: Dave Chinner <david@fromorbit.com>
To: Ben Myers <bpm@sgi.com>
Cc: Christoph Hellwig <hch@infradead.org>, xfs@oss.sgi.com
Subject: Re: [patch 15/19] xfs: simplify xfs_qm_detach_gdquots
Date: Fri, 16 Dec 2011 11:48:45 +1100	[thread overview]
Message-ID: <20111216004845.GB23662@dastard> (raw)
In-Reply-To: <20111215164314.GI29840@sgi.com>

On Thu, Dec 15, 2011 at 10:43:14AM -0600, Ben Myers wrote:
> On Tue, Dec 06, 2011 at 04:58:21PM -0500, Christoph Hellwig wrote:
> > There is no reason to drop qi_dqlist_lock around calls to xfs_qm_dqrele
> > because the free list lock now nests inside qi_dqlist_lock and the
> > dquot lock.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > Reviewed-by: Dave Chinner <dchinner@redhat.com>
> > 
> > ---
> >  fs/xfs/xfs_qm.c |   22 +++++-----------------
> >  1 file changed, 5 insertions(+), 17 deletions(-)
> > 
> > Index: xfs/fs/xfs/xfs_qm.c
> > ===================================================================
> > --- xfs.orig/fs/xfs/xfs_qm.c	2011-10-27 22:40:07.538179215 +0200
> > +++ xfs/fs/xfs/xfs_qm.c	2011-10-27 22:40:08.124671538 +0200
> > @@ -449,7 +449,6 @@ xfs_qm_detach_gdquots(
> >  {
> >  	struct xfs_quotainfo	*q = mp->m_quotainfo;
> >  	struct xfs_dquot	*dqp, *gdqp;
> > -	int			nrecl;
> >  
> >   again:
> >  	ASSERT(mutex_is_locked(&q->qi_dqlist_lock));
> > @@ -462,25 +461,14 @@ xfs_qm_detach_gdquots(
> >  			mutex_lock(&q->qi_dqlist_lock);
> >  			goto again;
> >  		}
> > -		if ((gdqp = dqp->q_gdquot)) {
> > -			xfs_dqlock(gdqp);
> 
> Why don't we need to take the dqlock on gdqp here before dropping the
> lock on dqp?

Because we have an active reference on it, it's guaranteed not to go
away from under us. Hence we don't need to lock it before we unlock
the dqp which holds that reference. As it is, the subsequent
xfs_qm_dqrele() takes the lock before dropping the reference we
currently hold.

> 
> > +
> > +		gdqp = dqp->q_gdquot;
> > +		if (gdqp)
> >  			dqp->q_gdquot = NULL;
> > -		}
> >  		xfs_dqunlock(dqp);
> >  
> > -		if (gdqp) {
> > -			/*
> > -			 * Can't hold the mplist lock across a dqput.
> > -			 * XXXmust convert to marker based iterations here.
> > -			 */
> > -			nrecl = q->qi_dqreclaims;
> > -			mutex_unlock(&q->qi_dqlist_lock);
> > -			xfs_qm_dqput(gdqp);
> > -
> > -			mutex_lock(&q->qi_dqlist_lock);
> > -			if (nrecl != q->qi_dqreclaims)
> 
> Why is it ok to ignore di_dqreclaims now?

That was there to tell us if the list we are traversing was modified
or not while we had the lock dropped. If is was, then out list next
pointer may not be valid, so we have to restart the traversal from
the start.

We don't drop the lock any more, so we know that the list cannot be
modified when we drop the current reference on the gdqp. Hence we
don't need the check anymore.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs