Re: [PATCH 11/11] xfs: flush speculative space allocations when we run out of space

Christoph Hellwig
2021-01-24T09:49:23Z

> +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;

?