cleanup quota locking v2

cleanup quota locking v2

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

Changes since v1:
 - use min instead of the incorrect max for s_incoredqs
 - add a patch to fix a pre-existing leak identified by the build bot
 - fix a new locking issue identified by the buildbot
 - reorder patches a bit to avoid inconsistent intermediate states
 - add a patch to not retry non-EEXIST errors from radix_tree_insert

Diffstat:
 libxfs/xfs_quota_defs.h   |    4 -
 scrub/quota.c             |    8 --
 scrub/quota_repair.c      |   18 ++---
 scrub/quotacheck.c        |   11 +--
 scrub/quotacheck_repair.c |   21 +-----
 xfs_dquot.c               |  143 +++++++++++++++++-------------------------
 xfs_dquot.h               |   22 ------
 xfs_dquot_item.c          |    6 -
 xfs_qm.c                  |  154 ++++++++++++----------------------------------
 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, 148 insertions(+), 281 deletions(-)

[PATCH 01/18] xfs: don't leak a locked dquot when xfs_dquot_attach_buf fails

[Christoph Hellwig]

xfs_qm_quotacheck_dqadjust acquired the dquot through xfs_qm_dqget,
which means it owns a reference and holds q_qlock.  Both need to
be dropped on an error exit.

Fixes: ca378189fdfa ("xfs: convert quotacheck to attach dquot buffers")
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_qm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 23ba84ec919a..18a19947bbdb 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -1318,7 +1318,7 @@ xfs_qm_quotacheck_dqadjust(
 
 	error = xfs_dquot_attach_buf(NULL, dqp);
 	if (error)
-		return error;
+		goto out_unlock;
 
 	trace_xfs_dqadjust(dqp);
 
@@ -1348,8 +1348,9 @@ xfs_qm_quotacheck_dqadjust(
 	}
 
 	dqp->q_flags |= XFS_DQFLAG_DIRTY;
+out_unlock:
 	xfs_qm_dqput(dqp);
-	return 0;
+	return error;
 }
 
 /*
-- 
2.47.3

Re: [PATCH 01/18] xfs: don't leak a locked dquot when xfs_dquot_attach_buf fails

[Darrick J. Wong]

On Mon, Nov 10, 2025 at 02:22:53PM +0100, Christoph Hellwig wrote:
> xfs_qm_quotacheck_dqadjust acquired the dquot through xfs_qm_dqget,
> which means it owns a reference and holds q_qlock.  Both need to
> be dropped on an error exit.
> 
> Fixes: ca378189fdfa ("xfs: convert quotacheck to attach dquot buffers")
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Yep, that was a bug, thanks for fixing that.

Cc: <stable@vger.kernel.org> # v6.13
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_qm.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index 23ba84ec919a..18a19947bbdb 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -1318,7 +1318,7 @@ xfs_qm_quotacheck_dqadjust(
>  
>  	error = xfs_dquot_attach_buf(NULL, dqp);
>  	if (error)
> -		return error;
> +		goto out_unlock;
>  
>  	trace_xfs_dqadjust(dqp);
>  
> @@ -1348,8 +1348,9 @@ xfs_qm_quotacheck_dqadjust(
>  	}
>  
>  	dqp->q_flags |= XFS_DQFLAG_DIRTY;
> +out_unlock:
>  	xfs_qm_dqput(dqp);
> -	return 0;
> +	return error;
>  }
>  
>  /*
> -- 
> 2.47.3
> 
> 

[PATCH 02/18] 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..94fbe3d99ec7 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 = min_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

Re: [PATCH 02/18] xfs: make qi_dquots a 64-bit value

[Darrick J. Wong]

On Mon, Nov 10, 2025 at 02:22:54PM +0100, 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>

Heh, what a bug. :)
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  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..94fbe3d99ec7 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 = min_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 03/18] xfs: don't treat all radix_tree_insert errors as -EEXIST

[Christoph Hellwig]

Return other errors to the caller instead.  Note that there really
shouldn't be any other errors because the entry is preallocated, but
if there were, we'd better return them instead of retrying forever.

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

diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 0bd8022e47b4..79e14ee1d7a0 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -860,7 +860,6 @@ xfs_qm_dqget_cache_insert(
 	mutex_lock(&qi->qi_tree_lock);
 	error = radix_tree_insert(tree, id, dqp);
 	if (unlikely(error)) {
-		/* Duplicate found!  Caller must try again. */
 		trace_xfs_dqget_dup(dqp);
 		goto out_unlock;
 	}
@@ -935,13 +934,16 @@ xfs_qm_dqget(
 
 	error = xfs_qm_dqget_cache_insert(mp, qi, tree, id, dqp);
 	if (error) {
-		/*
-		 * Duplicate found. Just throw away the new dquot and start
-		 * over.
-		 */
 		xfs_qm_dqdestroy(dqp);
-		XFS_STATS_INC(mp, xs_qm_dquot_dups);
-		goto restart;
+		if (error == -EEXIST) {
+			/*
+			 * Duplicate found. Just throw away the new dquot and
+			 * start over.
+			 */
+			XFS_STATS_INC(mp, xs_qm_dquot_dups);
+			goto restart;
+		}
+		return error;
 	}
 
 	trace_xfs_dqget_miss(dqp);
@@ -1060,13 +1062,16 @@ xfs_qm_dqget_inode(
 
 	error = xfs_qm_dqget_cache_insert(mp, qi, tree, id, dqp);
 	if (error) {
-		/*
-		 * Duplicate found. Just throw away the new dquot and start
-		 * over.
-		 */
 		xfs_qm_dqdestroy(dqp);
-		XFS_STATS_INC(mp, xs_qm_dquot_dups);
-		goto restart;
+		if (error == -EEXIST) {
+			/*
+			 * Duplicate found. Just throw away the new dquot and
+			 * start over.
+			 */
+			XFS_STATS_INC(mp, xs_qm_dquot_dups);
+			goto restart;
+		}
+		return error;
 	}
 
 dqret:
-- 
2.47.3

Re: [PATCH 03/18] xfs: don't treat all radix_tree_insert errors as -EEXIST

[Darrick J. Wong]

On Mon, Nov 10, 2025 at 02:22:55PM +0100, Christoph Hellwig wrote:
> Return other errors to the caller instead.  Note that there really
> shouldn't be any other errors because the entry is preallocated, but
> if there were, we'd better return them instead of retrying forever.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Good defensive strategy ;)
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_dquot.c | 31 ++++++++++++++++++-------------
>  1 file changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index 0bd8022e47b4..79e14ee1d7a0 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -860,7 +860,6 @@ xfs_qm_dqget_cache_insert(
>  	mutex_lock(&qi->qi_tree_lock);
>  	error = radix_tree_insert(tree, id, dqp);
>  	if (unlikely(error)) {
> -		/* Duplicate found!  Caller must try again. */
>  		trace_xfs_dqget_dup(dqp);
>  		goto out_unlock;
>  	}
> @@ -935,13 +934,16 @@ xfs_qm_dqget(
>  
>  	error = xfs_qm_dqget_cache_insert(mp, qi, tree, id, dqp);
>  	if (error) {
> -		/*
> -		 * Duplicate found. Just throw away the new dquot and start
> -		 * over.
> -		 */
>  		xfs_qm_dqdestroy(dqp);
> -		XFS_STATS_INC(mp, xs_qm_dquot_dups);
> -		goto restart;
> +		if (error == -EEXIST) {
> +			/*
> +			 * Duplicate found. Just throw away the new dquot and
> +			 * start over.
> +			 */
> +			XFS_STATS_INC(mp, xs_qm_dquot_dups);
> +			goto restart;
> +		}
> +		return error;
>  	}
>  
>  	trace_xfs_dqget_miss(dqp);
> @@ -1060,13 +1062,16 @@ xfs_qm_dqget_inode(
>  
>  	error = xfs_qm_dqget_cache_insert(mp, qi, tree, id, dqp);
>  	if (error) {
> -		/*
> -		 * Duplicate found. Just throw away the new dquot and start
> -		 * over.
> -		 */
>  		xfs_qm_dqdestroy(dqp);
> -		XFS_STATS_INC(mp, xs_qm_dquot_dups);
> -		goto restart;
> +		if (error == -EEXIST) {
> +			/*
> +			 * Duplicate found. Just throw away the new dquot and
> +			 * start over.
> +			 */
> +			XFS_STATS_INC(mp, xs_qm_dquot_dups);
> +			goto restart;
> +		}
> +		return error;
>  	}
>  
>  dqret:
> -- 
> 2.47.3
> 
> 

[PATCH 04/18] 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>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
---
 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 79e14ee1d7a0..c2326cee7fae 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);
@@ -865,7 +865,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++;
 
@@ -1051,7 +1051,7 @@ xfs_qm_dqget_inode(
 		if (dqp1) {
 			xfs_qm_dqdestroy(dqp);
 			dqp = dqp1;
-			xfs_dqlock(dqp);
+			mutex_lock(&dqp->q_qlock);
 			goto dqret;
 		}
 	} else {
@@ -1136,7 +1136,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);
 }
 
 /*
@@ -1152,7 +1152,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 18a19947bbdb..3e88bea9a465 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);
@@ -1467,7 +1467,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))
@@ -1489,7 +1489,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;
 }
 
@@ -1952,7 +1952,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 {
@@ -1974,7 +1974,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 {
@@ -1992,7 +1992,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 {
@@ -2079,7 +2079,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;
@@ -2087,7 +2087,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 05/18] 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 partially 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>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
---
 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 c2326cee7fae..34c325524ab9 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;
 }
 
@@ -866,7 +863,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:
@@ -1124,18 +1121,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 3e88bea9a465..f98f9fdac0b5 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);
@@ -1467,9 +1464,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;
 
@@ -1489,7 +1487,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/18] 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/quota_repair.c      |  3 ++-
 fs/xfs/scrub/quotacheck.c        |  6 +++--
 fs/xfs/scrub/quotacheck_repair.c |  6 +++--
 fs/xfs/xfs_dquot.c               | 45 +++++++-------------------------
 fs/xfs/xfs_dquot.h               |  1 -
 fs/xfs/xfs_qm.c                  |  6 +++--
 fs/xfs/xfs_qm_bhv.c              |  3 ++-
 fs/xfs/xfs_qm_syscalls.c         |  6 +++--
 fs/xfs/xfs_trace.h               |  3 +--
 10 files changed, 32 insertions(+), 50 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/quota_repair.c b/fs/xfs/scrub/quota_repair.c
index 8c89c6cc2950..d4ce9e56d3ef 100644
--- a/fs/xfs/scrub/quota_repair.c
+++ b/fs/xfs/scrub/quota_repair.c
@@ -513,7 +513,8 @@ 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);
+		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/scrub/quotacheck_repair.c b/fs/xfs/scrub/quotacheck_repair.c
index 415314911499..11153e24b565 100644
--- a/fs/xfs/scrub/quotacheck_repair.c
+++ b/fs/xfs/scrub/quotacheck_repair.c
@@ -156,7 +156,8 @@ 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);
+		mutex_unlock(&dq->q_qlock);
+		xfs_qm_dqrele(dq);
 		if (error)
 			break;
 	}
@@ -187,7 +188,8 @@ xqcheck_commit_dqtype(
 			return error;
 
 		error = xqcheck_commit_dquot(xqc, dqtype, dq);
-		xfs_qm_dqput(dq);
+		mutex_unlock(&dq->q_qlock);
+		xfs_qm_dqrele(dq);
 		if (error)
 			return error;
 
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 34c325524ab9..52c521a1402d 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -1105,62 +1105,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 f98f9fdac0b5..5e6aefb17f19 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -1346,7 +1346,8 @@ xfs_qm_quotacheck_dqadjust(
 
 	dqp->q_flags |= XFS_DQFLAG_DIRTY;
 out_unlock:
-	xfs_qm_dqput(dqp);
+	mutex_unlock(&dqp->q_qlock);
+	xfs_qm_dqrele(dqp);
 	return error;
 }
 
@@ -1487,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 06/18] xfs: remove xfs_qm_dqput and optimize dropping dquot references

[Darrick J. Wong]

On Mon, Nov 10, 2025 at 02:22:58PM +0100, 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>

I like this cleanup -- locking and refcounting are less entangled and
this reduces qlock cycling in the fsck code.

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

--D

> ---
>  fs/xfs/scrub/quota.c             |  3 ++-
>  fs/xfs/scrub/quota_repair.c      |  3 ++-
>  fs/xfs/scrub/quotacheck.c        |  6 +++--
>  fs/xfs/scrub/quotacheck_repair.c |  6 +++--
>  fs/xfs/xfs_dquot.c               | 45 +++++++-------------------------
>  fs/xfs/xfs_dquot.h               |  1 -
>  fs/xfs/xfs_qm.c                  |  6 +++--
>  fs/xfs/xfs_qm_bhv.c              |  3 ++-
>  fs/xfs/xfs_qm_syscalls.c         |  6 +++--
>  fs/xfs/xfs_trace.h               |  3 +--
>  10 files changed, 32 insertions(+), 50 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/quota_repair.c b/fs/xfs/scrub/quota_repair.c
> index 8c89c6cc2950..d4ce9e56d3ef 100644
> --- a/fs/xfs/scrub/quota_repair.c
> +++ b/fs/xfs/scrub/quota_repair.c
> @@ -513,7 +513,8 @@ 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);
> +		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/scrub/quotacheck_repair.c b/fs/xfs/scrub/quotacheck_repair.c
> index 415314911499..11153e24b565 100644
> --- a/fs/xfs/scrub/quotacheck_repair.c
> +++ b/fs/xfs/scrub/quotacheck_repair.c
> @@ -156,7 +156,8 @@ 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);
> +		mutex_unlock(&dq->q_qlock);
> +		xfs_qm_dqrele(dq);
>  		if (error)
>  			break;
>  	}
> @@ -187,7 +188,8 @@ xqcheck_commit_dqtype(
>  			return error;
>  
>  		error = xqcheck_commit_dquot(xqc, dqtype, dq);
> -		xfs_qm_dqput(dq);
> +		mutex_unlock(&dq->q_qlock);
> +		xfs_qm_dqrele(dq);
>  		if (error)
>  			return error;
>  
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index 34c325524ab9..52c521a1402d 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -1105,62 +1105,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 f98f9fdac0b5..5e6aefb17f19 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -1346,7 +1346,8 @@ xfs_qm_quotacheck_dqadjust(
>  
>  	dqp->q_flags |= XFS_DQFLAG_DIRTY;
>  out_unlock:
> -	xfs_qm_dqput(dqp);
> +	mutex_unlock(&dqp->q_qlock);
> +	xfs_qm_dqrele(dqp);
>  	return error;
>  }
>  
> @@ -1487,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/18] 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>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
---
 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 52c521a1402d..8b4434e6df09 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
@@ -861,8 +859,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++;
 
@@ -920,10 +916,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)
@@ -944,7 +938,9 @@ xfs_qm_dqget(
 	}
 
 	trace_xfs_dqget_miss(dqp);
+found:
 	*O_dqpp = dqp;
+	mutex_lock(&dqp->q_qlock);
 	return 0;
 }
 
@@ -1019,10 +1015,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
@@ -1048,7 +1042,6 @@ xfs_qm_dqget_inode(
 		if (dqp1) {
 			xfs_qm_dqdestroy(dqp);
 			dqp = dqp1;
-			mutex_lock(&dqp->q_qlock);
 			goto dqret;
 		}
 	} else {
@@ -1074,7 +1067,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/18] 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>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
---
 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 5e6aefb17f19..b571eff51694 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/18] 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>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
---
 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 8b4434e6df09..862fec529512 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -994,7 +994,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;
@@ -1003,6 +1003,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;
@@ -1068,8 +1071,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 b571eff51694..f3f7947bc4ed 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/18] 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>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
---
 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 11153e24b565..3b23219d43ed 100644
--- a/fs/xfs/scrub/quotacheck_repair.c
+++ b/fs/xfs/scrub/quotacheck_repair.c
@@ -187,6 +187,7 @@ xqcheck_commit_dqtype(
 		if (error)
 			return error;
 
+		mutex_lock(&dq->q_qlock);
 		error = xqcheck_commit_dquot(xqc, dqtype, dq);
 		mutex_unlock(&dq->q_qlock);
 		xfs_qm_dqrele(dq);
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 862fec529512..1c9c17892874 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -895,7 +895,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(
@@ -940,7 +940,6 @@ xfs_qm_dqget(
 	trace_xfs_dqget_miss(dqp);
 found:
 	*O_dqpp = dqp;
-	mutex_lock(&dqp->q_qlock);
 	return 0;
 }
 
@@ -1098,6 +1097,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 f3f7947bc4ed..a81b8b7a4e4f 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)
 		goto out_unlock;
@@ -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/18] 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>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
---
 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/18] 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>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
---
 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 d4ce9e56d3ef..dae4889bdc84 100644
--- a/fs/xfs/scrub/quota_repair.c
+++ b/fs/xfs/scrub/quota_repair.c
@@ -512,6 +512,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);
 		mutex_unlock(&dq->q_qlock);
 		xfs_qm_dqrele(dq);
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 3b23219d43ed..3013211fa6c1 100644
--- a/fs/xfs/scrub/quotacheck_repair.c
+++ b/fs/xfs/scrub/quotacheck_repair.c
@@ -155,6 +155,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);
 		mutex_unlock(&dq->q_qlock);
 		xfs_qm_dqrele(dq);
-- 
2.47.3

[PATCH 13/18] 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>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
---
 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/18] 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 | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/fs/xfs/scrub/quotacheck.c b/fs/xfs/scrub/quotacheck.c
index 20220afd90f1..d412a8359784 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)
@@ -589,7 +590,9 @@ xqcheck_compare_dquot(
 		xchk_set_incomplete(xqc->sc);
 		error = -ECANCELED;
 	}
+out_unlock:
 	mutex_unlock(&xqc->lock);
+	mutex_unlock(&dq->q_qlock);
 	if (error)
 		return error;
 
@@ -597,10 +600,6 @@ xqcheck_compare_dquot(
 		return -ECANCELED;
 
 	return 0;
-
-out_unlock:
-	mutex_unlock(&xqc->lock);
-	return error;
 }
 
 /*
@@ -635,9 +634,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 +672,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 14/18] xfs: move q_qlock locking into xqcheck_compare_dquot

[Darrick J. Wong]

On Mon, Nov 10, 2025 at 02:23:06PM +0100, Christoph Hellwig wrote:
> Instead of having both callers do it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Yeah, that makes sense now
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/scrub/quotacheck.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/xfs/scrub/quotacheck.c b/fs/xfs/scrub/quotacheck.c
> index 20220afd90f1..d412a8359784 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)
> @@ -589,7 +590,9 @@ xqcheck_compare_dquot(
>  		xchk_set_incomplete(xqc->sc);
>  		error = -ECANCELED;
>  	}
> +out_unlock:
>  	mutex_unlock(&xqc->lock);
> +	mutex_unlock(&dq->q_qlock);
>  	if (error)
>  		return error;
>  
> @@ -597,10 +600,6 @@ xqcheck_compare_dquot(
>  		return -ECANCELED;
>  
>  	return 0;
> -
> -out_unlock:
> -	mutex_unlock(&xqc->lock);
> -	return error;
>  }
>  
>  /*
> @@ -635,9 +634,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 +672,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/18] xfs: move quota locking into xqcheck_commit_dquot

[Christoph Hellwig]

Drop two redundant lock roundtrips by not requiring q_lock to be held on
entry and return.

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

diff --git a/fs/xfs/scrub/quotacheck_repair.c b/fs/xfs/scrub/quotacheck_repair.c
index 3013211fa6c1..51be8d8d261b 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)) {
@@ -115,23 +113,12 @@ xqcheck_commit_dquot(
 	if (dq->q_id)
 		xfs_qm_adjust_dqtimers(dq);
 	xfs_trans_log_dquot(xqc->sc->tp, dq);
-
-	/*
-	 * Transaction commit unlocks the dquot, so we must re-lock it so that
-	 * 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;
 }
 
@@ -155,9 +142,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);
-		mutex_unlock(&dq->q_qlock);
 		xfs_qm_dqrele(dq);
 		if (error)
 			break;
@@ -188,9 +173,7 @@ xqcheck_commit_dqtype(
 		if (error)
 			return error;
 
-		mutex_lock(&dq->q_qlock);
 		error = xqcheck_commit_dquot(xqc, dqtype, dq);
-		mutex_unlock(&dq->q_qlock);
 		xfs_qm_dqrele(dq);
 		if (error)
 			return error;
-- 
2.47.3

Re: [PATCH 15/18] xfs: move quota locking into xqcheck_commit_dquot

[Darrick J. Wong]

On Mon, Nov 10, 2025 at 02:23:07PM +0100, Christoph Hellwig wrote:
> Drop two redundant lock roundtrips by not requiring q_lock to be held on
> entry and return.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

--D

> ---
>  fs/xfs/scrub/quotacheck_repair.c | 21 ++-------------------
>  1 file changed, 2 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/xfs/scrub/quotacheck_repair.c b/fs/xfs/scrub/quotacheck_repair.c
> index 3013211fa6c1..51be8d8d261b 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)) {
> @@ -115,23 +113,12 @@ xqcheck_commit_dquot(
>  	if (dq->q_id)
>  		xfs_qm_adjust_dqtimers(dq);
>  	xfs_trans_log_dquot(xqc->sc->tp, dq);
> -
> -	/*
> -	 * Transaction commit unlocks the dquot, so we must re-lock it so that
> -	 * 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;
>  }
>  
> @@ -155,9 +142,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);
> -		mutex_unlock(&dq->q_qlock);
>  		xfs_qm_dqrele(dq);
>  		if (error)
>  			break;
> @@ -188,9 +173,7 @@ xqcheck_commit_dqtype(
>  		if (error)
>  			return error;
>  
> -		mutex_lock(&dq->q_qlock);
>  		error = xqcheck_commit_dquot(xqc, dqtype, dq);
> -		mutex_unlock(&dq->q_qlock);
>  		xfs_qm_dqrele(dq);
>  		if (error)
>  			return error;
> -- 
> 2.47.3
> 
> 

[PATCH 16/18] xfs: move quota locking into xrep_quota_item

[Christoph Hellwig]

Drop two redundant lock roundtrips by not requiring q_lock to be held on
entry and return.

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

diff --git a/fs/xfs/scrub/quota_repair.c b/fs/xfs/scrub/quota_repair.c
index dae4889bdc84..b1d661aa5f06 100644
--- a/fs/xfs/scrub/quota_repair.c
+++ b/fs/xfs/scrub/quota_repair.c
@@ -184,17 +184,13 @@ xrep_quota_item(
 	/*
 	 * We might need to fix holes in the bmap record for the storage
 	 * backing this dquot, so we need to lock the dquot and the quota file.
-	 * dqiterate gave us a locked dquot, so drop the dquot lock to get the
-	 * ILOCK_EXCL.
 	 */
-	mutex_unlock(&dq->q_qlock);
 	xchk_ilock(sc, XFS_ILOCK_EXCL);
 	mutex_lock(&dq->q_qlock);
-
 	error = xrep_quota_item_bmap(sc, dq, &dirty);
 	xchk_iunlock(sc, XFS_ILOCK_EXCL);
 	if (error)
-		return error;
+		goto out_unlock_dquot;
 
 	/* Check the limits. */
 	if (dq->q_blk.softlimit > dq->q_blk.hardlimit) {
@@ -246,7 +242,7 @@ xrep_quota_item(
 	xrep_quota_item_timer(sc, &dq->q_rtb, &dirty);
 
 	if (!dirty)
-		return 0;
+		goto out_unlock_dquot;
 
 	trace_xrep_dquot_item(sc->mp, dq->q_type, dq->q_id);
 
@@ -257,8 +253,10 @@ 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 xfs_trans_roll(&sc->tp);
+
+out_unlock_dquot:
+	mutex_unlock(&dq->q_qlock);
 	return error;
 }
 
@@ -512,9 +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);
-		mutex_unlock(&dq->q_qlock);
 		xfs_qm_dqrele(dq);
 		if (error)
 			break;
-- 
2.47.3

Re: [PATCH 16/18] xfs: move quota locking into xrep_quota_item

[Darrick J. Wong]

On Mon, Nov 10, 2025 at 02:23:08PM +0100, Christoph Hellwig wrote:
> Drop two redundant lock roundtrips by not requiring q_lock to be held on
> entry and return.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

--D

> ---
>  fs/xfs/scrub/quota_repair.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/scrub/quota_repair.c b/fs/xfs/scrub/quota_repair.c
> index dae4889bdc84..b1d661aa5f06 100644
> --- a/fs/xfs/scrub/quota_repair.c
> +++ b/fs/xfs/scrub/quota_repair.c
> @@ -184,17 +184,13 @@ xrep_quota_item(
>  	/*
>  	 * We might need to fix holes in the bmap record for the storage
>  	 * backing this dquot, so we need to lock the dquot and the quota file.
> -	 * dqiterate gave us a locked dquot, so drop the dquot lock to get the
> -	 * ILOCK_EXCL.
>  	 */
> -	mutex_unlock(&dq->q_qlock);
>  	xchk_ilock(sc, XFS_ILOCK_EXCL);
>  	mutex_lock(&dq->q_qlock);
> -
>  	error = xrep_quota_item_bmap(sc, dq, &dirty);
>  	xchk_iunlock(sc, XFS_ILOCK_EXCL);
>  	if (error)
> -		return error;
> +		goto out_unlock_dquot;
>  
>  	/* Check the limits. */
>  	if (dq->q_blk.softlimit > dq->q_blk.hardlimit) {
> @@ -246,7 +242,7 @@ xrep_quota_item(
>  	xrep_quota_item_timer(sc, &dq->q_rtb, &dirty);
>  
>  	if (!dirty)
> -		return 0;
> +		goto out_unlock_dquot;
>  
>  	trace_xrep_dquot_item(sc->mp, dq->q_type, dq->q_id);
>  
> @@ -257,8 +253,10 @@ 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 xfs_trans_roll(&sc->tp);
> +
> +out_unlock_dquot:
> +	mutex_unlock(&dq->q_qlock);
>  	return error;
>  }
>  
> @@ -512,9 +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);
> -		mutex_unlock(&dq->q_qlock);
>  		xfs_qm_dqrele(dq);
>  		if (error)
>  			break;
> -- 
> 2.47.3
> 
> 

[PATCH 17/18] 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>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
---
 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 1c9c17892874..612ca682a513 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;
 
@@ -905,8 +907,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;
 
@@ -915,7 +915,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;
 
@@ -923,7 +923,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) {
 		xfs_qm_dqdestroy(dqp);
 		if (error == -EEXIST) {
@@ -996,8 +996,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;
@@ -1016,7 +1014,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;
 
@@ -1052,7 +1050,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) {
 		xfs_qm_dqdestroy(dqp);
 		if (error == -EEXIST) {
-- 
2.47.3

[PATCH 18/18] 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 a81b8b7a4e4f..95be67ac6eb4 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 18/18] xfs: reduce ilock roundtrips in xfs_qm_vop_dqalloc

[Darrick J. Wong]

On Mon, Nov 10, 2025 at 02:23:10PM +0100, 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.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Hrmm.  Code-wise, this is reducing the ILOCK scope to the
xfs_qm_dqattach_locked call, so it looks correct to me.  So:

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

Zooming out a bit: As far as taking the ILOCK is concerned -- I think we
still need to do that to prevent [ugp]id changes to the inode while
we're doing a potentially expensive ondisk dquot allocation, right?

--D

> ---
>  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 a81b8b7a4e4f..95be67ac6eb4 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 18/18] xfs: reduce ilock roundtrips in xfs_qm_vop_dqalloc

[Christoph Hellwig]

On Mon, Nov 10, 2025 at 10:19:30AM -0800, Darrick J. Wong wrote:
> Zooming out a bit: As far as taking the ILOCK is concerned -- I think we
> still need to do that to prevent [ugp]id changes to the inode while
> we're doing a potentially expensive ondisk dquot allocation, right?

That's my assumption, and it was how things historically worked.  That
being said Linux calls all VFЅ operations that could change them under
i_rwsem aka the IOLOCK, so maybe we don't even need it at all, but that
would require a more indepth audit.

Re: cleanup quota locking v2

[Carlos Maiolino]

On Mon, 10 Nov 2025 14:22:52 +0100, Christoph Hellwig wrote:
> 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.
> 
> Changes since v1:
>  - use min instead of the incorrect max for s_incoredqs
>  - add a patch to fix a pre-existing leak identified by the build bot
>  - fix a new locking issue identified by the buildbot
>  - reorder patches a bit to avoid inconsistent intermediate states
>  - add a patch to not retry non-EEXIST errors from radix_tree_insert
> 
> [...]

Applied to for-next, thanks!

[01/18] xfs: don't leak a locked dquot when xfs_dquot_attach_buf fails
        commit: 204c8f77e8d4a3006f8abe40331f221a597ce608
[02/18] xfs: make qi_dquots a 64-bit value
        commit: 005d5ae0c585e11d31df1e721c04f113a8281443
[03/18] xfs: don't treat all radix_tree_insert errors as -EEXIST
        commit: 36cebabde7866c30b71ecd074e3773dbd768a1d9
[04/18] xfs: remove xfs_dqunlock and friends
        commit: 6129b088e1f10938b86f44948ad698b39dd19faa
[05/18] xfs: use a lockref for the xfs_dquot reference count
        commit: 0c5e80bd579f7bec3704bad6c1f72b13b0d73b53
[06/18] xfs: remove xfs_qm_dqput and optimize dropping dquot references
        commit: 6b6e6e75211687c61c5660f65b4155cd0eb7e187
[07/18] xfs: consolidate q_qlock locking in xfs_qm_dqget and xfs_qm_dqget_inode
        commit: 0494f04643de72e13acd556e402cc4edc6169950
[08/18] xfs: xfs_qm_dqattach_one is never called with a non-NULL *IO_idqpp
        commit: d0f93c0d7c9dc8f7fdbd1ce3f5d3bfd8e109da65
[09/18] xfs: fold xfs_qm_dqattach_one into xfs_qm_dqget_inode
        commit: bf5066e169eed0b7b705e3261a05db80f1b8358e
[10/18] xfs: return the dquot unlocked from xfs_qm_dqget
        commit: 55c1bc3eb9d0f39ea4c078b339a6228f5f62584b
[11/18] xfs: remove q_qlock locking in xfs_qm_scall_setqlim
        commit: e85e74e4c9a64993ec5f296719705a32feca93c9
[12/18] xfs: push q_qlock acquisition from xchk_dquot_iter to the callers.
        commit: a536bf9bec6ac461ec48bc8627545d56e4e71e9c
[13/18] xfs: move q_qlock locking into xchk_quota_item
        commit: bfca8760f47ecda61441950babbea6f79a51b377
[14/18] xfs: move q_qlock locking into xqcheck_compare_dquot
        commit: 7dd30acb4b3724ec4ecad1a6e2e19a33c0f0ace4
[15/18] xfs: move quota locking into xqcheck_commit_dquot
        commit: a2ebb21f8ae1a8cc9414677ac7ddbf5c7cc6f48d
[16/18] xfs: move quota locking into xrep_quota_item
        commit: b6d2ab27cc84b19afdf72eac1361fb343c4e0186
[17/18] xfs: move xfs_dquot_tree calls into xfs_qm_dqget_cache_{lookup,insert}
        commit: 13d3c1a045628e8453c31bd49578053c093e7a02
[18/18] xfs: reduce ilock roundtrips in xfs_qm_vop_dqalloc
        commit: 6a7bb6ccd00580461f01e86f592c7d8c7bb54793

Best regards,
-- 
Carlos Maiolino <cem@kernel.org>