Re: [PATCH 01/11] xfs: don't try to write a start record into every iclog

[Dave Chinner <david@fromorbit.com>]

On Wed, Mar 04, 2020 at 07:44:21AM -0800, Christoph Hellwig wrote:
> >  /*
> > - * 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 may need a
> > + * start record, and each region gets its own xlog_op_header_t and may need to
> > + * be double word aligned.
> 
> s/xlog_op_header_t/struct xlog_op_header/ while you're at it.
> 
> > @@ -2404,25 +2391,29 @@ xlog_write(
> >  	int			record_cnt = 0;
> >  	int			data_cnt = 0;
> >  	int			error = 0;
> > +	int			start_rec_size = sizeof(struct xlog_op_header);
> >  
> >  	*start_lsn = 0;
> >  
> > -	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.
> >  	 */
> > -	if (ticket->t_flags & XLOG_TIC_INITED)
> > +	if (ticket->t_flags & XLOG_TIC_INITED) {
> >  		ticket->t_curr_res -= sizeof(xlog_op_header_t);
> > +		ticket->t_flags &= ~XLOG_TIC_INITED;
> > +	}
> >  
> >  	/*
> >  	 * Commit record headers need to be accounted for. These
> >  	 * come in as separate writes so are easy to detect.
> >  	 */
> > -	if (flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS))
> > +	if (flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS)) {
> >  		ticket->t_curr_res -= sizeof(xlog_op_header_t);
> > +		start_rec_size = 0;
> > +	}
> >  
> >  	if (ticket->t_curr_res < 0) {
> >  		xfs_alert_tag(log->l_mp, XFS_PTAG_LOGRES,
> > @@ -2431,6 +2422,8 @@ xlog_write(
> >  		xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
> >  	}
> >  
> > +	len = xlog_write_calc_vec_length(ticket, log_vector, start_rec_size);
> 
> The last arg is used as a boolean in xlog_write_calc_vec_length. I
> think it would make sense to have a need_start_rec boolean in this
> function as well, and just hardcode the sizeof in the two places that
> actually need the size.

I originally had that and while the code looked kinda weird
opencoding an ophdr everywhere we wanted the size of a start record,
that wasn't an issue. The biggest problem was that using a boolean
resulted in _several_ logic bugs that I only tracked down once I
realised I'd forgotten to replace existing the start record size
variable that now wasn't initialised inside the inner loop.

So, yes, it gets converted to a boolean inside that function call,
but I think the code in this set of nested loops is more reliable if
it carries the size of the structure rather than open coding it
everywhere. Making it a boolean doesn't improve the readability of
the code at all.

> > +			copy_len += sizeof(xlog_op_header_t);
> 
> s/xlog_op_header_t/struct xlog_op_header/

Ok.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com