On Tue, Apr 07, 2020 at 04:04:43PM -0700, Allison Collins wrote:
>
>
> 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@xxxxxxxxxx>
> > ---
> > 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.
>
In the xlog_ticket_alloc() case, those are actually memory allocation
flags (as opposed to transaction flags used in xfs_trans_alloc()). The
former includes things like KM_NOFS, etc., that translate to GFP_*
allocation flags. So I don't think it's appropriate to stash a
transaction or ticket oriented flag there.
I did halfway expect some feedback wrt to XFS_TRANS_RELOG because it
technically could be buried in the reservation structure itself, similar
to how permanent state is handled (though we'd still need to pass the
boolean around a bit). I opted to introduce XFS_TRANS_RELOG here instead
because I need to use that for random buffer relogging later in the
series, so it accomplished both at least for the time being. For a
non-RFC series I could look into moving that flag into the reservation,
relegating TRANS_RELOG to debug mode and letting the test code use it
from there.
> Reviewed-by: Allison Collins <allison.henderson@xxxxxxxxxx>
>
Thanks!
Brian
> > {
> > 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)
> >
>
