From: "Darrick J. Wong" <djwong@kernel.org>
To: Christoph Hellwig <hch@lst.de>
Cc: Carlos Maiolino <cem@kernel.org>, linux-xfs@vger.kernel.org
Subject: Re: [PATCH 05/17] xfs: use a lockref for the xfs_dquot reference count
Date: Wed, 15 Oct 2025 14:02:19 -0700 [thread overview]
Message-ID: <20251015210219.GA2591640@frogsfrogsfrogs> (raw)
In-Reply-To: <20251013024851.4110053-6-hch@lst.de>
On Mon, Oct 13, 2025 at 11:48:06AM +0900, Christoph Hellwig wrote:
> The xfs_dquot structure currently uses the anti-pattern of using the
> in-object lock that protects the content to also serialize reference
> count updates for the structure, leading to a cumbersome free path.
> This is partiall papered over by the fact that we never free the dquot
partially
> directly but always through the LRU. Switch to use a lockref instead and
> move the reference counter manipulations out of q_qlock.
>
> To make this work, xfs_qm_flush_one and xfs_qm_flush_one are converted to
> acquire a dquot reference while flushing to integrate with the lockref
> "get if not dead" scheme.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/xfs/libxfs/xfs_quota_defs.h | 4 +--
> fs/xfs/xfs_dquot.c | 17 ++++++------
> fs/xfs/xfs_dquot.h | 6 ++--
> fs/xfs/xfs_qm.c | 50 ++++++++++++++++------------------
> fs/xfs/xfs_trace.h | 2 +-
> 5 files changed, 37 insertions(+), 42 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_quota_defs.h b/fs/xfs/libxfs/xfs_quota_defs.h
> index 763d941a8420..551d7ae46c5c 100644
> --- a/fs/xfs/libxfs/xfs_quota_defs.h
> +++ b/fs/xfs/libxfs/xfs_quota_defs.h
> @@ -29,11 +29,9 @@ typedef uint8_t xfs_dqtype_t;
> * flags for q_flags field in the dquot.
> */
> #define XFS_DQFLAG_DIRTY (1u << 0) /* dquot is dirty */
> -#define XFS_DQFLAG_FREEING (1u << 1) /* dquot is being torn down */
>
> #define XFS_DQFLAG_STRINGS \
> - { XFS_DQFLAG_DIRTY, "DIRTY" }, \
> - { XFS_DQFLAG_FREEING, "FREEING" }
> + { XFS_DQFLAG_DIRTY, "DIRTY" }
>
> /*
> * We have the possibility of all three quota types being active at once, and
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index 97f037fa4181..e53dffe2dcab 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -816,20 +816,17 @@ xfs_qm_dqget_cache_lookup(
> return NULL;
> }
>
> - mutex_lock(&dqp->q_qlock);
> - if (dqp->q_flags & XFS_DQFLAG_FREEING) {
> - mutex_unlock(&dqp->q_qlock);
> + if (!lockref_get_not_dead(&dqp->q_lockref)) {
> mutex_unlock(&qi->qi_tree_lock);
> trace_xfs_dqget_freeing(dqp);
> delay(1);
> goto restart;
> }
> -
> - dqp->q_nrefs++;
> mutex_unlock(&qi->qi_tree_lock);
>
> trace_xfs_dqget_hit(dqp);
> XFS_STATS_INC(mp, xs_qm_dqcachehits);
> + mutex_lock(&dqp->q_qlock);
> return dqp;
> }
>
> @@ -867,7 +864,7 @@ xfs_qm_dqget_cache_insert(
>
> /* Return a locked dquot to the caller, with a reference taken. */
> mutex_lock(&dqp->q_qlock);
> - dqp->q_nrefs = 1;
> + lockref_init(&dqp->q_lockref);
> qi->qi_dquots++;
>
> out_unlock:
> @@ -1119,18 +1116,22 @@ void
> xfs_qm_dqput(
> struct xfs_dquot *dqp)
> {
> - ASSERT(dqp->q_nrefs > 0);
> ASSERT(XFS_DQ_IS_LOCKED(dqp));
>
> trace_xfs_dqput(dqp);
>
> - if (--dqp->q_nrefs == 0) {
> + if (lockref_put_or_lock(&dqp->q_lockref))
> + goto out_unlock;
> +
> + if (!--dqp->q_lockref.count) {
> struct xfs_quotainfo *qi = dqp->q_mount->m_quotainfo;
> trace_xfs_dqput_free(dqp);
>
> if (list_lru_add_obj(&qi->qi_lru, &dqp->q_lru))
> XFS_STATS_INC(dqp->q_mount, xs_qm_dquot_unused);
> }
> + spin_unlock(&dqp->q_lockref.lock);
> +out_unlock:
> mutex_unlock(&dqp->q_qlock);
> }
>
> diff --git a/fs/xfs/xfs_dquot.h b/fs/xfs/xfs_dquot.h
> index 10c39b8cdd03..c56fbc39d089 100644
> --- a/fs/xfs/xfs_dquot.h
> +++ b/fs/xfs/xfs_dquot.h
> @@ -71,7 +71,7 @@ struct xfs_dquot {
> xfs_dqtype_t q_type;
> uint16_t q_flags;
> xfs_dqid_t q_id;
> - uint q_nrefs;
> + struct lockref q_lockref;
> int q_bufoffset;
> xfs_daddr_t q_blkno;
> xfs_fileoff_t q_fileoffset;
> @@ -231,9 +231,7 @@ void xfs_dquot_detach_buf(struct xfs_dquot *dqp);
>
> static inline struct xfs_dquot *xfs_qm_dqhold(struct xfs_dquot *dqp)
> {
> - mutex_lock(&dqp->q_qlock);
> - dqp->q_nrefs++;
> - mutex_unlock(&dqp->q_qlock);
> + lockref_get(&dqp->q_lockref);
> return dqp;
> }
>
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index ca3cbff9d873..0d2243d549ad 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -126,14 +126,16 @@ xfs_qm_dqpurge(
> void *data)
> {
> struct xfs_quotainfo *qi = dqp->q_mount->m_quotainfo;
> - int error = -EAGAIN;
>
> - mutex_lock(&dqp->q_qlock);
> - if ((dqp->q_flags & XFS_DQFLAG_FREEING) || dqp->q_nrefs != 0)
> - goto out_unlock;
> -
> - dqp->q_flags |= XFS_DQFLAG_FREEING;
> + spin_lock(&dqp->q_lockref.lock);
> + if (dqp->q_lockref.count > 0 || __lockref_is_dead(&dqp->q_lockref)) {
> + spin_unlock(&dqp->q_lockref.lock);
> + return -EAGAIN;
> + }
> + lockref_mark_dead(&dqp->q_lockref);
> + spin_unlock(&dqp->q_lockref.lock);
>
> + mutex_lock(&dqp->q_qlock);
> xfs_qm_dqunpin_wait(dqp);
> xfs_dqflock(dqp);
>
> @@ -144,6 +146,7 @@ xfs_qm_dqpurge(
> */
> if (XFS_DQ_IS_DIRTY(dqp)) {
> struct xfs_buf *bp = NULL;
> + int error;
>
> /*
> * We don't care about getting disk errors here. We need
> @@ -151,9 +154,9 @@ xfs_qm_dqpurge(
> */
> error = xfs_dquot_use_attached_buf(dqp, &bp);
> if (error == -EAGAIN) {
> - xfs_dqfunlock(dqp);
> - dqp->q_flags &= ~XFS_DQFLAG_FREEING;
> - goto out_unlock;
> + /* resurrect the refcount from the dead. */
> + dqp->q_lockref.count = 0;
Heh, undead dquots. With the typo fixed,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
--D
> + goto out_funlock;
> }
> if (!bp)
> goto out_funlock;
> @@ -192,10 +195,6 @@ xfs_qm_dqpurge(
>
> xfs_qm_dqdestroy(dqp);
> return 0;
> -
> -out_unlock:
> - mutex_unlock(&dqp->q_qlock);
> - return error;
> }
>
> /*
> @@ -468,7 +467,7 @@ xfs_qm_dquot_isolate(
> struct xfs_qm_isolate *isol = arg;
> enum lru_status ret = LRU_SKIP;
>
> - if (!mutex_trylock(&dqp->q_qlock))
> + if (!spin_trylock(&dqp->q_lockref.lock))
> goto out_miss_busy;
>
> /*
> @@ -476,7 +475,7 @@ xfs_qm_dquot_isolate(
> * from the LRU, leave it for the freeing task to complete the freeing
> * process rather than risk it being free from under us here.
> */
> - if (dqp->q_flags & XFS_DQFLAG_FREEING)
> + if (__lockref_is_dead(&dqp->q_lockref))
> goto out_miss_unlock;
>
> /*
> @@ -485,16 +484,15 @@ xfs_qm_dquot_isolate(
> * again.
> */
> ret = LRU_ROTATE;
> - if (XFS_DQ_IS_DIRTY(dqp) || atomic_read(&dqp->q_pincount) > 0) {
> + if (XFS_DQ_IS_DIRTY(dqp) || atomic_read(&dqp->q_pincount) > 0)
> goto out_miss_unlock;
> - }
>
> /*
> * This dquot has acquired a reference in the meantime remove it from
> * the freelist and try again.
> */
> - if (dqp->q_nrefs) {
> - mutex_unlock(&dqp->q_qlock);
> + if (dqp->q_lockref.count) {
> + spin_unlock(&dqp->q_lockref.lock);
> XFS_STATS_INC(dqp->q_mount, xs_qm_dqwants);
>
> trace_xfs_dqreclaim_want(dqp);
> @@ -518,10 +516,9 @@ xfs_qm_dquot_isolate(
> /*
> * Prevent lookups now that we are past the point of no return.
> */
> - dqp->q_flags |= XFS_DQFLAG_FREEING;
> - mutex_unlock(&dqp->q_qlock);
> + lockref_mark_dead(&dqp->q_lockref);
> + spin_unlock(&dqp->q_lockref.lock);
>
> - ASSERT(dqp->q_nrefs == 0);
> list_lru_isolate_move(lru, &dqp->q_lru, &isol->dispose);
> XFS_STATS_DEC(dqp->q_mount, xs_qm_dquot_unused);
> trace_xfs_dqreclaim_done(dqp);
> @@ -529,7 +526,7 @@ xfs_qm_dquot_isolate(
> return LRU_REMOVED;
>
> out_miss_unlock:
> - mutex_unlock(&dqp->q_qlock);
> + spin_unlock(&dqp->q_lockref.lock);
> out_miss_busy:
> trace_xfs_dqreclaim_busy(dqp);
> XFS_STATS_INC(dqp->q_mount, xs_qm_dqreclaim_misses);
> @@ -1466,9 +1463,10 @@ xfs_qm_flush_one(
> struct xfs_buf *bp = NULL;
> int error = 0;
>
> + if (!lockref_get_not_dead(&dqp->q_lockref))
> + return 0;
> +
> mutex_lock(&dqp->q_qlock);
> - if (dqp->q_flags & XFS_DQFLAG_FREEING)
> - goto out_unlock;
> if (!XFS_DQ_IS_DIRTY(dqp))
> goto out_unlock;
>
> @@ -1488,7 +1486,7 @@ xfs_qm_flush_one(
> xfs_buf_delwri_queue(bp, buffer_list);
> xfs_buf_relse(bp);
> out_unlock:
> - mutex_unlock(&dqp->q_qlock);
> + xfs_qm_dqput(dqp);
> return error;
> }
>
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 79b8641880ab..46d21eb11ccb 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -1350,7 +1350,7 @@ DECLARE_EVENT_CLASS(xfs_dquot_class,
> __entry->id = dqp->q_id;
> __entry->type = dqp->q_type;
> __entry->flags = dqp->q_flags;
> - __entry->nrefs = dqp->q_nrefs;
> + __entry->nrefs = data_race(dqp->q_lockref.count);
>
> __entry->res_bcount = dqp->q_blk.reserved;
> __entry->res_rtbcount = dqp->q_rtb.reserved;
> --
> 2.47.3
>
>
next prev parent reply other threads:[~2025-10-15 21:02 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-10-13 2:48 cleanup quota locking Christoph Hellwig
2025-10-13 2:48 ` [PATCH 01/17] xfs: make qi_dquots a 64-bit value Christoph Hellwig
2025-10-14 23:16 ` Darrick J. Wong
2025-10-15 4:48 ` Christoph Hellwig
2025-10-13 2:48 ` [PATCH 02/17] xfs: remove xfs_dqunlock and friends Christoph Hellwig
2025-10-14 23:17 ` Darrick J. Wong
2025-10-13 2:48 ` [PATCH 03/17] xfs: don't lock the dquot before return in xqcheck_commit_dquot Christoph Hellwig
2025-10-14 23:22 ` Darrick J. Wong
2025-10-15 5:00 ` Christoph Hellwig
2025-10-15 20:27 ` Darrick J. Wong
2025-10-13 2:48 ` [PATCH 04/17] xfs: don't lock the dquot before return in xrep_quota_item Christoph Hellwig
2025-10-14 23:24 ` Darrick J. Wong
2025-10-13 2:48 ` [PATCH 05/17] xfs: use a lockref for the xfs_dquot reference count Christoph Hellwig
2025-10-15 21:02 ` Darrick J. Wong [this message]
2025-10-13 2:48 ` [PATCH 06/17] xfs: remove xfs_qm_dqput and optimize dropping dquot references Christoph Hellwig
2025-10-15 21:04 ` Darrick J. Wong
2025-10-13 2:48 ` [PATCH 07/17] xfs: consolidate q_qlock locking in xfs_qm_dqget and xfs_qm_dqget_inode Christoph Hellwig
2025-10-15 21:05 ` Darrick J. Wong
2025-10-13 2:48 ` [PATCH 08/17] xfs: xfs_qm_dqattach_one is never called with a non-NULL *IO_idqpp Christoph Hellwig
2025-10-14 23:27 ` Darrick J. Wong
2025-10-13 2:48 ` [PATCH 09/17] xfs: fold xfs_qm_dqattach_one into xfs_qm_dqget_inode Christoph Hellwig
2025-10-15 21:13 ` Darrick J. Wong
2025-10-13 2:48 ` [PATCH 10/17] xfs: return the dquot unlocked from xfs_qm_dqget Christoph Hellwig
2025-10-15 21:17 ` Darrick J. Wong
2025-10-15 21:18 ` Darrick J. Wong
2025-10-16 4:21 ` Christoph Hellwig
2025-10-13 2:48 ` [PATCH 11/17] xfs: remove q_qlock locking in xfs_qm_scall_setqlim Christoph Hellwig
2025-10-15 21:17 ` Darrick J. Wong
2025-10-13 2:48 ` [PATCH 12/17] xfs: push q_qlock acquisition from xchk_dquot_iter to the callers Christoph Hellwig
2025-10-15 21:19 ` Darrick J. Wong
2025-10-13 2:48 ` [PATCH 13/17] xfs: move q_qlock locking into xchk_quota_item Christoph Hellwig
2025-10-15 21:19 ` Darrick J. Wong
2025-10-13 2:48 ` [PATCH 14/17] xfs: move q_qlock locking into xqcheck_compare_dquot Christoph Hellwig
2025-10-15 21:20 ` Darrick J. Wong
2025-10-13 2:48 ` [PATCH 15/17] xfs: move q_qlock acquisition into xqcheck_commit_dquot Christoph Hellwig
2025-10-15 21:20 ` Darrick J. Wong
2025-10-16 4:22 ` Christoph Hellwig
2025-10-13 2:48 ` [PATCH 16/17] xfs: move xfs_dquot_tree calls into xfs_qm_dqget_cache_{lookup,insert} Christoph Hellwig
2025-10-15 21:21 ` Darrick J. Wong
2025-10-13 2:48 ` [PATCH 17/17] xfs: reduce ilock roundtrips in xfs_qm_vop_dqalloc Christoph Hellwig
2025-10-15 21:27 ` Darrick J. Wong
2025-10-16 4:23 ` Christoph Hellwig
2025-10-16 15:59 ` Darrick J. Wong
2025-10-17 3:50 ` Christoph Hellwig
2025-10-17 23:09 ` Darrick J. Wong
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20251015210219.GA2591640@frogsfrogsfrogs \
--to=djwong@kernel.org \
--cc=cem@kernel.org \
--cc=hch@lst.de \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox