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

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

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

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

--D

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