All of lore.kernel.org
 help / color / mirror / Atom feed
* log write cleanups
@ 2021-06-16 16:32 Christoph Hellwig
  2021-06-16 16:32 ` [PATCH 1/8] xfs: change the type of ic_datap Christoph Hellwig
                   ` (8 more replies)
  0 siblings, 9 replies; 26+ messages in thread
From: Christoph Hellwig @ 2021-06-16 16:32 UTC (permalink / raw)
  To: linux-xfs

Hi all,

this series cleans up a few loose ends that I noticed while reviewing
the recent log rework.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/8] xfs: change the type of ic_datap
  2021-06-16 16:32 log write cleanups Christoph Hellwig
@ 2021-06-16 16:32 ` Christoph Hellwig
  2021-06-17 16:22   ` Brian Foster
  2021-06-18  5:52   ` Chandan Babu R
  2021-06-16 16:32 ` [PATCH 2/8] xfs: list entry elements don't need to be initialized Christoph Hellwig
                   ` (7 subsequent siblings)
  8 siblings, 2 replies; 26+ messages in thread
From: Christoph Hellwig @ 2021-06-16 16:32 UTC (permalink / raw)
  To: linux-xfs

Turn ic_datap from a char into a void pointer given that it points
to arbitrary data.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log.c      | 2 +-
 fs/xfs/xfs_log_priv.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index e921b554b68367..8999c78f3ac6d9 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -3613,7 +3613,7 @@ xlog_verify_iclog(
 		if (field_offset & 0x1ff) {
 			clientid = ophead->oh_clientid;
 		} else {
-			idx = BTOBBT((char *)&ophead->oh_clientid - iclog->ic_datap);
+			idx = BTOBBT((void *)&ophead->oh_clientid - iclog->ic_datap);
 			if (idx >= (XLOG_HEADER_CYCLE_SIZE / BBSIZE)) {
 				j = idx / (XLOG_HEADER_CYCLE_SIZE / BBSIZE);
 				k = idx % (XLOG_HEADER_CYCLE_SIZE / BBSIZE);
diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index e4e421a7033558..96dbe713954f7e 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -185,7 +185,7 @@ typedef struct xlog_in_core {
 	u32			ic_offset;
 	enum xlog_iclog_state	ic_state;
 	unsigned int		ic_flags;
-	char			*ic_datap;	/* pointer to iclog data */
+	void			*ic_datap;	/* pointer to iclog data */
 
 	/* Callback structures need their own cacheline */
 	spinlock_t		ic_callback_lock ____cacheline_aligned_in_smp;
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 2/8] xfs: list entry elements don't need to be initialized
  2021-06-16 16:32 log write cleanups Christoph Hellwig
  2021-06-16 16:32 ` [PATCH 1/8] xfs: change the type of ic_datap Christoph Hellwig
@ 2021-06-16 16:32 ` Christoph Hellwig
  2021-06-17 16:23   ` Brian Foster
  2021-06-18  5:59   ` Chandan Babu R
  2021-06-16 16:32 ` [PATCH 3/8] xfs: factor out a helper to write a log_iovec into the iclog Christoph Hellwig
                   ` (6 subsequent siblings)
  8 siblings, 2 replies; 26+ messages in thread
From: Christoph Hellwig @ 2021-06-16 16:32 UTC (permalink / raw)
  To: linux-xfs

list_add does not require the added element to be initialized.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 8999c78f3ac6d9..32cb0fc459a364 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -851,7 +851,7 @@ xlog_write_unmount_record(
 		.lv_iovecp = &reg,
 	};
 	LIST_HEAD(lv_chain);
-	INIT_LIST_HEAD(&vec.lv_list);
+
 	list_add(&vec.lv_list, &lv_chain);
 
 	BUILD_BUG_ON((sizeof(struct xlog_op_header) +
@@ -1587,7 +1587,7 @@ xlog_commit_record(
 	};
 	int	error;
 	LIST_HEAD(lv_chain);
-	INIT_LIST_HEAD(&vec.lv_list);
+
 	list_add(&vec.lv_list, &lv_chain);
 
 	if (XLOG_FORCED_SHUTDOWN(log))
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 3/8] xfs: factor out a helper to write a log_iovec into the iclog
  2021-06-16 16:32 log write cleanups Christoph Hellwig
  2021-06-16 16:32 ` [PATCH 1/8] xfs: change the type of ic_datap Christoph Hellwig
  2021-06-16 16:32 ` [PATCH 2/8] xfs: list entry elements don't need to be initialized Christoph Hellwig
@ 2021-06-16 16:32 ` Christoph Hellwig
  2021-06-17 16:23   ` Brian Foster
  2021-06-18  7:42   ` Chandan Babu R
  2021-06-16 16:32 ` [PATCH 4/8] xfs: remove xlog_write_adv_cnt and simplify xlog_write_partial Christoph Hellwig
                   ` (5 subsequent siblings)
  8 siblings, 2 replies; 26+ messages in thread
From: Christoph Hellwig @ 2021-06-16 16:32 UTC (permalink / raw)
  To: linux-xfs

Add a new helper to copy the log iovec into the in-core log buffer,
and open code the handling continuation opheader as a special case.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log.c | 55 +++++++++++++++++++++++++++---------------------
 1 file changed, 31 insertions(+), 24 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 32cb0fc459a364..5b431d53287d2c 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2124,6 +2124,26 @@ xlog_print_trans(
 	}
 }
 
+static inline void
+xlog_write_iovec(
+	struct xlog_in_core	*iclog,
+	uint32_t		*log_offset,
+	void			*data,
+	uint32_t		write_len,
+	int			*bytes_left,
+	uint32_t		*record_cnt,
+	uint32_t		*data_cnt)
+{
+	ASSERT(*log_offset % sizeof(int32_t) == 0);
+	ASSERT(write_len % sizeof(int32_t) == 0);
+
+	memcpy(iclog->ic_datap + *log_offset, data, write_len);
+	*log_offset += write_len;
+	*bytes_left -= write_len;
+	(*record_cnt)++;
+	*data_cnt += write_len;
+}
+
 /*
  * Write whole log vectors into a single iclog which is guaranteed to have
  * either sufficient space for the entire log vector chain to be written or
@@ -2145,13 +2165,11 @@ xlog_write_single(
 	uint32_t		*data_cnt)
 {
 	struct xfs_log_vec	*lv;
-	void			*ptr;
 	int			index;
 
 	ASSERT(*log_offset + *len <= iclog->ic_size ||
 		iclog->ic_state == XLOG_STATE_WANT_SYNC);
 
-	ptr = iclog->ic_datap + *log_offset;
 	for (lv = log_vector;
 	     !list_entry_is_head(lv, lv_chain, lv_list);
 	     lv = list_next_entry(lv, lv_list)) {
@@ -2171,16 +2189,13 @@ xlog_write_single(
 			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)++;
-			*data_cnt += reg->i_len;
+
+			xlog_write_iovec(iclog, log_offset, reg->i_addr,
+					 reg->i_len, len, record_cnt,
+					 data_cnt);
 		}
 	}
 	if (list_entry_is_head(lv, lv_chain, lv_list))
@@ -2249,12 +2264,10 @@ xlog_write_partial(
 	int			error;
 
 	/* walk the logvec, copying until we run out of space in the iclog */
-	ptr = iclog->ic_datap + *log_offset;
 	for (index = 0; index < lv->lv_niovecs; index++) {
 		uint32_t	reg_offset = 0;
 
 		reg = &lv->lv_iovecp[index];
-		ASSERT(reg->i_len % sizeof(int32_t) == 0);
 
 		/*
 		 * The first region of a continuation must have a non-zero
@@ -2274,7 +2287,6 @@ xlog_write_partial(
 					data_cnt);
 			if (error)
 				return ERR_PTR(error);
-			ptr = iclog->ic_datap + *log_offset;
 		}
 
 		ophdr = reg->i_addr;
@@ -2285,12 +2297,9 @@ xlog_write_partial(
 		if (rlen != reg->i_len)
 			ophdr->oh_flags |= XLOG_CONTINUE_TRANS;
 
-		ASSERT((unsigned long)ptr % sizeof(int32_t) == 0);
-		xlog_verify_dest_ptr(log, ptr);
-		memcpy(ptr, reg->i_addr, rlen);
-		xlog_write_adv_cnt(&ptr, len, log_offset, rlen);
-		(*record_cnt)++;
-		*data_cnt += rlen;
+		xlog_verify_dest_ptr(log, iclog->ic_datap + *log_offset);
+		xlog_write_iovec(iclog, log_offset, reg->i_addr, rlen, len,
+				 record_cnt, data_cnt);
 
 		/* If we wrote the whole region, move to the next. */
 		if (rlen == reg->i_len)
@@ -2356,12 +2365,10 @@ xlog_write_partial(
 			rlen = min_t(uint32_t, rlen, iclog->ic_size - *log_offset);
 			ophdr->oh_len = cpu_to_be32(rlen);
 
-			xlog_verify_dest_ptr(log, ptr);
-			memcpy(ptr, reg->i_addr + reg_offset, rlen);
-			xlog_write_adv_cnt(&ptr, len, log_offset, rlen);
-			(*record_cnt)++;
-			*data_cnt += rlen;
-
+			xlog_verify_dest_ptr(log, iclog->ic_datap + *log_offset);
+			xlog_write_iovec(iclog, log_offset,
+					 reg->i_addr + reg_offset, rlen, len,
+					 record_cnt, data_cnt);
 		} while (ophdr->oh_flags & XLOG_CONTINUE_TRANS);
 	}
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 4/8] xfs: remove xlog_write_adv_cnt and simplify xlog_write_partial
  2021-06-16 16:32 log write cleanups Christoph Hellwig
                   ` (2 preceding siblings ...)
  2021-06-16 16:32 ` [PATCH 3/8] xfs: factor out a helper to write a log_iovec into the iclog Christoph Hellwig
@ 2021-06-16 16:32 ` Christoph Hellwig
  2021-06-17 16:23   ` Brian Foster
  2021-06-18  7:50   ` Chandan Babu R
  2021-06-16 16:32 ` [PATCH 5/8] xfs: remove xlog_verify_dest_ptr Christoph Hellwig
                   ` (4 subsequent siblings)
  8 siblings, 2 replies; 26+ messages in thread
From: Christoph Hellwig @ 2021-06-16 16:32 UTC (permalink / raw)
  To: linux-xfs

xlog_write_adv_cnt is now only used for writing the continuation ophdr.
Remove xlog_write_adv_cnt and simplify the caller now that we don't need
the ptr iteration variable, and don't need to increment / decrement
len for the accounting shengians.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log.c      | 12 +++++-------
 fs/xfs/xfs_log_priv.h |  8 --------
 2 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 5b431d53287d2c..1bc32f056a5bcf 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2331,24 +2331,22 @@ xlog_write_partial(
 			 * a new iclog. This is necessary so that we reserve
 			 * space in the iclog for it.
 			 */
-			*len += sizeof(struct xlog_op_header);
 			ticket->t_curr_res -= sizeof(struct xlog_op_header);
 
 			error = xlog_write_get_more_iclog_space(log, ticket,
-					&iclog, log_offset, *len, record_cnt,
-					data_cnt);
+					&iclog, log_offset,
+					*len + sizeof(struct xlog_op_header),
+					record_cnt, data_cnt);
 			if (error)
 				return ERR_PTR(error);
-			ptr = iclog->ic_datap + *log_offset;
 
-			ophdr = ptr;
+			ophdr = iclog->ic_datap + *log_offset;
 			ophdr->oh_tid = cpu_to_be32(ticket->t_tid);
 			ophdr->oh_clientid = XFS_TRANSACTION;
 			ophdr->oh_res2 = 0;
 			ophdr->oh_flags = XLOG_WAS_CONT_TRANS;
 
-			xlog_write_adv_cnt(&ptr, len, log_offset,
-						sizeof(struct xlog_op_header));
+			*log_offset += sizeof(struct xlog_op_header);
 			*data_cnt += sizeof(struct xlog_op_header);
 
 			/*
diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index 96dbe713954f7e..1b3b3d2bb8a5d1 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -467,14 +467,6 @@ extern kmem_zone_t *xfs_log_ticket_zone;
 struct xlog_ticket *xlog_ticket_alloc(struct xlog *log, int unit_bytes,
 		int count, bool permanent);
 
-static inline void
-xlog_write_adv_cnt(void **ptr, int *len, int *off, size_t bytes)
-{
-	*ptr += bytes;
-	*len -= bytes;
-	*off += bytes;
-}
-
 void	xlog_print_tic_res(struct xfs_mount *mp, struct xlog_ticket *ticket);
 void	xlog_print_trans(struct xfs_trans *);
 int	xlog_write(struct xlog *log, struct list_head *lv_chain,
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 5/8] xfs: remove xlog_verify_dest_ptr
  2021-06-16 16:32 log write cleanups Christoph Hellwig
                   ` (3 preceding siblings ...)
  2021-06-16 16:32 ` [PATCH 4/8] xfs: remove xlog_write_adv_cnt and simplify xlog_write_partial Christoph Hellwig
@ 2021-06-16 16:32 ` Christoph Hellwig
  2021-06-17 16:24   ` Brian Foster
  2021-06-18 10:31   ` Chandan Babu R
  2021-06-16 16:32 ` [PATCH 6/8] xfs: simplify the xlog_write_partial calling conventions Christoph Hellwig
                   ` (3 subsequent siblings)
  8 siblings, 2 replies; 26+ messages in thread
From: Christoph Hellwig @ 2021-06-16 16:32 UTC (permalink / raw)
  To: linux-xfs

Just check that the offset in xlog_write_vec is smaller than the iclog
size and remove the expensive cycling through all iclogs.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log.c      | 35 +----------------------------------
 fs/xfs/xfs_log_priv.h |  4 ----
 2 files changed, 1 insertion(+), 38 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 1bc32f056a5bcf..5d55d4fff63035 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -59,10 +59,6 @@ xlog_sync(
 	struct xlog_ticket	*ticket);
 #if defined(DEBUG)
 STATIC void
-xlog_verify_dest_ptr(
-	struct xlog		*log,
-	void			*ptr);
-STATIC void
 xlog_verify_grant_tail(
 	struct xlog *log);
 STATIC void
@@ -76,7 +72,6 @@ xlog_verify_tail_lsn(
 	struct xlog_in_core	*iclog,
 	xfs_lsn_t		tail_lsn);
 #else
-#define xlog_verify_dest_ptr(a,b)
 #define xlog_verify_grant_tail(a)
 #define xlog_verify_iclog(a,b,c)
 #define xlog_verify_tail_lsn(a,b,c)
@@ -1501,9 +1496,6 @@ xlog_alloc_log(
 						KM_MAYFAIL | KM_ZERO);
 		if (!iclog->ic_data)
 			goto out_free_iclog;
-#ifdef DEBUG
-		log->l_iclog_bak[i] = &iclog->ic_header;
-#endif
 		head = &iclog->ic_header;
 		memset(head, 0, sizeof(xlog_rec_header_t));
 		head->h_magicno = cpu_to_be32(XLOG_HEADER_MAGIC_NUM);
@@ -2134,6 +2126,7 @@ xlog_write_iovec(
 	uint32_t		*record_cnt,
 	uint32_t		*data_cnt)
 {
+	ASSERT(*log_offset < iclog->ic_log->l_iclog_size);
 	ASSERT(*log_offset % sizeof(int32_t) == 0);
 	ASSERT(write_len % sizeof(int32_t) == 0);
 
@@ -2258,7 +2251,6 @@ xlog_write_partial(
 	struct xfs_log_vec	*lv = log_vector;
 	struct xfs_log_iovec	*reg;
 	struct xlog_op_header	*ophdr;
-	void			*ptr;
 	int			index = 0;
 	uint32_t		rlen;
 	int			error;
@@ -2297,7 +2289,6 @@ xlog_write_partial(
 		if (rlen != reg->i_len)
 			ophdr->oh_flags |= XLOG_CONTINUE_TRANS;
 
-		xlog_verify_dest_ptr(log, iclog->ic_datap + *log_offset);
 		xlog_write_iovec(iclog, log_offset, reg->i_addr, rlen, len,
 				 record_cnt, data_cnt);
 
@@ -2363,7 +2354,6 @@ xlog_write_partial(
 			rlen = min_t(uint32_t, rlen, iclog->ic_size - *log_offset);
 			ophdr->oh_len = cpu_to_be32(rlen);
 
-			xlog_verify_dest_ptr(log, iclog->ic_datap + *log_offset);
 			xlog_write_iovec(iclog, log_offset,
 					 reg->i_addr + reg_offset, rlen, len,
 					 record_cnt, data_cnt);
@@ -3466,29 +3456,6 @@ xlog_ticket_alloc(
 }
 
 #if defined(DEBUG)
-/*
- * Make sure that the destination ptr is within the valid data region of
- * one of the iclogs.  This uses backup pointers stored in a different
- * part of the log in case we trash the log structure.
- */
-STATIC void
-xlog_verify_dest_ptr(
-	struct xlog	*log,
-	void		*ptr)
-{
-	int i;
-	int good_ptr = 0;
-
-	for (i = 0; i < log->l_iclog_bufs; i++) {
-		if (ptr >= log->l_iclog_bak[i] &&
-		    ptr <= log->l_iclog_bak[i] + log->l_iclog_size)
-			good_ptr++;
-	}
-
-	if (!good_ptr)
-		xfs_emerg(log->l_mp, "%s: invalid ptr", __func__);
-}
-
 /*
  * Check to make sure the grant write head didn't just over lap the tail.  If
  * the cycles are the same, we can't be overlapping.  Otherwise, make sure that
diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index 1b3b3d2bb8a5d1..b829d8ba5c6a3f 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -434,10 +434,6 @@ struct xlog {
 
 	struct xfs_kobj		l_kobj;
 
-	/* The following field are used for debugging; need to hold icloglock */
-#ifdef DEBUG
-	void			*l_iclog_bak[XLOG_MAX_ICLOGS];
-#endif
 	/* log recovery lsn tracking (for buffer submission */
 	xfs_lsn_t		l_recovery_lsn;
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 6/8] xfs: simplify the xlog_write_partial calling conventions
  2021-06-16 16:32 log write cleanups Christoph Hellwig
                   ` (4 preceding siblings ...)
  2021-06-16 16:32 ` [PATCH 5/8] xfs: remove xlog_verify_dest_ptr Christoph Hellwig
@ 2021-06-16 16:32 ` Christoph Hellwig
  2021-06-17 16:24   ` Brian Foster
  2021-06-18 11:07   ` Chandan Babu R
  2021-06-16 16:32 ` [PATCH 7/8] xfs: factor out a xlog_write_full_log_vec helper Christoph Hellwig
                   ` (2 subsequent siblings)
  8 siblings, 2 replies; 26+ messages in thread
From: Christoph Hellwig @ 2021-06-16 16:32 UTC (permalink / raw)
  To: linux-xfs

Lift the iteration to the next log_vec into the callers, and drop
the pointless log argument that can be trivially derived.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log.c | 38 +++++++++++++++-----------------------
 1 file changed, 15 insertions(+), 23 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 5d55d4fff63035..4afa8ff1a82076 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2232,14 +2232,11 @@ xlog_write_get_more_iclog_space(
 /*
  * Write log vectors into a single iclog which is smaller than the current chain
  * length. We write until we cannot fit a full record into the remaining space
- * and then stop. We return the log vector that is to be written that cannot
- * wholly fit in the iclog.
+ * and then stop.
  */
-static struct xfs_log_vec *
+static int
 xlog_write_partial(
-	struct xlog		*log,
-	struct list_head	*lv_chain,
-	struct xfs_log_vec	*log_vector,
+	struct xfs_log_vec	*lv,
 	struct xlog_ticket	*ticket,
 	struct xlog_in_core	**iclogp,
 	uint32_t		*log_offset,
@@ -2248,8 +2245,7 @@ xlog_write_partial(
 	uint32_t		*data_cnt)
 {
 	struct xlog_in_core	*iclog = *iclogp;
-	struct xfs_log_vec	*lv = log_vector;
-	struct xfs_log_iovec	*reg;
+	struct xlog		*log = iclog->ic_log;
 	struct xlog_op_header	*ophdr;
 	int			index = 0;
 	uint32_t		rlen;
@@ -2257,9 +2253,8 @@ xlog_write_partial(
 
 	/* walk the logvec, copying until we run out of space in the iclog */
 	for (index = 0; index < lv->lv_niovecs; index++) {
-		uint32_t	reg_offset = 0;
-
-		reg = &lv->lv_iovecp[index];
+		struct xfs_log_iovec	*reg = &lv->lv_iovecp[index];
+		uint32_t		reg_offset = 0;
 
 		/*
 		 * The first region of a continuation must have a non-zero
@@ -2278,7 +2273,7 @@ xlog_write_partial(
 					&iclog, log_offset, *len, record_cnt,
 					data_cnt);
 			if (error)
-				return ERR_PTR(error);
+				return error;
 		}
 
 		ophdr = reg->i_addr;
@@ -2329,7 +2324,7 @@ xlog_write_partial(
 					*len + sizeof(struct xlog_op_header),
 					record_cnt, data_cnt);
 			if (error)
-				return ERR_PTR(error);
+				return error;
 
 			ophdr = iclog->ic_datap + *log_offset;
 			ophdr->oh_tid = cpu_to_be32(ticket->t_tid);
@@ -2365,10 +2360,7 @@ xlog_write_partial(
 	 * the caller so it can go back to fast path copying.
 	 */
 	*iclogp = iclog;
-	lv = list_next_entry(lv, lv_list);
-	if (list_entry_is_head(lv, lv_chain, lv_list))
-		return NULL;
-	return lv;
+	return 0;
 }
 
 /*
@@ -2450,13 +2442,13 @@ xlog_write(
 		if (!lv)
 			break;
 
-		lv = xlog_write_partial(log, lv_chain, lv, ticket, &iclog,
-					&log_offset, &len, &record_cnt,
-					&data_cnt);
-		if (IS_ERR_OR_NULL(lv)) {
-			error = PTR_ERR_OR_ZERO(lv);
+		error = xlog_write_partial(lv, ticket, &iclog, &log_offset,
+					   &len, &record_cnt, &data_cnt);
+		if (error)
+			break;
+		lv = list_next_entry(lv, lv_list);
+		if (list_entry_is_head(lv, lv_chain, lv_list))
 			break;
-		}
 	}
 	ASSERT((len == 0 && !lv) || error);
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 7/8] xfs: factor out a xlog_write_full_log_vec helper
  2021-06-16 16:32 log write cleanups Christoph Hellwig
                   ` (5 preceding siblings ...)
  2021-06-16 16:32 ` [PATCH 6/8] xfs: simplify the xlog_write_partial calling conventions Christoph Hellwig
@ 2021-06-16 16:32 ` Christoph Hellwig
  2021-06-17 16:24   ` Brian Foster
  2021-06-16 16:32 ` [PATCH 8/8] xfs: simplify the list iteration in xlog_write Christoph Hellwig
  2021-06-18 22:51 ` log write cleanups Dave Chinner
  8 siblings, 1 reply; 26+ messages in thread
From: Christoph Hellwig @ 2021-06-16 16:32 UTC (permalink / raw)
  To: linux-xfs

Add a helper to write out an entire log_vec to prepare for additional
cleanups.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log.c | 61 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 40 insertions(+), 21 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 4afa8ff1a82076..f8df09f37c3b84 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2137,6 +2137,44 @@ xlog_write_iovec(
 	*data_cnt += write_len;
 }
 
+/*
+ * Write a whole log vector into a single iclog which is guaranteed to have
+ * either sufficient space for the entire log vector chain to be written or
+ * exclusive access to the remaining space in the iclog.
+ *
+ * Return the number of iovecs and data written into the iclog.
+ */
+static void
+xlog_write_full(
+	struct xfs_log_vec	*lv,
+	struct xlog_ticket	*ticket,
+	struct xlog_in_core	*iclog,
+	uint32_t		*log_offset,
+	uint32_t		*len,
+	uint32_t		*record_cnt,
+	uint32_t		*data_cnt)
+{
+	int			i;
+
+	ASSERT(*log_offset + *len <= iclog->ic_size ||
+		iclog->ic_state == XLOG_STATE_WANT_SYNC);
+
+	/*
+	 * Ordered log vectors have no regions to write so this loop will
+	 * naturally skip them.
+	 */
+	for (i = 0; i < lv->lv_niovecs; i++) {
+		struct xfs_log_iovec	*reg = &lv->lv_iovecp[i];
+		struct xlog_op_header	*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));
+		xlog_write_iovec(iclog, log_offset, reg->i_addr, reg->i_len,
+				 len, record_cnt, data_cnt);
+	}
+}
+
 /*
  * Write whole log vectors into a single iclog which is guaranteed to have
  * either sufficient space for the entire log vector chain to be written or
@@ -2158,10 +2196,6 @@ xlog_write_single(
 	uint32_t		*data_cnt)
 {
 	struct xfs_log_vec	*lv;
-	int			index;
-
-	ASSERT(*log_offset + *len <= iclog->ic_size ||
-		iclog->ic_state == XLOG_STATE_WANT_SYNC);
 
 	for (lv = log_vector;
 	     !list_entry_is_head(lv, lv_chain, lv_list);
@@ -2173,23 +2207,8 @@ xlog_write_single(
 		if (lv->lv_niovecs &&
 		    lv->lv_bytes > iclog->ic_size - *log_offset)
 			break;
-
-		/*
-		 * Ordered log vectors have no regions to write so this
-		 * loop will naturally skip them.
-		 */
-		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;
-
-			ophdr->oh_tid = cpu_to_be32(ticket->t_tid);
-			ophdr->oh_len = cpu_to_be32(reg->i_len -
-						sizeof(struct xlog_op_header));
-
-			xlog_write_iovec(iclog, log_offset, reg->i_addr,
-					 reg->i_len, len, record_cnt,
-					 data_cnt);
-		}
+		xlog_write_full(lv, ticket, iclog, log_offset, len, record_cnt,
+				data_cnt);
 	}
 	if (list_entry_is_head(lv, lv_chain, lv_list))
 		lv = NULL;
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 8/8] xfs: simplify the list iteration in xlog_write
  2021-06-16 16:32 log write cleanups Christoph Hellwig
                   ` (6 preceding siblings ...)
  2021-06-16 16:32 ` [PATCH 7/8] xfs: factor out a xlog_write_full_log_vec helper Christoph Hellwig
@ 2021-06-16 16:32 ` Christoph Hellwig
  2021-06-17 16:24   ` Brian Foster
  2021-06-18 22:51 ` log write cleanups Dave Chinner
  8 siblings, 1 reply; 26+ messages in thread
From: Christoph Hellwig @ 2021-06-16 16:32 UTC (permalink / raw)
  To: linux-xfs

Just use a single list_for_each_entry in xlog_write which then
dispatches to the simple or partial cases instead of using nested
loops.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log.c | 73 +++++++++++-------------------------------------
 1 file changed, 17 insertions(+), 56 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index f8df09f37c3b84..365914c25ff0f0 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2175,47 +2175,6 @@ xlog_write_full(
 	}
 }
 
-/*
- * Write whole log vectors into a single iclog which is guaranteed to have
- * either sufficient space for the entire log vector chain to be written or
- * exclusive access to the remaining space in the iclog.
- *
- * Return the number of iovecs and data written into the iclog, as well as
- * a pointer to the logvec that doesn't fit in the log (or NULL if we hit the
- * end of the chain.
- */
-static struct xfs_log_vec *
-xlog_write_single(
-	struct list_head	*lv_chain,
-	struct xfs_log_vec	*log_vector,
-	struct xlog_ticket	*ticket,
-	struct xlog_in_core	*iclog,
-	uint32_t		*log_offset,
-	uint32_t		*len,
-	uint32_t		*record_cnt,
-	uint32_t		*data_cnt)
-{
-	struct xfs_log_vec	*lv;
-
-	for (lv = log_vector;
-	     !list_entry_is_head(lv, lv_chain, lv_list);
-	     lv = list_next_entry(lv, lv_list)) {
-		/*
-		 * If the entire log vec does not fit in the iclog, punt it to
-		 * the partial copy loop which can handle this case.
-		 */
-		if (lv->lv_niovecs &&
-		    lv->lv_bytes > iclog->ic_size - *log_offset)
-			break;
-		xlog_write_full(lv, ticket, iclog, log_offset, len, record_cnt,
-				data_cnt);
-	}
-	if (list_entry_is_head(lv, lv_chain, lv_list))
-		lv = NULL;
-	ASSERT(*len == 0 || lv);
-	return lv;
-}
-
 static int
 xlog_write_get_more_iclog_space(
 	struct xlog		*log,
@@ -2454,22 +2413,24 @@ xlog_write(
 	if (start_lsn)
 		*start_lsn = be64_to_cpu(iclog->ic_header.h_lsn);
 
-	lv = list_first_entry_or_null(lv_chain, struct xfs_log_vec, lv_list);
-	while (lv) {
-		lv = xlog_write_single(lv_chain, lv, ticket, iclog, &log_offset,
-					&len, &record_cnt, &data_cnt);
-		if (!lv)
-			break;
-
-		error = xlog_write_partial(lv, ticket, &iclog, &log_offset,
-					   &len, &record_cnt, &data_cnt);
-		if (error)
-			break;
-		lv = list_next_entry(lv, lv_list);
-		if (list_entry_is_head(lv, lv_chain, lv_list))
-			break;
+	list_for_each_entry(lv, lv_chain, lv_list) {
+		/*
+		 * If the entire log vec does not fit in the iclog, punt it to
+		 * the partial copy loop which can handle this case.
+		 */
+		if (lv->lv_niovecs &&
+		    lv->lv_bytes > iclog->ic_size - log_offset) {
+			error = xlog_write_partial(lv, ticket, &iclog,
+						   &log_offset, &len,
+						   &record_cnt, &data_cnt);
+			if (error)
+				break;
+		} else {
+			xlog_write_full(lv, ticket, iclog, &log_offset, &len,
+					&record_cnt, &data_cnt);
+		}
 	}
-	ASSERT((len == 0 && !lv) || error);
+	ASSERT(len == 0 || error);
 
 	/*
 	 * We've already been guaranteed that the last writes will fit inside
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/8] xfs: change the type of ic_datap
  2021-06-16 16:32 ` [PATCH 1/8] xfs: change the type of ic_datap Christoph Hellwig
@ 2021-06-17 16:22   ` Brian Foster
  2021-06-18  5:52   ` Chandan Babu R
  1 sibling, 0 replies; 26+ messages in thread
From: Brian Foster @ 2021-06-17 16:22 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs

On Wed, Jun 16, 2021 at 06:32:05PM +0200, Christoph Hellwig wrote:
> Turn ic_datap from a char into a void pointer given that it points
> to arbitrary data.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_log.c      | 2 +-
>  fs/xfs/xfs_log_priv.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index e921b554b68367..8999c78f3ac6d9 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -3613,7 +3613,7 @@ xlog_verify_iclog(
>  		if (field_offset & 0x1ff) {
>  			clientid = ophead->oh_clientid;
>  		} else {
> -			idx = BTOBBT((char *)&ophead->oh_clientid - iclog->ic_datap);
> +			idx = BTOBBT((void *)&ophead->oh_clientid - iclog->ic_datap);
>  			if (idx >= (XLOG_HEADER_CYCLE_SIZE / BBSIZE)) {
>  				j = idx / (XLOG_HEADER_CYCLE_SIZE / BBSIZE);
>  				k = idx % (XLOG_HEADER_CYCLE_SIZE / BBSIZE);
> diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> index e4e421a7033558..96dbe713954f7e 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -185,7 +185,7 @@ typedef struct xlog_in_core {
>  	u32			ic_offset;
>  	enum xlog_iclog_state	ic_state;
>  	unsigned int		ic_flags;
> -	char			*ic_datap;	/* pointer to iclog data */
> +	void			*ic_datap;	/* pointer to iclog data */
>  
>  	/* Callback structures need their own cacheline */
>  	spinlock_t		ic_callback_lock ____cacheline_aligned_in_smp;
> -- 
> 2.30.2
> 


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/8] xfs: list entry elements don't need to be initialized
  2021-06-16 16:32 ` [PATCH 2/8] xfs: list entry elements don't need to be initialized Christoph Hellwig
@ 2021-06-17 16:23   ` Brian Foster
  2021-06-18  5:59   ` Chandan Babu R
  1 sibling, 0 replies; 26+ messages in thread
From: Brian Foster @ 2021-06-17 16:23 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs

On Wed, Jun 16, 2021 at 06:32:06PM +0200, Christoph Hellwig wrote:
> list_add does not require the added element to be initialized.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_log.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 8999c78f3ac6d9..32cb0fc459a364 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -851,7 +851,7 @@ xlog_write_unmount_record(
>  		.lv_iovecp = &reg,
>  	};
>  	LIST_HEAD(lv_chain);
> -	INIT_LIST_HEAD(&vec.lv_list);
> +
>  	list_add(&vec.lv_list, &lv_chain);
>  
>  	BUILD_BUG_ON((sizeof(struct xlog_op_header) +
> @@ -1587,7 +1587,7 @@ xlog_commit_record(
>  	};
>  	int	error;
>  	LIST_HEAD(lv_chain);
> -	INIT_LIST_HEAD(&vec.lv_list);
> +
>  	list_add(&vec.lv_list, &lv_chain);
>  
>  	if (XLOG_FORCED_SHUTDOWN(log))
> -- 
> 2.30.2
> 


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/8] xfs: factor out a helper to write a log_iovec into the iclog
  2021-06-16 16:32 ` [PATCH 3/8] xfs: factor out a helper to write a log_iovec into the iclog Christoph Hellwig
@ 2021-06-17 16:23   ` Brian Foster
  2021-06-18 13:24     ` Christoph Hellwig
  2021-06-18  7:42   ` Chandan Babu R
  1 sibling, 1 reply; 26+ messages in thread
From: Brian Foster @ 2021-06-17 16:23 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs

On Wed, Jun 16, 2021 at 06:32:07PM +0200, Christoph Hellwig wrote:
> Add a new helper to copy the log iovec into the in-core log buffer,
> and open code the handling continuation opheader as a special case.
> 

What do you mean by "open code the handling continuation opheader?"

Otherwise looks reasonable to me:

Reviewed-by: Brian Foster <bfoster@redhat.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_log.c | 55 +++++++++++++++++++++++++++---------------------
>  1 file changed, 31 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 32cb0fc459a364..5b431d53287d2c 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2124,6 +2124,26 @@ xlog_print_trans(
>  	}
>  }
>  
> +static inline void
> +xlog_write_iovec(
> +	struct xlog_in_core	*iclog,
> +	uint32_t		*log_offset,
> +	void			*data,
> +	uint32_t		write_len,
> +	int			*bytes_left,
> +	uint32_t		*record_cnt,
> +	uint32_t		*data_cnt)
> +{
> +	ASSERT(*log_offset % sizeof(int32_t) == 0);
> +	ASSERT(write_len % sizeof(int32_t) == 0);
> +
> +	memcpy(iclog->ic_datap + *log_offset, data, write_len);
> +	*log_offset += write_len;
> +	*bytes_left -= write_len;
> +	(*record_cnt)++;
> +	*data_cnt += write_len;
> +}
> +
>  /*
>   * Write whole log vectors into a single iclog which is guaranteed to have
>   * either sufficient space for the entire log vector chain to be written or
> @@ -2145,13 +2165,11 @@ xlog_write_single(
>  	uint32_t		*data_cnt)
>  {
>  	struct xfs_log_vec	*lv;
> -	void			*ptr;
>  	int			index;
>  
>  	ASSERT(*log_offset + *len <= iclog->ic_size ||
>  		iclog->ic_state == XLOG_STATE_WANT_SYNC);
>  
> -	ptr = iclog->ic_datap + *log_offset;
>  	for (lv = log_vector;
>  	     !list_entry_is_head(lv, lv_chain, lv_list);
>  	     lv = list_next_entry(lv, lv_list)) {
> @@ -2171,16 +2189,13 @@ xlog_write_single(
>  			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)++;
> -			*data_cnt += reg->i_len;
> +
> +			xlog_write_iovec(iclog, log_offset, reg->i_addr,
> +					 reg->i_len, len, record_cnt,
> +					 data_cnt);
>  		}
>  	}
>  	if (list_entry_is_head(lv, lv_chain, lv_list))
> @@ -2249,12 +2264,10 @@ xlog_write_partial(
>  	int			error;
>  
>  	/* walk the logvec, copying until we run out of space in the iclog */
> -	ptr = iclog->ic_datap + *log_offset;
>  	for (index = 0; index < lv->lv_niovecs; index++) {
>  		uint32_t	reg_offset = 0;
>  
>  		reg = &lv->lv_iovecp[index];
> -		ASSERT(reg->i_len % sizeof(int32_t) == 0);
>  
>  		/*
>  		 * The first region of a continuation must have a non-zero
> @@ -2274,7 +2287,6 @@ xlog_write_partial(
>  					data_cnt);
>  			if (error)
>  				return ERR_PTR(error);
> -			ptr = iclog->ic_datap + *log_offset;
>  		}
>  
>  		ophdr = reg->i_addr;
> @@ -2285,12 +2297,9 @@ xlog_write_partial(
>  		if (rlen != reg->i_len)
>  			ophdr->oh_flags |= XLOG_CONTINUE_TRANS;
>  
> -		ASSERT((unsigned long)ptr % sizeof(int32_t) == 0);
> -		xlog_verify_dest_ptr(log, ptr);
> -		memcpy(ptr, reg->i_addr, rlen);
> -		xlog_write_adv_cnt(&ptr, len, log_offset, rlen);
> -		(*record_cnt)++;
> -		*data_cnt += rlen;
> +		xlog_verify_dest_ptr(log, iclog->ic_datap + *log_offset);
> +		xlog_write_iovec(iclog, log_offset, reg->i_addr, rlen, len,
> +				 record_cnt, data_cnt);
>  
>  		/* If we wrote the whole region, move to the next. */
>  		if (rlen == reg->i_len)
> @@ -2356,12 +2365,10 @@ xlog_write_partial(
>  			rlen = min_t(uint32_t, rlen, iclog->ic_size - *log_offset);
>  			ophdr->oh_len = cpu_to_be32(rlen);
>  
> -			xlog_verify_dest_ptr(log, ptr);
> -			memcpy(ptr, reg->i_addr + reg_offset, rlen);
> -			xlog_write_adv_cnt(&ptr, len, log_offset, rlen);
> -			(*record_cnt)++;
> -			*data_cnt += rlen;
> -
> +			xlog_verify_dest_ptr(log, iclog->ic_datap + *log_offset);
> +			xlog_write_iovec(iclog, log_offset,
> +					 reg->i_addr + reg_offset, rlen, len,
> +					 record_cnt, data_cnt);
>  		} while (ophdr->oh_flags & XLOG_CONTINUE_TRANS);
>  	}
>  
> -- 
> 2.30.2
> 


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/8] xfs: remove xlog_write_adv_cnt and simplify xlog_write_partial
  2021-06-16 16:32 ` [PATCH 4/8] xfs: remove xlog_write_adv_cnt and simplify xlog_write_partial Christoph Hellwig
@ 2021-06-17 16:23   ` Brian Foster
  2021-06-18  7:50   ` Chandan Babu R
  1 sibling, 0 replies; 26+ messages in thread
From: Brian Foster @ 2021-06-17 16:23 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs

On Wed, Jun 16, 2021 at 06:32:08PM +0200, Christoph Hellwig wrote:
> xlog_write_adv_cnt is now only used for writing the continuation ophdr.
> Remove xlog_write_adv_cnt and simplify the caller now that we don't need
> the ptr iteration variable, and don't need to increment / decrement
> len for the accounting shengians.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

fs/xfs/xfs_log.c: In function ‘xlog_write_partial’:
fs/xfs/xfs_log.c:2261:10: warning: unused variable ‘ptr’ [-Wunused-variable]

With that fixed:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_log.c      | 12 +++++-------
>  fs/xfs/xfs_log_priv.h |  8 --------
>  2 files changed, 5 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 5b431d53287d2c..1bc32f056a5bcf 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2331,24 +2331,22 @@ xlog_write_partial(
>  			 * a new iclog. This is necessary so that we reserve
>  			 * space in the iclog for it.
>  			 */
> -			*len += sizeof(struct xlog_op_header);
>  			ticket->t_curr_res -= sizeof(struct xlog_op_header);
>  
>  			error = xlog_write_get_more_iclog_space(log, ticket,
> -					&iclog, log_offset, *len, record_cnt,
> -					data_cnt);
> +					&iclog, log_offset,
> +					*len + sizeof(struct xlog_op_header),
> +					record_cnt, data_cnt);
>  			if (error)
>  				return ERR_PTR(error);
> -			ptr = iclog->ic_datap + *log_offset;
>  
> -			ophdr = ptr;
> +			ophdr = iclog->ic_datap + *log_offset;
>  			ophdr->oh_tid = cpu_to_be32(ticket->t_tid);
>  			ophdr->oh_clientid = XFS_TRANSACTION;
>  			ophdr->oh_res2 = 0;
>  			ophdr->oh_flags = XLOG_WAS_CONT_TRANS;
>  
> -			xlog_write_adv_cnt(&ptr, len, log_offset,
> -						sizeof(struct xlog_op_header));
> +			*log_offset += sizeof(struct xlog_op_header);
>  			*data_cnt += sizeof(struct xlog_op_header);
>  
>  			/*
> diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> index 96dbe713954f7e..1b3b3d2bb8a5d1 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -467,14 +467,6 @@ extern kmem_zone_t *xfs_log_ticket_zone;
>  struct xlog_ticket *xlog_ticket_alloc(struct xlog *log, int unit_bytes,
>  		int count, bool permanent);
>  
> -static inline void
> -xlog_write_adv_cnt(void **ptr, int *len, int *off, size_t bytes)
> -{
> -	*ptr += bytes;
> -	*len -= bytes;
> -	*off += bytes;
> -}
> -
>  void	xlog_print_tic_res(struct xfs_mount *mp, struct xlog_ticket *ticket);
>  void	xlog_print_trans(struct xfs_trans *);
>  int	xlog_write(struct xlog *log, struct list_head *lv_chain,
> -- 
> 2.30.2
> 


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 5/8] xfs: remove xlog_verify_dest_ptr
  2021-06-16 16:32 ` [PATCH 5/8] xfs: remove xlog_verify_dest_ptr Christoph Hellwig
@ 2021-06-17 16:24   ` Brian Foster
  2021-06-18 10:31   ` Chandan Babu R
  1 sibling, 0 replies; 26+ messages in thread
From: Brian Foster @ 2021-06-17 16:24 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs

On Wed, Jun 16, 2021 at 06:32:09PM +0200, Christoph Hellwig wrote:
> Just check that the offset in xlog_write_vec is smaller than the iclog
> size and remove the expensive cycling through all iclogs.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_log.c      | 35 +----------------------------------
>  fs/xfs/xfs_log_priv.h |  4 ----
>  2 files changed, 1 insertion(+), 38 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 1bc32f056a5bcf..5d55d4fff63035 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -59,10 +59,6 @@ xlog_sync(
>  	struct xlog_ticket	*ticket);
>  #if defined(DEBUG)
>  STATIC void
> -xlog_verify_dest_ptr(
> -	struct xlog		*log,
> -	void			*ptr);
> -STATIC void
>  xlog_verify_grant_tail(
>  	struct xlog *log);
>  STATIC void
> @@ -76,7 +72,6 @@ xlog_verify_tail_lsn(
>  	struct xlog_in_core	*iclog,
>  	xfs_lsn_t		tail_lsn);
>  #else
> -#define xlog_verify_dest_ptr(a,b)
>  #define xlog_verify_grant_tail(a)
>  #define xlog_verify_iclog(a,b,c)
>  #define xlog_verify_tail_lsn(a,b,c)
> @@ -1501,9 +1496,6 @@ xlog_alloc_log(
>  						KM_MAYFAIL | KM_ZERO);
>  		if (!iclog->ic_data)
>  			goto out_free_iclog;
> -#ifdef DEBUG
> -		log->l_iclog_bak[i] = &iclog->ic_header;
> -#endif
>  		head = &iclog->ic_header;
>  		memset(head, 0, sizeof(xlog_rec_header_t));
>  		head->h_magicno = cpu_to_be32(XLOG_HEADER_MAGIC_NUM);
> @@ -2134,6 +2126,7 @@ xlog_write_iovec(
>  	uint32_t		*record_cnt,
>  	uint32_t		*data_cnt)
>  {
> +	ASSERT(*log_offset < iclog->ic_log->l_iclog_size);
>  	ASSERT(*log_offset % sizeof(int32_t) == 0);
>  	ASSERT(write_len % sizeof(int32_t) == 0);
>  
> @@ -2258,7 +2251,6 @@ xlog_write_partial(
>  	struct xfs_log_vec	*lv = log_vector;
>  	struct xfs_log_iovec	*reg;
>  	struct xlog_op_header	*ophdr;
> -	void			*ptr;
>  	int			index = 0;
>  	uint32_t		rlen;
>  	int			error;
> @@ -2297,7 +2289,6 @@ xlog_write_partial(
>  		if (rlen != reg->i_len)
>  			ophdr->oh_flags |= XLOG_CONTINUE_TRANS;
>  
> -		xlog_verify_dest_ptr(log, iclog->ic_datap + *log_offset);
>  		xlog_write_iovec(iclog, log_offset, reg->i_addr, rlen, len,
>  				 record_cnt, data_cnt);
>  
> @@ -2363,7 +2354,6 @@ xlog_write_partial(
>  			rlen = min_t(uint32_t, rlen, iclog->ic_size - *log_offset);
>  			ophdr->oh_len = cpu_to_be32(rlen);
>  
> -			xlog_verify_dest_ptr(log, iclog->ic_datap + *log_offset);
>  			xlog_write_iovec(iclog, log_offset,
>  					 reg->i_addr + reg_offset, rlen, len,
>  					 record_cnt, data_cnt);
> @@ -3466,29 +3456,6 @@ xlog_ticket_alloc(
>  }
>  
>  #if defined(DEBUG)
> -/*
> - * Make sure that the destination ptr is within the valid data region of
> - * one of the iclogs.  This uses backup pointers stored in a different
> - * part of the log in case we trash the log structure.
> - */
> -STATIC void
> -xlog_verify_dest_ptr(
> -	struct xlog	*log,
> -	void		*ptr)
> -{
> -	int i;
> -	int good_ptr = 0;
> -
> -	for (i = 0; i < log->l_iclog_bufs; i++) {
> -		if (ptr >= log->l_iclog_bak[i] &&
> -		    ptr <= log->l_iclog_bak[i] + log->l_iclog_size)
> -			good_ptr++;
> -	}
> -
> -	if (!good_ptr)
> -		xfs_emerg(log->l_mp, "%s: invalid ptr", __func__);
> -}
> -
>  /*
>   * Check to make sure the grant write head didn't just over lap the tail.  If
>   * the cycles are the same, we can't be overlapping.  Otherwise, make sure that
> diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> index 1b3b3d2bb8a5d1..b829d8ba5c6a3f 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -434,10 +434,6 @@ struct xlog {
>  
>  	struct xfs_kobj		l_kobj;
>  
> -	/* The following field are used for debugging; need to hold icloglock */
> -#ifdef DEBUG
> -	void			*l_iclog_bak[XLOG_MAX_ICLOGS];
> -#endif
>  	/* log recovery lsn tracking (for buffer submission */
>  	xfs_lsn_t		l_recovery_lsn;
>  
> -- 
> 2.30.2
> 


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 6/8] xfs: simplify the xlog_write_partial calling conventions
  2021-06-16 16:32 ` [PATCH 6/8] xfs: simplify the xlog_write_partial calling conventions Christoph Hellwig
@ 2021-06-17 16:24   ` Brian Foster
  2021-06-18 11:07   ` Chandan Babu R
  1 sibling, 0 replies; 26+ messages in thread
From: Brian Foster @ 2021-06-17 16:24 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs

On Wed, Jun 16, 2021 at 06:32:10PM +0200, Christoph Hellwig wrote:
> Lift the iteration to the next log_vec into the callers, and drop
> the pointless log argument that can be trivially derived.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Heh, I actually had patches in my local tree very similar to these last
few to fix up the xlog_write() iteration logic. Nice cleanup overall:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_log.c | 38 +++++++++++++++-----------------------
>  1 file changed, 15 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 5d55d4fff63035..4afa8ff1a82076 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2232,14 +2232,11 @@ xlog_write_get_more_iclog_space(
>  /*
>   * Write log vectors into a single iclog which is smaller than the current chain
>   * length. We write until we cannot fit a full record into the remaining space
> - * and then stop. We return the log vector that is to be written that cannot
> - * wholly fit in the iclog.
> + * and then stop.
>   */
> -static struct xfs_log_vec *
> +static int
>  xlog_write_partial(
> -	struct xlog		*log,
> -	struct list_head	*lv_chain,
> -	struct xfs_log_vec	*log_vector,
> +	struct xfs_log_vec	*lv,
>  	struct xlog_ticket	*ticket,
>  	struct xlog_in_core	**iclogp,
>  	uint32_t		*log_offset,
> @@ -2248,8 +2245,7 @@ xlog_write_partial(
>  	uint32_t		*data_cnt)
>  {
>  	struct xlog_in_core	*iclog = *iclogp;
> -	struct xfs_log_vec	*lv = log_vector;
> -	struct xfs_log_iovec	*reg;
> +	struct xlog		*log = iclog->ic_log;
>  	struct xlog_op_header	*ophdr;
>  	int			index = 0;
>  	uint32_t		rlen;
> @@ -2257,9 +2253,8 @@ xlog_write_partial(
>  
>  	/* walk the logvec, copying until we run out of space in the iclog */
>  	for (index = 0; index < lv->lv_niovecs; index++) {
> -		uint32_t	reg_offset = 0;
> -
> -		reg = &lv->lv_iovecp[index];
> +		struct xfs_log_iovec	*reg = &lv->lv_iovecp[index];
> +		uint32_t		reg_offset = 0;
>  
>  		/*
>  		 * The first region of a continuation must have a non-zero
> @@ -2278,7 +2273,7 @@ xlog_write_partial(
>  					&iclog, log_offset, *len, record_cnt,
>  					data_cnt);
>  			if (error)
> -				return ERR_PTR(error);
> +				return error;
>  		}
>  
>  		ophdr = reg->i_addr;
> @@ -2329,7 +2324,7 @@ xlog_write_partial(
>  					*len + sizeof(struct xlog_op_header),
>  					record_cnt, data_cnt);
>  			if (error)
> -				return ERR_PTR(error);
> +				return error;
>  
>  			ophdr = iclog->ic_datap + *log_offset;
>  			ophdr->oh_tid = cpu_to_be32(ticket->t_tid);
> @@ -2365,10 +2360,7 @@ xlog_write_partial(
>  	 * the caller so it can go back to fast path copying.
>  	 */
>  	*iclogp = iclog;
> -	lv = list_next_entry(lv, lv_list);
> -	if (list_entry_is_head(lv, lv_chain, lv_list))
> -		return NULL;
> -	return lv;
> +	return 0;
>  }
>  
>  /*
> @@ -2450,13 +2442,13 @@ xlog_write(
>  		if (!lv)
>  			break;
>  
> -		lv = xlog_write_partial(log, lv_chain, lv, ticket, &iclog,
> -					&log_offset, &len, &record_cnt,
> -					&data_cnt);
> -		if (IS_ERR_OR_NULL(lv)) {
> -			error = PTR_ERR_OR_ZERO(lv);
> +		error = xlog_write_partial(lv, ticket, &iclog, &log_offset,
> +					   &len, &record_cnt, &data_cnt);
> +		if (error)
> +			break;
> +		lv = list_next_entry(lv, lv_list);
> +		if (list_entry_is_head(lv, lv_chain, lv_list))
>  			break;
> -		}
>  	}
>  	ASSERT((len == 0 && !lv) || error);
>  
> -- 
> 2.30.2
> 


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 7/8] xfs: factor out a xlog_write_full_log_vec helper
  2021-06-16 16:32 ` [PATCH 7/8] xfs: factor out a xlog_write_full_log_vec helper Christoph Hellwig
@ 2021-06-17 16:24   ` Brian Foster
  2021-06-18 13:33     ` Christoph Hellwig
  0 siblings, 1 reply; 26+ messages in thread
From: Brian Foster @ 2021-06-17 16:24 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs

On Wed, Jun 16, 2021 at 06:32:11PM +0200, Christoph Hellwig wrote:
> Add a helper to write out an entire log_vec to prepare for additional
> cleanups.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_log.c | 61 +++++++++++++++++++++++++++++++-----------------
>  1 file changed, 40 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 4afa8ff1a82076..f8df09f37c3b84 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2137,6 +2137,44 @@ xlog_write_iovec(
>  	*data_cnt += write_len;
>  }
>  
> +/*
> + * Write a whole log vector into a single iclog which is guaranteed to have
> + * either sufficient space for the entire log vector chain to be written or
> + * exclusive access to the remaining space in the iclog.
> + *
> + * Return the number of iovecs and data written into the iclog.
> + */
> +static void
> +xlog_write_full(
> +	struct xfs_log_vec	*lv,
> +	struct xlog_ticket	*ticket,
> +	struct xlog_in_core	*iclog,
> +	uint32_t		*log_offset,
> +	uint32_t		*len,
> +	uint32_t		*record_cnt,
> +	uint32_t		*data_cnt)
> +{
> +	int			i;
> +
> +	ASSERT(*log_offset + *len <= iclog->ic_size ||
> +		iclog->ic_state == XLOG_STATE_WANT_SYNC);

I suspect this could be lifted into the original caller (xlog_write())
since it doesn't have much to do with the current lv (and this is
eventually no longer called for each lv in the chain)..? Otherwise the
patch LGTM.

Brian

> +
> +	/*
> +	 * Ordered log vectors have no regions to write so this loop will
> +	 * naturally skip them.
> +	 */
> +	for (i = 0; i < lv->lv_niovecs; i++) {
> +		struct xfs_log_iovec	*reg = &lv->lv_iovecp[i];
> +		struct xlog_op_header	*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));
> +		xlog_write_iovec(iclog, log_offset, reg->i_addr, reg->i_len,
> +				 len, record_cnt, data_cnt);
> +	}
> +}
> +
>  /*
>   * Write whole log vectors into a single iclog which is guaranteed to have
>   * either sufficient space for the entire log vector chain to be written or
> @@ -2158,10 +2196,6 @@ xlog_write_single(
>  	uint32_t		*data_cnt)
>  {
>  	struct xfs_log_vec	*lv;
> -	int			index;
> -
> -	ASSERT(*log_offset + *len <= iclog->ic_size ||
> -		iclog->ic_state == XLOG_STATE_WANT_SYNC);
>  
>  	for (lv = log_vector;
>  	     !list_entry_is_head(lv, lv_chain, lv_list);
> @@ -2173,23 +2207,8 @@ xlog_write_single(
>  		if (lv->lv_niovecs &&
>  		    lv->lv_bytes > iclog->ic_size - *log_offset)
>  			break;
> -
> -		/*
> -		 * Ordered log vectors have no regions to write so this
> -		 * loop will naturally skip them.
> -		 */
> -		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;
> -
> -			ophdr->oh_tid = cpu_to_be32(ticket->t_tid);
> -			ophdr->oh_len = cpu_to_be32(reg->i_len -
> -						sizeof(struct xlog_op_header));
> -
> -			xlog_write_iovec(iclog, log_offset, reg->i_addr,
> -					 reg->i_len, len, record_cnt,
> -					 data_cnt);
> -		}
> +		xlog_write_full(lv, ticket, iclog, log_offset, len, record_cnt,
> +				data_cnt);
>  	}
>  	if (list_entry_is_head(lv, lv_chain, lv_list))
>  		lv = NULL;
> -- 
> 2.30.2
> 


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 8/8] xfs: simplify the list iteration in xlog_write
  2021-06-16 16:32 ` [PATCH 8/8] xfs: simplify the list iteration in xlog_write Christoph Hellwig
@ 2021-06-17 16:24   ` Brian Foster
  0 siblings, 0 replies; 26+ messages in thread
From: Brian Foster @ 2021-06-17 16:24 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs

On Wed, Jun 16, 2021 at 06:32:12PM +0200, Christoph Hellwig wrote:
> Just use a single list_for_each_entry in xlog_write which then
> dispatches to the simple or partial cases instead of using nested
> loops.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_log.c | 73 +++++++++++-------------------------------------
>  1 file changed, 17 insertions(+), 56 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index f8df09f37c3b84..365914c25ff0f0 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2175,47 +2175,6 @@ xlog_write_full(
>  	}
>  }
>  
> -/*
> - * Write whole log vectors into a single iclog which is guaranteed to have
> - * either sufficient space for the entire log vector chain to be written or
> - * exclusive access to the remaining space in the iclog.
> - *
> - * Return the number of iovecs and data written into the iclog, as well as
> - * a pointer to the logvec that doesn't fit in the log (or NULL if we hit the
> - * end of the chain.
> - */
> -static struct xfs_log_vec *
> -xlog_write_single(
> -	struct list_head	*lv_chain,
> -	struct xfs_log_vec	*log_vector,
> -	struct xlog_ticket	*ticket,
> -	struct xlog_in_core	*iclog,
> -	uint32_t		*log_offset,
> -	uint32_t		*len,
> -	uint32_t		*record_cnt,
> -	uint32_t		*data_cnt)
> -{
> -	struct xfs_log_vec	*lv;
> -
> -	for (lv = log_vector;
> -	     !list_entry_is_head(lv, lv_chain, lv_list);
> -	     lv = list_next_entry(lv, lv_list)) {
> -		/*
> -		 * If the entire log vec does not fit in the iclog, punt it to
> -		 * the partial copy loop which can handle this case.
> -		 */
> -		if (lv->lv_niovecs &&
> -		    lv->lv_bytes > iclog->ic_size - *log_offset)
> -			break;
> -		xlog_write_full(lv, ticket, iclog, log_offset, len, record_cnt,
> -				data_cnt);
> -	}
> -	if (list_entry_is_head(lv, lv_chain, lv_list))
> -		lv = NULL;
> -	ASSERT(*len == 0 || lv);
> -	return lv;
> -}
> -
>  static int
>  xlog_write_get_more_iclog_space(
>  	struct xlog		*log,
> @@ -2454,22 +2413,24 @@ xlog_write(
>  	if (start_lsn)
>  		*start_lsn = be64_to_cpu(iclog->ic_header.h_lsn);
>  
> -	lv = list_first_entry_or_null(lv_chain, struct xfs_log_vec, lv_list);
> -	while (lv) {
> -		lv = xlog_write_single(lv_chain, lv, ticket, iclog, &log_offset,
> -					&len, &record_cnt, &data_cnt);
> -		if (!lv)
> -			break;
> -
> -		error = xlog_write_partial(lv, ticket, &iclog, &log_offset,
> -					   &len, &record_cnt, &data_cnt);
> -		if (error)
> -			break;
> -		lv = list_next_entry(lv, lv_list);
> -		if (list_entry_is_head(lv, lv_chain, lv_list))
> -			break;
> +	list_for_each_entry(lv, lv_chain, lv_list) {
> +		/*
> +		 * If the entire log vec does not fit in the iclog, punt it to
> +		 * the partial copy loop which can handle this case.
> +		 */
> +		if (lv->lv_niovecs &&
> +		    lv->lv_bytes > iclog->ic_size - log_offset) {
> +			error = xlog_write_partial(lv, ticket, &iclog,
> +						   &log_offset, &len,
> +						   &record_cnt, &data_cnt);
> +			if (error)
> +				break;
> +		} else {
> +			xlog_write_full(lv, ticket, iclog, &log_offset, &len,
> +					&record_cnt, &data_cnt);
> +		}
>  	}
> -	ASSERT((len == 0 && !lv) || error);
> +	ASSERT(len == 0 || error);
>  
>  	/*
>  	 * We've already been guaranteed that the last writes will fit inside
> -- 
> 2.30.2
> 


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/8] xfs: change the type of ic_datap
  2021-06-16 16:32 ` [PATCH 1/8] xfs: change the type of ic_datap Christoph Hellwig
  2021-06-17 16:22   ` Brian Foster
@ 2021-06-18  5:52   ` Chandan Babu R
  1 sibling, 0 replies; 26+ messages in thread
From: Chandan Babu R @ 2021-06-18  5:52 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs

On 16 Jun 2021 at 22:02, Christoph Hellwig wrote:
> Turn ic_datap from a char into a void pointer given that it points
> to arbitrary data.
>

xlog_alloc_log() has the following statement,

iclog->ic_datap = (char *)iclog->ic_data + log->l_iclog_hsize;

Maybe the "char *" typecast can be converted to "void *" as part of this
patch.

The remaining changes look good to me.

Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_log.c      | 2 +-
>  fs/xfs/xfs_log_priv.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index e921b554b68367..8999c78f3ac6d9 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -3613,7 +3613,7 @@ xlog_verify_iclog(
>  		if (field_offset & 0x1ff) {
>  			clientid = ophead->oh_clientid;
>  		} else {
> -			idx = BTOBBT((char *)&ophead->oh_clientid - iclog->ic_datap);
> +			idx = BTOBBT((void *)&ophead->oh_clientid - iclog->ic_datap);
>  			if (idx >= (XLOG_HEADER_CYCLE_SIZE / BBSIZE)) {
>  				j = idx / (XLOG_HEADER_CYCLE_SIZE / BBSIZE);
>  				k = idx % (XLOG_HEADER_CYCLE_SIZE / BBSIZE);
> diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> index e4e421a7033558..96dbe713954f7e 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -185,7 +185,7 @@ typedef struct xlog_in_core {
>  	u32			ic_offset;
>  	enum xlog_iclog_state	ic_state;
>  	unsigned int		ic_flags;
> -	char			*ic_datap;	/* pointer to iclog data */
> +	void			*ic_datap;	/* pointer to iclog data */
>  
>  	/* Callback structures need their own cacheline */
>  	spinlock_t		ic_callback_lock ____cacheline_aligned_in_smp;


-- 
chandan

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/8] xfs: list entry elements don't need to be initialized
  2021-06-16 16:32 ` [PATCH 2/8] xfs: list entry elements don't need to be initialized Christoph Hellwig
  2021-06-17 16:23   ` Brian Foster
@ 2021-06-18  5:59   ` Chandan Babu R
  1 sibling, 0 replies; 26+ messages in thread
From: Chandan Babu R @ 2021-06-18  5:59 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs

On 16 Jun 2021 at 22:02, Christoph Hellwig wrote:
> list_add does not require the added element to be initialized.
>

Looks good.

Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_log.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 8999c78f3ac6d9..32cb0fc459a364 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -851,7 +851,7 @@ xlog_write_unmount_record(
>  		.lv_iovecp = &reg,
>  	};
>  	LIST_HEAD(lv_chain);
> -	INIT_LIST_HEAD(&vec.lv_list);
> +
>  	list_add(&vec.lv_list, &lv_chain);
>  
>  	BUILD_BUG_ON((sizeof(struct xlog_op_header) +
> @@ -1587,7 +1587,7 @@ xlog_commit_record(
>  	};
>  	int	error;
>  	LIST_HEAD(lv_chain);
> -	INIT_LIST_HEAD(&vec.lv_list);
> +
>  	list_add(&vec.lv_list, &lv_chain);
>  
>  	if (XLOG_FORCED_SHUTDOWN(log))


-- 
chandan

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/8] xfs: factor out a helper to write a log_iovec into the iclog
  2021-06-16 16:32 ` [PATCH 3/8] xfs: factor out a helper to write a log_iovec into the iclog Christoph Hellwig
  2021-06-17 16:23   ` Brian Foster
@ 2021-06-18  7:42   ` Chandan Babu R
  1 sibling, 0 replies; 26+ messages in thread
From: Chandan Babu R @ 2021-06-18  7:42 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs

On 16 Jun 2021 at 22:02, Christoph Hellwig wrote:
> Add a new helper to copy the log iovec into the in-core log buffer,
> and open code the handling continuation opheader as a special case.
>

Looks good.

Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_log.c | 55 +++++++++++++++++++++++++++---------------------
>  1 file changed, 31 insertions(+), 24 deletions(-)
>
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 32cb0fc459a364..5b431d53287d2c 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2124,6 +2124,26 @@ xlog_print_trans(
>  	}
>  }
>  
> +static inline void
> +xlog_write_iovec(
> +	struct xlog_in_core	*iclog,
> +	uint32_t		*log_offset,
> +	void			*data,
> +	uint32_t		write_len,
> +	int			*bytes_left,
> +	uint32_t		*record_cnt,
> +	uint32_t		*data_cnt)
> +{
> +	ASSERT(*log_offset % sizeof(int32_t) == 0);
> +	ASSERT(write_len % sizeof(int32_t) == 0);
> +
> +	memcpy(iclog->ic_datap + *log_offset, data, write_len);
> +	*log_offset += write_len;
> +	*bytes_left -= write_len;
> +	(*record_cnt)++;
> +	*data_cnt += write_len;
> +}
> +
>  /*
>   * Write whole log vectors into a single iclog which is guaranteed to have
>   * either sufficient space for the entire log vector chain to be written or
> @@ -2145,13 +2165,11 @@ xlog_write_single(
>  	uint32_t		*data_cnt)
>  {
>  	struct xfs_log_vec	*lv;
> -	void			*ptr;
>  	int			index;
>  
>  	ASSERT(*log_offset + *len <= iclog->ic_size ||
>  		iclog->ic_state == XLOG_STATE_WANT_SYNC);
>  
> -	ptr = iclog->ic_datap + *log_offset;
>  	for (lv = log_vector;
>  	     !list_entry_is_head(lv, lv_chain, lv_list);
>  	     lv = list_next_entry(lv, lv_list)) {
> @@ -2171,16 +2189,13 @@ xlog_write_single(
>  			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)++;
> -			*data_cnt += reg->i_len;
> +
> +			xlog_write_iovec(iclog, log_offset, reg->i_addr,
> +					 reg->i_len, len, record_cnt,
> +					 data_cnt);
>  		}
>  	}
>  	if (list_entry_is_head(lv, lv_chain, lv_list))
> @@ -2249,12 +2264,10 @@ xlog_write_partial(
>  	int			error;
>  
>  	/* walk the logvec, copying until we run out of space in the iclog */
> -	ptr = iclog->ic_datap + *log_offset;
>  	for (index = 0; index < lv->lv_niovecs; index++) {
>  		uint32_t	reg_offset = 0;
>  
>  		reg = &lv->lv_iovecp[index];
> -		ASSERT(reg->i_len % sizeof(int32_t) == 0);
>  
>  		/*
>  		 * The first region of a continuation must have a non-zero
> @@ -2274,7 +2287,6 @@ xlog_write_partial(
>  					data_cnt);
>  			if (error)
>  				return ERR_PTR(error);
> -			ptr = iclog->ic_datap + *log_offset;
>  		}
>  
>  		ophdr = reg->i_addr;
> @@ -2285,12 +2297,9 @@ xlog_write_partial(
>  		if (rlen != reg->i_len)
>  			ophdr->oh_flags |= XLOG_CONTINUE_TRANS;
>  
> -		ASSERT((unsigned long)ptr % sizeof(int32_t) == 0);
> -		xlog_verify_dest_ptr(log, ptr);
> -		memcpy(ptr, reg->i_addr, rlen);
> -		xlog_write_adv_cnt(&ptr, len, log_offset, rlen);
> -		(*record_cnt)++;
> -		*data_cnt += rlen;
> +		xlog_verify_dest_ptr(log, iclog->ic_datap + *log_offset);
> +		xlog_write_iovec(iclog, log_offset, reg->i_addr, rlen, len,
> +				 record_cnt, data_cnt);
>  
>  		/* If we wrote the whole region, move to the next. */
>  		if (rlen == reg->i_len)
> @@ -2356,12 +2365,10 @@ xlog_write_partial(
>  			rlen = min_t(uint32_t, rlen, iclog->ic_size - *log_offset);
>  			ophdr->oh_len = cpu_to_be32(rlen);
>  
> -			xlog_verify_dest_ptr(log, ptr);
> -			memcpy(ptr, reg->i_addr + reg_offset, rlen);
> -			xlog_write_adv_cnt(&ptr, len, log_offset, rlen);
> -			(*record_cnt)++;
> -			*data_cnt += rlen;
> -
> +			xlog_verify_dest_ptr(log, iclog->ic_datap + *log_offset);
> +			xlog_write_iovec(iclog, log_offset,
> +					 reg->i_addr + reg_offset, rlen, len,
> +					 record_cnt, data_cnt);
>  		} while (ophdr->oh_flags & XLOG_CONTINUE_TRANS);
>  	}


-- 
chandan

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/8] xfs: remove xlog_write_adv_cnt and simplify xlog_write_partial
  2021-06-16 16:32 ` [PATCH 4/8] xfs: remove xlog_write_adv_cnt and simplify xlog_write_partial Christoph Hellwig
  2021-06-17 16:23   ` Brian Foster
@ 2021-06-18  7:50   ` Chandan Babu R
  1 sibling, 0 replies; 26+ messages in thread
From: Chandan Babu R @ 2021-06-18  7:50 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs

On 16 Jun 2021 at 22:02, Christoph Hellwig wrote:
> xlog_write_adv_cnt is now only used for writing the continuation ophdr.
> Remove xlog_write_adv_cnt and simplify the caller now that we don't need
> the ptr iteration variable, and don't need to increment / decrement
> len for the accounting shengians.
>

Looks good.

Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_log.c      | 12 +++++-------
>  fs/xfs/xfs_log_priv.h |  8 --------
>  2 files changed, 5 insertions(+), 15 deletions(-)
>
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 5b431d53287d2c..1bc32f056a5bcf 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2331,24 +2331,22 @@ xlog_write_partial(
>  			 * a new iclog. This is necessary so that we reserve
>  			 * space in the iclog for it.
>  			 */
> -			*len += sizeof(struct xlog_op_header);
>  			ticket->t_curr_res -= sizeof(struct xlog_op_header);
>  
>  			error = xlog_write_get_more_iclog_space(log, ticket,
> -					&iclog, log_offset, *len, record_cnt,
> -					data_cnt);
> +					&iclog, log_offset,
> +					*len + sizeof(struct xlog_op_header),
> +					record_cnt, data_cnt);
>  			if (error)
>  				return ERR_PTR(error);
> -			ptr = iclog->ic_datap + *log_offset;
>  
> -			ophdr = ptr;
> +			ophdr = iclog->ic_datap + *log_offset;
>  			ophdr->oh_tid = cpu_to_be32(ticket->t_tid);
>  			ophdr->oh_clientid = XFS_TRANSACTION;
>  			ophdr->oh_res2 = 0;
>  			ophdr->oh_flags = XLOG_WAS_CONT_TRANS;
>  
> -			xlog_write_adv_cnt(&ptr, len, log_offset,
> -						sizeof(struct xlog_op_header));
> +			*log_offset += sizeof(struct xlog_op_header);
>  			*data_cnt += sizeof(struct xlog_op_header);
>  
>  			/*
> diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> index 96dbe713954f7e..1b3b3d2bb8a5d1 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -467,14 +467,6 @@ extern kmem_zone_t *xfs_log_ticket_zone;
>  struct xlog_ticket *xlog_ticket_alloc(struct xlog *log, int unit_bytes,
>  		int count, bool permanent);
>  
> -static inline void
> -xlog_write_adv_cnt(void **ptr, int *len, int *off, size_t bytes)
> -{
> -	*ptr += bytes;
> -	*len -= bytes;
> -	*off += bytes;
> -}
> -
>  void	xlog_print_tic_res(struct xfs_mount *mp, struct xlog_ticket *ticket);
>  void	xlog_print_trans(struct xfs_trans *);
>  int	xlog_write(struct xlog *log, struct list_head *lv_chain,


-- 
chandan

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 5/8] xfs: remove xlog_verify_dest_ptr
  2021-06-16 16:32 ` [PATCH 5/8] xfs: remove xlog_verify_dest_ptr Christoph Hellwig
  2021-06-17 16:24   ` Brian Foster
@ 2021-06-18 10:31   ` Chandan Babu R
  1 sibling, 0 replies; 26+ messages in thread
From: Chandan Babu R @ 2021-06-18 10:31 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs

On 16 Jun 2021 at 22:02, Christoph Hellwig wrote:
> Just check that the offset in xlog_write_vec is smaller than the iclog
> size and remove the expensive cycling through all iclogs.
>

Looks good.

Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_log.c      | 35 +----------------------------------
>  fs/xfs/xfs_log_priv.h |  4 ----
>  2 files changed, 1 insertion(+), 38 deletions(-)
>
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 1bc32f056a5bcf..5d55d4fff63035 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -59,10 +59,6 @@ xlog_sync(
>  	struct xlog_ticket	*ticket);
>  #if defined(DEBUG)
>  STATIC void
> -xlog_verify_dest_ptr(
> -	struct xlog		*log,
> -	void			*ptr);
> -STATIC void
>  xlog_verify_grant_tail(
>  	struct xlog *log);
>  STATIC void
> @@ -76,7 +72,6 @@ xlog_verify_tail_lsn(
>  	struct xlog_in_core	*iclog,
>  	xfs_lsn_t		tail_lsn);
>  #else
> -#define xlog_verify_dest_ptr(a,b)
>  #define xlog_verify_grant_tail(a)
>  #define xlog_verify_iclog(a,b,c)
>  #define xlog_verify_tail_lsn(a,b,c)
> @@ -1501,9 +1496,6 @@ xlog_alloc_log(
>  						KM_MAYFAIL | KM_ZERO);
>  		if (!iclog->ic_data)
>  			goto out_free_iclog;
> -#ifdef DEBUG
> -		log->l_iclog_bak[i] = &iclog->ic_header;
> -#endif
>  		head = &iclog->ic_header;
>  		memset(head, 0, sizeof(xlog_rec_header_t));
>  		head->h_magicno = cpu_to_be32(XLOG_HEADER_MAGIC_NUM);
> @@ -2134,6 +2126,7 @@ xlog_write_iovec(
>  	uint32_t		*record_cnt,
>  	uint32_t		*data_cnt)
>  {
> +	ASSERT(*log_offset < iclog->ic_log->l_iclog_size);
>  	ASSERT(*log_offset % sizeof(int32_t) == 0);
>  	ASSERT(write_len % sizeof(int32_t) == 0);
>  
> @@ -2258,7 +2251,6 @@ xlog_write_partial(
>  	struct xfs_log_vec	*lv = log_vector;
>  	struct xfs_log_iovec	*reg;
>  	struct xlog_op_header	*ophdr;
> -	void			*ptr;
>  	int			index = 0;
>  	uint32_t		rlen;
>  	int			error;
> @@ -2297,7 +2289,6 @@ xlog_write_partial(
>  		if (rlen != reg->i_len)
>  			ophdr->oh_flags |= XLOG_CONTINUE_TRANS;
>  
> -		xlog_verify_dest_ptr(log, iclog->ic_datap + *log_offset);
>  		xlog_write_iovec(iclog, log_offset, reg->i_addr, rlen, len,
>  				 record_cnt, data_cnt);
>  
> @@ -2363,7 +2354,6 @@ xlog_write_partial(
>  			rlen = min_t(uint32_t, rlen, iclog->ic_size - *log_offset);
>  			ophdr->oh_len = cpu_to_be32(rlen);
>  
> -			xlog_verify_dest_ptr(log, iclog->ic_datap + *log_offset);
>  			xlog_write_iovec(iclog, log_offset,
>  					 reg->i_addr + reg_offset, rlen, len,
>  					 record_cnt, data_cnt);
> @@ -3466,29 +3456,6 @@ xlog_ticket_alloc(
>  }
>  
>  #if defined(DEBUG)
> -/*
> - * Make sure that the destination ptr is within the valid data region of
> - * one of the iclogs.  This uses backup pointers stored in a different
> - * part of the log in case we trash the log structure.
> - */
> -STATIC void
> -xlog_verify_dest_ptr(
> -	struct xlog	*log,
> -	void		*ptr)
> -{
> -	int i;
> -	int good_ptr = 0;
> -
> -	for (i = 0; i < log->l_iclog_bufs; i++) {
> -		if (ptr >= log->l_iclog_bak[i] &&
> -		    ptr <= log->l_iclog_bak[i] + log->l_iclog_size)
> -			good_ptr++;
> -	}
> -
> -	if (!good_ptr)
> -		xfs_emerg(log->l_mp, "%s: invalid ptr", __func__);
> -}
> -
>  /*
>   * Check to make sure the grant write head didn't just over lap the tail.  If
>   * the cycles are the same, we can't be overlapping.  Otherwise, make sure that
> diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> index 1b3b3d2bb8a5d1..b829d8ba5c6a3f 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -434,10 +434,6 @@ struct xlog {
>  
>  	struct xfs_kobj		l_kobj;
>  
> -	/* The following field are used for debugging; need to hold icloglock */
> -#ifdef DEBUG
> -	void			*l_iclog_bak[XLOG_MAX_ICLOGS];
> -#endif
>  	/* log recovery lsn tracking (for buffer submission */
>  	xfs_lsn_t		l_recovery_lsn;


-- 
chandan

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 6/8] xfs: simplify the xlog_write_partial calling conventions
  2021-06-16 16:32 ` [PATCH 6/8] xfs: simplify the xlog_write_partial calling conventions Christoph Hellwig
  2021-06-17 16:24   ` Brian Foster
@ 2021-06-18 11:07   ` Chandan Babu R
  1 sibling, 0 replies; 26+ messages in thread
From: Chandan Babu R @ 2021-06-18 11:07 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs

On 16 Jun 2021 at 22:02, Christoph Hellwig wrote:
> Lift the iteration to the next log_vec into the callers, and drop
> the pointless log argument that can be trivially derived.
>

Looks good.

Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_log.c | 38 +++++++++++++++-----------------------
>  1 file changed, 15 insertions(+), 23 deletions(-)
>
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 5d55d4fff63035..4afa8ff1a82076 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2232,14 +2232,11 @@ xlog_write_get_more_iclog_space(
>  /*
>   * Write log vectors into a single iclog which is smaller than the current chain
>   * length. We write until we cannot fit a full record into the remaining space
> - * and then stop. We return the log vector that is to be written that cannot
> - * wholly fit in the iclog.
> + * and then stop.
>   */
> -static struct xfs_log_vec *
> +static int
>  xlog_write_partial(
> -	struct xlog		*log,
> -	struct list_head	*lv_chain,
> -	struct xfs_log_vec	*log_vector,
> +	struct xfs_log_vec	*lv,
>  	struct xlog_ticket	*ticket,
>  	struct xlog_in_core	**iclogp,
>  	uint32_t		*log_offset,
> @@ -2248,8 +2245,7 @@ xlog_write_partial(
>  	uint32_t		*data_cnt)
>  {
>  	struct xlog_in_core	*iclog = *iclogp;
> -	struct xfs_log_vec	*lv = log_vector;
> -	struct xfs_log_iovec	*reg;
> +	struct xlog		*log = iclog->ic_log;
>  	struct xlog_op_header	*ophdr;
>  	int			index = 0;
>  	uint32_t		rlen;
> @@ -2257,9 +2253,8 @@ xlog_write_partial(
>  
>  	/* walk the logvec, copying until we run out of space in the iclog */
>  	for (index = 0; index < lv->lv_niovecs; index++) {
> -		uint32_t	reg_offset = 0;
> -
> -		reg = &lv->lv_iovecp[index];
> +		struct xfs_log_iovec	*reg = &lv->lv_iovecp[index];
> +		uint32_t		reg_offset = 0;
>  
>  		/*
>  		 * The first region of a continuation must have a non-zero
> @@ -2278,7 +2273,7 @@ xlog_write_partial(
>  					&iclog, log_offset, *len, record_cnt,
>  					data_cnt);
>  			if (error)
> -				return ERR_PTR(error);
> +				return error;
>  		}
>  
>  		ophdr = reg->i_addr;
> @@ -2329,7 +2324,7 @@ xlog_write_partial(
>  					*len + sizeof(struct xlog_op_header),
>  					record_cnt, data_cnt);
>  			if (error)
> -				return ERR_PTR(error);
> +				return error;
>  
>  			ophdr = iclog->ic_datap + *log_offset;
>  			ophdr->oh_tid = cpu_to_be32(ticket->t_tid);
> @@ -2365,10 +2360,7 @@ xlog_write_partial(
>  	 * the caller so it can go back to fast path copying.
>  	 */
>  	*iclogp = iclog;
> -	lv = list_next_entry(lv, lv_list);
> -	if (list_entry_is_head(lv, lv_chain, lv_list))
> -		return NULL;
> -	return lv;
> +	return 0;
>  }
>  
>  /*
> @@ -2450,13 +2442,13 @@ xlog_write(
>  		if (!lv)
>  			break;
>  
> -		lv = xlog_write_partial(log, lv_chain, lv, ticket, &iclog,
> -					&log_offset, &len, &record_cnt,
> -					&data_cnt);
> -		if (IS_ERR_OR_NULL(lv)) {
> -			error = PTR_ERR_OR_ZERO(lv);
> +		error = xlog_write_partial(lv, ticket, &iclog, &log_offset,
> +					   &len, &record_cnt, &data_cnt);
> +		if (error)
> +			break;
> +		lv = list_next_entry(lv, lv_list);
> +		if (list_entry_is_head(lv, lv_chain, lv_list))
>  			break;
> -		}
>  	}
>  	ASSERT((len == 0 && !lv) || error);


-- 
chandan

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/8] xfs: factor out a helper to write a log_iovec into the iclog
  2021-06-17 16:23   ` Brian Foster
@ 2021-06-18 13:24     ` Christoph Hellwig
  0 siblings, 0 replies; 26+ messages in thread
From: Christoph Hellwig @ 2021-06-18 13:24 UTC (permalink / raw)
  To: Brian Foster; +Cc: Christoph Hellwig, linux-xfs

On Thu, Jun 17, 2021 at 12:23:12PM -0400, Brian Foster wrote:
> On Wed, Jun 16, 2021 at 06:32:07PM +0200, Christoph Hellwig wrote:
> > Add a new helper to copy the log iovec into the in-core log buffer,
> > and open code the handling continuation opheader as a special case.
> > 
> 
> What do you mean by "open code the handling continuation opheader?"

This is left from my commit message when this and the next patch were
still one and relates to the then removed xlog_write_adv_cnt helper.
In the current form it makes no sense, so I'll reword the commit message.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 7/8] xfs: factor out a xlog_write_full_log_vec helper
  2021-06-17 16:24   ` Brian Foster
@ 2021-06-18 13:33     ` Christoph Hellwig
  0 siblings, 0 replies; 26+ messages in thread
From: Christoph Hellwig @ 2021-06-18 13:33 UTC (permalink / raw)
  To: Brian Foster; +Cc: Christoph Hellwig, linux-xfs

On Thu, Jun 17, 2021 at 12:24:26PM -0400, Brian Foster wrote:
> > +{
> > +	int			i;
> > +
> > +	ASSERT(*log_offset + *len <= iclog->ic_size ||
> > +		iclog->ic_state == XLOG_STATE_WANT_SYNC);
> 
> I suspect this could be lifted into the original caller (xlog_write())
> since it doesn't have much to do with the current lv (and this is
> eventually no longer called for each lv in the chain)..? Otherwise the
> patch LGTM.

Yes, I guess we should just move it before the main loop over the lv
in the next patch and just keep it where it is for now.  That will
remove the additional invocations if we switch back from partial
to simple iclog writes, but given that len and log_offset are updated
together the extra asserts are rather pointless anyway.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: log write cleanups
  2021-06-16 16:32 log write cleanups Christoph Hellwig
                   ` (7 preceding siblings ...)
  2021-06-16 16:32 ` [PATCH 8/8] xfs: simplify the list iteration in xlog_write Christoph Hellwig
@ 2021-06-18 22:51 ` Dave Chinner
  8 siblings, 0 replies; 26+ messages in thread
From: Dave Chinner @ 2021-06-18 22:51 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs

On Wed, Jun 16, 2021 at 06:32:04PM +0200, Christoph Hellwig wrote:
> Hi all,
> 
> this series cleans up a few loose ends that I noticed while reviewing
> the recent log rework.

This is all going to have to be rebased once I fix all the current
problems with the log code we are hitting.

In the mean time, can we focus on sorting out the current
correctness issues rather than largely cosmetic changes to the log
code?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2021-06-18 22:51 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-16 16:32 log write cleanups Christoph Hellwig
2021-06-16 16:32 ` [PATCH 1/8] xfs: change the type of ic_datap Christoph Hellwig
2021-06-17 16:22   ` Brian Foster
2021-06-18  5:52   ` Chandan Babu R
2021-06-16 16:32 ` [PATCH 2/8] xfs: list entry elements don't need to be initialized Christoph Hellwig
2021-06-17 16:23   ` Brian Foster
2021-06-18  5:59   ` Chandan Babu R
2021-06-16 16:32 ` [PATCH 3/8] xfs: factor out a helper to write a log_iovec into the iclog Christoph Hellwig
2021-06-17 16:23   ` Brian Foster
2021-06-18 13:24     ` Christoph Hellwig
2021-06-18  7:42   ` Chandan Babu R
2021-06-16 16:32 ` [PATCH 4/8] xfs: remove xlog_write_adv_cnt and simplify xlog_write_partial Christoph Hellwig
2021-06-17 16:23   ` Brian Foster
2021-06-18  7:50   ` Chandan Babu R
2021-06-16 16:32 ` [PATCH 5/8] xfs: remove xlog_verify_dest_ptr Christoph Hellwig
2021-06-17 16:24   ` Brian Foster
2021-06-18 10:31   ` Chandan Babu R
2021-06-16 16:32 ` [PATCH 6/8] xfs: simplify the xlog_write_partial calling conventions Christoph Hellwig
2021-06-17 16:24   ` Brian Foster
2021-06-18 11:07   ` Chandan Babu R
2021-06-16 16:32 ` [PATCH 7/8] xfs: factor out a xlog_write_full_log_vec helper Christoph Hellwig
2021-06-17 16:24   ` Brian Foster
2021-06-18 13:33     ` Christoph Hellwig
2021-06-16 16:32 ` [PATCH 8/8] xfs: simplify the list iteration in xlog_write Christoph Hellwig
2021-06-17 16:24   ` Brian Foster
2021-06-18 22:51 ` log write cleanups Dave Chinner

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.