On Tue, Jul 15, 2025 at 02:25:37PM +0200, Christoph Hellwig wrote:
> xfs_trans_roll uses xfs_trans_reserve to basically just call into
> xfs_log_regrant while bypassing the reset of xfs_trans_reserve.
>
> Open code the call to xfs_log_regrant in xfs_trans_roll and simplify
> xfs_trans_reserve now that it never regrants and always asks for a log
> reservation.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
> fs/xfs/xfs_trans.c | 122 +++++++++++++++++++--------------------------
> 1 file changed, 51 insertions(+), 71 deletions(-)
>
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index e43f44f62c5f..553128b7f86a 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -134,18 +134,14 @@ xfs_trans_dup(
> }
>
> /*
> - * This is called to reserve free disk blocks and log space for the
> - * given transaction. This must be done before allocating any resources
> - * within the transaction.
> + * This is called to reserve free disk blocks and log space for the given
> + * transaction before allocating any resources within the transaction.
> *
> * This will return ENOSPC if there are not enough blocks available.
> * It will sleep waiting for available log space.
> - * The only valid value for the flags parameter is XFS_RES_LOG_PERM, which
> - * is used by long running transactions. If any one of the reservations
> - * fails then they will all be backed out.
> *
> - * This does not do quota reservations. That typically is done by the
> - * caller afterwards.
> + * This does not do quota reservations. That typically is done by the caller
> + * afterwards.
> */
> static int
> xfs_trans_reserve(
> @@ -158,10 +154,12 @@ xfs_trans_reserve(
> int error = 0;
> bool rsvd = (tp->t_flags & XFS_TRANS_RESERVE) != 0;
>
> + ASSERT(resp->tr_logres > 0);
> +
> /*
> - * Attempt to reserve the needed disk blocks by decrementing
> - * the number needed from the number available. This will
> - * fail if the count would go below zero.
> + * Attempt to reserve the needed disk blocks by decrementing the number
> + * needed from the number available. This will fail if the count would
> + * go below zero.
> */
> if (blocks > 0) {
> error = xfs_dec_fdblocks(mp, blocks, rsvd);
> @@ -173,42 +171,20 @@ xfs_trans_reserve(
> /*
> * Reserve the log space needed for this transaction.
> */
> - if (resp->tr_logres > 0) {
> - bool permanent = false;
> -
> - ASSERT(tp->t_log_res == 0 ||
> - tp->t_log_res == resp->tr_logres);
> - ASSERT(tp->t_log_count == 0 ||
> - tp->t_log_count == resp->tr_logcount);
> -
> - if (resp->tr_logflags & XFS_TRANS_PERM_LOG_RES) {
> - tp->t_flags |= XFS_TRANS_PERM_LOG_RES;
> - permanent = true;
> - } else {
> - ASSERT(tp->t_ticket == NULL);
> - ASSERT(!(tp->t_flags & XFS_TRANS_PERM_LOG_RES));
> - }
> -
> - if (tp->t_ticket != NULL) {
> - ASSERT(resp->tr_logflags & XFS_TRANS_PERM_LOG_RES);
> - error = xfs_log_regrant(mp, tp->t_ticket);
> - } else {
> - error = xfs_log_reserve(mp, resp->tr_logres,
> - resp->tr_logcount,
> - &tp->t_ticket, permanent);
> - }
> -
> - if (error)
> - goto undo_blocks;
> + if (resp->tr_logflags & XFS_TRANS_PERM_LOG_RES)
> + tp->t_flags |= XFS_TRANS_PERM_LOG_RES;
> + error = xfs_log_reserve(mp, resp->tr_logres, resp->tr_logcount,
> + &tp->t_ticket, (tp->t_flags & XFS_TRANS_PERM_LOG_RES));
> + if (error)
> + goto undo_blocks;
>
> - tp->t_log_res = resp->tr_logres;
> - tp->t_log_count = resp->tr_logcount;
> - }
> + tp->t_log_res = resp->tr_logres;
> + tp->t_log_count = resp->tr_logcount;
>
> /*
> - * Attempt to reserve the needed realtime extents by decrementing
> - * the number needed from the number available. This will
> - * fail if the count would go below zero.
> + * Attempt to reserve the needed realtime extents by decrementing the
> + * number needed from the number available. This will fail if the
> + * count would go below zero.
> */
> if (rtextents > 0) {
> error = xfs_dec_frextents(mp, rtextents);
> @@ -221,18 +197,11 @@ xfs_trans_reserve(
>
> return 0;
>
> - /*
> - * Error cases jump to one of these labels to undo any
> - * reservations which have already been performed.
> - */
> undo_log:
> - if (resp->tr_logres > 0) {
> - xfs_log_ticket_ungrant(mp->m_log, tp->t_ticket);
> - tp->t_ticket = NULL;
> - tp->t_log_res = 0;
> - tp->t_flags &= ~XFS_TRANS_PERM_LOG_RES;
> - }
> -
> + xfs_log_ticket_ungrant(mp->m_log, tp->t_ticket);
> + tp->t_ticket = NULL;
> + tp->t_log_res = 0;
> + tp->t_flags &= ~XFS_TRANS_PERM_LOG_RES;
> undo_blocks:
> if (blocks > 0) {
> xfs_add_fdblocks(mp, blocks);
> @@ -1030,36 +999,41 @@ xfs_trans_cancel(
> }
>
> /*
> - * Roll from one trans in the sequence of PERMANENT transactions to
> - * the next: permanent transactions are only flushed out when
> - * committed with xfs_trans_commit(), but we still want as soon
> - * as possible to let chunks of it go to the log. So we commit the
> - * chunk we've been working on and get a new transaction to continue.
> + * Roll from one trans in the sequence of PERMANENT transactions to the next:
> + * permanent transactions are only flushed out when committed with
> + * xfs_trans_commit(), but we still want as soon as possible to let chunks of it
> + * go to the log. So we commit the chunk we've been working on and get a new
> + * transaction to continue.
> */
> int
> xfs_trans_roll(
> struct xfs_trans **tpp)
> {
> struct xfs_trans *trans = *tpp;
> + struct xfs_mount *mp = trans->t_mountp;
> struct xfs_trans_res tres;
> int error;
>
> trace_xfs_trans_roll(trans, _RET_IP_);
>
> + ASSERT(trans->t_log_res > 0);
> +
> /*
> * Copy the critical parameters from one trans to the next.
> */
> tres.tr_logres = trans->t_log_res;
> tres.tr_logcount = trans->t_log_count;
> + tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
As a dumb optimization, you could initialize tres at declaration time
instead of down here.
Other than that minor nitpicking, this does make it easier for me to
figure out what xfs_trans_roll does because now I don't have to go
picking through the _reserve logic so on those grounds
Reviewed-by: "Darrick J. Wong" <djwong@xxxxxxxxxx>
--D
>
> *tpp = xfs_trans_dup(trans);
>
> /*
> * Commit the current transaction.
> - * If this commit failed, then it'd just unlock those items that
> - * are not marked ihold. That also means that a filesystem shutdown
> - * is in progress. The caller takes the responsibility to cancel
> - * the duplicate transaction that gets returned.
> + *
> + * If this commit failed, then it'd just unlock those items that are not
> + * marked ihold. That also means that a filesystem shutdown is in
> + * progress. The caller takes the responsibility to cancel the
> + * duplicate transaction that gets returned.
> */
> error = __xfs_trans_commit(trans, true);
> if (error)
> @@ -1067,14 +1041,20 @@ xfs_trans_roll(
>
> /*
> * Reserve space in the log for the next transaction.
> - * This also pushes items in the "AIL", the list of logged items,
> - * out to disk if they are taking up space at the tail of the log
> - * that we want to use. This requires that either nothing be locked
> - * across this call, or that anything that is locked be logged in
> - * the prior and the next transactions.
> + *
> + * This also pushes items in the AIL out to disk if they are taking up
> + * space at the tail of the log that we want to use. This requires that
> + * either nothing be locked across this call, or that anything that is
> + * locked be logged in the prior and the next transactions.
> */
> - tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
> - return xfs_trans_reserve(*tpp, &tres, 0, 0);
> + trans = *tpp;
> + trans->t_flags |= XFS_TRANS_PERM_LOG_RES;
> + error = xfs_log_regrant(mp, trans->t_ticket);
> + if (error)
> + return error;
> + trans->t_log_res = tres.tr_logres;
> + trans->t_log_count = tres.tr_logcount;
> + return 0;
> }
>
> /*
> --
> 2.47.2
>
>
