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

["Darrick J. Wong" <djwong@kernel.org>]

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