On 19 Nov 2021 at 04:43, Dave Chinner wrote:
> Remove the final case where xlog_write() has to prepend an opheader
> to a log transaction. Similar to the start record, the commit record
> is just an empty opheader with a XLOG_COMMIT_TRANS type, so we can
> just make this the payload for the region being passed to
> xlog_write() and remove the special handling in xlog_write() for
> the commit record.
>
I have verfified the following,
1. xfs_log_vec and xfs_log_iovec contents have been initialized correctly.
2. Checkpoint transaction's log reservation is accounted for correctly.
3. Commit record's embedded xlog_op_header's oh_len is correctly set to 0
by xlog_write() since there is no corresponding payload.
Reviewed-by: Chandan Babu R <chandan.babu@xxxxxxxxxx>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> ---
> fs/xfs/xfs_log.c | 22 ++++++----------------
> fs/xfs/xfs_log_cil.c | 11 +++++++++--
> 2 files changed, 15 insertions(+), 18 deletions(-)
>
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index e5515d0c85c4..f789acd2f755 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2281,11 +2281,10 @@ xlog_write_calc_vec_length(
>
> /* Don't account for regions with embedded ophdrs */
> if (optype && headers > 0) {
> - if (optype & XLOG_UNMOUNT_TRANS)
> - headers--;
> + headers--;
> if (optype & XLOG_START_TRANS) {
> - ASSERT(headers >= 2);
> - headers -= 2;
> + ASSERT(headers >= 1);
> + headers--;
> }
> }
>
> @@ -2486,14 +2485,6 @@ xlog_write(
> int data_cnt = 0;
> int error = 0;
>
> - /*
> - * If this is a commit or unmount transaction, we don't need a start
> - * record to be written. We do, however, have to account for the commit
> - * header that gets written. Hence we always have to account for an
> - * extra xlog_op_header here for commit records.
> - */
> - if (optype & XLOG_COMMIT_TRANS)
> - ticket->t_curr_res -= sizeof(struct xlog_op_header);
> if (ticket->t_curr_res < 0) {
> xfs_alert_tag(log->l_mp, XFS_PTAG_LOGRES,
> "ctx ticket reservation ran out. Need to up reservation");
> @@ -2550,14 +2541,13 @@ xlog_write(
> /*
> * The XLOG_START_TRANS has embedded ophdrs for the
> * start record and transaction header. They will always
> - * be the first two regions in the lv chain.
> + * be the first two regions in the lv chain. Commit and
> + * unmount records also have embedded ophdrs.
> */
> - if (optype & XLOG_START_TRANS) {
> + if (optype) {
> ophdr = reg->i_addr;
> if (index)
> optype &= ~XLOG_START_TRANS;
> - } else if (optype & XLOG_UNMOUNT_TRANS) {
> - ophdr = reg->i_addr;
> } else {
> ophdr = xlog_write_setup_ophdr(log, ptr,
> ticket, optype);
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index 9a810a2c92e9..2c4fb55d4897 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -810,9 +810,14 @@ xlog_cil_write_commit_record(
> struct xfs_cil_ctx *ctx)
> {
> struct xlog *log = ctx->cil->xc_log;
> + struct xlog_op_header ophdr = {
> + .oh_clientid = XFS_TRANSACTION,
> + .oh_tid = cpu_to_be32(ctx->ticket->t_tid),
> + .oh_flags = XLOG_COMMIT_TRANS,
> + };
> struct xfs_log_iovec reg = {
> - .i_addr = NULL,
> - .i_len = 0,
> + .i_addr = &ophdr,
> + .i_len = sizeof(struct xlog_op_header),
> .i_type = XLOG_REG_TYPE_COMMIT,
> };
> struct xfs_log_vec vec = {
> @@ -828,6 +833,8 @@ xlog_cil_write_commit_record(
> if (error)
> return error;
>
> + /* account for space used by record data */
> + ctx->ticket->t_curr_res -= reg.i_len;
> error = xlog_write(log, ctx, &vec, ctx->ticket, XLOG_COMMIT_TRANS);
> if (error)
> xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
--
chandan
