On Sun, Jan 24, 2021 at 09:48:16AM +0000, Christoph Hellwig wrote:
> > +retry:
> > /*
> > * Allocate the handle before we do our freeze accounting and setting up
> > * GFP_NOFS allocation context so that we avoid lockdep false positives
> > @@ -285,6 +289,22 @@ xfs_trans_alloc(
> > tp->t_firstblock = NULLFSBLOCK;
> >
> > error = xfs_trans_reserve(tp, resp, blocks, rtextents);
> > + if (error == -ENOSPC && tries > 0) {
> > + xfs_trans_cancel(tp);
> > +
> > + /*
> > + * We weren't able to reserve enough space for the transaction.
> > + * Flush the other speculative space allocations to free space.
> > + * Do not perform a synchronous scan because callers can hold
> > + * other locks.
> > + */
> > + error = xfs_blockgc_free_space(mp, NULL);
> > + if (error)
> > + return error;
> > +
> > + tries--;
> > + goto retry;
> > + }
> > if (error) {
> > xfs_trans_cancel(tp);
> > return error;
>
> Why do we need to restart the whole function? A failing
> xfs_trans_reserve should restore tp to its initial state, and keeping
> the SB_FREEZE_FS counter increased also doesn't look harmful as far as
> I can tell. So why not:
>
> error = xfs_trans_reserve(tp, resp, blocks, rtextents);
> if (error == -ENOSPC) {
> /*
> * We weren't able to reserve enough space for the transaction.
> * Flush the other speculative space allocations to free space.
> * Do not perform a synchronous scan because callers can hold
> * other locks.
> */
> error = xfs_blockgc_free_space(mp, NULL);
> if (error)
> return error;
> error = xfs_trans_reserve(tp, resp, blocks, rtextents);
> }
> if (error) {
> xfs_trans_cancel(tp);
> return error;
>
> ?
>
That looks cleaner to me, but similar to the earlier quota res patch I'm
wondering if this should be pushed down into xfs_trans_reserve() (or
lifted into a new xfs_trans_reserve_blks() helper called from there)
such that it can handle the various scan/retry scenarios in one place.
Brian
