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

[Brian Foster <bfoster@redhat.com>]

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
>