Re: [PATCH 35/39] xfs: convert log vector chain to use list heads

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

On Thu, Jun 03, 2021 at 10:38:19AM +1000, Dave Chinner wrote:
> On Thu, May 27, 2021 at 12:13:19PM -0700, Darrick J. Wong wrote:
> > On Wed, May 19, 2021 at 10:13:13PM +1000, Dave Chinner wrote:
> .....
> > > @@ -913,25 +912,23 @@ xlog_cil_push_work(
> > >  	xlog_cil_pcp_aggregate(cil, ctx);
> > >  
> > >  	list_sort(NULL, &ctx->log_items, xlog_cil_order_cmp);
> > > -
> > >  	while (!list_empty(&ctx->log_items)) {
> > >  		struct xfs_log_item	*item;
> > >  
> > >  		item = list_first_entry(&ctx->log_items,
> > >  					struct xfs_log_item, li_cil);
> > > +		lv = item->li_lv;
> > >  		list_del_init(&item->li_cil);
> > >  		item->li_order_id = 0;
> > > -		if (!ctx->lv_chain)
> > > -			ctx->lv_chain = item->li_lv;
> > > -		else
> > > -			lv->lv_next = item->li_lv;
> > > -		lv = item->li_lv;
> > >  		item->li_lv = NULL;
> > > -		num_iovecs += lv->lv_niovecs;
> > >  
> > > +		num_iovecs += lv->lv_niovecs;
> > 
> > Not sure why "lv = item->li_lv" needed to move up?
> >
> > I think the only change needed here is replacing the lv_chain/lv_next
> > business with the list_add_tail?
> 
> Yes, but someone complained about the awful diff in the next patch,
> so moving the "lv = item->li_lv" made the diff in the next patch
> much, much cleaner...
> 
> <shrug>
> 
> I can move it back to the next patch if you really want, but it's
> really just shuffling deck chairs at this point...

Nope, don't care that much.

> > > @@ -985,8 +985,14 @@ xlog_cil_push_work(
> > >  	 * use the commit record lsn then we can move the tail beyond the grant
> > >  	 * write head.
> > >  	 */
> > > -	error = xlog_write(log, &lvhdr, ctx->ticket, &ctx->start_lsn, NULL,
> > > -				num_bytes);
> > > +	error = xlog_write(log, &ctx->lv_chain, ctx->ticket, &ctx->start_lsn,
> > > +				NULL, num_bytes);
> > > +
> > > +	/*
> > > +	 * Take the lvhdr back off the lv_chain as it should not be passed
> > > +	 * to log IO completion.
> > > +	 */
> > > +	list_del(&lvhdr.lv_list);
> > 
> > Seems a little clunky, but I guess I see why it's needed.
> 
> I could replace the stack structure with a memory allocation and
> then we wouldn't need to care, but I'm trying to keep memory
> allocation out of this fast path as much as possible....

Oh, that's much worse.

> > I /think/ I don't see any place where the onstack lvhdr can escape out
> > of the chain after _push_work returns, so this is safe enough.
> 
> It can't, because we own the chain here and are completely
> responsible for cleaning it up on failure.

Ok.  I think I'm satisfied now:
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

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