Re: [RFC v5 PATCH 3/9] xfs: automatic relogging reservation management

[Dave Chinner <david@fromorbit.com>]

On Tue, Mar 03, 2020 at 10:12:17AM -0500, Brian Foster wrote:
> On Tue, Mar 03, 2020 at 03:07:35PM +1100, Dave Chinner wrote:
> > On Tue, Mar 03, 2020 at 10:25:29AM +1100, Dave Chinner wrote:
> > > On Mon, Mar 02, 2020 at 01:06:50PM -0500, Brian Foster wrote:
> > > OK, XLOG_TIC_INITED is redundant, and should be removed. And
> > > xfs_log_done() needs to be split into two, one for releasing the
> > > ticket, one for completing the xlog_write() call. Compile tested
> > > only patch below for you :P
> > 
> > And now with sample patch.
> > 
> > -Dave.
> > -- 
> > Dave Chinner
> > david@fromorbit.com
> > 
> > xfs: kill XLOG_TIC_INITED
> > 
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Delayed logging made this redundant as we never directly write
> > transactions to the log anymore. Hence we no longer make multiple
> > xlog_write() calls for a transaction as we format individual items
> > in a transaction, and hence don't need to keep track of whether we
> > should be writing a start record for every xlog_write call.
> > 
> 
> FWIW the commit log could use a bit more context, perhaps from your
> previous description, about the original semantics of _INITED flag.
> E.g., it's always been rather vague to me, probably because it seems to
> be a remnant of some no longer fully in place functionality.

Yup, it was a quick "here's what it looks like" patch.

> 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/xfs/xfs_log.c      | 79 ++++++++++++++++++---------------------------------
> >  fs/xfs/xfs_log.h      |  4 ---
> >  fs/xfs/xfs_log_cil.c  | 13 +++++----
> >  fs/xfs/xfs_log_priv.h | 18 ++++++------
> >  fs/xfs/xfs_trans.c    | 24 ++++++++--------
> >  5 files changed, 55 insertions(+), 83 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> > index f6006d94a581..a45f3eefee39 100644
> > --- a/fs/xfs/xfs_log.c
> > +++ b/fs/xfs/xfs_log.c
> > @@ -496,8 +496,8 @@ xfs_log_reserve(
> >   * This routine is called when a user of a log manager ticket is done with
> >   * the reservation.  If the ticket was ever used, then a commit record for
> >   * the associated transaction is written out as a log operation header with
> > - * no data.  The flag XLOG_TIC_INITED is set when the first write occurs with
> > - * a given ticket.  If the ticket was one with a permanent reservation, then
> > + * no data. 
> 
> 	      ^ trailing whitespace

Forgot to <ctrl-j> to join the lines back together :P

> 
> > + * If the ticket was one with a permanent reservation, then
> >   * a few operations are done differently.  Permanent reservation tickets by
> >   * default don't release the reservation.  They just commit the current
> >   * transaction with the belief that the reservation is still needed.  A flag
> > @@ -506,49 +506,38 @@ xfs_log_reserve(
> >   * the inited state again.  By doing this, a start record will be written
> >   * out when the next write occurs.
> >   */
> > -xfs_lsn_t
> > -xfs_log_done(
> > -	struct xfs_mount	*mp,
> > +int
> > +xlog_write_done(
> > +	struct xlog		*log,
> >  	struct xlog_ticket	*ticket,
> >  	struct xlog_in_core	**iclog,
> > -	bool			regrant)
> > +	xfs_lsn_t		*lsn)
> >  {
> > -	struct xlog		*log = mp->m_log;
> > -	xfs_lsn_t		lsn = 0;
> > -
> > -	if (XLOG_FORCED_SHUTDOWN(log) ||
> > -	    /*
> > -	     * If nothing was ever written, don't write out commit record.
> > -	     * If we get an error, just continue and give back the log ticket.
> > -	     */
> > -	    (((ticket->t_flags & XLOG_TIC_INITED) == 0) &&
> > -	     (xlog_commit_record(log, ticket, iclog, &lsn)))) {
> > -		lsn = (xfs_lsn_t) -1;
> > -		regrant = false;
> > -	}
> > +	if (XLOG_FORCED_SHUTDOWN(log))
> > +		return -EIO;
> >  
> > +	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) {
> >  		trace_xfs_log_done_nonperm(log, ticket);
> > -
> > -		/*
> > -		 * Release ticket if not permanent reservation or a specific
> > -		 * request has been made to release a permanent reservation.
> > -		 */
> >  		xlog_ungrant_log_space(log, ticket);
> >  	} else {
> >  		trace_xfs_log_done_perm(log, ticket);
> > -
> >  		xlog_regrant_reserve_log_space(log, ticket);
> > -		/* If this ticket was a permanent reservation and we aren't
> > -		 * trying to release it, reset the inited flags; so next time
> > -		 * we write, a start record will be written out.
> > -		 */
> > -		ticket->t_flags |= XLOG_TIC_INITED;
> >  	}
> > -
> >  	xfs_log_ticket_put(ticket);
> > -	return lsn;
> >  }
> 
> In general it would be nicer to split off as much refactoring as
> possible into separate patches, even though it's not yet clear to me
> what granularity is possible with this patch...

Yeah, there's heaps mor cleanups that can be done as a result of
this - e.g. xlog_write_done() and xlog_commit_record() should be
merged. The one caller of xlog_write() that does not provide a
commit_iclog variable shoudl do that call xlog_commit_record()
itself, then xlog_write() can just assume that always returns the
last iclog, etc....


> >  static bool
> > @@ -2148,8 +2137,9 @@ xlog_print_trans(
> >  }
> >  
> >  /*
> > - * Calculate the potential space needed by the log vector.  Each region gets
> > - * its own xlog_op_header_t and may need to be double word aligned.
> > + * Calculate the potential space needed by the log vector.  We always write a
> > + * start record, and each region gets its own xlog_op_header_t and may need to
> > + * be double word aligned.
> >   */
> >  static int
> >  xlog_write_calc_vec_length(
> > @@ -2157,14 +2147,10 @@ xlog_write_calc_vec_length(
> >  	struct xfs_log_vec	*log_vector)
> >  {
> >  	struct xfs_log_vec	*lv;
> > -	int			headers = 0;
> > +	int			headers = 1;
> >  	int			len = 0;
> >  	int			i;
> >  
> > -	/* acct for start rec of xact */
> > -	if (ticket->t_flags & XLOG_TIC_INITED)
> > -		headers++;
> > -
> >  	for (lv = log_vector; lv; lv = lv->lv_next) {
> >  		/* we don't write ordered log vectors */
> >  		if (lv->lv_buf_len == XFS_LOG_VEC_ORDERED)
> > @@ -2195,17 +2181,11 @@ xlog_write_start_rec(
> >  	struct xlog_op_header	*ophdr,
> >  	struct xlog_ticket	*ticket)
> >  {
> > -	if (!(ticket->t_flags & XLOG_TIC_INITED))
> > -		return 0;
> > -
> >  	ophdr->oh_tid	= cpu_to_be32(ticket->t_tid);
> >  	ophdr->oh_clientid = ticket->t_clientid;
> >  	ophdr->oh_len = 0;
> >  	ophdr->oh_flags = XLOG_START_TRANS;
> >  	ophdr->oh_res2 = 0;
> > -
> > -	ticket->t_flags &= ~XLOG_TIC_INITED;
> > -
> >  	return sizeof(struct xlog_op_header);
> >  }
> 
> The header comment to this function refers to the "inited" state.

Missed that...

> Also note that there's a similar reference in
> xfs_log_write_unmount_record(), but that instance sets ->t_flags to zero
> so might be fine outside of the stale comment.

More cleanups!

> > @@ -2410,12 +2390,10 @@ xlog_write(
> >  	len = xlog_write_calc_vec_length(ticket, log_vector);
> >  
> >  	/*
> > -	 * Region headers and bytes are already accounted for.
> > -	 * We only need to take into account start records and
> > -	 * split regions in this function.
> > +	 * Region headers and bytes are already accounted for.  We only need to
> > +	 * take into account start records and split regions in this function.
> >  	 */
> > -	if (ticket->t_flags & XLOG_TIC_INITED)
> > -		ticket->t_curr_res -= sizeof(xlog_op_header_t);
> > +	ticket->t_curr_res -= sizeof(xlog_op_header_t);
> >  
> 
> So AFAICT the CIL allocates a ticket and up to this point only mucks
> around with the reservation value. That means _INITED is still in place
> once we get to xlog_write(). xlog_write() immediately calls
> xlog_write_calc_vec_length() and makes the ->t_curr_res adjustment
> before touching ->t_flags, so those bits all seems fine.
> 
> We then get into the log vector loops, where it looks like we call
> xlog_write_start_rec() for each log vector region and rely on the
> _INITED flag to only write a start record once per associated ticket.
> Unless I'm missing something, this looks like it would change behavior
> to perhaps write a start record per-region..? Note that this might not
> preclude the broader change to kill off _INITED since we're using the
> same ticket throughout the call, but some initial refactoring might be
> required to remove this dependency first.

Ah, yes, well spotted.

I need to move the call to xlog_write_start_rec() outside
the loop - it only needs to be written once per ticket, and we only
ever supply one ticket to xlog_write() now, and it is never reused
to call back into xlog_write again for the same transaction context.

I did say "compile tested only " :)

> > diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> > index b192c5a9f9fd..6965d164ff45 100644
> > --- a/fs/xfs/xfs_log_priv.h
> > +++ b/fs/xfs/xfs_log_priv.h
> > @@ -53,11 +53,9 @@ enum xlog_iclog_state {
> >  /*
> >   * Flags to log ticket
> >   */
> > -#define XLOG_TIC_INITED		0x1	/* has been initialized */
> >  #define XLOG_TIC_PERM_RESERV	0x2	/* permanent reservation */
> 
> These values don't end up on disk, right? If not, it might be worth
> resetting the _PERM_RESERV value to 1. Otherwise the rest looks like
> mostly straightforward refactoring. 

*nod*

-Dave.
-- 
Dave Chinner
david@fromorbit.com