Re: [PATCH 28/45] xfs: introduce xlog_write_single()

Darrick J. Wong
2021-03-09T02:40:24Z

On Fri, Mar 05, 2021 at 04:11:26PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Introduce an optimised version of xlog_write() that is used when the
> entire write will fit in a single iclog. This greatly simplifies the
> implementation of writing a log vector chain into an iclog, and sets
> the ground work for a much more understandable xlog_write()
> implementation.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
> fs/xfs/xfs_log.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 56 insertions(+), 1 deletion(-)
>
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 22f97914ab99..590c1e6db475 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2214,6 +2214,52 @@ xlog_write_copy_finish(
> return error;
> }
>
> +/*
> + * Write log vectors into a single iclog which is guaranteed by the caller
> + * to have enough space to write the entire log vector into. Return the number
> + * of log vectors written into the iclog.
> + */
> +static int
> +xlog_write_single(
> + struct xfs_log_vec *log_vector,
> + struct xlog_ticket *ticket,
> + struct xlog_in_core *iclog,
> + uint32_t log_offset,
> + uint32_t len)
> +{
> + struct xfs_log_vec *lv = log_vector;
> + void *ptr;
> + int index = 0;
> + int record_cnt = 0;

Any reason these (and the return type) can't be unsigned? I don't think
negative indices or record counts have any meaning, right?

Otherwise this looks ok to me.

--D

> +
> + ASSERT(log_offset + len <= iclog->ic_size);
> +
> + ptr = iclog->ic_datap + log_offset;
> + for (lv = log_vector; lv; lv = lv->lv_next) {
> + /*
> + * 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;
> +
> + 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++;
> + }
> + }
> + ASSERT(len == 0);
> + return record_cnt;
> +}
> +
> +
> /*
> * Write some region out to in-core log
> *
> @@ -2294,7 +2340,6 @@ xlog_write(
> return error;
>
> ASSERT(log_offset <= iclog->ic_size - 1);
> - ptr = iclog->ic_datap + log_offset;
>
> /* Start_lsn is the first lsn written to. */
> if (start_lsn && !*start_lsn)
> @@ -2311,10 +2356,20 @@ xlog_write(
> XLOG_ICL_NEED_FUA);
> }
>
> + /* If this is a single iclog write, go fast... */
> + if (!contwr && lv == log_vector) {
> + record_cnt = xlog_write_single(lv, ticket, iclog,
> + log_offset, len);
> + len = 0;
> + data_cnt = len;
> + break;
> + }
> +
> /*
> * This loop writes out as many regions as can fit in the amount
> * of space which was allocated by xlog_state_get_iclog_space().
> */
> + ptr = iclog->ic_datap + log_offset;
> while (lv && (!lv->lv_niovecs || index < lv->lv_niovecs)) {
> struct xfs_log_iovec *reg;
> struct xlog_op_header *ophdr;
> --
> 2.28.0
>