Re: [PATCH 20/39] xfs: pass lv chain length into xlog_write()

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Jun 03, 2021 at 08:18:52AM +1000, Dave Chinner wrote:
> On Thu, May 27, 2021 at 10:20:27AM -0700, Darrick J. Wong wrote:
> > On Wed, May 19, 2021 at 10:12:58PM +1000, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > The caller of xlog_write() usually has a close accounting of the
> > > aggregated vector length contained in the log vector chain passed to
> > > xlog_write(). There is no need to iterate the chain to calculate he
> > > length of the data in xlog_write_calculate_len() if the caller is
> > > already iterating that chain to build it.
> > > 
> > > Passing in the vector length avoids doing an extra chain iteration,
> > > which can be a significant amount of work given that large CIL
> > > commits can have hundreds of thousands of vectors attached to the
> > > chain.
> > > 
> > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ....
> > > @@ -849,6 +850,10 @@ xlog_cil_push_work(
> > >  		lv = item->li_lv;
> > >  		item->li_lv = NULL;
> > >  		num_iovecs += lv->lv_niovecs;
> > > +
> > > +		/* we don't write ordered log vectors */
> > > +		if (lv->lv_buf_len != XFS_LOG_VEC_ORDERED)
> > > +			num_bytes += lv->lv_bytes;
> > >  	}
> > >  
> > >  	/*
> > > @@ -887,6 +892,8 @@ xlog_cil_push_work(
> > >  	 * transaction header here as it is not accounted for in xlog_write().
> > >  	 */
> > >  	xlog_cil_build_trans_hdr(ctx, &thdr, &lvhdr, num_iovecs);
> > > +	num_iovecs += lvhdr.lv_niovecs;
> > 
> > I have the same question that Brian had last time, which is: What's the
> > point of updating num_iovecs here?  It's not used after
> > xlog_cil_build_trans_hdr, either here or at the end of the patchset.
> > 
> > Is the idea that num_{iovecs,bytes} will always reflect everything
> > in the cil context chain that's about to be passed to xlog_write?
> 
> I left it there because I did want to keep the two variables up to
> date for future use. i.e. I didn't want to leave a landmine later
> down the track if I need to use num_iovecs in future changes. I've
> also used it a few times for temporary debugging code, so I'd
> prefer to keep it even though it isn't used.
> 
> But if "not used" is the only reason for people not giving rvbs,
> then I can remove it...

...or feed it to a tracepoint, if you find it useful for debugging the
size of log writes?  <shrug>

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com