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

[Brian Foster <bfoster@redhat.com>]

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@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.
> 

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@oracle.com>
> 

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)
> > 
>