On Mon, Nov 28, 2011 at 03:27:31AM -0500, Christoph Hellwig wrote:
> Introduce a new XFS_DQ_FREEING flag that tells lookup and mplist walks
> to skip a dquot that is beeing freed, and use this avoid the trylock
> on the hash and mplist locks in xfs_qm_dqreclaim_one.
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?
> Also simplify
> xfs_dqpurge by moving the inodes to a dispose list after marking them
> XFS_DQ_FREEING and avoid the locker ordering constraints.
Ok, so changing to a 2 phase reclaim algorithm - mark them for
reclaim and gather them needing only one lock for the list being
walked, then walk that gathered list and free them properly, knowing
that lookups won't find them due to the XFS_DQ_FREEING flag.
Basically the same principles as the VFS cache item reclaim, again?
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
......
> @@ -1335,31 +1314,34 @@ xfs_qm_dqpurge(
> __func__, dqp);
> xfs_dqflock(dqp);
> }
> +
> ASSERT(atomic_read(&dqp->q_pincount) == 0);
> ASSERT(XFS_FORCED_SHUTDOWN(mp) ||
> !(dqp->q_logitem.qli_item.li_flags & XFS_LI_IN_AIL));
>
> + xfs_dqfunlock(dqp);
> + xfs_dqunlock(dqp);
> +
> + mutex_lock(&qh->qh_lock);
> list_del_init(&dqp->q_hashlist);
> qh->qh_version++;
> + mutex_unlock(&qh->qh_lock);
>
> + mutex_lock(&mp->m_quotainfo->qi_dqlist_lock);
> list_del_init(&dqp->q_mplist);
> mp->m_quotainfo->qi_dqreclaims++;
> mp->m_quotainfo->qi_dquots--;
> + mutex_unlock(&mp->m_quotainfo->qi_dqlist_lock);
>
> + 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....
.....
> @@ -489,8 +498,8 @@ xfs_qm_dqpurge_int(
> struct xfs_quotainfo *q = mp->m_quotainfo;
> struct xfs_dquot *dqp, *n;
> uint dqtype;
> - int nrecl;
> - int nmisses;
> + int nmisses = 0;
> + LIST_HEAD (dispose_list);
Curious style. I haven't seen that before - not sure whether I like
it or not yet...
>
> if (!q)
> return 0;
> @@ -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.
> - 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....
.....
> @@ -1737,57 +1717,40 @@ again:
> }
> goto dqunlock;
> }
> + xfs_dqfunlock(dqp);
>
> /*
> - * We're trying to get the hashlock out of order. This races
> - * with dqlookup; so, we giveup and goto the next dquot if
> - * we couldn't get the hashlock. This way, we won't starve
> - * a dqlookup process that holds the hashlock that is
> - * waiting for the freelist lock.
> + * 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.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
