Re: [RFC v6 PATCH 03/10] xfs: extra runtime reservation overhead for relog transactions

Allison Collins
2020-04-07T23:04:49Z

On 4/6/20 5:36 AM, Brian Foster wrote:
> Every transaction reservation includes runtime overhead on top of
> the reservation calculated in the struct xfs_trans_res. This
> overhead is required for things like the CIL context ticket, log
> headers, etc., that are stolen from individual transactions. Since
> reservation for the relog transaction is entirely contributed by
> regular transactions, this runtime reservation overhead must be
> contributed as well. This means that a transaction that relogs one
> or more items must include overhead for the current transaction as
> well as for the relog transaction.
>
> Define a new transaction flag to indicate that a transaction is
> relog enabled. Plumb this state down to the log ticket allocation
> and use it to bump the worst case overhead included in the
> transaction. The overhead will eventually be transferred to the
> relog system as needed for individual log items.
>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
> fs/xfs/libxfs/xfs_shared.h | 1 +
> fs/xfs/xfs_log.c | 12 +++++++++---
> fs/xfs/xfs_log.h | 3 ++-
> fs/xfs/xfs_log_cil.c | 2 +-
> fs/xfs/xfs_log_priv.h | 1 +
> fs/xfs/xfs_trans.c | 3 ++-
> 6 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
> index c45acbd3add9..1ede1e720a5c 100644
> --- a/fs/xfs/libxfs/xfs_shared.h
> +++ b/fs/xfs/libxfs/xfs_shared.h
> @@ -65,6 +65,7 @@ void xfs_log_get_max_trans_res(struct xfs_mount *mp,
> #define XFS_TRANS_DQ_DIRTY 0x10 /* at least one dquot in trx dirty */
> #define XFS_TRANS_RESERVE 0x20 /* OK to use reserved data blocks */
> #define XFS_TRANS_NO_WRITECOUNT 0x40 /* do not elevate SB writecount */
> +#define XFS_TRANS_RELOG 0x80 /* requires extra relog overhead */
> /*
> * LOWMODE is used by the allocator to activate the lowspace algorithm - when
> * free space is running low the extent allocator may choose to allocate an
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index d6b63490a78b..b55abde6c142 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -418,7 +418,8 @@ xfs_log_reserve(
> int cnt,
> struct xlog_ticket **ticp,
> uint8_t client,
> - bool permanent)
> + bool permanent,
> + bool relog)
> {
> struct xlog *log = mp->m_log;
> struct xlog_ticket *tic;
> @@ -433,7 +434,8 @@ xfs_log_reserve(
> XFS_STATS_INC(mp, xs_try_logspace);
>
> ASSERT(*ticp == NULL);
> - tic = xlog_ticket_alloc(log, unit_bytes, cnt, client, permanent, 0);
> + tic = xlog_ticket_alloc(log, unit_bytes, cnt, client, permanent, relog,
> + 0);
> *ticp = tic;
>
> xlog_grant_push_ail(log, tic->t_cnt ? tic->t_unit_res * tic->t_cnt
> @@ -831,7 +833,7 @@ xlog_unmount_write(
> uint flags = XLOG_UNMOUNT_TRANS;
> int error;
>
> - error = xfs_log_reserve(mp, 600, 1, &tic, XFS_LOG, 0);
> + error = xfs_log_reserve(mp, 600, 1, &tic, XFS_LOG, false, false);
> if (error)
> goto out_err;
>
> @@ -3421,6 +3423,7 @@ xlog_ticket_alloc(
> int cnt,
> char client,
> bool permanent,
> + bool relog,
> xfs_km_flags_t alloc_flags)
I notice this routine already has a flag param. Wondering if it would
make sense to have a KM_RELOG flag instead of an extra bool param? It
would just be one less thing to pass around. Other than that, it seems
straight forward.

Reviewed-by: Allison Collins <allison.henderson@oracle.com>

> {
> struct xlog_ticket *tic;
> @@ -3431,6 +3434,9 @@ xlog_ticket_alloc(
> return NULL;
>
> unit_res = xfs_log_calc_unit_res(log->l_mp, unit_bytes);
> + /* double the overhead for the relog transaction */
> + if (relog)
> + unit_res += (unit_res - unit_bytes);
>
> atomic_set(&tic->t_ref, 1);
> tic->t_task = current;
> diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h
> index 6d2f30f42245..f1089a4b299c 100644
> --- a/fs/xfs/xfs_log.h
> +++ b/fs/xfs/xfs_log.h
> @@ -123,7 +123,8 @@ int xfs_log_reserve(struct xfs_mount *mp,
> int count,
> struct xlog_ticket **ticket,
> uint8_t clientid,
> - bool permanent);
> + bool permanent,
> + bool relog);
> int xfs_log_regrant(struct xfs_mount *mp, struct xlog_ticket *tic);
> void xfs_log_ungrant_bytes(struct xfs_mount *mp, int bytes);
> void xfs_log_unmount(struct xfs_mount *mp);
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index b43f0e8f43f2..1c48e95402aa 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -37,7 +37,7 @@ xlog_cil_ticket_alloc(
> {
> struct xlog_ticket *tic;
>
> - tic = xlog_ticket_alloc(log, 0, 1, XFS_TRANSACTION, 0,
> + tic = xlog_ticket_alloc(log, 0, 1, XFS_TRANSACTION, false, false,
> KM_NOFS);
>
> /*
> diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> index ec22c7a3867f..08d8ff9bce1a 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -465,6 +465,7 @@ xlog_ticket_alloc(
> int count,
> char client,
> bool permanent,
> + bool relog,
> xfs_km_flags_t alloc_flags);
>
>
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 4fbe11485bbb..1b25980315bd 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -177,6 +177,7 @@ xfs_trans_reserve(
> */
> if (resp->tr_logres > 0) {
> bool permanent = false;
> + bool relog = (tp->t_flags & XFS_TRANS_RELOG);
>
> ASSERT(tp->t_log_res == 0 ||
> tp->t_log_res == resp->tr_logres);
> @@ -199,7 +200,7 @@ xfs_trans_reserve(
> resp->tr_logres,
> resp->tr_logcount,
> &tp->t_ticket, XFS_TRANSACTION,
> - permanent);
> + permanent, relog);
> }
>
> if (error)
>