On Mon, Jan 25, 2021 at 12:02:16PM -0800, Darrick J. Wong wrote:
> 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);
>
> xfs_blockgc_free_space runs the blockgc scan directly, which means that
> it creates transactions to free blocks. Since we can't have nested
> transactions, we have to drop tp here.
>
Technically, I don't think it's a problem to hold a transaction memory
allocation (and superblock write access?) while diving into the scanning
mechanism. BTW, this also looks like a landmine passing a NULL eofb into
the xfs_blockgc_free_space() tracepoint.
Brian
> --D
>
> > if (error)
> > return error;
> > error = xfs_trans_reserve(tp, resp, blocks, rtextents);
> > }
> > if (error) {
> > xfs_trans_cancel(tp);
> > return error;
> >
> > ?
>
