xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 4/3] xfs: xfs_qm_dqrele mostly doesn't need locking
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Fri, 13 Dec 2013 05:28:07 -0800
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20131212102507.GX10988@dastard>
References: <1386841258-22183-1-git-send-email-david@xxxxxxxxxxxxx> <20131212102507.GX10988@dastard>
User-agent: Mutt/1.5.21 (2010-09-15)
On Thu, Dec 12, 2013 at 09:25:07PM +1100, Dave Chinner wrote:
> 
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> 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?
> 
> 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.

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