On Tue, Mar 24, 2020 at 06:44:56PM +0100, Christoph Hellwig wrote:
> Split the regrant case out of xlog_ticket_done and into a new
> xlog_ticket_regrant helper. Merge both functions with the low-level
> functions implementing the actual functionality and adjust the
> tracepoints.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>
More cohesive functions FTW,
Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
--D
> ---
> fs/xfs/xfs_log.c | 84 ++++++++++++++-----------------------------
> fs/xfs/xfs_log_cil.c | 9 +++--
> fs/xfs/xfs_log_priv.h | 4 +--
> fs/xfs/xfs_trace.h | 14 ++++----
> fs/xfs/xfs_trans.c | 9 +++--
> 5 files changed, 47 insertions(+), 73 deletions(-)
>
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index b30bb6452494..9a26ee8db238 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -66,14 +66,6 @@ xlog_grant_push_ail(
> struct xlog *log,
> int need_bytes);
> STATIC void
> -xlog_regrant_reserve_log_space(
> - struct xlog *log,
> - struct xlog_ticket *ticket);
> -STATIC void
> -xlog_ungrant_log_space(
> - struct xlog *log,
> - struct xlog_ticket *ticket);
> -STATIC void
> xlog_sync(
> struct xlog *log,
> struct xlog_in_core *iclog);
> @@ -502,27 +494,6 @@ xlog_write_done(
> return xlog_commit_record(log, ticket, iclog, lsn);
> }
>
> -/*
> - * Release or regrant the ticket reservation now the transaction is done with
> - * it depending on caller context. Rolling transactions need the ticket
> - * regranted, otherwise we release it completely.
> - */
> -void
> -xlog_ticket_done(
> - struct xlog *log,
> - struct xlog_ticket *ticket,
> - bool regrant)
> -{
> - if (!regrant || XLOG_FORCED_SHUTDOWN(log)) {
> - trace_xfs_log_done_nonperm(log, ticket);
> - xlog_ungrant_log_space(log, ticket);
> - } else {
> - trace_xfs_log_done_perm(log, ticket);
> - xlog_regrant_reserve_log_space(log, ticket);
> - }
> - xfs_log_ticket_put(ticket);
> -}
> -
> static bool
> __xlog_state_release_iclog(
> struct xlog *log,
> @@ -921,8 +892,7 @@ xfs_log_write_unmount_record(
>
> if (tic) {
> trace_xfs_log_umount_write(log, tic);
> - xlog_ungrant_log_space(log, tic);
> - xfs_log_ticket_put(tic);
> + xlog_ticket_done(log, tic);
> }
> }
>
> @@ -2987,19 +2957,18 @@ xlog_state_get_iclog_space(
> return 0;
> } /* xlog_state_get_iclog_space */
>
> -/* The first cnt-1 times through here we don't need to
> - * move the grant write head because the permanent
> - * reservation has reserved cnt times the unit amount.
> - * Release part of current permanent unit reservation and
> - * reset current reservation to be one units worth. Also
> - * move grant reservation head forward.
> +/*
> + * The first cnt-1 times through here we don't need to move the grant write head
> + * because the permanent reservation has reserved cnt times the unit amount.
> + * Release part of current permanent unit reservation and reset current
> + * reservation to be one units worth. Also move grant reservation head forward.
> */
> -STATIC void
> -xlog_regrant_reserve_log_space(
> +void
> +xlog_ticket_regrant(
> struct xlog *log,
> struct xlog_ticket *ticket)
> {
> - trace_xfs_log_regrant_reserve_enter(log, ticket);
> + trace_xfs_log_ticket_regrant(log, ticket);
>
> if (ticket->t_cnt > 0)
> ticket->t_cnt--;
> @@ -3011,21 +2980,20 @@ xlog_regrant_reserve_log_space(
> ticket->t_curr_res = ticket->t_unit_res;
> xlog_tic_reset_res(ticket);
>
> - trace_xfs_log_regrant_reserve_sub(log, ticket);
> + trace_xfs_log_ticket_regrant_sub(log, ticket);
>
> /* just return if we still have some of the pre-reserved space */
> - if (ticket->t_cnt > 0)
> - return;
> + if (!ticket->t_cnt) {
> + xlog_grant_add_space(log, &log->l_reserve_head.grant,
> + ticket->t_unit_res);
> + trace_xfs_log_ticket_regrant_exit(log, ticket);
>
> - xlog_grant_add_space(log, &log->l_reserve_head.grant,
> - ticket->t_unit_res);
> -
> - trace_xfs_log_regrant_reserve_exit(log, ticket);
> -
> - ticket->t_curr_res = ticket->t_unit_res;
> - xlog_tic_reset_res(ticket);
> -} /* xlog_regrant_reserve_log_space */
> + ticket->t_curr_res = ticket->t_unit_res;
> + xlog_tic_reset_res(ticket);
> + }
>
> + xfs_log_ticket_put(ticket);
> +}
>
> /*
> * Give back the space left from a reservation.
> @@ -3041,18 +3009,19 @@ xlog_regrant_reserve_log_space(
> * space, the count will stay at zero and the only space remaining will be
> * in the current reservation field.
> */
> -STATIC void
> -xlog_ungrant_log_space(
> +void
> +xlog_ticket_done(
> struct xlog *log,
> struct xlog_ticket *ticket)
> {
> - int bytes;
> + int bytes;
> +
> + trace_xfs_log_ticket_done(log, ticket);
>
> if (ticket->t_cnt > 0)
> ticket->t_cnt--;
>
> - trace_xfs_log_ungrant_enter(log, ticket);
> - trace_xfs_log_ungrant_sub(log, ticket);
> + trace_xfs_log_ticket_done_sub(log, ticket);
>
> /*
> * If this is a permanent reservation ticket, we may be able to free
> @@ -3067,9 +3036,10 @@ xlog_ungrant_log_space(
> xlog_grant_sub_space(log, &log->l_reserve_head.grant, bytes);
> xlog_grant_sub_space(log, &log->l_write_head.grant, bytes);
>
> - trace_xfs_log_ungrant_exit(log, ticket);
> + trace_xfs_log_ticket_done_exit(log, ticket);
>
> xfs_log_space_wake(log->l_mp);
> + xfs_log_ticket_put(ticket);
> }
>
> /*
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index 880de1aa4288..1189c8cfa525 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -843,7 +843,7 @@ xlog_cil_push_work(
> if (error)
> goto out_abort_free_ticket;
>
> - xlog_ticket_done(log, tic, false);
> + xlog_ticket_done(log, tic);
>
> spin_lock(&commit_iclog->ic_callback_lock);
> if (commit_iclog->ic_state == XLOG_STATE_IOERROR) {
> @@ -876,7 +876,7 @@ xlog_cil_push_work(
> return;
>
> out_abort_free_ticket:
> - xlog_ticket_done(log, tic, false);
> + xlog_ticket_done(log, tic);
> out_abort:
> ASSERT(XLOG_FORCED_SHUTDOWN(log));
> xlog_cil_committed(ctx);
> @@ -1008,7 +1008,10 @@ xfs_log_commit_cil(
> if (commit_lsn)
> *commit_lsn = xc_commit_lsn;
>
> - xlog_ticket_done(log, tp->t_ticket, regrant);
> + if (regrant && !XLOG_FORCED_SHUTDOWN(log))
> + xlog_ticket_regrant(log, tp->t_ticket);
> + else
> + xlog_ticket_done(log, tp->t_ticket);
> tp->t_ticket = NULL;
> xfs_trans_unreserve_and_mod_sb(tp);
>
> diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> index cfe5295ef4e3..cfcf3f02e30a 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -443,8 +443,8 @@ int xlog_write(struct xlog *log, struct xfs_log_vec *log_vector,
> struct xlog_in_core **commit_iclog, uint flags);
> int xlog_write_done(struct xlog *log, struct xlog_ticket *ticket,
> struct xlog_in_core **iclog, xfs_lsn_t *lsn);
> -void xlog_ticket_done(struct xlog *log, struct xlog_ticket *ticket,
> - bool regrant);
> +void xlog_ticket_done(struct xlog *log, struct xlog_ticket *ticket);
> +void xlog_ticket_regrant(struct xlog *log, struct xlog_ticket *ticket);
>
> /*
> * When we crack an atomic LSN, we sample it first so that the value will not
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index efc7751550d9..fbfdd9cf160d 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -1001,8 +1001,6 @@ DECLARE_EVENT_CLASS(xfs_loggrant_class,
> DEFINE_EVENT(xfs_loggrant_class, name, \
> TP_PROTO(struct xlog *log, struct xlog_ticket *tic), \
> TP_ARGS(log, tic))
> -DEFINE_LOGGRANT_EVENT(xfs_log_done_nonperm);
> -DEFINE_LOGGRANT_EVENT(xfs_log_done_perm);
> DEFINE_LOGGRANT_EVENT(xfs_log_umount_write);
> DEFINE_LOGGRANT_EVENT(xfs_log_grant_sleep);
> DEFINE_LOGGRANT_EVENT(xfs_log_grant_wake);
> @@ -1011,12 +1009,12 @@ DEFINE_LOGGRANT_EVENT(xfs_log_reserve);
> DEFINE_LOGGRANT_EVENT(xfs_log_reserve_exit);
> DEFINE_LOGGRANT_EVENT(xfs_log_regrant);
> DEFINE_LOGGRANT_EVENT(xfs_log_regrant_exit);
> -DEFINE_LOGGRANT_EVENT(xfs_log_regrant_reserve_enter);
> -DEFINE_LOGGRANT_EVENT(xfs_log_regrant_reserve_exit);
> -DEFINE_LOGGRANT_EVENT(xfs_log_regrant_reserve_sub);
> -DEFINE_LOGGRANT_EVENT(xfs_log_ungrant_enter);
> -DEFINE_LOGGRANT_EVENT(xfs_log_ungrant_exit);
> -DEFINE_LOGGRANT_EVENT(xfs_log_ungrant_sub);
> +DEFINE_LOGGRANT_EVENT(xfs_log_ticket_regrant);
> +DEFINE_LOGGRANT_EVENT(xfs_log_ticket_regrant_exit);
> +DEFINE_LOGGRANT_EVENT(xfs_log_ticket_regrant_sub);
> +DEFINE_LOGGRANT_EVENT(xfs_log_ticket_done);
> +DEFINE_LOGGRANT_EVENT(xfs_log_ticket_done_sub);
> +DEFINE_LOGGRANT_EVENT(xfs_log_ticket_done_exit);
>
> DECLARE_EVENT_CLASS(xfs_log_item_class,
> TP_PROTO(struct xfs_log_item *lip),
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 123ecc8435f6..d7c66c3331ec 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -231,7 +231,7 @@ xfs_trans_reserve(
> */
> undo_log:
> if (resp->tr_logres > 0) {
> - xlog_ticket_done(mp->m_log, tp->t_ticket, false);
> + xlog_ticket_done(mp->m_log, tp->t_ticket);
> tp->t_ticket = NULL;
> tp->t_log_res = 0;
> tp->t_flags &= ~XFS_TRANS_PERM_LOG_RES;
> @@ -1001,7 +1001,10 @@ __xfs_trans_commit(
> */
> xfs_trans_unreserve_and_mod_dquots(tp);
> if (tp->t_ticket) {
> - xlog_ticket_done(mp->m_log, tp->t_ticket, regrant);
> + if (regrant && !XLOG_FORCED_SHUTDOWN(mp->m_log))
> + xlog_ticket_regrant(mp->m_log, tp->t_ticket);
> + else
> + xlog_ticket_done(mp->m_log, tp->t_ticket);
> tp->t_ticket = NULL;
> }
> current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
> @@ -1060,7 +1063,7 @@ xfs_trans_cancel(
> xfs_trans_unreserve_and_mod_dquots(tp);
>
> if (tp->t_ticket) {
> - xlog_ticket_done(mp->m_log, tp->t_ticket, false);
> + xlog_ticket_done(mp->m_log, tp->t_ticket);
> tp->t_ticket = NULL;
> }
>
> --
> 2.25.1
>
