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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.