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...
> > @@ -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....
> 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.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
