xfs
[Top] [All Lists]

Re: [PATCH 09/16] xfs: flatten the dquot lock ordering

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 09/16] xfs: flatten the dquot lock ordering
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Mon, 5 Dec 2011 04:11:21 -0500
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <20111205050428.GP7046@dastard>
References: <20111128082722.604873274@xxxxxxxxxxxxxxxxxxxxxx> <20111128082837.808570926@xxxxxxxxxxxxxxxxxxxxxx> <20111205050428.GP7046@dastard>
User-agent: Mutt/1.5.21 (2010-09-15)
On Mon, Dec 05, 2011 at 04:04:28PM +1100, Dave Chinner wrote:
> Ok, so we now mark dquots being freed with a flag, and then avoid
> those inodes during various operations as they dquots are considered
> dead.
> 
> That means we can use the fact that nothing new will ever use the
> dquot being freed while it is still active on lists, so we don't
> need to nest locks during reclaim of the dquot to prevent concurrent
> lookups from finding it.
> 
> Did i understand the intent correctly?

Yes.

> > +   mutex_lock(&xfs_Gqm->qm_dqfrlist_lock);
> > +   ASSERT(!list_empty(&dqp->q_freelist));
> >     list_del_init(&dqp->q_freelist);
> 
> That assert could do with a comment - I had to think hard about why
> that was correct. It's because when the dquot refcount goes to zero it
> is moved onto the free list, so when reclaiming a dquot we should
> always find it on the free list....

Indeed.  I'll see if I can come up with a sensible comment.

> > @@ -509,46 +518,27 @@ xfs_qm_dqpurge_int(
> >      */
> >     xfs_qm_detach_gdquots(mp);
> >  
> > -      again:
> > -   nmisses = 0;
> 
> I don't think that nmisses is initialised to zero correctly anymore.

We do a

        int                     nmisses = 0;

at the top of the function.  Now that there are no retry loops that's
sufficient.

> 
> > -   ASSERT(mutex_is_locked(&q->qi_dqlist_lock));
> >     /*
> >      * Try to get rid of all of the unwanted dquots. The idea is to
> >      * get them off mplist and hashlist, but leave them on freelist.
> >      */
> 
> That comment is no longer correct - they purged dquots don't remain
> on the freelist anymore - they are freed....

I'll fix it up.

> >             /*
> > +            * Prevent lookups now that we are past the point of no return.
> >              */
> > -           if (!mutex_trylock(&dqp->q_hash->qh_lock)) {
> > -                   restarts++;
> > -                   goto dqfunlock;
> > -           }
> > +           dqp->dq_flags |= XFS_DQ_FREEING;
> > +           xfs_dqunlock(dqp);
> 
> Probably worth commenting here that it is safe to access the dquot
> unlocked because we own the XFS_DQ_FREEING flag that guarantees
> nobody else will start using the dquot once we unlock it.

I'll make the comment a bit more verbose.

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