log write cleanups

log write cleanups

[Christoph Hellwig]

Hi all,

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

[PATCH 1/8] xfs: change the type of ic_datap

[Christoph Hellwig]

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

Re: [PATCH 1/8] xfs: change the type of ic_datap

[Brian Foster]

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
> 

Re: [PATCH 1/8] xfs: change the type of ic_datap

[Chandan Babu R]

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

[PATCH 2/8] xfs: list entry elements don't need to be initialized

[Christoph Hellwig]

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

Re: [PATCH 2/8] xfs: list entry elements don't need to be initialized

[Brian Foster]

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
> 

Re: [PATCH 2/8] xfs: list entry elements don't need to be initialized

[Chandan Babu R]

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

[PATCH 3/8] xfs: factor out a helper to write a log_iovec into the iclog

[Christoph Hellwig]

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

Re: [PATCH 3/8] xfs: factor out a helper to write a log_iovec into the iclog

[Brian Foster]

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
> 

Re: [PATCH 3/8] xfs: factor out a helper to write a log_iovec into the iclog

[Christoph Hellwig]

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.

Re: [PATCH 3/8] xfs: factor out a helper to write a log_iovec into the iclog

[Chandan Babu R]

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

[PATCH 4/8] xfs: remove xlog_write_adv_cnt and simplify xlog_write_partial

[Christoph Hellwig]

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

Re: [PATCH 4/8] xfs: remove xlog_write_adv_cnt and simplify xlog_write_partial

[Brian Foster]

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
> 

Re: [PATCH 4/8] xfs: remove xlog_write_adv_cnt and simplify xlog_write_partial

[Chandan Babu R]

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

[PATCH 5/8] xfs: remove xlog_verify_dest_ptr

[Christoph Hellwig]

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

Re: [PATCH 5/8] xfs: remove xlog_verify_dest_ptr

[Brian Foster]

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
> 

Re: [PATCH 5/8] xfs: remove xlog_verify_dest_ptr

[Chandan Babu R]

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

[PATCH 6/8] xfs: simplify the xlog_write_partial calling conventions

[Christoph Hellwig]

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

Re: [PATCH 6/8] xfs: simplify the xlog_write_partial calling conventions

[Brian Foster]

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
> 

Re: [PATCH 6/8] xfs: simplify the xlog_write_partial calling conventions

[Chandan Babu R]

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

[PATCH 7/8] xfs: factor out a xlog_write_full_log_vec helper

[Christoph Hellwig]

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

Re: [PATCH 7/8] xfs: factor out a xlog_write_full_log_vec helper

[Brian Foster]

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
> 

Re: [PATCH 7/8] xfs: factor out a xlog_write_full_log_vec helper

[Christoph Hellwig]

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.

[PATCH 8/8] xfs: simplify the list iteration in xlog_write

[Christoph Hellwig]

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

Re: [PATCH 8/8] xfs: simplify the list iteration in xlog_write

[Brian Foster]

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
> 

Re: log write cleanups

[Dave Chinner]

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