Re: [PATCH 5/5] xfs: repair quotas

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

On Wed, Nov 29, 2023 at 09:33:41PM -0800, Christoph Hellwig wrote:
> > @@ -328,7 +328,6 @@ xchk_quota(
> >  		if (error)
> >  			break;
> >  	}
> > -	xchk_ilock(sc, XFS_ILOCK_EXCL);
> >  	if (error == -ECANCELED)
> >  		error = 0;
> >  	if (!xchk_fblock_process_error(sc, XFS_DATA_FORK,
> 
> What is the replacement for this lock?  The call in xrep_quota_item?

The replacement is the conditional re-lock at the start of xrep_quota.
I could have left this alone, though for the scrub-only case it reduces
lock cycling by one.

> I'm a little confused on how locking works - about all flags in
> sc->ilock_flags are released, and here we used just lock the
> exclusive ilock directly without tracking it.

Fixing quota files is a bit of a locking mess because dqget needs to
take the ILOCK to read the data fork if the dquot isn't already in
memory.  As a result, the scrub functions have to take the lock to
inspect the data fork for mapping problems only to drop it before
iterating the incore dquots.

For each dquot, we then have to take the ILOCK *again* to check that its
q_fileoffset and q_blkno fields actually match the bmap.

Similarly, repair has to retake the lock to fix any problems that were
found with the mapping before dropping once again to walk the incore
dquots.  Then we cycle ILOCK for every dquot to fix the mapping.

Not sure what you meant about "we used just lock the exclusive lock
directly without tracking it" -- both files call xchk_{ilock,iunlock}.
The telemetry data I've collected shows that quota file checking is
sorta slow, so perhaps it would be justified to create a special
no-alloc dqget function where the caller is allowed to pre-acquire the
ILOCK.

--D