Re: [PATCH 4/3] xfs: xfs_qm_dqrele mostly doesn't need locking

[Dave Chinner <david@fromorbit.com>]

On Fri, Dec 13, 2013 at 05:28:07AM -0800, Christoph Hellwig wrote:
> On Thu, Dec 12, 2013 at 09:25:07PM +1100, Dave Chinner wrote:
> > 
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Now that we have an atomic variable for the reference count, we
> > don't need to take the dquot lock if we are not removing the last
> > reference count. The dquot lock is a mutex, so we can't use
> > atomic_dec_and_lock(), but we can open code it in xfs_qm_dqrele and
> > hence avoid the dquot lock for most of the cases where we drop a
> > reference count.
> > 
> > The result is that concurrent file creates jump from 24,000/s to
> > 28,000/s, and the entire workload is now serialised on the dquot
> > being locked during transaction commit. Another significant win,
> > even though it's not the big one...
> 
> Maybe I'm missing something, but shou;dn't the following be enough to
> be a valid dqput (plus asserts & tracing):
> 
> 
> 	if (atomic_dec_and_test(&dqp->q_nrefs)) {
> 		if (list_lru_add(&mp->m_quotainfo->qi_lru, &dqp->q_lru))
> 			XFS_STATS_INC(xs_qm_dquot_unused);
> 	}
> 
> given that the only locking we need is the internal lru lock?

Yes, I think it is.

However, that involves changing all the callers of dqput to not hold
the dqlock when they call, which is a bigger change than was
necessary to avoid the lock contention problem. i.e. it doesn't seem
to be in a fast path that needed immediate fixing, so I didn't touch
it.

> > 
> > While there, rename xfs_qm_dqrele to xfs_dqrele - the "qm" part of
> > the name means nothing and just makes the code harder to read.
> 
> Please keep that out of the patch.  I don't mind dropping the
> qm_ part, but there's a lot of functions that have it, and it should
> be done for all of them at the same time.

OK.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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