On Sun, Jan 24, 2021 at 09:39:53AM +0000, Christoph Hellwig wrote:
> > + /* We only allow one retry for EDQUOT/ENOSPC. */
> > + if (*retry || (error != -EDQUOT && error != -ENOSPC)) {
> > + *retry = false;
> > + return error;
> > + }
>
> > + /* Release resources, prepare for scan. */
> > + xfs_trans_cancel(*tpp);
> > + *tpp = NULL;
> > + xfs_iunlock(ip, XFS_ILOCK_EXCL);
> > +
> > + /* Try to free some quota for this file's dquots. */
> > + *retry = true;
> > + xfs_blockgc_free_quota(ip, 0);
> > + return 0;
>
> I till have grave reservations about this calling conventions. And if
> you just remove the unlock and th call to xfs_blockgc_free_quota here
> we don't equire a whole lot of boilerplate code in the callers while
> making the code possible to reason about for a mere human.
>
I agree that the retry pattern is rather odd. I'm curious, is there a
specific reason this scanning task has to execute outside of transaction
context in the first place? Assuming it does because the underlying work
may involve more transactions or whatnot, I'm wondering if this logic
could be buried further down in the transaction allocation path.
For example, if we passed the quota reservation and inode down into a
new variant of xfs_trans_alloc(), it could acquire the ilock and attempt
the quota reservation as a final step (to avoid adding an extra
unconditional ilock cycle). If quota res fails, iunlock and release the
log res internally and perform the scan. From there, perhaps we could
retry the quota reservation immediately without logres or the ilock by
saving references to the dquots, and then only reacquire logres/ilock on
success..? Just thinking out loud so that might require further
thought...
Brian
