On Thu, Feb 25, 2021 at 07:43:38PM +0100, Christoph Hellwig wrote:

On Wed, Feb 24, 2021 at 05:34:56PM +1100, Dave Chinner wrote:

quoted

From: Dave Chinner <redacted>

Vector out to an optimised version of xlog_write() if the entire

s/Vector/Factor/ ?

quoted

+	ptr = iclog->ic_datap + log_offset;
+	while (lv && (!lv->lv_niovecs || index < lv->lv_niovecs)) {
+
+
+		/* ordered log vectors have no regions to write */

The two empty lines above look a little strange.

quoted

+		if (lv->lv_buf_len == XFS_LOG_VEC_ORDERED) {
+			ASSERT(lv->lv_niovecs == 0);
+			goto next_lv;
+		}
+
+		reg = &lv->lv_iovecp[index];
+		ASSERT(reg->i_len % sizeof(int32_t) == 0);
+		ASSERT((unsigned long)ptr % sizeof(int32_t) == 0);
+
+		ophdr = reg->i_addr;
+		ophdr->oh_tid = cpu_to_be32(ticket->t_tid);
+		ophdr->oh_len = cpu_to_be32(reg->i_len -
+					sizeof(struct xlog_op_header));
+
+		memcpy(ptr, reg->i_addr, reg->i_len);
+		xlog_write_adv_cnt(&ptr, &len, &log_offset, reg->i_len);
+		record_cnt++;
+
+		/* move to the next iovec */
+		if (++index < lv->lv_niovecs)
+			continue;
+next_lv:
+		/* move to the next logvec */
+		lv = lv->lv_next;
+		index = 0;
+	}

I always hated this (pre-existing) loop style.

Me too, but my brain was stretched just getting it factored into a
tighter loop so I wasn't looking too hard at the iteration methof
itself.

What do you think of
something like this (just whiteboard coding, might be completely broken),
which also handles the ordered case with lv_niovecs == 0 as part of
the natural loop:

	for (lv = log_vector; lv; lv->lv_next) {
		for (index = 0; index < lv->lv_niovecs; index++) {
			struct xfs_log_iovec	*reg = &lv->lv_iovecp[index];
			struct xlog_op_header	*ophdr = reg->i_addr;

			ASSERT(reg->i_len % sizeof(int32_t) == 0);
			ASSERT((unsigned long)ptr % sizeof(int32_t) == 0);

			ophdr->oh_tid = cpu_to_be32(ticket->t_tid);
			ophdr->oh_len = cpu_to_be32(reg->i_len -
				sizeof(struct xlog_op_header));
			memcpy(ptr, reg->i_addr, reg->i_len);
			xlog_write_adv_cnt(&ptr, &len, &log_offset, reg->i_len);
			record_cnt++;
		}
	}

Yup, that is definitely an improvement. It wasnt' an option with the
way the existing code nested, but this is much, much neater. Thanks!

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com