On Sun, Dec 03, 2023 at 08:48:20PM -0800, Christoph Hellwig wrote:
> On Thu, Nov 30, 2023 at 02:10:15PM -0800, Darrick J. Wong wrote:
> > 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.
>
> Hmm. but not all scrub calls do even end up in the repair callbacks,
> do they? Ok, I guess the xchk_iunlock call in xchk_teardown would have
> just released it a bit later and we skip the cycle. Would have been
> a lot easier to understand if this was in a well-explained
> self-contained patch..
How about I not remove the xchk_ilock call, then? Repair is already
smart enough to take the lock if it doesn't have it, so it's not
strictly necessary for correct operation.
> > 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.
>
> My confusions was more about checking/using sc->ilock_flags in the
> callers, while it is maintained by the locking helpers. Probably not
> *THAT* unusual, but I might have simply been too tired to understand it.
Ah, got it. I'll ponder a no-alloc dqget in the meantime.
--D
