diff mbox series

[10/13] xfs: introduce xlog_write_single()

Message ID 20210224063459.3436852-11-david@fromorbit.com (mailing list archive)
State Superseded
Headers show
Series xfs: rewrite xlog_write() | expand

Commit Message

Dave Chinner Feb. 24, 2021, 6:34 a.m. UTC 
From: Dave Chinner <dchinner@redhat.com>

Vector out to an optimised version of xlog_write() if the entire
write will fit in a single iclog. This greatly simplifies the
implementation of writing a log vector chain into an iclog, and
sets the ground work for a much more understandable xlog_write()
implementation.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_log.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 68 insertions(+), 1 deletion(-)

Comments

Christoph Hellwig Feb. 25, 2021, 6:43 p.m. UTC | #1 
On Wed, Feb 24, 2021 at 05:34:56PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Vector out to an optimised version of xlog_write() if the entire

s/Vector/Factor/ ?

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

> +		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.  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++;
		}
	}
Dave Chinner Feb. 25, 2021, 10:21 p.m. UTC | #2 
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:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Vector out to an optimised version of xlog_write() if the entire
> 
> s/Vector/Factor/ ?
> 
> > +	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.
> 
> > +		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.
diff mbox series

Patch

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 50c7ed3972d7..456ab3294621 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2243,6 +2243,64 @@  xlog_write_copy_finish(
 	return error;
 }
 
+/*
+ * Write log vectors into a single iclog which is guaranteed by the caller
+ * to have enough space to write the entire log vector into. Return the number
+ * of log vectors written into the iclog.
+ */
+static int
+xlog_write_single(
+	struct xfs_log_vec	*log_vector,
+	struct xlog_ticket	*ticket,
+	struct xlog_in_core	*iclog,
+	uint32_t		log_offset,
+	uint32_t		len)
+{
+	struct xfs_log_vec	*lv = log_vector;
+	struct xfs_log_iovec	*reg;
+	struct xlog_op_header	*ophdr;
+	void			*ptr;
+	int			index = 0;
+	int			record_cnt = 0;
+
+	ASSERT(log_offset + len <= iclog->ic_size);
+
+	ptr = iclog->ic_datap + log_offset;
+	while (lv && (!lv->lv_niovecs || index < lv->lv_niovecs)) {
+
+
+		/* ordered log vectors have no regions to write */
+		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;
+	}
+	ASSERT(len == 0);
+	return record_cnt;
+}
+
+
 /*
  * Write some region out to in-core log
  *
@@ -2323,7 +2381,6 @@  xlog_write(
 			return error;
 
 		ASSERT(log_offset <= iclog->ic_size - 1);
-		ptr = iclog->ic_datap + log_offset;
 
 		/* Start_lsn is the first lsn written to. */
 		if (start_lsn && !*start_lsn)
@@ -2340,10 +2397,20 @@  xlog_write(
 						XLOG_ICL_NEED_FUA);
 		}
 
+		/* If this is a single iclog write, go fast... */
+		if (!contwr && lv == log_vector) {
+			record_cnt = xlog_write_single(lv, ticket, iclog,
+						log_offset, len);
+			len = 0;
+			data_cnt = len;
+			break;
+		}
+
 		/*
 		 * This loop writes out as many regions as can fit in the amount
 		 * of space which was allocated by xlog_state_get_iclog_space().
 		 */
+		ptr = iclog->ic_datap + log_offset;
 		while (lv && (!lv->lv_niovecs || index < lv->lv_niovecs)) {
 			struct xfs_log_iovec	*reg;
 			struct xlog_op_header	*ophdr;