cleanup quota locking

cleanup quota locking

[Christoph Hellwig]

Hi all,

this series cleans up the xfs quota locking, but splitting the
synchronization of the quota refcount from the protection of the data
in the object, and then leveraging that to push down the content locking
only into the places that need it.

Diffstat:
 libxfs/xfs_quota_defs.h   |    4 -
 scrub/quota.c             |    8 --
 scrub/quota_repair.c      |   11 +--
 scrub/quotacheck.c        |    6 +
 scrub/quotacheck_repair.c |   15 +---
 xfs_dquot.c               |  112 ++++++++++++----------------------
 xfs_dquot.h               |   22 ------
 xfs_dquot_item.c          |    6 -
 xfs_qm.c                  |  150 +++++++++++-----------------------------------
 xfs_qm.h                  |    2 
 xfs_qm_bhv.c              |    4 -
 xfs_qm_syscalls.c         |   10 +--
 xfs_quotaops.c            |    2 
 xfs_trace.h               |    6 -
 xfs_trans_dquot.c         |   18 ++---
 15 files changed, 124 insertions(+), 252 deletions(-)

[PATCH 01/17] xfs: make qi_dquots a 64-bit value

[Christoph Hellwig]

qi_dquots counts all quotas in the file system, which can be up to
3 * UINT_MAX and overflow a 32-bit counter, but can't be negative.
Make qi_dquots a uint64_t, and saturate the value to UINT_MAX for
userspace reporting.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_qm.h       | 2 +-
 fs/xfs/xfs_quotaops.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_qm.h b/fs/xfs/xfs_qm.h
index 35b64bc3a7a8..e88ed6ad0e65 100644
--- a/fs/xfs/xfs_qm.h
+++ b/fs/xfs/xfs_qm.h
@@ -57,7 +57,7 @@ struct xfs_quotainfo {
 	struct xfs_inode	*qi_pquotaip;	/* project quota inode */
 	struct xfs_inode	*qi_dirip;	/* quota metadir */
 	struct list_lru		qi_lru;
-	int			qi_dquots;
+	uint64_t		qi_dquots;
 	struct mutex		qi_quotaofflock;/* to serialize quotaoff */
 	xfs_filblks_t		qi_dqchunklen;	/* # BBs in a chunk of dqs */
 	uint			qi_dqperchunk;	/* # ondisk dq in above chunk */
diff --git a/fs/xfs/xfs_quotaops.c b/fs/xfs/xfs_quotaops.c
index 4c7f7ce4fd2f..1045c4c262ad 100644
--- a/fs/xfs/xfs_quotaops.c
+++ b/fs/xfs/xfs_quotaops.c
@@ -65,7 +65,7 @@ xfs_fs_get_quota_state(
 	memset(state, 0, sizeof(*state));
 	if (!XFS_IS_QUOTA_ON(mp))
 		return 0;
-	state->s_incoredqs = q->qi_dquots;
+	state->s_incoredqs = max_t(uint64_t, q->qi_dquots, UINT_MAX);
 	if (XFS_IS_UQUOTA_ON(mp))
 		state->s_state[USRQUOTA].flags |= QCI_ACCT_ENABLED;
 	if (XFS_IS_UQUOTA_ENFORCED(mp))
-- 
2.47.3

[PATCH 02/17] xfs: remove xfs_dqunlock and friends

[Christoph Hellwig]

There's really no point in wrapping the basic mutex operations.  Remove
the wrapper to ease lock analysis annotations and make the code a litte
easier to read.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/scrub/quota.c             |  4 ++--
 fs/xfs/scrub/quota_repair.c      |  6 +++---
 fs/xfs/scrub/quotacheck_repair.c |  8 ++++----
 fs/xfs/xfs_dquot.c               | 14 +++++++-------
 fs/xfs/xfs_dquot.h               | 19 ++-----------------
 fs/xfs/xfs_dquot_item.c          |  6 +++---
 fs/xfs/xfs_qm.c                  | 30 +++++++++++++++---------------
 fs/xfs/xfs_qm_syscalls.c         |  4 ++--
 fs/xfs/xfs_trans_dquot.c         | 18 +++++++++---------
 9 files changed, 47 insertions(+), 62 deletions(-)

diff --git a/fs/xfs/scrub/quota.c b/fs/xfs/scrub/quota.c
index 58d6d4ed2853..c78cf9f96cf6 100644
--- a/fs/xfs/scrub/quota.c
+++ b/fs/xfs/scrub/quota.c
@@ -158,9 +158,9 @@ xchk_quota_item(
 	 * However, dqiterate gave us a locked dquot, so drop the dquot lock to
 	 * get the ILOCK.
 	 */
-	xfs_dqunlock(dq);
+	mutex_unlock(&dq->q_qlock);
 	xchk_ilock(sc, XFS_ILOCK_SHARED);
-	xfs_dqlock(dq);
+	mutex_lock(&dq->q_qlock);
 
 	/*
 	 * Except for the root dquot, the actual dquot we got must either have
diff --git a/fs/xfs/scrub/quota_repair.c b/fs/xfs/scrub/quota_repair.c
index 8f4c8d41f308..8c89c6cc2950 100644
--- a/fs/xfs/scrub/quota_repair.c
+++ b/fs/xfs/scrub/quota_repair.c
@@ -187,9 +187,9 @@ xrep_quota_item(
 	 * dqiterate gave us a locked dquot, so drop the dquot lock to get the
 	 * ILOCK_EXCL.
 	 */
-	xfs_dqunlock(dq);
+	mutex_unlock(&dq->q_qlock);
 	xchk_ilock(sc, XFS_ILOCK_EXCL);
-	xfs_dqlock(dq);
+	mutex_lock(&dq->q_qlock);
 
 	error = xrep_quota_item_bmap(sc, dq, &dirty);
 	xchk_iunlock(sc, XFS_ILOCK_EXCL);
@@ -258,7 +258,7 @@ xrep_quota_item(
 	}
 	xfs_trans_log_dquot(sc->tp, dq);
 	error = xfs_trans_roll(&sc->tp);
-	xfs_dqlock(dq);
+	mutex_lock(&dq->q_qlock);
 	return error;
 }
 
diff --git a/fs/xfs/scrub/quotacheck_repair.c b/fs/xfs/scrub/quotacheck_repair.c
index dd8554c755b5..415314911499 100644
--- a/fs/xfs/scrub/quotacheck_repair.c
+++ b/fs/xfs/scrub/quotacheck_repair.c
@@ -53,9 +53,9 @@ xqcheck_commit_dquot(
 	int			error = 0;
 
 	/* Unlock the dquot just long enough to allocate a transaction. */
-	xfs_dqunlock(dq);
+	mutex_unlock(&dq->q_qlock);
 	error = xchk_trans_alloc(xqc->sc, 0);
-	xfs_dqlock(dq);
+	mutex_lock(&dq->q_qlock);
 	if (error)
 		return error;
 
@@ -122,7 +122,7 @@ xqcheck_commit_dquot(
 	 * dquot).
 	 */
 	error = xrep_trans_commit(xqc->sc);
-	xfs_dqlock(dq);
+	mutex_lock(&dq->q_qlock);
 	return error;
 
 out_unlock:
@@ -131,7 +131,7 @@ xqcheck_commit_dquot(
 	xchk_trans_cancel(xqc->sc);
 
 	/* Re-lock the dquot so the caller can put the reference. */
-	xfs_dqlock(dq);
+	mutex_lock(&dq->q_qlock);
 	return error;
 }
 
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 0bd8022e47b4..97f037fa4181 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -31,7 +31,7 @@
  *
  * ip->i_lock
  *   qi->qi_tree_lock
- *     dquot->q_qlock (xfs_dqlock() and friends)
+ *     dquot->q_qlock
  *       dquot->q_flush (xfs_dqflock() and friends)
  *       qi->qi_lru_lock
  *
@@ -816,9 +816,9 @@ xfs_qm_dqget_cache_lookup(
 		return NULL;
 	}
 
-	xfs_dqlock(dqp);
+	mutex_lock(&dqp->q_qlock);
 	if (dqp->q_flags & XFS_DQFLAG_FREEING) {
-		xfs_dqunlock(dqp);
+		mutex_unlock(&dqp->q_qlock);
 		mutex_unlock(&qi->qi_tree_lock);
 		trace_xfs_dqget_freeing(dqp);
 		delay(1);
@@ -866,7 +866,7 @@ xfs_qm_dqget_cache_insert(
 	}
 
 	/* Return a locked dquot to the caller, with a reference taken. */
-	xfs_dqlock(dqp);
+	mutex_lock(&dqp->q_qlock);
 	dqp->q_nrefs = 1;
 	qi->qi_dquots++;
 
@@ -1049,7 +1049,7 @@ xfs_qm_dqget_inode(
 		if (dqp1) {
 			xfs_qm_dqdestroy(dqp);
 			dqp = dqp1;
-			xfs_dqlock(dqp);
+			mutex_lock(&dqp->q_qlock);
 			goto dqret;
 		}
 	} else {
@@ -1131,7 +1131,7 @@ xfs_qm_dqput(
 		if (list_lru_add_obj(&qi->qi_lru, &dqp->q_lru))
 			XFS_STATS_INC(dqp->q_mount, xs_qm_dquot_unused);
 	}
-	xfs_dqunlock(dqp);
+	mutex_unlock(&dqp->q_qlock);
 }
 
 /*
@@ -1147,7 +1147,7 @@ xfs_qm_dqrele(
 
 	trace_xfs_dqrele(dqp);
 
-	xfs_dqlock(dqp);
+	mutex_lock(&dqp->q_qlock);
 	/*
 	 * We don't care to flush it if the dquot is dirty here.
 	 * That will create stutters that we want to avoid.
diff --git a/fs/xfs/xfs_dquot.h b/fs/xfs/xfs_dquot.h
index 61217adf5ba5..10c39b8cdd03 100644
--- a/fs/xfs/xfs_dquot.h
+++ b/fs/xfs/xfs_dquot.h
@@ -121,21 +121,6 @@ static inline void xfs_dqfunlock(struct xfs_dquot *dqp)
 	complete(&dqp->q_flush);
 }
 
-static inline int xfs_dqlock_nowait(struct xfs_dquot *dqp)
-{
-	return mutex_trylock(&dqp->q_qlock);
-}
-
-static inline void xfs_dqlock(struct xfs_dquot *dqp)
-{
-	mutex_lock(&dqp->q_qlock);
-}
-
-static inline void xfs_dqunlock(struct xfs_dquot *dqp)
-{
-	mutex_unlock(&dqp->q_qlock);
-}
-
 static inline int
 xfs_dquot_type(const struct xfs_dquot *dqp)
 {
@@ -246,9 +231,9 @@ void xfs_dquot_detach_buf(struct xfs_dquot *dqp);
 
 static inline struct xfs_dquot *xfs_qm_dqhold(struct xfs_dquot *dqp)
 {
-	xfs_dqlock(dqp);
+	mutex_lock(&dqp->q_qlock);
 	dqp->q_nrefs++;
-	xfs_dqunlock(dqp);
+	mutex_unlock(&dqp->q_qlock);
 	return dqp;
 }
 
diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
index 271b195ebb93..b374cd9f1900 100644
--- a/fs/xfs/xfs_dquot_item.c
+++ b/fs/xfs/xfs_dquot_item.c
@@ -132,7 +132,7 @@ xfs_qm_dquot_logitem_push(
 	if (atomic_read(&dqp->q_pincount) > 0)
 		return XFS_ITEM_PINNED;
 
-	if (!xfs_dqlock_nowait(dqp))
+	if (!mutex_trylock(&dqp->q_qlock))
 		return XFS_ITEM_LOCKED;
 
 	/*
@@ -177,7 +177,7 @@ xfs_qm_dquot_logitem_push(
 out_relock_ail:
 	spin_lock(&lip->li_ailp->ail_lock);
 out_unlock:
-	xfs_dqunlock(dqp);
+	mutex_unlock(&dqp->q_qlock);
 	return rval;
 }
 
@@ -195,7 +195,7 @@ xfs_qm_dquot_logitem_release(
 	 * transaction layer, within trans_commit. Hence, no LI_HOLD flag
 	 * for the logitem.
 	 */
-	xfs_dqunlock(dqp);
+	mutex_unlock(&dqp->q_qlock);
 }
 
 STATIC void
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 23ba84ec919a..ca3cbff9d873 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -128,7 +128,7 @@ xfs_qm_dqpurge(
 	struct xfs_quotainfo	*qi = dqp->q_mount->m_quotainfo;
 	int			error = -EAGAIN;
 
-	xfs_dqlock(dqp);
+	mutex_lock(&dqp->q_qlock);
 	if ((dqp->q_flags & XFS_DQFLAG_FREEING) || dqp->q_nrefs != 0)
 		goto out_unlock;
 
@@ -177,7 +177,7 @@ xfs_qm_dqpurge(
 		!test_bit(XFS_LI_IN_AIL, &dqp->q_logitem.qli_item.li_flags));
 
 	xfs_dqfunlock(dqp);
-	xfs_dqunlock(dqp);
+	mutex_unlock(&dqp->q_qlock);
 
 	radix_tree_delete(xfs_dquot_tree(qi, xfs_dquot_type(dqp)), dqp->q_id);
 	qi->qi_dquots--;
@@ -194,7 +194,7 @@ xfs_qm_dqpurge(
 	return 0;
 
 out_unlock:
-	xfs_dqunlock(dqp);
+	mutex_unlock(&dqp->q_qlock);
 	return error;
 }
 
@@ -329,7 +329,7 @@ xfs_qm_dqattach_one(
 	 * that the dquot returned is the one that should go in the inode.
 	 */
 	*IO_idqpp = dqp;
-	xfs_dqunlock(dqp);
+	mutex_unlock(&dqp->q_qlock);
 	return 0;
 }
 
@@ -468,7 +468,7 @@ xfs_qm_dquot_isolate(
 	struct xfs_qm_isolate	*isol = arg;
 	enum lru_status		ret = LRU_SKIP;
 
-	if (!xfs_dqlock_nowait(dqp))
+	if (!mutex_trylock(&dqp->q_qlock))
 		goto out_miss_busy;
 
 	/*
@@ -494,7 +494,7 @@ xfs_qm_dquot_isolate(
 	 * the freelist and try again.
 	 */
 	if (dqp->q_nrefs) {
-		xfs_dqunlock(dqp);
+		mutex_unlock(&dqp->q_qlock);
 		XFS_STATS_INC(dqp->q_mount, xs_qm_dqwants);
 
 		trace_xfs_dqreclaim_want(dqp);
@@ -519,7 +519,7 @@ xfs_qm_dquot_isolate(
 	 * Prevent lookups now that we are past the point of no return.
 	 */
 	dqp->q_flags |= XFS_DQFLAG_FREEING;
-	xfs_dqunlock(dqp);
+	mutex_unlock(&dqp->q_qlock);
 
 	ASSERT(dqp->q_nrefs == 0);
 	list_lru_isolate_move(lru, &dqp->q_lru, &isol->dispose);
@@ -529,7 +529,7 @@ xfs_qm_dquot_isolate(
 	return LRU_REMOVED;
 
 out_miss_unlock:
-	xfs_dqunlock(dqp);
+	mutex_unlock(&dqp->q_qlock);
 out_miss_busy:
 	trace_xfs_dqreclaim_busy(dqp);
 	XFS_STATS_INC(dqp->q_mount, xs_qm_dqreclaim_misses);
@@ -1466,7 +1466,7 @@ xfs_qm_flush_one(
 	struct xfs_buf		*bp = NULL;
 	int			error = 0;
 
-	xfs_dqlock(dqp);
+	mutex_lock(&dqp->q_qlock);
 	if (dqp->q_flags & XFS_DQFLAG_FREEING)
 		goto out_unlock;
 	if (!XFS_DQ_IS_DIRTY(dqp))
@@ -1488,7 +1488,7 @@ xfs_qm_flush_one(
 		xfs_buf_delwri_queue(bp, buffer_list);
 	xfs_buf_relse(bp);
 out_unlock:
-	xfs_dqunlock(dqp);
+	mutex_unlock(&dqp->q_qlock);
 	return error;
 }
 
@@ -1951,7 +1951,7 @@ xfs_qm_vop_dqalloc(
 			/*
 			 * Get the ilock in the right order.
 			 */
-			xfs_dqunlock(uq);
+			mutex_unlock(&uq->q_qlock);
 			lockflags = XFS_ILOCK_SHARED;
 			xfs_ilock(ip, lockflags);
 		} else {
@@ -1973,7 +1973,7 @@ xfs_qm_vop_dqalloc(
 				ASSERT(error != -ENOENT);
 				goto error_rele;
 			}
-			xfs_dqunlock(gq);
+			mutex_unlock(&gq->q_qlock);
 			lockflags = XFS_ILOCK_SHARED;
 			xfs_ilock(ip, lockflags);
 		} else {
@@ -1991,7 +1991,7 @@ xfs_qm_vop_dqalloc(
 				ASSERT(error != -ENOENT);
 				goto error_rele;
 			}
-			xfs_dqunlock(pq);
+			mutex_unlock(&pq->q_qlock);
 			lockflags = XFS_ILOCK_SHARED;
 			xfs_ilock(ip, lockflags);
 		} else {
@@ -2078,7 +2078,7 @@ xfs_qm_vop_chown(
 	 * back now.
 	 */
 	tp->t_flags |= XFS_TRANS_DIRTY;
-	xfs_dqlock(prevdq);
+	mutex_lock(&prevdq->q_qlock);
 	if (isrt) {
 		ASSERT(prevdq->q_rtb.reserved >= ip->i_delayed_blks);
 		prevdq->q_rtb.reserved -= ip->i_delayed_blks;
@@ -2086,7 +2086,7 @@ xfs_qm_vop_chown(
 		ASSERT(prevdq->q_blk.reserved >= ip->i_delayed_blks);
 		prevdq->q_blk.reserved -= ip->i_delayed_blks;
 	}
-	xfs_dqunlock(prevdq);
+	mutex_unlock(&prevdq->q_qlock);
 
 	/*
 	 * Take an extra reference, because the inode is going to keep
diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
index 0c78f30fa4a3..59ef382900fe 100644
--- a/fs/xfs/xfs_qm_syscalls.c
+++ b/fs/xfs/xfs_qm_syscalls.c
@@ -303,13 +303,13 @@ xfs_qm_scall_setqlim(
 	}
 
 	defq = xfs_get_defquota(q, xfs_dquot_type(dqp));
-	xfs_dqunlock(dqp);
+	mutex_unlock(&dqp->q_qlock);
 
 	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_qm_setqlim, 0, 0, 0, &tp);
 	if (error)
 		goto out_rele;
 
-	xfs_dqlock(dqp);
+	mutex_lock(&dqp->q_qlock);
 	xfs_trans_dqjoin(tp, dqp);
 
 	/*
diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
index 765456bf3428..c842ce06acd6 100644
--- a/fs/xfs/xfs_trans_dquot.c
+++ b/fs/xfs/xfs_trans_dquot.c
@@ -393,7 +393,7 @@ xfs_trans_dqlockedjoin(
 	unsigned int		i;
 	ASSERT(q[0].qt_dquot != NULL);
 	if (q[1].qt_dquot == NULL) {
-		xfs_dqlock(q[0].qt_dquot);
+		mutex_lock(&q[0].qt_dquot->q_qlock);
 		xfs_trans_dqjoin(tp, q[0].qt_dquot);
 	} else if (q[2].qt_dquot == NULL) {
 		xfs_dqlock2(q[0].qt_dquot, q[1].qt_dquot);
@@ -693,7 +693,7 @@ xfs_trans_unreserve_and_mod_dquots(
 			locked = already_locked;
 			if (qtrx->qt_blk_res) {
 				if (!locked) {
-					xfs_dqlock(dqp);
+					mutex_lock(&dqp->q_qlock);
 					locked = true;
 				}
 				dqp->q_blk.reserved -=
@@ -701,7 +701,7 @@ xfs_trans_unreserve_and_mod_dquots(
 			}
 			if (qtrx->qt_ino_res) {
 				if (!locked) {
-					xfs_dqlock(dqp);
+					mutex_lock(&dqp->q_qlock);
 					locked = true;
 				}
 				dqp->q_ino.reserved -=
@@ -710,14 +710,14 @@ xfs_trans_unreserve_and_mod_dquots(
 
 			if (qtrx->qt_rtblk_res) {
 				if (!locked) {
-					xfs_dqlock(dqp);
+					mutex_lock(&dqp->q_qlock);
 					locked = true;
 				}
 				dqp->q_rtb.reserved -=
 					(xfs_qcnt_t)qtrx->qt_rtblk_res;
 			}
 			if (locked && !already_locked)
-				xfs_dqunlock(dqp);
+				mutex_unlock(&dqp->q_qlock);
 
 		}
 	}
@@ -820,7 +820,7 @@ xfs_trans_dqresv(
 	struct xfs_dquot_res	*blkres;
 	struct xfs_quota_limits	*qlim;
 
-	xfs_dqlock(dqp);
+	mutex_lock(&dqp->q_qlock);
 
 	defq = xfs_get_defquota(q, xfs_dquot_type(dqp));
 
@@ -887,16 +887,16 @@ xfs_trans_dqresv(
 	    XFS_IS_CORRUPT(mp, dqp->q_ino.reserved < dqp->q_ino.count))
 		goto error_corrupt;
 
-	xfs_dqunlock(dqp);
+	mutex_unlock(&dqp->q_qlock);
 	return 0;
 
 error_return:
-	xfs_dqunlock(dqp);
+	mutex_unlock(&dqp->q_qlock);
 	if (xfs_dquot_type(dqp) == XFS_DQTYPE_PROJ)
 		return -ENOSPC;
 	return -EDQUOT;
 error_corrupt:
-	xfs_dqunlock(dqp);
+	mutex_unlock(&dqp->q_qlock);
 	xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
 	xfs_fs_mark_sick(mp, XFS_SICK_FS_QUOTACHECK);
 	return -EFSCORRUPTED;
-- 
2.47.3

[PATCH 03/17] xfs: don't lock the dquot before return in xqcheck_commit_dquot

[Christoph Hellwig]

While xfs_qm_dqput requires the dquot to be locked, the callers can use
the more common xfs_qm_dqrele helper that takes care of locking the dquot
instead.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/scrub/quotacheck_repair.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/fs/xfs/scrub/quotacheck_repair.c b/fs/xfs/scrub/quotacheck_repair.c
index 415314911499..67bdc872996a 100644
--- a/fs/xfs/scrub/quotacheck_repair.c
+++ b/fs/xfs/scrub/quotacheck_repair.c
@@ -121,17 +121,12 @@ xqcheck_commit_dquot(
 	 * the caller can put the reference (which apparently requires a locked
 	 * dquot).
 	 */
-	error = xrep_trans_commit(xqc->sc);
-	mutex_lock(&dq->q_qlock);
-	return error;
+	return xrep_trans_commit(xqc->sc);
 
 out_unlock:
 	mutex_unlock(&xqc->lock);
 out_cancel:
 	xchk_trans_cancel(xqc->sc);
-
-	/* Re-lock the dquot so the caller can put the reference. */
-	mutex_lock(&dq->q_qlock);
 	return error;
 }
 
@@ -156,7 +151,7 @@ xqcheck_commit_dqtype(
 	xchk_dqiter_init(&cursor, sc, dqtype);
 	while ((error = xchk_dquot_iter(&cursor, &dq)) == 1) {
 		error = xqcheck_commit_dquot(xqc, dqtype, dq);
-		xfs_qm_dqput(dq);
+		xfs_qm_dqrele(dq);
 		if (error)
 			break;
 	}
@@ -187,7 +182,7 @@ xqcheck_commit_dqtype(
 			return error;
 
 		error = xqcheck_commit_dquot(xqc, dqtype, dq);
-		xfs_qm_dqput(dq);
+		xfs_qm_dqrele(dq);
 		if (error)
 			return error;
 
-- 
2.47.3

[PATCH 04/17] xfs: don't lock the dquot before return in xrep_quota_item

[Christoph Hellwig]

While xfs_qm_dqput requires the dquot to be locked, the caller can use
the more common xfs_qm_dqrele helper that takes care of locking the
dquot instead.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/scrub/quota_repair.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/scrub/quota_repair.c b/fs/xfs/scrub/quota_repair.c
index 8c89c6cc2950..00a4d2e75797 100644
--- a/fs/xfs/scrub/quota_repair.c
+++ b/fs/xfs/scrub/quota_repair.c
@@ -257,9 +257,7 @@ xrep_quota_item(
 		xfs_qm_adjust_dqtimers(dq);
 	}
 	xfs_trans_log_dquot(sc->tp, dq);
-	error = xfs_trans_roll(&sc->tp);
-	mutex_lock(&dq->q_qlock);
-	return error;
+	return xfs_trans_roll(&sc->tp);
 }
 
 /* Fix a quota timer so that we can pass the verifier. */
@@ -513,7 +511,7 @@ xrep_quota_problems(
 	xchk_dqiter_init(&cursor, sc, dqtype);
 	while ((error = xchk_dquot_iter(&cursor, &dq)) == 1) {
 		error = xrep_quota_item(&rqi, dq);
-		xfs_qm_dqput(dq);
+		xfs_qm_dqrele(dq);
 		if (error)
 			break;
 	}
-- 
2.47.3

[PATCH 05/17] xfs: use a lockref for the xfs_dquot reference count

[Christoph Hellwig]

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
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;
+			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

[PATCH 06/17] xfs: remove xfs_qm_dqput and optimize dropping dquot references

[Christoph Hellwig]

With the new lockref-based dquot reference counting, there is no need to
hold q_qlock for dropping the reference.  Make xfs_qm_dqrele the main
function to drop dquot references without taking q_qlock and convert all
callers of xfs_qm_dqput to unlock q_qlock and call xfs_qm_dqrele instead.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/scrub/quota.c      |  3 ++-
 fs/xfs/scrub/quotacheck.c |  6 ++++--
 fs/xfs/xfs_dquot.c        | 45 ++++++++-------------------------------
 fs/xfs/xfs_dquot.h        |  1 -
 fs/xfs/xfs_qm.c           |  7 ++++--
 fs/xfs/xfs_qm_bhv.c       |  3 ++-
 fs/xfs/xfs_qm_syscalls.c  |  6 ++++--
 fs/xfs/xfs_trace.h        |  3 +--
 8 files changed, 27 insertions(+), 47 deletions(-)

diff --git a/fs/xfs/scrub/quota.c b/fs/xfs/scrub/quota.c
index c78cf9f96cf6..cfcd0fb66845 100644
--- a/fs/xfs/scrub/quota.c
+++ b/fs/xfs/scrub/quota.c
@@ -330,7 +330,8 @@ xchk_quota(
 	xchk_dqiter_init(&cursor, sc, dqtype);
 	while ((error = xchk_dquot_iter(&cursor, &dq)) == 1) {
 		error = xchk_quota_item(&sqi, dq);
-		xfs_qm_dqput(dq);
+		mutex_unlock(&dq->q_qlock);
+		xfs_qm_dqrele(dq);
 		if (error)
 			break;
 	}
diff --git a/fs/xfs/scrub/quotacheck.c b/fs/xfs/scrub/quotacheck.c
index e4105aaafe84..180449f654f6 100644
--- a/fs/xfs/scrub/quotacheck.c
+++ b/fs/xfs/scrub/quotacheck.c
@@ -636,7 +636,8 @@ xqcheck_walk_observations(
 			return error;
 
 		error = xqcheck_compare_dquot(xqc, dqtype, dq);
-		xfs_qm_dqput(dq);
+		mutex_unlock(&dq->q_qlock);
+		xfs_qm_dqrele(dq);
 		if (error)
 			return error;
 
@@ -674,7 +675,8 @@ xqcheck_compare_dqtype(
 	xchk_dqiter_init(&cursor, sc, dqtype);
 	while ((error = xchk_dquot_iter(&cursor, &dq)) == 1) {
 		error = xqcheck_compare_dquot(xqc, dqtype, dq);
-		xfs_qm_dqput(dq);
+		mutex_unlock(&dq->q_qlock);
+		xfs_qm_dqrele(dq);
 		if (error)
 			break;
 	}
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index e53dffe2dcab..ceddbbb41999 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -1100,62 +1100,35 @@ xfs_qm_dqget_next(
 			return 0;
 		}
 
-		xfs_qm_dqput(dqp);
+		mutex_unlock(&dqp->q_qlock);
+		xfs_qm_dqrele(dqp);
 	}
 
 	return error;
 }
 
 /*
- * Release a reference to the dquot (decrement ref-count) and unlock it.
- *
- * If there is a group quota attached to this dquot, carefully release that
- * too without tripping over deadlocks'n'stuff.
+ * Release a reference to the dquot.
  */
 void
-xfs_qm_dqput(
+xfs_qm_dqrele(
 	struct xfs_dquot	*dqp)
 {
-	ASSERT(XFS_DQ_IS_LOCKED(dqp));
+	if (!dqp)
+		return;
 
-	trace_xfs_dqput(dqp);
+	trace_xfs_dqrele(dqp);
 
 	if (lockref_put_or_lock(&dqp->q_lockref))
-		goto out_unlock;
-
+		return;
 	if (!--dqp->q_lockref.count) {
 		struct xfs_quotainfo	*qi = dqp->q_mount->m_quotainfo;
-		trace_xfs_dqput_free(dqp);
 
+		trace_xfs_dqrele_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);
-}
-
-/*
- * Release a dquot. Flush it if dirty, then dqput() it.
- * dquot must not be locked.
- */
-void
-xfs_qm_dqrele(
-	struct xfs_dquot	*dqp)
-{
-	if (!dqp)
-		return;
-
-	trace_xfs_dqrele(dqp);
-
-	mutex_lock(&dqp->q_qlock);
-	/*
-	 * We don't care to flush it if the dquot is dirty here.
-	 * That will create stutters that we want to avoid.
-	 * Instead we do a delayed write when we try to reclaim
-	 * a dirty dquot. Also xfs_sync will take part of the burden...
-	 */
-	xfs_qm_dqput(dqp);
 }
 
 /*
diff --git a/fs/xfs/xfs_dquot.h b/fs/xfs/xfs_dquot.h
index c56fbc39d089..bbb824adca82 100644
--- a/fs/xfs/xfs_dquot.h
+++ b/fs/xfs/xfs_dquot.h
@@ -218,7 +218,6 @@ int		xfs_qm_dqget_next(struct xfs_mount *mp, xfs_dqid_t id,
 int		xfs_qm_dqget_uncached(struct xfs_mount *mp,
 				xfs_dqid_t id, xfs_dqtype_t type,
 				struct xfs_dquot **dqpp);
-void		xfs_qm_dqput(struct xfs_dquot *dqp);
 
 void		xfs_dqlock2(struct xfs_dquot *, struct xfs_dquot *);
 void		xfs_dqlockn(struct xfs_dqtrx *q);
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 0d2243d549ad..9bd7068b9e5a 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -1345,7 +1345,9 @@ xfs_qm_quotacheck_dqadjust(
 	}
 
 	dqp->q_flags |= XFS_DQFLAG_DIRTY;
-	xfs_qm_dqput(dqp);
+	mutex_unlock(&dqp->q_qlock);
+
+	xfs_qm_dqrele(dqp);
 	return 0;
 }
 
@@ -1486,7 +1488,8 @@ xfs_qm_flush_one(
 		xfs_buf_delwri_queue(bp, buffer_list);
 	xfs_buf_relse(bp);
 out_unlock:
-	xfs_qm_dqput(dqp);
+	mutex_unlock(&dqp->q_qlock);
+	xfs_qm_dqrele(dqp);
 	return error;
 }
 
diff --git a/fs/xfs/xfs_qm_bhv.c b/fs/xfs/xfs_qm_bhv.c
index 245d754f382a..e5a30b12253c 100644
--- a/fs/xfs/xfs_qm_bhv.c
+++ b/fs/xfs/xfs_qm_bhv.c
@@ -74,7 +74,8 @@ xfs_qm_statvfs(
 
 	if (!xfs_qm_dqget(mp, ip->i_projid, XFS_DQTYPE_PROJ, false, &dqp)) {
 		xfs_fill_statvfs_from_dquot(statp, ip, dqp);
-		xfs_qm_dqput(dqp);
+		mutex_unlock(&dqp->q_qlock);
+		xfs_qm_dqrele(dqp);
 	}
 }
 
diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
index 59ef382900fe..441f9806cddb 100644
--- a/fs/xfs/xfs_qm_syscalls.c
+++ b/fs/xfs/xfs_qm_syscalls.c
@@ -467,7 +467,8 @@ xfs_qm_scall_getquota(
 	xfs_qm_scall_getquota_fill_qc(mp, type, dqp, dst);
 
 out_put:
-	xfs_qm_dqput(dqp);
+	mutex_unlock(&dqp->q_qlock);
+	xfs_qm_dqrele(dqp);
 	return error;
 }
 
@@ -497,7 +498,8 @@ xfs_qm_scall_getquota_next(
 	*id = dqp->q_id;
 
 	xfs_qm_scall_getquota_fill_qc(mp, type, dqp, dst);
+	mutex_unlock(&dqp->q_qlock);
 
-	xfs_qm_dqput(dqp);
+	xfs_qm_dqrele(dqp);
 	return error;
 }
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 46d21eb11ccb..fccc032b3c6c 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -1409,9 +1409,8 @@ DEFINE_DQUOT_EVENT(xfs_dqget_hit);
 DEFINE_DQUOT_EVENT(xfs_dqget_miss);
 DEFINE_DQUOT_EVENT(xfs_dqget_freeing);
 DEFINE_DQUOT_EVENT(xfs_dqget_dup);
-DEFINE_DQUOT_EVENT(xfs_dqput);
-DEFINE_DQUOT_EVENT(xfs_dqput_free);
 DEFINE_DQUOT_EVENT(xfs_dqrele);
+DEFINE_DQUOT_EVENT(xfs_dqrele_free);
 DEFINE_DQUOT_EVENT(xfs_dqflush);
 DEFINE_DQUOT_EVENT(xfs_dqflush_force);
 DEFINE_DQUOT_EVENT(xfs_dqflush_done);
-- 
2.47.3

[PATCH 07/17] xfs: consolidate q_qlock locking in xfs_qm_dqget and xfs_qm_dqget_inode

[Christoph Hellwig]

Move taking q_qlock from the cache lookup / insert helpers into the
main functions and do it just before returning to the caller.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_dquot.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index ceddbbb41999..a6030c53a1f9 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -826,15 +826,13 @@ xfs_qm_dqget_cache_lookup(
 
 	trace_xfs_dqget_hit(dqp);
 	XFS_STATS_INC(mp, xs_qm_dqcachehits);
-	mutex_lock(&dqp->q_qlock);
 	return dqp;
 }
 
 /*
  * Try to insert a new dquot into the in-core cache.  If an error occurs the
  * caller should throw away the dquot and start over.  Otherwise, the dquot
- * is returned locked (and held by the cache) as if there had been a cache
- * hit.
+ * is returned (and held by the cache) as if there had been a cache hit.
  *
  * The insert needs to be done under memalloc_nofs context because the radix
  * tree can do memory allocation during insert. The qi->qi_tree_lock is taken in
@@ -862,8 +860,6 @@ xfs_qm_dqget_cache_insert(
 		goto out_unlock;
 	}
 
-	/* Return a locked dquot to the caller, with a reference taken. */
-	mutex_lock(&dqp->q_qlock);
 	lockref_init(&dqp->q_lockref);
 	qi->qi_dquots++;
 
@@ -921,10 +917,8 @@ xfs_qm_dqget(
 
 restart:
 	dqp = xfs_qm_dqget_cache_lookup(mp, qi, tree, id);
-	if (dqp) {
-		*O_dqpp = dqp;
-		return 0;
-	}
+	if (dqp)
+		goto found;
 
 	error = xfs_qm_dqread(mp, id, type, can_alloc, &dqp);
 	if (error)
@@ -942,7 +936,9 @@ xfs_qm_dqget(
 	}
 
 	trace_xfs_dqget_miss(dqp);
+found:
 	*O_dqpp = dqp;
+	mutex_lock(&dqp->q_qlock);
 	return 0;
 }
 
@@ -1017,10 +1013,8 @@ xfs_qm_dqget_inode(
 
 restart:
 	dqp = xfs_qm_dqget_cache_lookup(mp, qi, tree, id);
-	if (dqp) {
-		*O_dqpp = dqp;
-		return 0;
-	}
+	if (dqp)
+		goto found;
 
 	/*
 	 * Dquot cache miss. We don't want to keep the inode lock across
@@ -1046,7 +1040,6 @@ xfs_qm_dqget_inode(
 		if (dqp1) {
 			xfs_qm_dqdestroy(dqp);
 			dqp = dqp1;
-			mutex_lock(&dqp->q_qlock);
 			goto dqret;
 		}
 	} else {
@@ -1069,7 +1062,9 @@ xfs_qm_dqget_inode(
 dqret:
 	xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
 	trace_xfs_dqget_miss(dqp);
+found:
 	*O_dqpp = dqp;
+	mutex_lock(&dqp->q_qlock);
 	return 0;
 }
 
-- 
2.47.3

[PATCH 08/17] xfs: xfs_qm_dqattach_one is never called with a non-NULL *IO_idqpp

[Christoph Hellwig]

The caller already checks that, so replace the handling of this case with
an assert that it does not happen.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_qm.c    | 13 +------------
 fs/xfs/xfs_trace.h |  1 -
 2 files changed, 1 insertion(+), 13 deletions(-)

diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 9bd7068b9e5a..80c99ef91edb 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -297,19 +297,8 @@ xfs_qm_dqattach_one(
 	struct xfs_dquot	*dqp;
 	int			error;
 
+	ASSERT(!*IO_idqpp);
 	xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
-	error = 0;
-
-	/*
-	 * See if we already have it in the inode itself. IO_idqpp is &i_udquot
-	 * or &i_gdquot. This made the code look weird, but made the logic a lot
-	 * simpler.
-	 */
-	dqp = *IO_idqpp;
-	if (dqp) {
-		trace_xfs_dqattach_found(dqp);
-		return 0;
-	}
 
 	/*
 	 * Find the dquot from somewhere. This bumps the reference count of
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index fccc032b3c6c..90582ff7c2cf 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -1399,7 +1399,6 @@ DEFINE_DQUOT_EVENT(xfs_dqadjust);
 DEFINE_DQUOT_EVENT(xfs_dqreclaim_want);
 DEFINE_DQUOT_EVENT(xfs_dqreclaim_busy);
 DEFINE_DQUOT_EVENT(xfs_dqreclaim_done);
-DEFINE_DQUOT_EVENT(xfs_dqattach_found);
 DEFINE_DQUOT_EVENT(xfs_dqattach_get);
 DEFINE_DQUOT_EVENT(xfs_dqalloc);
 DEFINE_DQUOT_EVENT(xfs_dqtobp_read);
-- 
2.47.3

[PATCH 09/17] xfs: fold xfs_qm_dqattach_one into xfs_qm_dqget_inode

[Christoph Hellwig]

xfs_qm_dqattach_one is a thin wrapper around xfs_qm_dqget_inode.  Move
the extra asserts into xfs_qm_dqget_inode, drop the unneeded q_qlock
roundtrip and merge the two functions.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_dquot.c |  9 ++++++---
 fs/xfs/xfs_qm.c    | 40 +++-------------------------------------
 2 files changed, 9 insertions(+), 40 deletions(-)

diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index a6030c53a1f9..fa493520bea6 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -992,7 +992,7 @@ xfs_qm_dqget_inode(
 	struct xfs_inode	*ip,
 	xfs_dqtype_t		type,
 	bool			can_alloc,
-	struct xfs_dquot	**O_dqpp)
+	struct xfs_dquot	**dqpp)
 {
 	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_quotainfo	*qi = mp->m_quotainfo;
@@ -1001,6 +1001,9 @@ xfs_qm_dqget_inode(
 	xfs_dqid_t		id;
 	int			error;
 
+	ASSERT(!*dqpp);
+	xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
+
 	error = xfs_qm_dqget_checks(mp, type);
 	if (error)
 		return error;
@@ -1063,8 +1066,8 @@ xfs_qm_dqget_inode(
 	xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
 	trace_xfs_dqget_miss(dqp);
 found:
-	*O_dqpp = dqp;
-	mutex_lock(&dqp->q_qlock);
+	trace_xfs_dqattach_get(dqp);
+	*dqpp = dqp;
 	return 0;
 }
 
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 80c99ef91edb..9e173a4b18eb 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -287,40 +287,6 @@ xfs_qm_unmount_quotas(
 		xfs_qm_destroy_quotainos(mp->m_quotainfo);
 }
 
-STATIC int
-xfs_qm_dqattach_one(
-	struct xfs_inode	*ip,
-	xfs_dqtype_t		type,
-	bool			doalloc,
-	struct xfs_dquot	**IO_idqpp)
-{
-	struct xfs_dquot	*dqp;
-	int			error;
-
-	ASSERT(!*IO_idqpp);
-	xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
-
-	/*
-	 * Find the dquot from somewhere. This bumps the reference count of
-	 * dquot and returns it locked.  This can return ENOENT if dquot didn't
-	 * exist on disk and we didn't ask it to allocate; ESRCH if quotas got
-	 * turned off suddenly.
-	 */
-	error = xfs_qm_dqget_inode(ip, type, doalloc, &dqp);
-	if (error)
-		return error;
-
-	trace_xfs_dqattach_get(dqp);
-
-	/*
-	 * dqget may have dropped and re-acquired the ilock, but it guarantees
-	 * that the dquot returned is the one that should go in the inode.
-	 */
-	*IO_idqpp = dqp;
-	mutex_unlock(&dqp->q_qlock);
-	return 0;
-}
-
 static bool
 xfs_qm_need_dqattach(
 	struct xfs_inode	*ip)
@@ -360,7 +326,7 @@ xfs_qm_dqattach_locked(
 	ASSERT(!xfs_is_metadir_inode(ip));
 
 	if (XFS_IS_UQUOTA_ON(mp) && !ip->i_udquot) {
-		error = xfs_qm_dqattach_one(ip, XFS_DQTYPE_USER,
+		error = xfs_qm_dqget_inode(ip, XFS_DQTYPE_USER,
 				doalloc, &ip->i_udquot);
 		if (error)
 			goto done;
@@ -368,7 +334,7 @@ xfs_qm_dqattach_locked(
 	}
 
 	if (XFS_IS_GQUOTA_ON(mp) && !ip->i_gdquot) {
-		error = xfs_qm_dqattach_one(ip, XFS_DQTYPE_GROUP,
+		error = xfs_qm_dqget_inode(ip, XFS_DQTYPE_GROUP,
 				doalloc, &ip->i_gdquot);
 		if (error)
 			goto done;
@@ -376,7 +342,7 @@ xfs_qm_dqattach_locked(
 	}
 
 	if (XFS_IS_PQUOTA_ON(mp) && !ip->i_pdquot) {
-		error = xfs_qm_dqattach_one(ip, XFS_DQTYPE_PROJ,
+		error = xfs_qm_dqget_inode(ip, XFS_DQTYPE_PROJ,
 				doalloc, &ip->i_pdquot);
 		if (error)
 			goto done;
-- 
2.47.3

[PATCH 10/17] xfs: return the dquot unlocked from xfs_qm_dqget

[Christoph Hellwig]

There is no reason to lock the dquot in xfs_qm_dqget, which just acquires
a reference.  Move the locking to the callers, or remove it in cases where
the caller instantly unlocks the dquot.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/scrub/dqiterate.c         | 1 +
 fs/xfs/scrub/quotacheck.c        | 1 +
 fs/xfs/scrub/quotacheck_repair.c | 1 +
 fs/xfs/xfs_dquot.c               | 4 ++--
 fs/xfs/xfs_qm.c                  | 4 +---
 fs/xfs/xfs_qm_bhv.c              | 1 +
 fs/xfs/xfs_qm_syscalls.c         | 2 ++
 7 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/scrub/dqiterate.c b/fs/xfs/scrub/dqiterate.c
index 20c4daedd48d..6f1185afbf39 100644
--- a/fs/xfs/scrub/dqiterate.c
+++ b/fs/xfs/scrub/dqiterate.c
@@ -205,6 +205,7 @@ xchk_dquot_iter(
 	if (error)
 		return error;
 
+	mutex_lock(&dq->q_qlock);
 	cursor->id = dq->q_id + 1;
 	*dqpp = dq;
 	return 1;
diff --git a/fs/xfs/scrub/quotacheck.c b/fs/xfs/scrub/quotacheck.c
index 180449f654f6..bef63f19cd87 100644
--- a/fs/xfs/scrub/quotacheck.c
+++ b/fs/xfs/scrub/quotacheck.c
@@ -635,6 +635,7 @@ xqcheck_walk_observations(
 		if (error)
 			return error;
 
+		mutex_lock(&dq->q_qlock);
 		error = xqcheck_compare_dquot(xqc, dqtype, dq);
 		mutex_unlock(&dq->q_qlock);
 		xfs_qm_dqrele(dq);
diff --git a/fs/xfs/scrub/quotacheck_repair.c b/fs/xfs/scrub/quotacheck_repair.c
index 67bdc872996a..f7b1add43a2c 100644
--- a/fs/xfs/scrub/quotacheck_repair.c
+++ b/fs/xfs/scrub/quotacheck_repair.c
@@ -181,6 +181,7 @@ xqcheck_commit_dqtype(
 		if (error)
 			return error;
 
+		mutex_lock(&dq->q_qlock);
 		error = xqcheck_commit_dquot(xqc, dqtype, dq);
 		xfs_qm_dqrele(dq);
 		if (error)
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index fa493520bea6..98593b380e94 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -896,7 +896,7 @@ xfs_qm_dqget_checks(
 
 /*
  * Given the file system, id, and type (UDQUOT/GDQUOT/PDQUOT), return a
- * locked dquot, doing an allocation (if requested) as needed.
+ * dquot, doing an allocation (if requested) as needed.
  */
 int
 xfs_qm_dqget(
@@ -938,7 +938,6 @@ xfs_qm_dqget(
 	trace_xfs_dqget_miss(dqp);
 found:
 	*O_dqpp = dqp;
-	mutex_lock(&dqp->q_qlock);
 	return 0;
 }
 
@@ -1093,6 +1092,7 @@ xfs_qm_dqget_next(
 		else if (error != 0)
 			break;
 
+		mutex_lock(&dqp->q_qlock);
 		if (!XFS_IS_DQUOT_UNINITIALIZED(dqp)) {
 			*dqpp = dqp;
 			return 0;
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 9e173a4b18eb..7fbb89fcdeb9 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -1268,6 +1268,7 @@ xfs_qm_quotacheck_dqadjust(
 		return error;
 	}
 
+	mutex_lock(&dqp->q_qlock);
 	error = xfs_dquot_attach_buf(NULL, dqp);
 	if (error)
 		return error;
@@ -1907,7 +1908,6 @@ xfs_qm_vop_dqalloc(
 			/*
 			 * Get the ilock in the right order.
 			 */
-			mutex_unlock(&uq->q_qlock);
 			lockflags = XFS_ILOCK_SHARED;
 			xfs_ilock(ip, lockflags);
 		} else {
@@ -1929,7 +1929,6 @@ xfs_qm_vop_dqalloc(
 				ASSERT(error != -ENOENT);
 				goto error_rele;
 			}
-			mutex_unlock(&gq->q_qlock);
 			lockflags = XFS_ILOCK_SHARED;
 			xfs_ilock(ip, lockflags);
 		} else {
@@ -1947,7 +1946,6 @@ xfs_qm_vop_dqalloc(
 				ASSERT(error != -ENOENT);
 				goto error_rele;
 			}
-			mutex_unlock(&pq->q_qlock);
 			lockflags = XFS_ILOCK_SHARED;
 			xfs_ilock(ip, lockflags);
 		} else {
diff --git a/fs/xfs/xfs_qm_bhv.c b/fs/xfs/xfs_qm_bhv.c
index e5a30b12253c..edc0aef3cf34 100644
--- a/fs/xfs/xfs_qm_bhv.c
+++ b/fs/xfs/xfs_qm_bhv.c
@@ -73,6 +73,7 @@ xfs_qm_statvfs(
 	struct xfs_dquot	*dqp;
 
 	if (!xfs_qm_dqget(mp, ip->i_projid, XFS_DQTYPE_PROJ, false, &dqp)) {
+		mutex_lock(&dqp->q_qlock);
 		xfs_fill_statvfs_from_dquot(statp, ip, dqp);
 		mutex_unlock(&dqp->q_qlock);
 		xfs_qm_dqrele(dqp);
diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
index 441f9806cddb..6c8924780d7a 100644
--- a/fs/xfs/xfs_qm_syscalls.c
+++ b/fs/xfs/xfs_qm_syscalls.c
@@ -302,6 +302,7 @@ xfs_qm_scall_setqlim(
 		return error;
 	}
 
+	mutex_lock(&dqp->q_qlock);
 	defq = xfs_get_defquota(q, xfs_dquot_type(dqp));
 	mutex_unlock(&dqp->q_qlock);
 
@@ -459,6 +460,7 @@ xfs_qm_scall_getquota(
 	 * If everything's NULL, this dquot doesn't quite exist as far as
 	 * our utility programs are concerned.
 	 */
+	mutex_lock(&dqp->q_qlock);
 	if (XFS_IS_DQUOT_UNINITIALIZED(dqp)) {
 		error = -ENOENT;
 		goto out_put;
-- 
2.47.3

[PATCH 11/17] xfs: remove q_qlock locking in xfs_qm_scall_setqlim

[Christoph Hellwig]

q_type can't change for an existing dquot, so there is no need for
the locking here.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_qm_syscalls.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
index 6c8924780d7a..022e2179c06b 100644
--- a/fs/xfs/xfs_qm_syscalls.c
+++ b/fs/xfs/xfs_qm_syscalls.c
@@ -302,9 +302,7 @@ xfs_qm_scall_setqlim(
 		return error;
 	}
 
-	mutex_lock(&dqp->q_qlock);
 	defq = xfs_get_defquota(q, xfs_dquot_type(dqp));
-	mutex_unlock(&dqp->q_qlock);
 
 	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_qm_setqlim, 0, 0, 0, &tp);
 	if (error)
-- 
2.47.3

[PATCH 12/17] xfs: push q_qlock acquisition from xchk_dquot_iter to the callers.

[Christoph Hellwig]

There is no good reason to take q_qlock in xchk_dquot_iter, which just
provides a reference to the dquot.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/scrub/dqiterate.c         | 1 -
 fs/xfs/scrub/quota.c             | 1 +
 fs/xfs/scrub/quota_repair.c      | 1 +
 fs/xfs/scrub/quotacheck.c        | 1 +
 fs/xfs/scrub/quotacheck_repair.c | 1 +
 5 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/scrub/dqiterate.c b/fs/xfs/scrub/dqiterate.c
index 6f1185afbf39..20c4daedd48d 100644
--- a/fs/xfs/scrub/dqiterate.c
+++ b/fs/xfs/scrub/dqiterate.c
@@ -205,7 +205,6 @@ xchk_dquot_iter(
 	if (error)
 		return error;
 
-	mutex_lock(&dq->q_qlock);
 	cursor->id = dq->q_id + 1;
 	*dqpp = dq;
 	return 1;
diff --git a/fs/xfs/scrub/quota.c b/fs/xfs/scrub/quota.c
index cfcd0fb66845..b711d36c5ec9 100644
--- a/fs/xfs/scrub/quota.c
+++ b/fs/xfs/scrub/quota.c
@@ -329,6 +329,7 @@ xchk_quota(
 	/* Now look for things that the quota verifiers won't complain about. */
 	xchk_dqiter_init(&cursor, sc, dqtype);
 	while ((error = xchk_dquot_iter(&cursor, &dq)) == 1) {
+		mutex_lock(&dq->q_qlock);
 		error = xchk_quota_item(&sqi, dq);
 		mutex_unlock(&dq->q_qlock);
 		xfs_qm_dqrele(dq);
diff --git a/fs/xfs/scrub/quota_repair.c b/fs/xfs/scrub/quota_repair.c
index 00a4d2e75797..7897b93c639a 100644
--- a/fs/xfs/scrub/quota_repair.c
+++ b/fs/xfs/scrub/quota_repair.c
@@ -510,6 +510,7 @@ xrep_quota_problems(
 
 	xchk_dqiter_init(&cursor, sc, dqtype);
 	while ((error = xchk_dquot_iter(&cursor, &dq)) == 1) {
+		mutex_lock(&dq->q_qlock);
 		error = xrep_quota_item(&rqi, dq);
 		xfs_qm_dqrele(dq);
 		if (error)
diff --git a/fs/xfs/scrub/quotacheck.c b/fs/xfs/scrub/quotacheck.c
index bef63f19cd87..20220afd90f1 100644
--- a/fs/xfs/scrub/quotacheck.c
+++ b/fs/xfs/scrub/quotacheck.c
@@ -675,6 +675,7 @@ xqcheck_compare_dqtype(
 	/* Compare what we observed against the actual dquots. */
 	xchk_dqiter_init(&cursor, sc, dqtype);
 	while ((error = xchk_dquot_iter(&cursor, &dq)) == 1) {
+		mutex_lock(&dq->q_qlock);
 		error = xqcheck_compare_dquot(xqc, dqtype, dq);
 		mutex_unlock(&dq->q_qlock);
 		xfs_qm_dqrele(dq);
diff --git a/fs/xfs/scrub/quotacheck_repair.c b/fs/xfs/scrub/quotacheck_repair.c
index f7b1add43a2c..de1521739ec9 100644
--- a/fs/xfs/scrub/quotacheck_repair.c
+++ b/fs/xfs/scrub/quotacheck_repair.c
@@ -150,6 +150,7 @@ xqcheck_commit_dqtype(
 	 */
 	xchk_dqiter_init(&cursor, sc, dqtype);
 	while ((error = xchk_dquot_iter(&cursor, &dq)) == 1) {
+		mutex_lock(&dq->q_qlock);
 		error = xqcheck_commit_dquot(xqc, dqtype, dq);
 		xfs_qm_dqrele(dq);
 		if (error)
-- 
2.47.3

[PATCH 13/17] xfs: move q_qlock locking into xchk_quota_item

[Christoph Hellwig]

This avoids a pointless roundtrip because ilock needs to be taken first.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/scrub/quota.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/fs/xfs/scrub/quota.c b/fs/xfs/scrub/quota.c
index b711d36c5ec9..5c5374c44c5a 100644
--- a/fs/xfs/scrub/quota.c
+++ b/fs/xfs/scrub/quota.c
@@ -155,10 +155,7 @@ xchk_quota_item(
 	 * We want to validate the bmap record for the storage backing this
 	 * dquot, so we need to lock the dquot and the quota file.  For quota
 	 * operations, the locking order is first the ILOCK and then the dquot.
-	 * However, dqiterate gave us a locked dquot, so drop the dquot lock to
-	 * get the ILOCK.
 	 */
-	mutex_unlock(&dq->q_qlock);
 	xchk_ilock(sc, XFS_ILOCK_SHARED);
 	mutex_lock(&dq->q_qlock);
 
@@ -251,6 +248,7 @@ xchk_quota_item(
 	xchk_quota_item_timer(sc, offset, &dq->q_rtb);
 
 out:
+	mutex_unlock(&dq->q_qlock);
 	if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
 		return -ECANCELED;
 
@@ -329,9 +327,7 @@ xchk_quota(
 	/* Now look for things that the quota verifiers won't complain about. */
 	xchk_dqiter_init(&cursor, sc, dqtype);
 	while ((error = xchk_dquot_iter(&cursor, &dq)) == 1) {
-		mutex_lock(&dq->q_qlock);
 		error = xchk_quota_item(&sqi, dq);
-		mutex_unlock(&dq->q_qlock);
 		xfs_qm_dqrele(dq);
 		if (error)
 			break;
-- 
2.47.3

[PATCH 14/17] xfs: move q_qlock locking into xqcheck_compare_dquot

[Christoph Hellwig]

Instead of having both callers do it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/scrub/quotacheck.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/scrub/quotacheck.c b/fs/xfs/scrub/quotacheck.c
index 20220afd90f1..cc2b2dfc9261 100644
--- a/fs/xfs/scrub/quotacheck.c
+++ b/fs/xfs/scrub/quotacheck.c
@@ -563,6 +563,7 @@ xqcheck_compare_dquot(
 		return -ECANCELED;
 	}
 
+	mutex_lock(&dq->q_qlock);
 	mutex_lock(&xqc->lock);
 	error = xfarray_load_sparse(counts, dq->q_id, &xcdq);
 	if (error)
@@ -590,6 +591,7 @@ xqcheck_compare_dquot(
 		error = -ECANCELED;
 	}
 	mutex_unlock(&xqc->lock);
+	mutex_unlock(&dq->q_qlock);
 	if (error)
 		return error;
 
@@ -635,9 +637,7 @@ xqcheck_walk_observations(
 		if (error)
 			return error;
 
-		mutex_lock(&dq->q_qlock);
 		error = xqcheck_compare_dquot(xqc, dqtype, dq);
-		mutex_unlock(&dq->q_qlock);
 		xfs_qm_dqrele(dq);
 		if (error)
 			return error;
@@ -675,9 +675,7 @@ xqcheck_compare_dqtype(
 	/* Compare what we observed against the actual dquots. */
 	xchk_dqiter_init(&cursor, sc, dqtype);
 	while ((error = xchk_dquot_iter(&cursor, &dq)) == 1) {
-		mutex_lock(&dq->q_qlock);
 		error = xqcheck_compare_dquot(xqc, dqtype, dq);
-		mutex_unlock(&dq->q_qlock);
 		xfs_qm_dqrele(dq);
 		if (error)
 			break;
-- 
2.47.3

[PATCH 15/17] xfs: move q_qlock acquisition into xqcheck_commit_dquot

[Christoph Hellwig]

This removes a pointless roundtrip because xqcheck_commit_dquot has to
drop the lock for allocating a transaction right now.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/scrub/quotacheck_repair.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/fs/xfs/scrub/quotacheck_repair.c b/fs/xfs/scrub/quotacheck_repair.c
index de1521739ec9..942df94e1215 100644
--- a/fs/xfs/scrub/quotacheck_repair.c
+++ b/fs/xfs/scrub/quotacheck_repair.c
@@ -52,13 +52,11 @@ xqcheck_commit_dquot(
 	bool			dirty = false;
 	int			error = 0;
 
-	/* Unlock the dquot just long enough to allocate a transaction. */
-	mutex_unlock(&dq->q_qlock);
 	error = xchk_trans_alloc(xqc->sc, 0);
-	mutex_lock(&dq->q_qlock);
 	if (error)
 		return error;
 
+	mutex_lock(&dq->q_qlock);
 	xfs_trans_dqjoin(xqc->sc->tp, dq);
 
 	if (xchk_iscan_aborted(&xqc->iscan)) {
@@ -150,7 +148,6 @@ xqcheck_commit_dqtype(
 	 */
 	xchk_dqiter_init(&cursor, sc, dqtype);
 	while ((error = xchk_dquot_iter(&cursor, &dq)) == 1) {
-		mutex_lock(&dq->q_qlock);
 		error = xqcheck_commit_dquot(xqc, dqtype, dq);
 		xfs_qm_dqrele(dq);
 		if (error)
@@ -182,7 +179,6 @@ xqcheck_commit_dqtype(
 		if (error)
 			return error;
 
-		mutex_lock(&dq->q_qlock);
 		error = xqcheck_commit_dquot(xqc, dqtype, dq);
 		xfs_qm_dqrele(dq);
 		if (error)
-- 
2.47.3

[PATCH 16/17] xfs: move xfs_dquot_tree calls into xfs_qm_dqget_cache_{lookup,insert}

[Christoph Hellwig]

These are the low-level functions that needs them, so localize the
(trivial) calculation of the radix tree root there.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_dquot.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 98593b380e94..29f578d66230 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -801,10 +801,11 @@ xfs_dq_get_next_id(
 static struct xfs_dquot *
 xfs_qm_dqget_cache_lookup(
 	struct xfs_mount	*mp,
-	struct xfs_quotainfo	*qi,
-	struct radix_tree_root	*tree,
-	xfs_dqid_t		id)
+	xfs_dqid_t		id,
+	xfs_dqtype_t		type)
 {
+	struct xfs_quotainfo	*qi = mp->m_quotainfo;
+	struct radix_tree_root	*tree = xfs_dquot_tree(qi, type);
 	struct xfs_dquot	*dqp;
 
 restart:
@@ -843,11 +844,12 @@ xfs_qm_dqget_cache_lookup(
 static int
 xfs_qm_dqget_cache_insert(
 	struct xfs_mount	*mp,
-	struct xfs_quotainfo	*qi,
-	struct radix_tree_root	*tree,
 	xfs_dqid_t		id,
+	xfs_dqtype_t		type,
 	struct xfs_dquot	*dqp)
 {
+	struct xfs_quotainfo	*qi = mp->m_quotainfo;
+	struct radix_tree_root	*tree = xfs_dquot_tree(qi, type);
 	unsigned int		nofs_flags;
 	int			error;
 
@@ -906,8 +908,6 @@ xfs_qm_dqget(
 	bool			can_alloc,
 	struct xfs_dquot	**O_dqpp)
 {
-	struct xfs_quotainfo	*qi = mp->m_quotainfo;
-	struct radix_tree_root	*tree = xfs_dquot_tree(qi, type);
 	struct xfs_dquot	*dqp;
 	int			error;
 
@@ -916,7 +916,7 @@ xfs_qm_dqget(
 		return error;
 
 restart:
-	dqp = xfs_qm_dqget_cache_lookup(mp, qi, tree, id);
+	dqp = xfs_qm_dqget_cache_lookup(mp, id, type);
 	if (dqp)
 		goto found;
 
@@ -924,7 +924,7 @@ xfs_qm_dqget(
 	if (error)
 		return error;
 
-	error = xfs_qm_dqget_cache_insert(mp, qi, tree, id, dqp);
+	error = xfs_qm_dqget_cache_insert(mp, id, type, dqp);
 	if (error) {
 		/*
 		 * Duplicate found. Just throw away the new dquot and start
@@ -994,8 +994,6 @@ xfs_qm_dqget_inode(
 	struct xfs_dquot	**dqpp)
 {
 	struct xfs_mount	*mp = ip->i_mount;
-	struct xfs_quotainfo	*qi = mp->m_quotainfo;
-	struct radix_tree_root	*tree = xfs_dquot_tree(qi, type);
 	struct xfs_dquot	*dqp;
 	xfs_dqid_t		id;
 	int			error;
@@ -1014,7 +1012,7 @@ xfs_qm_dqget_inode(
 	id = xfs_qm_id_for_quotatype(ip, type);
 
 restart:
-	dqp = xfs_qm_dqget_cache_lookup(mp, qi, tree, id);
+	dqp = xfs_qm_dqget_cache_lookup(mp, id, type);
 	if (dqp)
 		goto found;
 
@@ -1050,7 +1048,7 @@ xfs_qm_dqget_inode(
 		return -ESRCH;
 	}
 
-	error = xfs_qm_dqget_cache_insert(mp, qi, tree, id, dqp);
+	error = xfs_qm_dqget_cache_insert(mp, id, type, dqp);
 	if (error) {
 		/*
 		 * Duplicate found. Just throw away the new dquot and start
-- 
2.47.3

[PATCH 17/17] xfs: reduce ilock roundtrips in xfs_qm_vop_dqalloc

[Christoph Hellwig]

xfs_qm_vop_dqalloc only needs the (exclusive) ilock for attaching dquots
to the inode if not done so yet.  All the other locks don't touch the inode
and don't need the ilock - the i_rwsem / iolock protects against changes
to the IDs while we are in a method, and the ilock would not help because
dropping it for the dqget calls would be racy anyway.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_qm.c | 32 +++-----------------------------
 1 file changed, 3 insertions(+), 29 deletions(-)

diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 7fbb89fcdeb9..336de0479022 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -1861,16 +1861,12 @@ xfs_qm_vop_dqalloc(
 	struct xfs_dquot	*gq = NULL;
 	struct xfs_dquot	*pq = NULL;
 	int			error;
-	uint			lockflags;
 
 	if (!XFS_IS_QUOTA_ON(mp))
 		return 0;
 
 	ASSERT(!xfs_is_metadir_inode(ip));
 
-	lockflags = XFS_ILOCK_EXCL;
-	xfs_ilock(ip, lockflags);
-
 	if ((flags & XFS_QMOPT_INHERIT) && XFS_INHERIT_GID(ip))
 		gid = inode->i_gid;
 
@@ -1879,37 +1875,22 @@ xfs_qm_vop_dqalloc(
 	 * if necessary. The dquot(s) will not be locked.
 	 */
 	if (XFS_NOT_DQATTACHED(mp, ip)) {
+		xfs_ilock(ip, XFS_ILOCK_EXCL);
 		error = xfs_qm_dqattach_locked(ip, true);
-		if (error) {
-			xfs_iunlock(ip, lockflags);
+		xfs_iunlock(ip, XFS_ILOCK_EXCL);
+		if (error)
 			return error;
-		}
 	}
 
 	if ((flags & XFS_QMOPT_UQUOTA) && XFS_IS_UQUOTA_ON(mp)) {
 		ASSERT(O_udqpp);
 		if (!uid_eq(inode->i_uid, uid)) {
-			/*
-			 * What we need is the dquot that has this uid, and
-			 * if we send the inode to dqget, the uid of the inode
-			 * takes priority over what's sent in the uid argument.
-			 * We must unlock inode here before calling dqget if
-			 * we're not sending the inode, because otherwise
-			 * we'll deadlock by doing trans_reserve while
-			 * holding ilock.
-			 */
-			xfs_iunlock(ip, lockflags);
 			error = xfs_qm_dqget(mp, from_kuid(user_ns, uid),
 					XFS_DQTYPE_USER, true, &uq);
 			if (error) {
 				ASSERT(error != -ENOENT);
 				return error;
 			}
-			/*
-			 * Get the ilock in the right order.
-			 */
-			lockflags = XFS_ILOCK_SHARED;
-			xfs_ilock(ip, lockflags);
 		} else {
 			/*
 			 * Take an extra reference, because we'll return
@@ -1922,15 +1903,12 @@ xfs_qm_vop_dqalloc(
 	if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp)) {
 		ASSERT(O_gdqpp);
 		if (!gid_eq(inode->i_gid, gid)) {
-			xfs_iunlock(ip, lockflags);
 			error = xfs_qm_dqget(mp, from_kgid(user_ns, gid),
 					XFS_DQTYPE_GROUP, true, &gq);
 			if (error) {
 				ASSERT(error != -ENOENT);
 				goto error_rele;
 			}
-			lockflags = XFS_ILOCK_SHARED;
-			xfs_ilock(ip, lockflags);
 		} else {
 			ASSERT(ip->i_gdquot);
 			gq = xfs_qm_dqhold(ip->i_gdquot);
@@ -1939,15 +1917,12 @@ xfs_qm_vop_dqalloc(
 	if ((flags & XFS_QMOPT_PQUOTA) && XFS_IS_PQUOTA_ON(mp)) {
 		ASSERT(O_pdqpp);
 		if (ip->i_projid != prid) {
-			xfs_iunlock(ip, lockflags);
 			error = xfs_qm_dqget(mp, prid,
 					XFS_DQTYPE_PROJ, true, &pq);
 			if (error) {
 				ASSERT(error != -ENOENT);
 				goto error_rele;
 			}
-			lockflags = XFS_ILOCK_SHARED;
-			xfs_ilock(ip, lockflags);
 		} else {
 			ASSERT(ip->i_pdquot);
 			pq = xfs_qm_dqhold(ip->i_pdquot);
@@ -1955,7 +1930,6 @@ xfs_qm_vop_dqalloc(
 	}
 	trace_xfs_dquot_dqalloc(ip);
 
-	xfs_iunlock(ip, lockflags);
 	if (O_udqpp)
 		*O_udqpp = uq;
 	else
-- 
2.47.3

Re: [PATCH 01/17] xfs: make qi_dquots a 64-bit value

[Darrick J. Wong]

On Mon, Oct 13, 2025 at 11:48:02AM +0900, Christoph Hellwig wrote:
> qi_dquots counts all quotas in the file system, which can be up to
> 3 * UINT_MAX and overflow a 32-bit counter, but can't be negative.
> Make qi_dquots a uint64_t, and saturate the value to UINT_MAX for
> userspace reporting.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_qm.h       | 2 +-
>  fs/xfs/xfs_quotaops.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_qm.h b/fs/xfs/xfs_qm.h
> index 35b64bc3a7a8..e88ed6ad0e65 100644
> --- a/fs/xfs/xfs_qm.h
> +++ b/fs/xfs/xfs_qm.h
> @@ -57,7 +57,7 @@ struct xfs_quotainfo {
>  	struct xfs_inode	*qi_pquotaip;	/* project quota inode */
>  	struct xfs_inode	*qi_dirip;	/* quota metadir */
>  	struct list_lru		qi_lru;
> -	int			qi_dquots;
> +	uint64_t		qi_dquots;
>  	struct mutex		qi_quotaofflock;/* to serialize quotaoff */
>  	xfs_filblks_t		qi_dqchunklen;	/* # BBs in a chunk of dqs */
>  	uint			qi_dqperchunk;	/* # ondisk dq in above chunk */
> diff --git a/fs/xfs/xfs_quotaops.c b/fs/xfs/xfs_quotaops.c
> index 4c7f7ce4fd2f..1045c4c262ad 100644
> --- a/fs/xfs/xfs_quotaops.c
> +++ b/fs/xfs/xfs_quotaops.c
> @@ -65,7 +65,7 @@ xfs_fs_get_quota_state(
>  	memset(state, 0, sizeof(*state));
>  	if (!XFS_IS_QUOTA_ON(mp))
>  		return 0;
> -	state->s_incoredqs = q->qi_dquots;
> +	state->s_incoredqs = max_t(uint64_t, q->qi_dquots, UINT_MAX);

Isn't this min_t?  Surely we don't want to return 0xFFFFFFFF when
there's only 3 dquots loaded in the system?

--D

>  	if (XFS_IS_UQUOTA_ON(mp))
>  		state->s_state[USRQUOTA].flags |= QCI_ACCT_ENABLED;
>  	if (XFS_IS_UQUOTA_ENFORCED(mp))
> -- 
> 2.47.3
> 
> 

Re: [PATCH 02/17] xfs: remove xfs_dqunlock and friends

[Darrick J. Wong]

On Mon, Oct 13, 2025 at 11:48:03AM +0900, Christoph Hellwig wrote:
> There's really no point in wrapping the basic mutex operations.  Remove
> the wrapper to ease lock analysis annotations and make the code a litte
> easier to read.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/scrub/quota.c             |  4 ++--
>  fs/xfs/scrub/quota_repair.c      |  6 +++---
>  fs/xfs/scrub/quotacheck_repair.c |  8 ++++----
>  fs/xfs/xfs_dquot.c               | 14 +++++++-------
>  fs/xfs/xfs_dquot.h               | 19 ++-----------------
>  fs/xfs/xfs_dquot_item.c          |  6 +++---
>  fs/xfs/xfs_qm.c                  | 30 +++++++++++++++---------------
>  fs/xfs/xfs_qm_syscalls.c         |  4 ++--
>  fs/xfs/xfs_trans_dquot.c         | 18 +++++++++---------
>  9 files changed, 47 insertions(+), 62 deletions(-)
> 
> diff --git a/fs/xfs/scrub/quota.c b/fs/xfs/scrub/quota.c
> index 58d6d4ed2853..c78cf9f96cf6 100644
> --- a/fs/xfs/scrub/quota.c
> +++ b/fs/xfs/scrub/quota.c
> @@ -158,9 +158,9 @@ xchk_quota_item(
>  	 * However, dqiterate gave us a locked dquot, so drop the dquot lock to
>  	 * get the ILOCK.
>  	 */
> -	xfs_dqunlock(dq);
> +	mutex_unlock(&dq->q_qlock);

Heh, and I was going to turn these into tracepointed lock helpers the
next time something got screwed up with dquot locking.

Oh well, that didn't happen so
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

>  	xchk_ilock(sc, XFS_ILOCK_SHARED);
> -	xfs_dqlock(dq);
> +	mutex_lock(&dq->q_qlock);
>  
>  	/*
>  	 * Except for the root dquot, the actual dquot we got must either have
> diff --git a/fs/xfs/scrub/quota_repair.c b/fs/xfs/scrub/quota_repair.c
> index 8f4c8d41f308..8c89c6cc2950 100644
> --- a/fs/xfs/scrub/quota_repair.c
> +++ b/fs/xfs/scrub/quota_repair.c
> @@ -187,9 +187,9 @@ xrep_quota_item(
>  	 * dqiterate gave us a locked dquot, so drop the dquot lock to get the
>  	 * ILOCK_EXCL.
>  	 */
> -	xfs_dqunlock(dq);
> +	mutex_unlock(&dq->q_qlock);
>  	xchk_ilock(sc, XFS_ILOCK_EXCL);
> -	xfs_dqlock(dq);
> +	mutex_lock(&dq->q_qlock);
>  
>  	error = xrep_quota_item_bmap(sc, dq, &dirty);
>  	xchk_iunlock(sc, XFS_ILOCK_EXCL);
> @@ -258,7 +258,7 @@ xrep_quota_item(
>  	}
>  	xfs_trans_log_dquot(sc->tp, dq);
>  	error = xfs_trans_roll(&sc->tp);
> -	xfs_dqlock(dq);
> +	mutex_lock(&dq->q_qlock);
>  	return error;
>  }
>  
> diff --git a/fs/xfs/scrub/quotacheck_repair.c b/fs/xfs/scrub/quotacheck_repair.c
> index dd8554c755b5..415314911499 100644
> --- a/fs/xfs/scrub/quotacheck_repair.c
> +++ b/fs/xfs/scrub/quotacheck_repair.c
> @@ -53,9 +53,9 @@ xqcheck_commit_dquot(
>  	int			error = 0;
>  
>  	/* Unlock the dquot just long enough to allocate a transaction. */
> -	xfs_dqunlock(dq);
> +	mutex_unlock(&dq->q_qlock);
>  	error = xchk_trans_alloc(xqc->sc, 0);
> -	xfs_dqlock(dq);
> +	mutex_lock(&dq->q_qlock);
>  	if (error)
>  		return error;
>  
> @@ -122,7 +122,7 @@ xqcheck_commit_dquot(
>  	 * dquot).
>  	 */
>  	error = xrep_trans_commit(xqc->sc);
> -	xfs_dqlock(dq);
> +	mutex_lock(&dq->q_qlock);
>  	return error;
>  
>  out_unlock:
> @@ -131,7 +131,7 @@ xqcheck_commit_dquot(
>  	xchk_trans_cancel(xqc->sc);
>  
>  	/* Re-lock the dquot so the caller can put the reference. */
> -	xfs_dqlock(dq);
> +	mutex_lock(&dq->q_qlock);
>  	return error;
>  }
>  
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index 0bd8022e47b4..97f037fa4181 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -31,7 +31,7 @@
>   *
>   * ip->i_lock
>   *   qi->qi_tree_lock
> - *     dquot->q_qlock (xfs_dqlock() and friends)
> + *     dquot->q_qlock
>   *       dquot->q_flush (xfs_dqflock() and friends)
>   *       qi->qi_lru_lock
>   *
> @@ -816,9 +816,9 @@ xfs_qm_dqget_cache_lookup(
>  		return NULL;
>  	}
>  
> -	xfs_dqlock(dqp);
> +	mutex_lock(&dqp->q_qlock);
>  	if (dqp->q_flags & XFS_DQFLAG_FREEING) {
> -		xfs_dqunlock(dqp);
> +		mutex_unlock(&dqp->q_qlock);
>  		mutex_unlock(&qi->qi_tree_lock);
>  		trace_xfs_dqget_freeing(dqp);
>  		delay(1);
> @@ -866,7 +866,7 @@ xfs_qm_dqget_cache_insert(
>  	}
>  
>  	/* Return a locked dquot to the caller, with a reference taken. */
> -	xfs_dqlock(dqp);
> +	mutex_lock(&dqp->q_qlock);
>  	dqp->q_nrefs = 1;
>  	qi->qi_dquots++;
>  
> @@ -1049,7 +1049,7 @@ xfs_qm_dqget_inode(
>  		if (dqp1) {
>  			xfs_qm_dqdestroy(dqp);
>  			dqp = dqp1;
> -			xfs_dqlock(dqp);
> +			mutex_lock(&dqp->q_qlock);
>  			goto dqret;
>  		}
>  	} else {
> @@ -1131,7 +1131,7 @@ xfs_qm_dqput(
>  		if (list_lru_add_obj(&qi->qi_lru, &dqp->q_lru))
>  			XFS_STATS_INC(dqp->q_mount, xs_qm_dquot_unused);
>  	}
> -	xfs_dqunlock(dqp);
> +	mutex_unlock(&dqp->q_qlock);
>  }
>  
>  /*
> @@ -1147,7 +1147,7 @@ xfs_qm_dqrele(
>  
>  	trace_xfs_dqrele(dqp);
>  
> -	xfs_dqlock(dqp);
> +	mutex_lock(&dqp->q_qlock);
>  	/*
>  	 * We don't care to flush it if the dquot is dirty here.
>  	 * That will create stutters that we want to avoid.
> diff --git a/fs/xfs/xfs_dquot.h b/fs/xfs/xfs_dquot.h
> index 61217adf5ba5..10c39b8cdd03 100644
> --- a/fs/xfs/xfs_dquot.h
> +++ b/fs/xfs/xfs_dquot.h
> @@ -121,21 +121,6 @@ static inline void xfs_dqfunlock(struct xfs_dquot *dqp)
>  	complete(&dqp->q_flush);
>  }
>  
> -static inline int xfs_dqlock_nowait(struct xfs_dquot *dqp)
> -{
> -	return mutex_trylock(&dqp->q_qlock);
> -}
> -
> -static inline void xfs_dqlock(struct xfs_dquot *dqp)
> -{
> -	mutex_lock(&dqp->q_qlock);
> -}
> -
> -static inline void xfs_dqunlock(struct xfs_dquot *dqp)
> -{
> -	mutex_unlock(&dqp->q_qlock);
> -}
> -
>  static inline int
>  xfs_dquot_type(const struct xfs_dquot *dqp)
>  {
> @@ -246,9 +231,9 @@ void xfs_dquot_detach_buf(struct xfs_dquot *dqp);
>  
>  static inline struct xfs_dquot *xfs_qm_dqhold(struct xfs_dquot *dqp)
>  {
> -	xfs_dqlock(dqp);
> +	mutex_lock(&dqp->q_qlock);
>  	dqp->q_nrefs++;
> -	xfs_dqunlock(dqp);
> +	mutex_unlock(&dqp->q_qlock);
>  	return dqp;
>  }
>  
> diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
> index 271b195ebb93..b374cd9f1900 100644
> --- a/fs/xfs/xfs_dquot_item.c
> +++ b/fs/xfs/xfs_dquot_item.c
> @@ -132,7 +132,7 @@ xfs_qm_dquot_logitem_push(
>  	if (atomic_read(&dqp->q_pincount) > 0)
>  		return XFS_ITEM_PINNED;
>  
> -	if (!xfs_dqlock_nowait(dqp))
> +	if (!mutex_trylock(&dqp->q_qlock))
>  		return XFS_ITEM_LOCKED;
>  
>  	/*
> @@ -177,7 +177,7 @@ xfs_qm_dquot_logitem_push(
>  out_relock_ail:
>  	spin_lock(&lip->li_ailp->ail_lock);
>  out_unlock:
> -	xfs_dqunlock(dqp);
> +	mutex_unlock(&dqp->q_qlock);
>  	return rval;
>  }
>  
> @@ -195,7 +195,7 @@ xfs_qm_dquot_logitem_release(
>  	 * transaction layer, within trans_commit. Hence, no LI_HOLD flag
>  	 * for the logitem.
>  	 */
> -	xfs_dqunlock(dqp);
> +	mutex_unlock(&dqp->q_qlock);
>  }
>  
>  STATIC void
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index 23ba84ec919a..ca3cbff9d873 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -128,7 +128,7 @@ xfs_qm_dqpurge(
>  	struct xfs_quotainfo	*qi = dqp->q_mount->m_quotainfo;
>  	int			error = -EAGAIN;
>  
> -	xfs_dqlock(dqp);
> +	mutex_lock(&dqp->q_qlock);
>  	if ((dqp->q_flags & XFS_DQFLAG_FREEING) || dqp->q_nrefs != 0)
>  		goto out_unlock;
>  
> @@ -177,7 +177,7 @@ xfs_qm_dqpurge(
>  		!test_bit(XFS_LI_IN_AIL, &dqp->q_logitem.qli_item.li_flags));
>  
>  	xfs_dqfunlock(dqp);
> -	xfs_dqunlock(dqp);
> +	mutex_unlock(&dqp->q_qlock);
>  
>  	radix_tree_delete(xfs_dquot_tree(qi, xfs_dquot_type(dqp)), dqp->q_id);
>  	qi->qi_dquots--;
> @@ -194,7 +194,7 @@ xfs_qm_dqpurge(
>  	return 0;
>  
>  out_unlock:
> -	xfs_dqunlock(dqp);
> +	mutex_unlock(&dqp->q_qlock);
>  	return error;
>  }
>  
> @@ -329,7 +329,7 @@ xfs_qm_dqattach_one(
>  	 * that the dquot returned is the one that should go in the inode.
>  	 */
>  	*IO_idqpp = dqp;
> -	xfs_dqunlock(dqp);
> +	mutex_unlock(&dqp->q_qlock);
>  	return 0;
>  }
>  
> @@ -468,7 +468,7 @@ xfs_qm_dquot_isolate(
>  	struct xfs_qm_isolate	*isol = arg;
>  	enum lru_status		ret = LRU_SKIP;
>  
> -	if (!xfs_dqlock_nowait(dqp))
> +	if (!mutex_trylock(&dqp->q_qlock))
>  		goto out_miss_busy;
>  
>  	/*
> @@ -494,7 +494,7 @@ xfs_qm_dquot_isolate(
>  	 * the freelist and try again.
>  	 */
>  	if (dqp->q_nrefs) {
> -		xfs_dqunlock(dqp);
> +		mutex_unlock(&dqp->q_qlock);
>  		XFS_STATS_INC(dqp->q_mount, xs_qm_dqwants);
>  
>  		trace_xfs_dqreclaim_want(dqp);
> @@ -519,7 +519,7 @@ xfs_qm_dquot_isolate(
>  	 * Prevent lookups now that we are past the point of no return.
>  	 */
>  	dqp->q_flags |= XFS_DQFLAG_FREEING;
> -	xfs_dqunlock(dqp);
> +	mutex_unlock(&dqp->q_qlock);
>  
>  	ASSERT(dqp->q_nrefs == 0);
>  	list_lru_isolate_move(lru, &dqp->q_lru, &isol->dispose);
> @@ -529,7 +529,7 @@ xfs_qm_dquot_isolate(
>  	return LRU_REMOVED;
>  
>  out_miss_unlock:
> -	xfs_dqunlock(dqp);
> +	mutex_unlock(&dqp->q_qlock);
>  out_miss_busy:
>  	trace_xfs_dqreclaim_busy(dqp);
>  	XFS_STATS_INC(dqp->q_mount, xs_qm_dqreclaim_misses);
> @@ -1466,7 +1466,7 @@ xfs_qm_flush_one(
>  	struct xfs_buf		*bp = NULL;
>  	int			error = 0;
>  
> -	xfs_dqlock(dqp);
> +	mutex_lock(&dqp->q_qlock);
>  	if (dqp->q_flags & XFS_DQFLAG_FREEING)
>  		goto out_unlock;
>  	if (!XFS_DQ_IS_DIRTY(dqp))
> @@ -1488,7 +1488,7 @@ xfs_qm_flush_one(
>  		xfs_buf_delwri_queue(bp, buffer_list);
>  	xfs_buf_relse(bp);
>  out_unlock:
> -	xfs_dqunlock(dqp);
> +	mutex_unlock(&dqp->q_qlock);
>  	return error;
>  }
>  
> @@ -1951,7 +1951,7 @@ xfs_qm_vop_dqalloc(
>  			/*
>  			 * Get the ilock in the right order.
>  			 */
> -			xfs_dqunlock(uq);
> +			mutex_unlock(&uq->q_qlock);
>  			lockflags = XFS_ILOCK_SHARED;
>  			xfs_ilock(ip, lockflags);
>  		} else {
> @@ -1973,7 +1973,7 @@ xfs_qm_vop_dqalloc(
>  				ASSERT(error != -ENOENT);
>  				goto error_rele;
>  			}
> -			xfs_dqunlock(gq);
> +			mutex_unlock(&gq->q_qlock);
>  			lockflags = XFS_ILOCK_SHARED;
>  			xfs_ilock(ip, lockflags);
>  		} else {
> @@ -1991,7 +1991,7 @@ xfs_qm_vop_dqalloc(
>  				ASSERT(error != -ENOENT);
>  				goto error_rele;
>  			}
> -			xfs_dqunlock(pq);
> +			mutex_unlock(&pq->q_qlock);
>  			lockflags = XFS_ILOCK_SHARED;
>  			xfs_ilock(ip, lockflags);
>  		} else {
> @@ -2078,7 +2078,7 @@ xfs_qm_vop_chown(
>  	 * back now.
>  	 */
>  	tp->t_flags |= XFS_TRANS_DIRTY;
> -	xfs_dqlock(prevdq);
> +	mutex_lock(&prevdq->q_qlock);
>  	if (isrt) {
>  		ASSERT(prevdq->q_rtb.reserved >= ip->i_delayed_blks);
>  		prevdq->q_rtb.reserved -= ip->i_delayed_blks;
> @@ -2086,7 +2086,7 @@ xfs_qm_vop_chown(
>  		ASSERT(prevdq->q_blk.reserved >= ip->i_delayed_blks);
>  		prevdq->q_blk.reserved -= ip->i_delayed_blks;
>  	}
> -	xfs_dqunlock(prevdq);
> +	mutex_unlock(&prevdq->q_qlock);
>  
>  	/*
>  	 * Take an extra reference, because the inode is going to keep
> diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
> index 0c78f30fa4a3..59ef382900fe 100644
> --- a/fs/xfs/xfs_qm_syscalls.c
> +++ b/fs/xfs/xfs_qm_syscalls.c
> @@ -303,13 +303,13 @@ xfs_qm_scall_setqlim(
>  	}
>  
>  	defq = xfs_get_defquota(q, xfs_dquot_type(dqp));
> -	xfs_dqunlock(dqp);
> +	mutex_unlock(&dqp->q_qlock);
>  
>  	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_qm_setqlim, 0, 0, 0, &tp);
>  	if (error)
>  		goto out_rele;
>  
> -	xfs_dqlock(dqp);
> +	mutex_lock(&dqp->q_qlock);
>  	xfs_trans_dqjoin(tp, dqp);
>  
>  	/*
> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
> index 765456bf3428..c842ce06acd6 100644
> --- a/fs/xfs/xfs_trans_dquot.c
> +++ b/fs/xfs/xfs_trans_dquot.c
> @@ -393,7 +393,7 @@ xfs_trans_dqlockedjoin(
>  	unsigned int		i;
>  	ASSERT(q[0].qt_dquot != NULL);
>  	if (q[1].qt_dquot == NULL) {
> -		xfs_dqlock(q[0].qt_dquot);
> +		mutex_lock(&q[0].qt_dquot->q_qlock);
>  		xfs_trans_dqjoin(tp, q[0].qt_dquot);
>  	} else if (q[2].qt_dquot == NULL) {
>  		xfs_dqlock2(q[0].qt_dquot, q[1].qt_dquot);
> @@ -693,7 +693,7 @@ xfs_trans_unreserve_and_mod_dquots(
>  			locked = already_locked;
>  			if (qtrx->qt_blk_res) {
>  				if (!locked) {
> -					xfs_dqlock(dqp);
> +					mutex_lock(&dqp->q_qlock);
>  					locked = true;
>  				}
>  				dqp->q_blk.reserved -=
> @@ -701,7 +701,7 @@ xfs_trans_unreserve_and_mod_dquots(
>  			}
>  			if (qtrx->qt_ino_res) {
>  				if (!locked) {
> -					xfs_dqlock(dqp);
> +					mutex_lock(&dqp->q_qlock);
>  					locked = true;
>  				}
>  				dqp->q_ino.reserved -=
> @@ -710,14 +710,14 @@ xfs_trans_unreserve_and_mod_dquots(
>  
>  			if (qtrx->qt_rtblk_res) {
>  				if (!locked) {
> -					xfs_dqlock(dqp);
> +					mutex_lock(&dqp->q_qlock);
>  					locked = true;
>  				}
>  				dqp->q_rtb.reserved -=
>  					(xfs_qcnt_t)qtrx->qt_rtblk_res;
>  			}
>  			if (locked && !already_locked)
> -				xfs_dqunlock(dqp);
> +				mutex_unlock(&dqp->q_qlock);
>  
>  		}
>  	}
> @@ -820,7 +820,7 @@ xfs_trans_dqresv(
>  	struct xfs_dquot_res	*blkres;
>  	struct xfs_quota_limits	*qlim;
>  
> -	xfs_dqlock(dqp);
> +	mutex_lock(&dqp->q_qlock);
>  
>  	defq = xfs_get_defquota(q, xfs_dquot_type(dqp));
>  
> @@ -887,16 +887,16 @@ xfs_trans_dqresv(
>  	    XFS_IS_CORRUPT(mp, dqp->q_ino.reserved < dqp->q_ino.count))
>  		goto error_corrupt;
>  
> -	xfs_dqunlock(dqp);
> +	mutex_unlock(&dqp->q_qlock);
>  	return 0;
>  
>  error_return:
> -	xfs_dqunlock(dqp);
> +	mutex_unlock(&dqp->q_qlock);
>  	if (xfs_dquot_type(dqp) == XFS_DQTYPE_PROJ)
>  		return -ENOSPC;
>  	return -EDQUOT;
>  error_corrupt:
> -	xfs_dqunlock(dqp);
> +	mutex_unlock(&dqp->q_qlock);
>  	xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
>  	xfs_fs_mark_sick(mp, XFS_SICK_FS_QUOTACHECK);
>  	return -EFSCORRUPTED;
> -- 
> 2.47.3
> 
> 

Re: [PATCH 03/17] xfs: don't lock the dquot before return in xqcheck_commit_dquot

[Darrick J. Wong]

On Mon, Oct 13, 2025 at 11:48:04AM +0900, Christoph Hellwig wrote:
> While xfs_qm_dqput requires the dquot to be locked, the callers can use
> the more common xfs_qm_dqrele helper that takes care of locking the dquot
> instead.

But the start of xqcheck_commit_dquot does:

	/* Unlock the dquot just long enough to allocate a transaction. */
	xfs_dqunlock(dq);
	error = xchk_trans_alloc(xqc->sc, 0);
>>	xfs_dqlock(dq);
	if (error)
		return error;

So that'll cause a livelock in xfs_qm_dqrele in the loop body below if
the transaction allocation fails.

--D

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/scrub/quotacheck_repair.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/xfs/scrub/quotacheck_repair.c b/fs/xfs/scrub/quotacheck_repair.c
> index 415314911499..67bdc872996a 100644
> --- a/fs/xfs/scrub/quotacheck_repair.c
> +++ b/fs/xfs/scrub/quotacheck_repair.c
> @@ -121,17 +121,12 @@ xqcheck_commit_dquot(
>  	 * the caller can put the reference (which apparently requires a locked
>  	 * dquot).
>  	 */
> -	error = xrep_trans_commit(xqc->sc);
> -	mutex_lock(&dq->q_qlock);
> -	return error;
> +	return xrep_trans_commit(xqc->sc);
>  
>  out_unlock:
>  	mutex_unlock(&xqc->lock);
>  out_cancel:
>  	xchk_trans_cancel(xqc->sc);
> -
> -	/* Re-lock the dquot so the caller can put the reference. */
> -	mutex_lock(&dq->q_qlock);
>  	return error;
>  }
>  
> @@ -156,7 +151,7 @@ xqcheck_commit_dqtype(
>  	xchk_dqiter_init(&cursor, sc, dqtype);
>  	while ((error = xchk_dquot_iter(&cursor, &dq)) == 1) {
>  		error = xqcheck_commit_dquot(xqc, dqtype, dq);
> -		xfs_qm_dqput(dq);
> +		xfs_qm_dqrele(dq);
>  		if (error)
>  			break;
>  	}
> @@ -187,7 +182,7 @@ xqcheck_commit_dqtype(
>  			return error;
>  
>  		error = xqcheck_commit_dquot(xqc, dqtype, dq);
> -		xfs_qm_dqput(dq);
> +		xfs_qm_dqrele(dq);
>  		if (error)
>  			return error;
>  
> -- 
> 2.47.3
> 
> 

Re: [PATCH 04/17] xfs: don't lock the dquot before return in xrep_quota_item

[Darrick J. Wong]

On Mon, Oct 13, 2025 at 11:48:05AM +0900, Christoph Hellwig wrote:
> While xfs_qm_dqput requires the dquot to be locked, the caller can use
> the more common xfs_qm_dqrele helper that takes care of locking the
> dquot instead.

Similar problem here -- there are other paths out of xrep_quota_item
that return with the dqlock held, and the new xfs_qm_dqrele will try to
re-take the lock and livelock.

--D

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/scrub/quota_repair.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/scrub/quota_repair.c b/fs/xfs/scrub/quota_repair.c
> index 8c89c6cc2950..00a4d2e75797 100644
> --- a/fs/xfs/scrub/quota_repair.c
> +++ b/fs/xfs/scrub/quota_repair.c
> @@ -257,9 +257,7 @@ xrep_quota_item(
>  		xfs_qm_adjust_dqtimers(dq);
>  	}
>  	xfs_trans_log_dquot(sc->tp, dq);
> -	error = xfs_trans_roll(&sc->tp);
> -	mutex_lock(&dq->q_qlock);
> -	return error;
> +	return xfs_trans_roll(&sc->tp);
>  }
>  
>  /* Fix a quota timer so that we can pass the verifier. */
> @@ -513,7 +511,7 @@ xrep_quota_problems(
>  	xchk_dqiter_init(&cursor, sc, dqtype);
>  	while ((error = xchk_dquot_iter(&cursor, &dq)) == 1) {
>  		error = xrep_quota_item(&rqi, dq);
> -		xfs_qm_dqput(dq);
> +		xfs_qm_dqrele(dq);
>  		if (error)
>  			break;
>  	}
> -- 
> 2.47.3
> 
> 

Re: [PATCH 08/17] xfs: xfs_qm_dqattach_one is never called with a non-NULL *IO_idqpp

[Darrick J. Wong]

On Mon, Oct 13, 2025 at 11:48:09AM +0900, Christoph Hellwig wrote:
> The caller already checks that, so replace the handling of this case with
> an assert that it does not happen.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Yep.
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_qm.c    | 13 +------------
>  fs/xfs/xfs_trace.h |  1 -
>  2 files changed, 1 insertion(+), 13 deletions(-)
> 
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index 9bd7068b9e5a..80c99ef91edb 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -297,19 +297,8 @@ xfs_qm_dqattach_one(
>  	struct xfs_dquot	*dqp;
>  	int			error;
>  
> +	ASSERT(!*IO_idqpp);
>  	xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
> -	error = 0;
> -
> -	/*
> -	 * See if we already have it in the inode itself. IO_idqpp is &i_udquot
> -	 * or &i_gdquot. This made the code look weird, but made the logic a lot
> -	 * simpler.
> -	 */
> -	dqp = *IO_idqpp;
> -	if (dqp) {
> -		trace_xfs_dqattach_found(dqp);
> -		return 0;
> -	}
>  
>  	/*
>  	 * Find the dquot from somewhere. This bumps the reference count of
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index fccc032b3c6c..90582ff7c2cf 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -1399,7 +1399,6 @@ DEFINE_DQUOT_EVENT(xfs_dqadjust);
>  DEFINE_DQUOT_EVENT(xfs_dqreclaim_want);
>  DEFINE_DQUOT_EVENT(xfs_dqreclaim_busy);
>  DEFINE_DQUOT_EVENT(xfs_dqreclaim_done);
> -DEFINE_DQUOT_EVENT(xfs_dqattach_found);
>  DEFINE_DQUOT_EVENT(xfs_dqattach_get);
>  DEFINE_DQUOT_EVENT(xfs_dqalloc);
>  DEFINE_DQUOT_EVENT(xfs_dqtobp_read);
> -- 
> 2.47.3
> 
> 

Re: [PATCH 01/17] xfs: make qi_dquots a 64-bit value

[Christoph Hellwig]

On Tue, Oct 14, 2025 at 04:16:27PM -0700, Darrick J. Wong wrote:
> > -	state->s_incoredqs = q->qi_dquots;
> > +	state->s_incoredqs = max_t(uint64_t, q->qi_dquots, UINT_MAX);
> 
> Isn't this min_t?  Surely we don't want to return 0xFFFFFFFF when
> there's only 3 dquots loaded in the system?

Yes, this should be a min.  Looks like nothing in our tests actually
cares about this value..

Re: [PATCH 03/17] xfs: don't lock the dquot before return in xqcheck_commit_dquot

[Christoph Hellwig]

On Tue, Oct 14, 2025 at 04:22:44PM -0700, Darrick J. Wong wrote:
> On Mon, Oct 13, 2025 at 11:48:04AM +0900, Christoph Hellwig wrote:
> > While xfs_qm_dqput requires the dquot to be locked, the callers can use
> > the more common xfs_qm_dqrele helper that takes care of locking the dquot
> > instead.
> 
> But the start of xqcheck_commit_dquot does:
> 
> 	/* Unlock the dquot just long enough to allocate a transaction. */
> 	xfs_dqunlock(dq);
> 	error = xchk_trans_alloc(xqc->sc, 0);
> >>	xfs_dqlock(dq);
> 	if (error)
> 		return error;
> 
> So that'll cause a livelock in xfs_qm_dqrele in the loop body below if
> the transaction allocation fails.

I think livelock is the wrong term, it's just a leaked lock.  This gets
fixed towards the end of the series, and I'll fix it by reshuffling
the series so that this only happens later and at the same time as those
changes.  Same for the next patch.

Re: [PATCH 03/17] xfs: don't lock the dquot before return in xqcheck_commit_dquot

[Darrick J. Wong]

On Wed, Oct 15, 2025 at 07:00:43AM +0200, Christoph Hellwig wrote:
> On Tue, Oct 14, 2025 at 04:22:44PM -0700, Darrick J. Wong wrote:
> > On Mon, Oct 13, 2025 at 11:48:04AM +0900, Christoph Hellwig wrote:
> > > While xfs_qm_dqput requires the dquot to be locked, the callers can use
> > > the more common xfs_qm_dqrele helper that takes care of locking the dquot
> > > instead.
> > 
> > But the start of xqcheck_commit_dquot does:
> > 
> > 	/* Unlock the dquot just long enough to allocate a transaction. */
> > 	xfs_dqunlock(dq);
> > 	error = xchk_trans_alloc(xqc->sc, 0);
> > >>	xfs_dqlock(dq);
> > 	if (error)
> > 		return error;
> > 
> > So that'll cause a livelock in xfs_qm_dqrele in the loop body below if
> > the transaction allocation fails.
> 
> I think livelock is the wrong term, it's just a leaked lock.  This gets
> fixed towards the end of the series, and I'll fix it by reshuffling
> the series so that this only happens later and at the same time as those
> changes.  Same for the next patch.

Ok let's see how far I get with ignoring dqrele/dqput for the rest of
the series then. ;)

--D

Re: [PATCH 05/17] xfs: use a lockref for the xfs_dquot reference count

[Darrick J. Wong]

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
> 
> 

Re: [PATCH 06/17] xfs: remove xfs_qm_dqput and optimize dropping dquot references

[Darrick J. Wong]

On Mon, Oct 13, 2025 at 11:48:07AM +0900, Christoph Hellwig wrote:
> With the new lockref-based dquot reference counting, there is no need to
> hold q_qlock for dropping the reference.  Make xfs_qm_dqrele the main
> function to drop dquot references without taking q_qlock and convert all
> callers of xfs_qm_dqput to unlock q_qlock and call xfs_qm_dqrele instead.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Code looks fine, but I'm guessing this is the later patch that cleans up
the weirdness in patch 3?  I'll wait for a new patch 3 then.

--D

> ---
>  fs/xfs/scrub/quota.c      |  3 ++-
>  fs/xfs/scrub/quotacheck.c |  6 ++++--
>  fs/xfs/xfs_dquot.c        | 45 ++++++++-------------------------------
>  fs/xfs/xfs_dquot.h        |  1 -
>  fs/xfs/xfs_qm.c           |  7 ++++--
>  fs/xfs/xfs_qm_bhv.c       |  3 ++-
>  fs/xfs/xfs_qm_syscalls.c  |  6 ++++--
>  fs/xfs/xfs_trace.h        |  3 +--
>  8 files changed, 27 insertions(+), 47 deletions(-)
> 
> diff --git a/fs/xfs/scrub/quota.c b/fs/xfs/scrub/quota.c
> index c78cf9f96cf6..cfcd0fb66845 100644
> --- a/fs/xfs/scrub/quota.c
> +++ b/fs/xfs/scrub/quota.c
> @@ -330,7 +330,8 @@ xchk_quota(
>  	xchk_dqiter_init(&cursor, sc, dqtype);
>  	while ((error = xchk_dquot_iter(&cursor, &dq)) == 1) {
>  		error = xchk_quota_item(&sqi, dq);
> -		xfs_qm_dqput(dq);
> +		mutex_unlock(&dq->q_qlock);
> +		xfs_qm_dqrele(dq);
>  		if (error)
>  			break;
>  	}
> diff --git a/fs/xfs/scrub/quotacheck.c b/fs/xfs/scrub/quotacheck.c
> index e4105aaafe84..180449f654f6 100644
> --- a/fs/xfs/scrub/quotacheck.c
> +++ b/fs/xfs/scrub/quotacheck.c
> @@ -636,7 +636,8 @@ xqcheck_walk_observations(
>  			return error;
>  
>  		error = xqcheck_compare_dquot(xqc, dqtype, dq);
> -		xfs_qm_dqput(dq);
> +		mutex_unlock(&dq->q_qlock);
> +		xfs_qm_dqrele(dq);
>  		if (error)
>  			return error;
>  
> @@ -674,7 +675,8 @@ xqcheck_compare_dqtype(
>  	xchk_dqiter_init(&cursor, sc, dqtype);
>  	while ((error = xchk_dquot_iter(&cursor, &dq)) == 1) {
>  		error = xqcheck_compare_dquot(xqc, dqtype, dq);
> -		xfs_qm_dqput(dq);
> +		mutex_unlock(&dq->q_qlock);
> +		xfs_qm_dqrele(dq);
>  		if (error)
>  			break;
>  	}
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index e53dffe2dcab..ceddbbb41999 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -1100,62 +1100,35 @@ xfs_qm_dqget_next(
>  			return 0;
>  		}
>  
> -		xfs_qm_dqput(dqp);
> +		mutex_unlock(&dqp->q_qlock);
> +		xfs_qm_dqrele(dqp);
>  	}
>  
>  	return error;
>  }
>  
>  /*
> - * Release a reference to the dquot (decrement ref-count) and unlock it.
> - *
> - * If there is a group quota attached to this dquot, carefully release that
> - * too without tripping over deadlocks'n'stuff.
> + * Release a reference to the dquot.
>   */
>  void
> -xfs_qm_dqput(
> +xfs_qm_dqrele(
>  	struct xfs_dquot	*dqp)
>  {
> -	ASSERT(XFS_DQ_IS_LOCKED(dqp));
> +	if (!dqp)
> +		return;
>  
> -	trace_xfs_dqput(dqp);
> +	trace_xfs_dqrele(dqp);
>  
>  	if (lockref_put_or_lock(&dqp->q_lockref))
> -		goto out_unlock;
> -
> +		return;
>  	if (!--dqp->q_lockref.count) {
>  		struct xfs_quotainfo	*qi = dqp->q_mount->m_quotainfo;
> -		trace_xfs_dqput_free(dqp);
>  
> +		trace_xfs_dqrele_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);
> -}
> -
> -/*
> - * Release a dquot. Flush it if dirty, then dqput() it.
> - * dquot must not be locked.
> - */
> -void
> -xfs_qm_dqrele(
> -	struct xfs_dquot	*dqp)
> -{
> -	if (!dqp)
> -		return;
> -
> -	trace_xfs_dqrele(dqp);
> -
> -	mutex_lock(&dqp->q_qlock);
> -	/*
> -	 * We don't care to flush it if the dquot is dirty here.
> -	 * That will create stutters that we want to avoid.
> -	 * Instead we do a delayed write when we try to reclaim
> -	 * a dirty dquot. Also xfs_sync will take part of the burden...
> -	 */
> -	xfs_qm_dqput(dqp);
>  }
>  
>  /*
> diff --git a/fs/xfs/xfs_dquot.h b/fs/xfs/xfs_dquot.h
> index c56fbc39d089..bbb824adca82 100644
> --- a/fs/xfs/xfs_dquot.h
> +++ b/fs/xfs/xfs_dquot.h
> @@ -218,7 +218,6 @@ int		xfs_qm_dqget_next(struct xfs_mount *mp, xfs_dqid_t id,
>  int		xfs_qm_dqget_uncached(struct xfs_mount *mp,
>  				xfs_dqid_t id, xfs_dqtype_t type,
>  				struct xfs_dquot **dqpp);
> -void		xfs_qm_dqput(struct xfs_dquot *dqp);
>  
>  void		xfs_dqlock2(struct xfs_dquot *, struct xfs_dquot *);
>  void		xfs_dqlockn(struct xfs_dqtrx *q);
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index 0d2243d549ad..9bd7068b9e5a 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -1345,7 +1345,9 @@ xfs_qm_quotacheck_dqadjust(
>  	}
>  
>  	dqp->q_flags |= XFS_DQFLAG_DIRTY;
> -	xfs_qm_dqput(dqp);
> +	mutex_unlock(&dqp->q_qlock);
> +
> +	xfs_qm_dqrele(dqp);
>  	return 0;
>  }
>  
> @@ -1486,7 +1488,8 @@ xfs_qm_flush_one(
>  		xfs_buf_delwri_queue(bp, buffer_list);
>  	xfs_buf_relse(bp);
>  out_unlock:
> -	xfs_qm_dqput(dqp);
> +	mutex_unlock(&dqp->q_qlock);
> +	xfs_qm_dqrele(dqp);
>  	return error;
>  }
>  
> diff --git a/fs/xfs/xfs_qm_bhv.c b/fs/xfs/xfs_qm_bhv.c
> index 245d754f382a..e5a30b12253c 100644
> --- a/fs/xfs/xfs_qm_bhv.c
> +++ b/fs/xfs/xfs_qm_bhv.c
> @@ -74,7 +74,8 @@ xfs_qm_statvfs(
>  
>  	if (!xfs_qm_dqget(mp, ip->i_projid, XFS_DQTYPE_PROJ, false, &dqp)) {
>  		xfs_fill_statvfs_from_dquot(statp, ip, dqp);
> -		xfs_qm_dqput(dqp);
> +		mutex_unlock(&dqp->q_qlock);
> +		xfs_qm_dqrele(dqp);
>  	}
>  }
>  
> diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
> index 59ef382900fe..441f9806cddb 100644
> --- a/fs/xfs/xfs_qm_syscalls.c
> +++ b/fs/xfs/xfs_qm_syscalls.c
> @@ -467,7 +467,8 @@ xfs_qm_scall_getquota(
>  	xfs_qm_scall_getquota_fill_qc(mp, type, dqp, dst);
>  
>  out_put:
> -	xfs_qm_dqput(dqp);
> +	mutex_unlock(&dqp->q_qlock);
> +	xfs_qm_dqrele(dqp);
>  	return error;
>  }
>  
> @@ -497,7 +498,8 @@ xfs_qm_scall_getquota_next(
>  	*id = dqp->q_id;
>  
>  	xfs_qm_scall_getquota_fill_qc(mp, type, dqp, dst);
> +	mutex_unlock(&dqp->q_qlock);
>  
> -	xfs_qm_dqput(dqp);
> +	xfs_qm_dqrele(dqp);
>  	return error;
>  }
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 46d21eb11ccb..fccc032b3c6c 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -1409,9 +1409,8 @@ DEFINE_DQUOT_EVENT(xfs_dqget_hit);
>  DEFINE_DQUOT_EVENT(xfs_dqget_miss);
>  DEFINE_DQUOT_EVENT(xfs_dqget_freeing);
>  DEFINE_DQUOT_EVENT(xfs_dqget_dup);
> -DEFINE_DQUOT_EVENT(xfs_dqput);
> -DEFINE_DQUOT_EVENT(xfs_dqput_free);
>  DEFINE_DQUOT_EVENT(xfs_dqrele);
> +DEFINE_DQUOT_EVENT(xfs_dqrele_free);
>  DEFINE_DQUOT_EVENT(xfs_dqflush);
>  DEFINE_DQUOT_EVENT(xfs_dqflush_force);
>  DEFINE_DQUOT_EVENT(xfs_dqflush_done);
> -- 
> 2.47.3
> 
> 

Re: [PATCH 07/17] xfs: consolidate q_qlock locking in xfs_qm_dqget and xfs_qm_dqget_inode

[Darrick J. Wong]

On Mon, Oct 13, 2025 at 11:48:08AM +0900, Christoph Hellwig wrote:
> Move taking q_qlock from the cache lookup / insert helpers into the
> main functions and do it just before returning to the caller.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Makes sense that dqget returns a qlock'd dquot no matter where it came
from
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_dquot.c | 23 +++++++++--------------
>  1 file changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index ceddbbb41999..a6030c53a1f9 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -826,15 +826,13 @@ xfs_qm_dqget_cache_lookup(
>  
>  	trace_xfs_dqget_hit(dqp);
>  	XFS_STATS_INC(mp, xs_qm_dqcachehits);
> -	mutex_lock(&dqp->q_qlock);
>  	return dqp;
>  }
>  
>  /*
>   * Try to insert a new dquot into the in-core cache.  If an error occurs the
>   * caller should throw away the dquot and start over.  Otherwise, the dquot
> - * is returned locked (and held by the cache) as if there had been a cache
> - * hit.
> + * is returned (and held by the cache) as if there had been a cache hit.
>   *
>   * The insert needs to be done under memalloc_nofs context because the radix
>   * tree can do memory allocation during insert. The qi->qi_tree_lock is taken in
> @@ -862,8 +860,6 @@ xfs_qm_dqget_cache_insert(
>  		goto out_unlock;
>  	}
>  
> -	/* Return a locked dquot to the caller, with a reference taken. */
> -	mutex_lock(&dqp->q_qlock);
>  	lockref_init(&dqp->q_lockref);
>  	qi->qi_dquots++;
>  
> @@ -921,10 +917,8 @@ xfs_qm_dqget(
>  
>  restart:
>  	dqp = xfs_qm_dqget_cache_lookup(mp, qi, tree, id);
> -	if (dqp) {
> -		*O_dqpp = dqp;
> -		return 0;
> -	}
> +	if (dqp)
> +		goto found;
>  
>  	error = xfs_qm_dqread(mp, id, type, can_alloc, &dqp);
>  	if (error)
> @@ -942,7 +936,9 @@ xfs_qm_dqget(
>  	}
>  
>  	trace_xfs_dqget_miss(dqp);
> +found:
>  	*O_dqpp = dqp;
> +	mutex_lock(&dqp->q_qlock);
>  	return 0;
>  }
>  
> @@ -1017,10 +1013,8 @@ xfs_qm_dqget_inode(
>  
>  restart:
>  	dqp = xfs_qm_dqget_cache_lookup(mp, qi, tree, id);
> -	if (dqp) {
> -		*O_dqpp = dqp;
> -		return 0;
> -	}
> +	if (dqp)
> +		goto found;
>  
>  	/*
>  	 * Dquot cache miss. We don't want to keep the inode lock across
> @@ -1046,7 +1040,6 @@ xfs_qm_dqget_inode(
>  		if (dqp1) {
>  			xfs_qm_dqdestroy(dqp);
>  			dqp = dqp1;
> -			mutex_lock(&dqp->q_qlock);
>  			goto dqret;
>  		}
>  	} else {
> @@ -1069,7 +1062,9 @@ xfs_qm_dqget_inode(
>  dqret:
>  	xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
>  	trace_xfs_dqget_miss(dqp);
> +found:
>  	*O_dqpp = dqp;
> +	mutex_lock(&dqp->q_qlock);
>  	return 0;
>  }
>  
> -- 
> 2.47.3
> 
> 

Re: [PATCH 09/17] xfs: fold xfs_qm_dqattach_one into xfs_qm_dqget_inode

[Darrick J. Wong]

On Mon, Oct 13, 2025 at 11:48:10AM +0900, Christoph Hellwig wrote:
> xfs_qm_dqattach_one is a thin wrapper around xfs_qm_dqget_inode.  Move
> the extra asserts into xfs_qm_dqget_inode, drop the unneeded q_qlock
> roundtrip and merge the two functions.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Nice straightforward collapse there!
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_dquot.c |  9 ++++++---
>  fs/xfs/xfs_qm.c    | 40 +++-------------------------------------
>  2 files changed, 9 insertions(+), 40 deletions(-)
> 
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index a6030c53a1f9..fa493520bea6 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -992,7 +992,7 @@ xfs_qm_dqget_inode(
>  	struct xfs_inode	*ip,
>  	xfs_dqtype_t		type,
>  	bool			can_alloc,
> -	struct xfs_dquot	**O_dqpp)
> +	struct xfs_dquot	**dqpp)
>  {
>  	struct xfs_mount	*mp = ip->i_mount;
>  	struct xfs_quotainfo	*qi = mp->m_quotainfo;
> @@ -1001,6 +1001,9 @@ xfs_qm_dqget_inode(
>  	xfs_dqid_t		id;
>  	int			error;
>  
> +	ASSERT(!*dqpp);
> +	xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
> +
>  	error = xfs_qm_dqget_checks(mp, type);
>  	if (error)
>  		return error;
> @@ -1063,8 +1066,8 @@ xfs_qm_dqget_inode(
>  	xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
>  	trace_xfs_dqget_miss(dqp);
>  found:
> -	*O_dqpp = dqp;
> -	mutex_lock(&dqp->q_qlock);
> +	trace_xfs_dqattach_get(dqp);
> +	*dqpp = dqp;
>  	return 0;
>  }
>  
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index 80c99ef91edb..9e173a4b18eb 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -287,40 +287,6 @@ xfs_qm_unmount_quotas(
>  		xfs_qm_destroy_quotainos(mp->m_quotainfo);
>  }
>  
> -STATIC int
> -xfs_qm_dqattach_one(
> -	struct xfs_inode	*ip,
> -	xfs_dqtype_t		type,
> -	bool			doalloc,
> -	struct xfs_dquot	**IO_idqpp)
> -{
> -	struct xfs_dquot	*dqp;
> -	int			error;
> -
> -	ASSERT(!*IO_idqpp);
> -	xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
> -
> -	/*
> -	 * Find the dquot from somewhere. This bumps the reference count of
> -	 * dquot and returns it locked.  This can return ENOENT if dquot didn't
> -	 * exist on disk and we didn't ask it to allocate; ESRCH if quotas got
> -	 * turned off suddenly.
> -	 */
> -	error = xfs_qm_dqget_inode(ip, type, doalloc, &dqp);
> -	if (error)
> -		return error;
> -
> -	trace_xfs_dqattach_get(dqp);
> -
> -	/*
> -	 * dqget may have dropped and re-acquired the ilock, but it guarantees
> -	 * that the dquot returned is the one that should go in the inode.
> -	 */
> -	*IO_idqpp = dqp;
> -	mutex_unlock(&dqp->q_qlock);
> -	return 0;
> -}
> -
>  static bool
>  xfs_qm_need_dqattach(
>  	struct xfs_inode	*ip)
> @@ -360,7 +326,7 @@ xfs_qm_dqattach_locked(
>  	ASSERT(!xfs_is_metadir_inode(ip));
>  
>  	if (XFS_IS_UQUOTA_ON(mp) && !ip->i_udquot) {
> -		error = xfs_qm_dqattach_one(ip, XFS_DQTYPE_USER,
> +		error = xfs_qm_dqget_inode(ip, XFS_DQTYPE_USER,
>  				doalloc, &ip->i_udquot);
>  		if (error)
>  			goto done;
> @@ -368,7 +334,7 @@ xfs_qm_dqattach_locked(
>  	}
>  
>  	if (XFS_IS_GQUOTA_ON(mp) && !ip->i_gdquot) {
> -		error = xfs_qm_dqattach_one(ip, XFS_DQTYPE_GROUP,
> +		error = xfs_qm_dqget_inode(ip, XFS_DQTYPE_GROUP,
>  				doalloc, &ip->i_gdquot);
>  		if (error)
>  			goto done;
> @@ -376,7 +342,7 @@ xfs_qm_dqattach_locked(
>  	}
>  
>  	if (XFS_IS_PQUOTA_ON(mp) && !ip->i_pdquot) {
> -		error = xfs_qm_dqattach_one(ip, XFS_DQTYPE_PROJ,
> +		error = xfs_qm_dqget_inode(ip, XFS_DQTYPE_PROJ,
>  				doalloc, &ip->i_pdquot);
>  		if (error)
>  			goto done;
> -- 
> 2.47.3
> 
> 

Re: [PATCH 10/17] xfs: return the dquot unlocked from xfs_qm_dqget

[Darrick J. Wong]

On Mon, Oct 13, 2025 at 11:48:11AM +0900, Christoph Hellwig wrote:
> There is no reason to lock the dquot in xfs_qm_dqget, which just acquires
> a reference.  Move the locking to the callers, or remove it in cases where
> the caller instantly unlocks the dquot.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/scrub/dqiterate.c         | 1 +
>  fs/xfs/scrub/quotacheck.c        | 1 +
>  fs/xfs/scrub/quotacheck_repair.c | 1 +
>  fs/xfs/xfs_dquot.c               | 4 ++--
>  fs/xfs/xfs_qm.c                  | 4 +---
>  fs/xfs/xfs_qm_bhv.c              | 1 +
>  fs/xfs/xfs_qm_syscalls.c         | 2 ++
>  7 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/xfs/scrub/dqiterate.c b/fs/xfs/scrub/dqiterate.c
> index 20c4daedd48d..6f1185afbf39 100644
> --- a/fs/xfs/scrub/dqiterate.c
> +++ b/fs/xfs/scrub/dqiterate.c
> @@ -205,6 +205,7 @@ xchk_dquot_iter(
>  	if (error)
>  		return error;
>  
> +	mutex_lock(&dq->q_qlock);
>  	cursor->id = dq->q_id + 1;
>  	*dqpp = dq;
>  	return 1;
> diff --git a/fs/xfs/scrub/quotacheck.c b/fs/xfs/scrub/quotacheck.c
> index 180449f654f6..bef63f19cd87 100644
> --- a/fs/xfs/scrub/quotacheck.c
> +++ b/fs/xfs/scrub/quotacheck.c
> @@ -635,6 +635,7 @@ xqcheck_walk_observations(
>  		if (error)
>  			return error;
>  
> +		mutex_lock(&dq->q_qlock);
>  		error = xqcheck_compare_dquot(xqc, dqtype, dq);
>  		mutex_unlock(&dq->q_qlock);
>  		xfs_qm_dqrele(dq);
> diff --git a/fs/xfs/scrub/quotacheck_repair.c b/fs/xfs/scrub/quotacheck_repair.c
> index 67bdc872996a..f7b1add43a2c 100644
> --- a/fs/xfs/scrub/quotacheck_repair.c
> +++ b/fs/xfs/scrub/quotacheck_repair.c
> @@ -181,6 +181,7 @@ xqcheck_commit_dqtype(
>  		if (error)
>  			return error;
>  
> +		mutex_lock(&dq->q_qlock);
>  		error = xqcheck_commit_dquot(xqc, dqtype, dq);
>  		xfs_qm_dqrele(dq);

Hmm.  No mutex_unlock() ?

Oh, because @dq gets added to the scrub transaction and the
commit/cancel and unlocks it, right?

(Maybe the mutex_lock should go in xqcheck_commit_dquot to avoid the
unbalanced lock state before after the function call?)

--D

>  		if (error)
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index fa493520bea6..98593b380e94 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -896,7 +896,7 @@ xfs_qm_dqget_checks(
>  
>  /*
>   * Given the file system, id, and type (UDQUOT/GDQUOT/PDQUOT), return a
> - * locked dquot, doing an allocation (if requested) as needed.
> + * dquot, doing an allocation (if requested) as needed.
>   */
>  int
>  xfs_qm_dqget(
> @@ -938,7 +938,6 @@ xfs_qm_dqget(
>  	trace_xfs_dqget_miss(dqp);
>  found:
>  	*O_dqpp = dqp;
> -	mutex_lock(&dqp->q_qlock);
>  	return 0;
>  }
>  
> @@ -1093,6 +1092,7 @@ xfs_qm_dqget_next(
>  		else if (error != 0)
>  			break;
>  
> +		mutex_lock(&dqp->q_qlock);
>  		if (!XFS_IS_DQUOT_UNINITIALIZED(dqp)) {
>  			*dqpp = dqp;
>  			return 0;
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index 9e173a4b18eb..7fbb89fcdeb9 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -1268,6 +1268,7 @@ xfs_qm_quotacheck_dqadjust(
>  		return error;
>  	}
>  
> +	mutex_lock(&dqp->q_qlock);
>  	error = xfs_dquot_attach_buf(NULL, dqp);
>  	if (error)
>  		return error;
> @@ -1907,7 +1908,6 @@ xfs_qm_vop_dqalloc(
>  			/*
>  			 * Get the ilock in the right order.
>  			 */
> -			mutex_unlock(&uq->q_qlock);
>  			lockflags = XFS_ILOCK_SHARED;
>  			xfs_ilock(ip, lockflags);
>  		} else {
> @@ -1929,7 +1929,6 @@ xfs_qm_vop_dqalloc(
>  				ASSERT(error != -ENOENT);
>  				goto error_rele;
>  			}
> -			mutex_unlock(&gq->q_qlock);
>  			lockflags = XFS_ILOCK_SHARED;
>  			xfs_ilock(ip, lockflags);
>  		} else {
> @@ -1947,7 +1946,6 @@ xfs_qm_vop_dqalloc(
>  				ASSERT(error != -ENOENT);
>  				goto error_rele;
>  			}
> -			mutex_unlock(&pq->q_qlock);
>  			lockflags = XFS_ILOCK_SHARED;
>  			xfs_ilock(ip, lockflags);
>  		} else {
> diff --git a/fs/xfs/xfs_qm_bhv.c b/fs/xfs/xfs_qm_bhv.c
> index e5a30b12253c..edc0aef3cf34 100644
> --- a/fs/xfs/xfs_qm_bhv.c
> +++ b/fs/xfs/xfs_qm_bhv.c
> @@ -73,6 +73,7 @@ xfs_qm_statvfs(
>  	struct xfs_dquot	*dqp;
>  
>  	if (!xfs_qm_dqget(mp, ip->i_projid, XFS_DQTYPE_PROJ, false, &dqp)) {
> +		mutex_lock(&dqp->q_qlock);
>  		xfs_fill_statvfs_from_dquot(statp, ip, dqp);
>  		mutex_unlock(&dqp->q_qlock);
>  		xfs_qm_dqrele(dqp);
> diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
> index 441f9806cddb..6c8924780d7a 100644
> --- a/fs/xfs/xfs_qm_syscalls.c
> +++ b/fs/xfs/xfs_qm_syscalls.c
> @@ -302,6 +302,7 @@ xfs_qm_scall_setqlim(
>  		return error;
>  	}
>  
> +	mutex_lock(&dqp->q_qlock);
>  	defq = xfs_get_defquota(q, xfs_dquot_type(dqp));
>  	mutex_unlock(&dqp->q_qlock);
>  
> @@ -459,6 +460,7 @@ xfs_qm_scall_getquota(
>  	 * If everything's NULL, this dquot doesn't quite exist as far as
>  	 * our utility programs are concerned.
>  	 */
> +	mutex_lock(&dqp->q_qlock);
>  	if (XFS_IS_DQUOT_UNINITIALIZED(dqp)) {
>  		error = -ENOENT;
>  		goto out_put;
> -- 
> 2.47.3
> 
> 

Re: [PATCH 11/17] xfs: remove q_qlock locking in xfs_qm_scall_setqlim

[Darrick J. Wong]

On Mon, Oct 13, 2025 at 11:48:12AM +0900, Christoph Hellwig wrote:
> q_type can't change for an existing dquot, so there is no need for
> the locking here.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Right.
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_qm_syscalls.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
> index 6c8924780d7a..022e2179c06b 100644
> --- a/fs/xfs/xfs_qm_syscalls.c
> +++ b/fs/xfs/xfs_qm_syscalls.c
> @@ -302,9 +302,7 @@ xfs_qm_scall_setqlim(
>  		return error;
>  	}
>  
> -	mutex_lock(&dqp->q_qlock);
>  	defq = xfs_get_defquota(q, xfs_dquot_type(dqp));
> -	mutex_unlock(&dqp->q_qlock);
>  
>  	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_qm_setqlim, 0, 0, 0, &tp);
>  	if (error)
> -- 
> 2.47.3
> 
> 

Re: [PATCH 10/17] xfs: return the dquot unlocked from xfs_qm_dqget

[Darrick J. Wong]

On Wed, Oct 15, 2025 at 02:17:14PM -0700, Darrick J. Wong wrote:
> On Mon, Oct 13, 2025 at 11:48:11AM +0900, Christoph Hellwig wrote:
> > There is no reason to lock the dquot in xfs_qm_dqget, which just acquires
> > a reference.  Move the locking to the callers, or remove it in cases where
> > the caller instantly unlocks the dquot.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  fs/xfs/scrub/dqiterate.c         | 1 +
> >  fs/xfs/scrub/quotacheck.c        | 1 +
> >  fs/xfs/scrub/quotacheck_repair.c | 1 +
> >  fs/xfs/xfs_dquot.c               | 4 ++--
> >  fs/xfs/xfs_qm.c                  | 4 +---
> >  fs/xfs/xfs_qm_bhv.c              | 1 +
> >  fs/xfs/xfs_qm_syscalls.c         | 2 ++
> >  7 files changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/xfs/scrub/dqiterate.c b/fs/xfs/scrub/dqiterate.c
> > index 20c4daedd48d..6f1185afbf39 100644
> > --- a/fs/xfs/scrub/dqiterate.c
> > +++ b/fs/xfs/scrub/dqiterate.c
> > @@ -205,6 +205,7 @@ xchk_dquot_iter(
> >  	if (error)
> >  		return error;
> >  
> > +	mutex_lock(&dq->q_qlock);
> >  	cursor->id = dq->q_id + 1;
> >  	*dqpp = dq;
> >  	return 1;
> > diff --git a/fs/xfs/scrub/quotacheck.c b/fs/xfs/scrub/quotacheck.c
> > index 180449f654f6..bef63f19cd87 100644
> > --- a/fs/xfs/scrub/quotacheck.c
> > +++ b/fs/xfs/scrub/quotacheck.c
> > @@ -635,6 +635,7 @@ xqcheck_walk_observations(
> >  		if (error)
> >  			return error;
> >  
> > +		mutex_lock(&dq->q_qlock);
> >  		error = xqcheck_compare_dquot(xqc, dqtype, dq);
> >  		mutex_unlock(&dq->q_qlock);
> >  		xfs_qm_dqrele(dq);
> > diff --git a/fs/xfs/scrub/quotacheck_repair.c b/fs/xfs/scrub/quotacheck_repair.c
> > index 67bdc872996a..f7b1add43a2c 100644
> > --- a/fs/xfs/scrub/quotacheck_repair.c
> > +++ b/fs/xfs/scrub/quotacheck_repair.c
> > @@ -181,6 +181,7 @@ xqcheck_commit_dqtype(
> >  		if (error)
> >  			return error;
> >  
> > +		mutex_lock(&dq->q_qlock);
> >  		error = xqcheck_commit_dquot(xqc, dqtype, dq);
> >  		xfs_qm_dqrele(dq);
> 
> Hmm.  No mutex_unlock() ?
> 
> Oh, because @dq gets added to the scrub transaction and the
> commit/cancel and unlocks it, right?
> 
> (Maybe the mutex_lock should go in xqcheck_commit_dquot to avoid the
> unbalanced lock state before after the function call?)

Oh, you /do/ do that a few patches from now.
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> --D
> 
> >  		if (error)
> > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> > index fa493520bea6..98593b380e94 100644
> > --- a/fs/xfs/xfs_dquot.c
> > +++ b/fs/xfs/xfs_dquot.c
> > @@ -896,7 +896,7 @@ xfs_qm_dqget_checks(
> >  
> >  /*
> >   * Given the file system, id, and type (UDQUOT/GDQUOT/PDQUOT), return a
> > - * locked dquot, doing an allocation (if requested) as needed.
> > + * dquot, doing an allocation (if requested) as needed.
> >   */
> >  int
> >  xfs_qm_dqget(
> > @@ -938,7 +938,6 @@ xfs_qm_dqget(
> >  	trace_xfs_dqget_miss(dqp);
> >  found:
> >  	*O_dqpp = dqp;
> > -	mutex_lock(&dqp->q_qlock);
> >  	return 0;
> >  }
> >  
> > @@ -1093,6 +1092,7 @@ xfs_qm_dqget_next(
> >  		else if (error != 0)
> >  			break;
> >  
> > +		mutex_lock(&dqp->q_qlock);
> >  		if (!XFS_IS_DQUOT_UNINITIALIZED(dqp)) {
> >  			*dqpp = dqp;
> >  			return 0;
> > diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> > index 9e173a4b18eb..7fbb89fcdeb9 100644
> > --- a/fs/xfs/xfs_qm.c
> > +++ b/fs/xfs/xfs_qm.c
> > @@ -1268,6 +1268,7 @@ xfs_qm_quotacheck_dqadjust(
> >  		return error;
> >  	}
> >  
> > +	mutex_lock(&dqp->q_qlock);
> >  	error = xfs_dquot_attach_buf(NULL, dqp);
> >  	if (error)
> >  		return error;
> > @@ -1907,7 +1908,6 @@ xfs_qm_vop_dqalloc(
> >  			/*
> >  			 * Get the ilock in the right order.
> >  			 */
> > -			mutex_unlock(&uq->q_qlock);
> >  			lockflags = XFS_ILOCK_SHARED;
> >  			xfs_ilock(ip, lockflags);
> >  		} else {
> > @@ -1929,7 +1929,6 @@ xfs_qm_vop_dqalloc(
> >  				ASSERT(error != -ENOENT);
> >  				goto error_rele;
> >  			}
> > -			mutex_unlock(&gq->q_qlock);
> >  			lockflags = XFS_ILOCK_SHARED;
> >  			xfs_ilock(ip, lockflags);
> >  		} else {
> > @@ -1947,7 +1946,6 @@ xfs_qm_vop_dqalloc(
> >  				ASSERT(error != -ENOENT);
> >  				goto error_rele;
> >  			}
> > -			mutex_unlock(&pq->q_qlock);
> >  			lockflags = XFS_ILOCK_SHARED;
> >  			xfs_ilock(ip, lockflags);
> >  		} else {
> > diff --git a/fs/xfs/xfs_qm_bhv.c b/fs/xfs/xfs_qm_bhv.c
> > index e5a30b12253c..edc0aef3cf34 100644
> > --- a/fs/xfs/xfs_qm_bhv.c
> > +++ b/fs/xfs/xfs_qm_bhv.c
> > @@ -73,6 +73,7 @@ xfs_qm_statvfs(
> >  	struct xfs_dquot	*dqp;
> >  
> >  	if (!xfs_qm_dqget(mp, ip->i_projid, XFS_DQTYPE_PROJ, false, &dqp)) {
> > +		mutex_lock(&dqp->q_qlock);
> >  		xfs_fill_statvfs_from_dquot(statp, ip, dqp);
> >  		mutex_unlock(&dqp->q_qlock);
> >  		xfs_qm_dqrele(dqp);
> > diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
> > index 441f9806cddb..6c8924780d7a 100644
> > --- a/fs/xfs/xfs_qm_syscalls.c
> > +++ b/fs/xfs/xfs_qm_syscalls.c
> > @@ -302,6 +302,7 @@ xfs_qm_scall_setqlim(
> >  		return error;
> >  	}
> >  
> > +	mutex_lock(&dqp->q_qlock);
> >  	defq = xfs_get_defquota(q, xfs_dquot_type(dqp));
> >  	mutex_unlock(&dqp->q_qlock);
> >  
> > @@ -459,6 +460,7 @@ xfs_qm_scall_getquota(
> >  	 * If everything's NULL, this dquot doesn't quite exist as far as
> >  	 * our utility programs are concerned.
> >  	 */
> > +	mutex_lock(&dqp->q_qlock);
> >  	if (XFS_IS_DQUOT_UNINITIALIZED(dqp)) {
> >  		error = -ENOENT;
> >  		goto out_put;
> > -- 
> > 2.47.3
> > 
> > 
> 

Re: [PATCH 12/17] xfs: push q_qlock acquisition from xchk_dquot_iter to the callers.

[Darrick J. Wong]

On Mon, Oct 13, 2025 at 11:48:13AM +0900, Christoph Hellwig wrote:
> There is no good reason to take q_qlock in xchk_dquot_iter, which just
> provides a reference to the dquot.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Yep.
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/scrub/dqiterate.c         | 1 -
>  fs/xfs/scrub/quota.c             | 1 +
>  fs/xfs/scrub/quota_repair.c      | 1 +
>  fs/xfs/scrub/quotacheck.c        | 1 +
>  fs/xfs/scrub/quotacheck_repair.c | 1 +
>  5 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/scrub/dqiterate.c b/fs/xfs/scrub/dqiterate.c
> index 6f1185afbf39..20c4daedd48d 100644
> --- a/fs/xfs/scrub/dqiterate.c
> +++ b/fs/xfs/scrub/dqiterate.c
> @@ -205,7 +205,6 @@ xchk_dquot_iter(
>  	if (error)
>  		return error;
>  
> -	mutex_lock(&dq->q_qlock);
>  	cursor->id = dq->q_id + 1;
>  	*dqpp = dq;
>  	return 1;
> diff --git a/fs/xfs/scrub/quota.c b/fs/xfs/scrub/quota.c
> index cfcd0fb66845..b711d36c5ec9 100644
> --- a/fs/xfs/scrub/quota.c
> +++ b/fs/xfs/scrub/quota.c
> @@ -329,6 +329,7 @@ xchk_quota(
>  	/* Now look for things that the quota verifiers won't complain about. */
>  	xchk_dqiter_init(&cursor, sc, dqtype);
>  	while ((error = xchk_dquot_iter(&cursor, &dq)) == 1) {
> +		mutex_lock(&dq->q_qlock);
>  		error = xchk_quota_item(&sqi, dq);
>  		mutex_unlock(&dq->q_qlock);
>  		xfs_qm_dqrele(dq);
> diff --git a/fs/xfs/scrub/quota_repair.c b/fs/xfs/scrub/quota_repair.c
> index 00a4d2e75797..7897b93c639a 100644
> --- a/fs/xfs/scrub/quota_repair.c
> +++ b/fs/xfs/scrub/quota_repair.c
> @@ -510,6 +510,7 @@ xrep_quota_problems(
>  
>  	xchk_dqiter_init(&cursor, sc, dqtype);
>  	while ((error = xchk_dquot_iter(&cursor, &dq)) == 1) {
> +		mutex_lock(&dq->q_qlock);
>  		error = xrep_quota_item(&rqi, dq);
>  		xfs_qm_dqrele(dq);
>  		if (error)
> diff --git a/fs/xfs/scrub/quotacheck.c b/fs/xfs/scrub/quotacheck.c
> index bef63f19cd87..20220afd90f1 100644
> --- a/fs/xfs/scrub/quotacheck.c
> +++ b/fs/xfs/scrub/quotacheck.c
> @@ -675,6 +675,7 @@ xqcheck_compare_dqtype(
>  	/* Compare what we observed against the actual dquots. */
>  	xchk_dqiter_init(&cursor, sc, dqtype);
>  	while ((error = xchk_dquot_iter(&cursor, &dq)) == 1) {
> +		mutex_lock(&dq->q_qlock);
>  		error = xqcheck_compare_dquot(xqc, dqtype, dq);
>  		mutex_unlock(&dq->q_qlock);
>  		xfs_qm_dqrele(dq);
> diff --git a/fs/xfs/scrub/quotacheck_repair.c b/fs/xfs/scrub/quotacheck_repair.c
> index f7b1add43a2c..de1521739ec9 100644
> --- a/fs/xfs/scrub/quotacheck_repair.c
> +++ b/fs/xfs/scrub/quotacheck_repair.c
> @@ -150,6 +150,7 @@ xqcheck_commit_dqtype(
>  	 */
>  	xchk_dqiter_init(&cursor, sc, dqtype);
>  	while ((error = xchk_dquot_iter(&cursor, &dq)) == 1) {
> +		mutex_lock(&dq->q_qlock);
>  		error = xqcheck_commit_dquot(xqc, dqtype, dq);
>  		xfs_qm_dqrele(dq);
>  		if (error)
> -- 
> 2.47.3
> 
> 

Re: [PATCH 13/17] xfs: move q_qlock locking into xchk_quota_item

[Darrick J. Wong]

On Mon, Oct 13, 2025 at 11:48:14AM +0900, Christoph Hellwig wrote:
> This avoids a pointless roundtrip because ilock needs to be taken first.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Yep!
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/scrub/quota.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/fs/xfs/scrub/quota.c b/fs/xfs/scrub/quota.c
> index b711d36c5ec9..5c5374c44c5a 100644
> --- a/fs/xfs/scrub/quota.c
> +++ b/fs/xfs/scrub/quota.c
> @@ -155,10 +155,7 @@ xchk_quota_item(
>  	 * We want to validate the bmap record for the storage backing this
>  	 * dquot, so we need to lock the dquot and the quota file.  For quota
>  	 * operations, the locking order is first the ILOCK and then the dquot.
> -	 * However, dqiterate gave us a locked dquot, so drop the dquot lock to
> -	 * get the ILOCK.
>  	 */
> -	mutex_unlock(&dq->q_qlock);
>  	xchk_ilock(sc, XFS_ILOCK_SHARED);
>  	mutex_lock(&dq->q_qlock);
>  
> @@ -251,6 +248,7 @@ xchk_quota_item(
>  	xchk_quota_item_timer(sc, offset, &dq->q_rtb);
>  
>  out:
> +	mutex_unlock(&dq->q_qlock);
>  	if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
>  		return -ECANCELED;
>  
> @@ -329,9 +327,7 @@ xchk_quota(
>  	/* Now look for things that the quota verifiers won't complain about. */
>  	xchk_dqiter_init(&cursor, sc, dqtype);
>  	while ((error = xchk_dquot_iter(&cursor, &dq)) == 1) {
> -		mutex_lock(&dq->q_qlock);
>  		error = xchk_quota_item(&sqi, dq);
> -		mutex_unlock(&dq->q_qlock);
>  		xfs_qm_dqrele(dq);
>  		if (error)
>  			break;
> -- 
> 2.47.3
> 
> 

Re: [PATCH 14/17] xfs: move q_qlock locking into xqcheck_compare_dquot

[Darrick J. Wong]

On Mon, Oct 13, 2025 at 11:48:15AM +0900, Christoph Hellwig wrote:
> Instead of having both callers do it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Yay less lock cycling!
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/scrub/quotacheck.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/scrub/quotacheck.c b/fs/xfs/scrub/quotacheck.c
> index 20220afd90f1..cc2b2dfc9261 100644
> --- a/fs/xfs/scrub/quotacheck.c
> +++ b/fs/xfs/scrub/quotacheck.c
> @@ -563,6 +563,7 @@ xqcheck_compare_dquot(
>  		return -ECANCELED;
>  	}
>  
> +	mutex_lock(&dq->q_qlock);
>  	mutex_lock(&xqc->lock);
>  	error = xfarray_load_sparse(counts, dq->q_id, &xcdq);
>  	if (error)
> @@ -590,6 +591,7 @@ xqcheck_compare_dquot(
>  		error = -ECANCELED;
>  	}
>  	mutex_unlock(&xqc->lock);
> +	mutex_unlock(&dq->q_qlock);
>  	if (error)
>  		return error;
>  
> @@ -635,9 +637,7 @@ xqcheck_walk_observations(
>  		if (error)
>  			return error;
>  
> -		mutex_lock(&dq->q_qlock);
>  		error = xqcheck_compare_dquot(xqc, dqtype, dq);
> -		mutex_unlock(&dq->q_qlock);
>  		xfs_qm_dqrele(dq);
>  		if (error)
>  			return error;
> @@ -675,9 +675,7 @@ xqcheck_compare_dqtype(
>  	/* Compare what we observed against the actual dquots. */
>  	xchk_dqiter_init(&cursor, sc, dqtype);
>  	while ((error = xchk_dquot_iter(&cursor, &dq)) == 1) {
> -		mutex_lock(&dq->q_qlock);
>  		error = xqcheck_compare_dquot(xqc, dqtype, dq);
> -		mutex_unlock(&dq->q_qlock);
>  		xfs_qm_dqrele(dq);
>  		if (error)
>  			break;
> -- 
> 2.47.3
> 
> 

Re: [PATCH 15/17] xfs: move q_qlock acquisition into xqcheck_commit_dquot

[Darrick J. Wong]

On Mon, Oct 13, 2025 at 11:48:16AM +0900, Christoph Hellwig wrote:
> This removes a pointless roundtrip because xqcheck_commit_dquot has to
> drop the lock for allocating a transaction right now.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Ohhhh this looks so much better!
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/scrub/quotacheck_repair.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/fs/xfs/scrub/quotacheck_repair.c b/fs/xfs/scrub/quotacheck_repair.c
> index de1521739ec9..942df94e1215 100644
> --- a/fs/xfs/scrub/quotacheck_repair.c
> +++ b/fs/xfs/scrub/quotacheck_repair.c
> @@ -52,13 +52,11 @@ xqcheck_commit_dquot(
>  	bool			dirty = false;
>  	int			error = 0;
>  
> -	/* Unlock the dquot just long enough to allocate a transaction. */
> -	mutex_unlock(&dq->q_qlock);
>  	error = xchk_trans_alloc(xqc->sc, 0);
> -	mutex_lock(&dq->q_qlock);
>  	if (error)
>  		return error;
>  
> +	mutex_lock(&dq->q_qlock);
>  	xfs_trans_dqjoin(xqc->sc->tp, dq);
>  
>  	if (xchk_iscan_aborted(&xqc->iscan)) {
> @@ -150,7 +148,6 @@ xqcheck_commit_dqtype(
>  	 */
>  	xchk_dqiter_init(&cursor, sc, dqtype);
>  	while ((error = xchk_dquot_iter(&cursor, &dq)) == 1) {
> -		mutex_lock(&dq->q_qlock);
>  		error = xqcheck_commit_dquot(xqc, dqtype, dq);
>  		xfs_qm_dqrele(dq);
>  		if (error)
> @@ -182,7 +179,6 @@ xqcheck_commit_dqtype(
>  		if (error)
>  			return error;
>  
> -		mutex_lock(&dq->q_qlock);
>  		error = xqcheck_commit_dquot(xqc, dqtype, dq);
>  		xfs_qm_dqrele(dq);
>  		if (error)
> -- 
> 2.47.3
> 
> 

Re: [PATCH 16/17] xfs: move xfs_dquot_tree calls into xfs_qm_dqget_cache_{lookup,insert}

[Darrick J. Wong]

On Mon, Oct 13, 2025 at 11:48:17AM +0900, Christoph Hellwig wrote:
> These are the low-level functions that needs them, so localize the
> (trivial) calculation of the radix tree root there.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Much cleaner, thanks for doing this.
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_dquot.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index 98593b380e94..29f578d66230 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -801,10 +801,11 @@ xfs_dq_get_next_id(
>  static struct xfs_dquot *
>  xfs_qm_dqget_cache_lookup(
>  	struct xfs_mount	*mp,
> -	struct xfs_quotainfo	*qi,
> -	struct radix_tree_root	*tree,
> -	xfs_dqid_t		id)
> +	xfs_dqid_t		id,
> +	xfs_dqtype_t		type)
>  {
> +	struct xfs_quotainfo	*qi = mp->m_quotainfo;
> +	struct radix_tree_root	*tree = xfs_dquot_tree(qi, type);
>  	struct xfs_dquot	*dqp;
>  
>  restart:
> @@ -843,11 +844,12 @@ xfs_qm_dqget_cache_lookup(
>  static int
>  xfs_qm_dqget_cache_insert(
>  	struct xfs_mount	*mp,
> -	struct xfs_quotainfo	*qi,
> -	struct radix_tree_root	*tree,
>  	xfs_dqid_t		id,
> +	xfs_dqtype_t		type,
>  	struct xfs_dquot	*dqp)
>  {
> +	struct xfs_quotainfo	*qi = mp->m_quotainfo;
> +	struct radix_tree_root	*tree = xfs_dquot_tree(qi, type);
>  	unsigned int		nofs_flags;
>  	int			error;
>  
> @@ -906,8 +908,6 @@ xfs_qm_dqget(
>  	bool			can_alloc,
>  	struct xfs_dquot	**O_dqpp)
>  {
> -	struct xfs_quotainfo	*qi = mp->m_quotainfo;
> -	struct radix_tree_root	*tree = xfs_dquot_tree(qi, type);
>  	struct xfs_dquot	*dqp;
>  	int			error;
>  
> @@ -916,7 +916,7 @@ xfs_qm_dqget(
>  		return error;
>  
>  restart:
> -	dqp = xfs_qm_dqget_cache_lookup(mp, qi, tree, id);
> +	dqp = xfs_qm_dqget_cache_lookup(mp, id, type);
>  	if (dqp)
>  		goto found;
>  
> @@ -924,7 +924,7 @@ xfs_qm_dqget(
>  	if (error)
>  		return error;
>  
> -	error = xfs_qm_dqget_cache_insert(mp, qi, tree, id, dqp);
> +	error = xfs_qm_dqget_cache_insert(mp, id, type, dqp);
>  	if (error) {
>  		/*
>  		 * Duplicate found. Just throw away the new dquot and start
> @@ -994,8 +994,6 @@ xfs_qm_dqget_inode(
>  	struct xfs_dquot	**dqpp)
>  {
>  	struct xfs_mount	*mp = ip->i_mount;
> -	struct xfs_quotainfo	*qi = mp->m_quotainfo;
> -	struct radix_tree_root	*tree = xfs_dquot_tree(qi, type);
>  	struct xfs_dquot	*dqp;
>  	xfs_dqid_t		id;
>  	int			error;
> @@ -1014,7 +1012,7 @@ xfs_qm_dqget_inode(
>  	id = xfs_qm_id_for_quotatype(ip, type);
>  
>  restart:
> -	dqp = xfs_qm_dqget_cache_lookup(mp, qi, tree, id);
> +	dqp = xfs_qm_dqget_cache_lookup(mp, id, type);
>  	if (dqp)
>  		goto found;
>  
> @@ -1050,7 +1048,7 @@ xfs_qm_dqget_inode(
>  		return -ESRCH;
>  	}
>  
> -	error = xfs_qm_dqget_cache_insert(mp, qi, tree, id, dqp);
> +	error = xfs_qm_dqget_cache_insert(mp, id, type, dqp);
>  	if (error) {
>  		/*
>  		 * Duplicate found. Just throw away the new dquot and start
> -- 
> 2.47.3
> 
> 

Re: [PATCH 17/17] xfs: reduce ilock roundtrips in xfs_qm_vop_dqalloc

[Darrick J. Wong]

On Mon, Oct 13, 2025 at 11:48:18AM +0900, Christoph Hellwig wrote:
> xfs_qm_vop_dqalloc only needs the (exclusive) ilock for attaching dquots
> to the inode if not done so yet.  All the other locks don't touch the inode
> and don't need the ilock - the i_rwsem / iolock protects against changes
> to the IDs while we are in a method, and the ilock would not help because
> dropping it for the dqget calls would be racy anyway.

...and I guess we no longer detach dquots from live inodes now, so we
really only need ILOCK_EXCL to prevent multiple threads from trying to
allocate and attach a new xfs_dquot object to the same inode, right?

Changing the i_dquot pointers (aka chown/chproj) is what's coordinated
under the iolock, right?

--D

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_qm.c | 32 +++-----------------------------
>  1 file changed, 3 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index 7fbb89fcdeb9..336de0479022 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -1861,16 +1861,12 @@ xfs_qm_vop_dqalloc(
>  	struct xfs_dquot	*gq = NULL;
>  	struct xfs_dquot	*pq = NULL;
>  	int			error;
> -	uint			lockflags;
>  
>  	if (!XFS_IS_QUOTA_ON(mp))
>  		return 0;
>  
>  	ASSERT(!xfs_is_metadir_inode(ip));
>  
> -	lockflags = XFS_ILOCK_EXCL;
> -	xfs_ilock(ip, lockflags);
> -
>  	if ((flags & XFS_QMOPT_INHERIT) && XFS_INHERIT_GID(ip))
>  		gid = inode->i_gid;
>  
> @@ -1879,37 +1875,22 @@ xfs_qm_vop_dqalloc(
>  	 * if necessary. The dquot(s) will not be locked.
>  	 */
>  	if (XFS_NOT_DQATTACHED(mp, ip)) {
> +		xfs_ilock(ip, XFS_ILOCK_EXCL);
>  		error = xfs_qm_dqattach_locked(ip, true);
> -		if (error) {
> -			xfs_iunlock(ip, lockflags);
> +		xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +		if (error)
>  			return error;
> -		}
>  	}
>  
>  	if ((flags & XFS_QMOPT_UQUOTA) && XFS_IS_UQUOTA_ON(mp)) {
>  		ASSERT(O_udqpp);
>  		if (!uid_eq(inode->i_uid, uid)) {
> -			/*
> -			 * What we need is the dquot that has this uid, and
> -			 * if we send the inode to dqget, the uid of the inode
> -			 * takes priority over what's sent in the uid argument.
> -			 * We must unlock inode here before calling dqget if
> -			 * we're not sending the inode, because otherwise
> -			 * we'll deadlock by doing trans_reserve while
> -			 * holding ilock.
> -			 */
> -			xfs_iunlock(ip, lockflags);
>  			error = xfs_qm_dqget(mp, from_kuid(user_ns, uid),
>  					XFS_DQTYPE_USER, true, &uq);
>  			if (error) {
>  				ASSERT(error != -ENOENT);
>  				return error;
>  			}
> -			/*
> -			 * Get the ilock in the right order.
> -			 */
> -			lockflags = XFS_ILOCK_SHARED;
> -			xfs_ilock(ip, lockflags);
>  		} else {
>  			/*
>  			 * Take an extra reference, because we'll return
> @@ -1922,15 +1903,12 @@ xfs_qm_vop_dqalloc(
>  	if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp)) {
>  		ASSERT(O_gdqpp);
>  		if (!gid_eq(inode->i_gid, gid)) {
> -			xfs_iunlock(ip, lockflags);
>  			error = xfs_qm_dqget(mp, from_kgid(user_ns, gid),
>  					XFS_DQTYPE_GROUP, true, &gq);
>  			if (error) {
>  				ASSERT(error != -ENOENT);
>  				goto error_rele;
>  			}
> -			lockflags = XFS_ILOCK_SHARED;
> -			xfs_ilock(ip, lockflags);
>  		} else {
>  			ASSERT(ip->i_gdquot);
>  			gq = xfs_qm_dqhold(ip->i_gdquot);
> @@ -1939,15 +1917,12 @@ xfs_qm_vop_dqalloc(
>  	if ((flags & XFS_QMOPT_PQUOTA) && XFS_IS_PQUOTA_ON(mp)) {
>  		ASSERT(O_pdqpp);
>  		if (ip->i_projid != prid) {
> -			xfs_iunlock(ip, lockflags);
>  			error = xfs_qm_dqget(mp, prid,
>  					XFS_DQTYPE_PROJ, true, &pq);
>  			if (error) {
>  				ASSERT(error != -ENOENT);
>  				goto error_rele;
>  			}
> -			lockflags = XFS_ILOCK_SHARED;
> -			xfs_ilock(ip, lockflags);
>  		} else {
>  			ASSERT(ip->i_pdquot);
>  			pq = xfs_qm_dqhold(ip->i_pdquot);
> @@ -1955,7 +1930,6 @@ xfs_qm_vop_dqalloc(
>  	}
>  	trace_xfs_dquot_dqalloc(ip);
>  
> -	xfs_iunlock(ip, lockflags);
>  	if (O_udqpp)
>  		*O_udqpp = uq;
>  	else
> -- 
> 2.47.3
> 
> 

Re: [PATCH 10/17] xfs: return the dquot unlocked from xfs_qm_dqget

[Christoph Hellwig]

On Wed, Oct 15, 2025 at 02:18:50PM -0700, Darrick J. Wong wrote:
> > >  		error = xqcheck_commit_dquot(xqc, dqtype, dq);
> > >  		xfs_qm_dqrele(dq);
> > 
> > Hmm.  No mutex_unlock() ?
> > 
> > Oh, because @dq gets added to the scrub transaction and the
> > commit/cancel and unlocks it, right?
> > 
> > (Maybe the mutex_lock should go in xqcheck_commit_dquot to avoid the
> > unbalanced lock state before after the function call?)
> 
> Oh, you /do/ do that a few patches from now.

Yes.  And that later patch is the one sorting out the locking bug
you pointed out the day before as well.

Re: [PATCH 15/17] xfs: move q_qlock acquisition into xqcheck_commit_dquot

[Christoph Hellwig]

On Wed, Oct 15, 2025 at 02:20:32PM -0700, Darrick J. Wong wrote:
> On Mon, Oct 13, 2025 at 11:48:16AM +0900, Christoph Hellwig wrote:
> > This removes a pointless roundtrip because xqcheck_commit_dquot has to
> > drop the lock for allocating a transaction right now.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Ohhhh this looks so much better!
> Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

This gets a bit updated based on moving more changes into it from
the earlier patch, so I won't add the reviewed-by to the new version
for now.

Re: [PATCH 17/17] xfs: reduce ilock roundtrips in xfs_qm_vop_dqalloc

[Christoph Hellwig]

On Wed, Oct 15, 2025 at 02:27:07PM -0700, Darrick J. Wong wrote:
> On Mon, Oct 13, 2025 at 11:48:18AM +0900, Christoph Hellwig wrote:
> > xfs_qm_vop_dqalloc only needs the (exclusive) ilock for attaching dquots
> > to the inode if not done so yet.  All the other locks don't touch the inode
> > and don't need the ilock - the i_rwsem / iolock protects against changes
> > to the IDs while we are in a method, and the ilock would not help because
> > dropping it for the dqget calls would be racy anyway.
> 
> ...and I guess we no longer detach dquots from live inodes now, so we
> really only need ILOCK_EXCL to prevent multiple threads from trying to
> allocate and attach a new xfs_dquot object to the same inode, right?

Yes.

> Changing the i_dquot pointers (aka chown/chproj) is what's coordinated
> under the iolock, right?

Yes.  i_rwsem in the VFS to be specific, but they are the same actual
lock.

Re: [PATCH 17/17] xfs: reduce ilock roundtrips in xfs_qm_vop_dqalloc

[Darrick J. Wong]

On Thu, Oct 16, 2025 at 06:23:48AM +0200, Christoph Hellwig wrote:
> On Wed, Oct 15, 2025 at 02:27:07PM -0700, Darrick J. Wong wrote:
> > On Mon, Oct 13, 2025 at 11:48:18AM +0900, Christoph Hellwig wrote:
> > > xfs_qm_vop_dqalloc only needs the (exclusive) ilock for attaching dquots
> > > to the inode if not done so yet.  All the other locks don't touch the inode
> > > and don't need the ilock - the i_rwsem / iolock protects against changes
> > > to the IDs while we are in a method, and the ilock would not help because
> > > dropping it for the dqget calls would be racy anyway.
> > 
> > ...and I guess we no longer detach dquots from live inodes now, so we
> > really only need ILOCK_EXCL to prevent multiple threads from trying to
> > allocate and attach a new xfs_dquot object to the same inode, right?
> 
> Yes.

I wonder then, if there /were/ two threads racing to dqattach the same
inode, won't the radix_tree_insert return EEXIST, preventing us
from exposing two dquot for the same id because xfs_qm_dqget will just
loop again?

[Though looking at that xfs_qm_dqget -> xfs_qm_dqget_cache_insert ->
radix_tree_insert sequence, it looks like we can also restart
indefinitely on other errors like ENOMEM.]

--D

> > Changing the i_dquot pointers (aka chown/chproj) is what's coordinated
> > under the iolock, right?
> 
> Yes.  i_rwsem in the VFS to be specific, but they are the same actual
> lock.
> 
> 

Re: [PATCH 17/17] xfs: reduce ilock roundtrips in xfs_qm_vop_dqalloc

[Christoph Hellwig]

On Thu, Oct 16, 2025 at 08:59:41AM -0700, Darrick J. Wong wrote:
> > > ...and I guess we no longer detach dquots from live inodes now, so we
> > > really only need ILOCK_EXCL to prevent multiple threads from trying to
> > > allocate and attach a new xfs_dquot object to the same inode, right?
> > 
> > Yes.
> 
> I wonder then, if there /were/ two threads racing to dqattach the same
> inode, won't the radix_tree_insert return EEXIST, preventing us
> from exposing two dquot for the same id because xfs_qm_dqget will just
> loop again?

I think so.

> [Though looking at that xfs_qm_dqget -> xfs_qm_dqget_cache_insert ->
> radix_tree_insert sequence, it looks like we can also restart
> indefinitely on other errors like ENOMEM.]

Yes.  I have patches fixing that as part of moving to xarrays instead
of radix trees.  But I suspect a resizable hash table might actually
be the better fit, so I didn't look into submitting that quite yet.

Re: [PATCH 17/17] xfs: reduce ilock roundtrips in xfs_qm_vop_dqalloc

[Darrick J. Wong]

On Fri, Oct 17, 2025 at 05:50:43AM +0200, Christoph Hellwig wrote:
> On Thu, Oct 16, 2025 at 08:59:41AM -0700, Darrick J. Wong wrote:
> > > > ...and I guess we no longer detach dquots from live inodes now, so we
> > > > really only need ILOCK_EXCL to prevent multiple threads from trying to
> > > > allocate and attach a new xfs_dquot object to the same inode, right?
> > > 
> > > Yes.
> > 
> > I wonder then, if there /were/ two threads racing to dqattach the same
> > inode, won't the radix_tree_insert return EEXIST, preventing us
> > from exposing two dquot for the same id because xfs_qm_dqget will just
> > loop again?
> 
> I think so.
> 
> > [Though looking at that xfs_qm_dqget -> xfs_qm_dqget_cache_insert ->
> > radix_tree_insert sequence, it looks like we can also restart
> > indefinitely on other errors like ENOMEM.]
> 
> Yes.  I have patches fixing that as part of moving to xarrays instead
> of radix trees.  But I suspect a resizable hash table might actually
> be the better fit, so I didn't look into submitting that quite yet.

<nod> Well if either of those are coming next then this looks ok to me.
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D