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:

quoted

On Wed, May 19, 2021 at 10:13:13PM +1000, Dave Chinner wrote:

.....

quoted

quoted

@@ -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.

quoted

quoted

@@ -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.

quoted

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