Re: [PATCH 6/8] xfs: remove need_start_rec parameter from xlog_write()

From: Dave Chinner <david@fromorbit.com>
To: Christoph Hellwig <hch@infradead.org>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 6/8] xfs: remove need_start_rec parameter from xlog_write()
Date: Fri, 26 Feb 2021 07:55:38 +1100	[thread overview]
Message-ID: <20210225205538.GL4662@dread.disaster.area> (raw)
In-Reply-To: <YDdkkPyKSnPfll3n@infradead.org>

On Thu, Feb 25, 2021 at 09:49:20AM +0100, Christoph Hellwig wrote:
> > +	if (optype & XLOG_START_TRANS)
> > +		headers++;
> 
> This deserves a comment.

It gets killed off later, so it's a waste of time to prettify this.

> > +	len = xlog_write_calc_vec_length(ticket, log_vector, optype);
> > +	if (start_lsn)
> > +		*start_lsn = 0;
> 
> I'd slightly prefer that allowing a NULL start_lsn was a separate prep
> patch.  As-is it really clutters the patch and detracts from the real
> change.

No, I've already got enough patches in this whole series to deal
with. I'm not splitting out simple, obvious changes into tiny two
line patches that require me to do more work for zero gain.

> >  			int			copy_len;
> >  			int			copy_off;
> >  			bool			ordered = false;
> > +			bool			wrote_start_rec = false;
> >  
> >  			/* ordered log vectors have no regions to write */
> >  			if (lv->lv_buf_len == XFS_LOG_VEC_ORDERED) {
> > @@ -2502,13 +2501,15 @@ xlog_write(
> >  			 * write a start record. Only do this for the first
> >  			 * iclog we write to.
> >  			 */
> > -			if (need_start_rec) {
> > +			if (optype & XLOG_START_TRANS) {
> 
> So this relies on the fact that the only callers that passes an optype of
> XLOG_START_TRANS only writes a single lv.  I think we want an assert for
> that somewhere to avoid a bad surprise later.

This also gets killed off later, so again such things are largely a
waste of my time as all it does is cause rebase conflicts in
multiple patches  and doesn't actually change the end result. So,
again, no.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com