From: "Darrick J. Wong" <djwong@kernel.org>
To: Christoph Hellwig <hch@lst.de>
Cc: Carlos Maiolino <cem@kernel.org>, linux-xfs@vger.kernel.org
Subject: Re: [PATCH 3/9] xfs: don't use xlog_in_core_2_t in struct xlog_in_core
Date: Tue, 14 Oct 2025 14:47:42 -0700 [thread overview]
Message-ID: <20251014214742.GI6188@frogsfrogsfrogs> (raw)
In-Reply-To: <20251013024228.4109032-4-hch@lst.de>
On Mon, Oct 13, 2025 at 11:42:07AM +0900, Christoph Hellwig wrote:
> Most accessed to the on-disk log record header are for the original
> xlog_rec_header. Make that the main structure, and case for the
> single remaining place using other union legs.
>
> This prepares for removing xlog_in_core_2_t entirely.
Er... so xlog_rec_header is the header that gets written out at the
start of any log buffer? And if a log record has more than
XLOG_CYCLE_DATA_SIZE basic blocks (BBs) in it, then it'll have some
quantity of "extended" headers in the form of a xlog_rec_ext_header
right after the xlog_rec_header, right? And both the regular and ext
headers both have a __be32 array containing the original first four
bytes of each BB, because each BB has a munged version of the LSN cycle
stamped into the first four bytes, right?
The previous patch refactored how the cycle_data transformation
happened, right?
So this patch just gets rid of the strange ic_header #define, and
updates the code to access ic_data->hic_header directly? And now that
we have xlog_cycle_data to abstract the xlog_rec_header ->
xlog_in_core_2_t casting, this just works fine here. Right?
--D
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/xfs/xfs_log.c | 74 +++++++++++++++++++++----------------------
> fs/xfs/xfs_log_cil.c | 6 ++--
> fs/xfs/xfs_log_priv.h | 9 ++----
> fs/xfs/xfs_trace.h | 2 +-
> 4 files changed, 44 insertions(+), 47 deletions(-)
>
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index a569a4320a3a..d9476124def6 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -534,8 +534,8 @@ xlog_state_release_iclog(
> */
> if ((iclog->ic_state == XLOG_STATE_WANT_SYNC ||
> (iclog->ic_flags & XLOG_ICL_NEED_FUA)) &&
> - !iclog->ic_header.h_tail_lsn) {
> - iclog->ic_header.h_tail_lsn =
> + !iclog->ic_header->h_tail_lsn) {
> + iclog->ic_header->h_tail_lsn =
> cpu_to_be64(atomic64_read(&log->l_tail_lsn));
> }
>
> @@ -1457,11 +1457,11 @@ xlog_alloc_log(
> iclog->ic_prev = prev_iclog;
> prev_iclog = iclog;
>
> - iclog->ic_data = kvzalloc(log->l_iclog_size,
> + iclog->ic_header = kvzalloc(log->l_iclog_size,
> GFP_KERNEL | __GFP_RETRY_MAYFAIL);
> - if (!iclog->ic_data)
> + if (!iclog->ic_header)
> goto out_free_iclog;
> - head = &iclog->ic_header;
> + head = iclog->ic_header;
> memset(head, 0, sizeof(xlog_rec_header_t));
> head->h_magicno = cpu_to_be32(XLOG_HEADER_MAGIC_NUM);
> head->h_version = cpu_to_be32(
> @@ -1476,7 +1476,7 @@ xlog_alloc_log(
> iclog->ic_log = log;
> atomic_set(&iclog->ic_refcnt, 0);
> INIT_LIST_HEAD(&iclog->ic_callbacks);
> - iclog->ic_datap = (void *)iclog->ic_data + log->l_iclog_hsize;
> + iclog->ic_datap = (void *)iclog->ic_header + log->l_iclog_hsize;
>
> init_waitqueue_head(&iclog->ic_force_wait);
> init_waitqueue_head(&iclog->ic_write_wait);
> @@ -1504,7 +1504,7 @@ xlog_alloc_log(
> out_free_iclog:
> for (iclog = log->l_iclog; iclog; iclog = prev_iclog) {
> prev_iclog = iclog->ic_next;
> - kvfree(iclog->ic_data);
> + kvfree(iclog->ic_header);
> kfree(iclog);
> if (prev_iclog == log->l_iclog)
> break;
> @@ -1524,7 +1524,7 @@ xlog_pack_data(
> struct xlog_in_core *iclog,
> int roundoff)
> {
> - struct xlog_rec_header *rhead = &iclog->ic_header;
> + struct xlog_rec_header *rhead = iclog->ic_header;
> __be32 cycle_lsn = CYCLE_LSN_DISK(rhead->h_lsn);
> char *dp = iclog->ic_datap;
> int i;
> @@ -1536,7 +1536,7 @@ xlog_pack_data(
> }
>
> if (xfs_has_logv2(log->l_mp)) {
> - xlog_in_core_2_t *xhdr = iclog->ic_data;
> + xlog_in_core_2_t *xhdr = (xlog_in_core_2_t *)iclog->ic_header;
>
> for (i = 1; i < log->l_iclog_heads; i++)
> xhdr[i].hic_xheader.xh_cycle = cycle_lsn;
> @@ -1658,11 +1658,11 @@ xlog_write_iclog(
>
> iclog->ic_flags &= ~(XLOG_ICL_NEED_FLUSH | XLOG_ICL_NEED_FUA);
>
> - if (is_vmalloc_addr(iclog->ic_data)) {
> - if (!bio_add_vmalloc(&iclog->ic_bio, iclog->ic_data, count))
> + if (is_vmalloc_addr(iclog->ic_header)) {
> + if (!bio_add_vmalloc(&iclog->ic_bio, iclog->ic_header, count))
> goto shutdown;
> } else {
> - bio_add_virt_nofail(&iclog->ic_bio, iclog->ic_data, count);
> + bio_add_virt_nofail(&iclog->ic_bio, iclog->ic_header, count);
> }
>
> /*
> @@ -1791,19 +1791,19 @@ xlog_sync(
> size = iclog->ic_offset;
> if (xfs_has_logv2(log->l_mp))
> size += roundoff;
> - iclog->ic_header.h_len = cpu_to_be32(size);
> + iclog->ic_header->h_len = cpu_to_be32(size);
>
> XFS_STATS_INC(log->l_mp, xs_log_writes);
> XFS_STATS_ADD(log->l_mp, xs_log_blocks, BTOBB(count));
>
> - bno = BLOCK_LSN(be64_to_cpu(iclog->ic_header.h_lsn));
> + bno = BLOCK_LSN(be64_to_cpu(iclog->ic_header->h_lsn));
>
> /* Do we need to split this write into 2 parts? */
> if (bno + BTOBB(count) > log->l_logBBsize)
> - xlog_split_iclog(log, &iclog->ic_header, bno, count);
> + xlog_split_iclog(log, iclog->ic_header, bno, count);
>
> /* calculcate the checksum */
> - iclog->ic_header.h_crc = xlog_cksum(log, &iclog->ic_header,
> + iclog->ic_header->h_crc = xlog_cksum(log, iclog->ic_header,
> iclog->ic_datap, XLOG_REC_SIZE, size);
> /*
> * Intentionally corrupt the log record CRC based on the error injection
> @@ -1814,11 +1814,11 @@ xlog_sync(
> */
> #ifdef DEBUG
> if (XFS_TEST_ERROR(log->l_mp, XFS_ERRTAG_LOG_BAD_CRC)) {
> - iclog->ic_header.h_crc &= cpu_to_le32(0xAAAAAAAA);
> + iclog->ic_header->h_crc &= cpu_to_le32(0xAAAAAAAA);
> iclog->ic_fail_crc = true;
> xfs_warn(log->l_mp,
> "Intentionally corrupted log record at LSN 0x%llx. Shutdown imminent.",
> - be64_to_cpu(iclog->ic_header.h_lsn));
> + be64_to_cpu(iclog->ic_header->h_lsn));
> }
> #endif
> xlog_verify_iclog(log, iclog, count);
> @@ -1845,7 +1845,7 @@ xlog_dealloc_log(
> iclog = log->l_iclog;
> for (i = 0; i < log->l_iclog_bufs; i++) {
> next_iclog = iclog->ic_next;
> - kvfree(iclog->ic_data);
> + kvfree(iclog->ic_header);
> kfree(iclog);
> iclog = next_iclog;
> }
> @@ -1867,7 +1867,7 @@ xlog_state_finish_copy(
> {
> lockdep_assert_held(&log->l_icloglock);
>
> - be32_add_cpu(&iclog->ic_header.h_num_logops, record_cnt);
> + be32_add_cpu(&iclog->ic_header->h_num_logops, record_cnt);
> iclog->ic_offset += copy_bytes;
> }
>
> @@ -2290,7 +2290,7 @@ xlog_state_activate_iclog(
> * We don't need to cover the dummy.
> */
> if (*iclogs_changed == 0 &&
> - iclog->ic_header.h_num_logops == cpu_to_be32(XLOG_COVER_OPS)) {
> + iclog->ic_header->h_num_logops == cpu_to_be32(XLOG_COVER_OPS)) {
> *iclogs_changed = 1;
> } else {
> /*
> @@ -2302,11 +2302,11 @@ xlog_state_activate_iclog(
>
> iclog->ic_state = XLOG_STATE_ACTIVE;
> iclog->ic_offset = 0;
> - iclog->ic_header.h_num_logops = 0;
> - memset(iclog->ic_header.h_cycle_data, 0,
> - sizeof(iclog->ic_header.h_cycle_data));
> - iclog->ic_header.h_lsn = 0;
> - iclog->ic_header.h_tail_lsn = 0;
> + iclog->ic_header->h_num_logops = 0;
> + memset(iclog->ic_header->h_cycle_data, 0,
> + sizeof(iclog->ic_header->h_cycle_data));
> + iclog->ic_header->h_lsn = 0;
> + iclog->ic_header->h_tail_lsn = 0;
> }
>
> /*
> @@ -2398,7 +2398,7 @@ xlog_get_lowest_lsn(
> iclog->ic_state == XLOG_STATE_DIRTY)
> continue;
>
> - lsn = be64_to_cpu(iclog->ic_header.h_lsn);
> + lsn = be64_to_cpu(iclog->ic_header->h_lsn);
> if ((lsn && !lowest_lsn) || XFS_LSN_CMP(lsn, lowest_lsn) < 0)
> lowest_lsn = lsn;
> } while ((iclog = iclog->ic_next) != log->l_iclog);
> @@ -2433,7 +2433,7 @@ xlog_state_iodone_process_iclog(
> * If this is not the lowest lsn iclog, then we will leave it
> * for another completion to process.
> */
> - header_lsn = be64_to_cpu(iclog->ic_header.h_lsn);
> + header_lsn = be64_to_cpu(iclog->ic_header->h_lsn);
> lowest_lsn = xlog_get_lowest_lsn(log);
> if (lowest_lsn && XFS_LSN_CMP(lowest_lsn, header_lsn) < 0)
> return false;
> @@ -2616,7 +2616,7 @@ xlog_state_get_iclog_space(
> goto restart;
> }
>
> - head = &iclog->ic_header;
> + head = iclog->ic_header;
>
> atomic_inc(&iclog->ic_refcnt); /* prevents sync */
> log_offset = iclog->ic_offset;
> @@ -2781,7 +2781,7 @@ xlog_state_switch_iclogs(
> if (!eventual_size)
> eventual_size = iclog->ic_offset;
> iclog->ic_state = XLOG_STATE_WANT_SYNC;
> - iclog->ic_header.h_prev_block = cpu_to_be32(log->l_prev_block);
> + iclog->ic_header->h_prev_block = cpu_to_be32(log->l_prev_block);
> log->l_prev_block = log->l_curr_block;
> log->l_prev_cycle = log->l_curr_cycle;
>
> @@ -2825,7 +2825,7 @@ xlog_force_and_check_iclog(
> struct xlog_in_core *iclog,
> bool *completed)
> {
> - xfs_lsn_t lsn = be64_to_cpu(iclog->ic_header.h_lsn);
> + xfs_lsn_t lsn = be64_to_cpu(iclog->ic_header->h_lsn);
> int error;
>
> *completed = false;
> @@ -2837,7 +2837,7 @@ xlog_force_and_check_iclog(
> * If the iclog has already been completed and reused the header LSN
> * will have been rewritten by completion
> */
> - if (be64_to_cpu(iclog->ic_header.h_lsn) != lsn)
> + if (be64_to_cpu(iclog->ic_header->h_lsn) != lsn)
> *completed = true;
> return 0;
> }
> @@ -2970,7 +2970,7 @@ xlog_force_lsn(
> goto out_error;
>
> iclog = log->l_iclog;
> - while (be64_to_cpu(iclog->ic_header.h_lsn) != lsn) {
> + while (be64_to_cpu(iclog->ic_header->h_lsn) != lsn) {
> trace_xlog_iclog_force_lsn(iclog, _RET_IP_);
> iclog = iclog->ic_next;
> if (iclog == log->l_iclog)
> @@ -3236,7 +3236,7 @@ xlog_verify_dump_tail(
> {
> xfs_alert(log->l_mp,
> "ran out of log space tail 0x%llx/0x%llx, head lsn 0x%llx, head 0x%x/0x%x, prev head 0x%x/0x%x",
> - iclog ? be64_to_cpu(iclog->ic_header.h_tail_lsn) : -1,
> + iclog ? be64_to_cpu(iclog->ic_header->h_tail_lsn) : -1,
> atomic64_read(&log->l_tail_lsn),
> log->l_ailp->ail_head_lsn,
> log->l_curr_cycle, log->l_curr_block,
> @@ -3255,7 +3255,7 @@ xlog_verify_tail_lsn(
> struct xlog *log,
> struct xlog_in_core *iclog)
> {
> - xfs_lsn_t tail_lsn = be64_to_cpu(iclog->ic_header.h_tail_lsn);
> + xfs_lsn_t tail_lsn = be64_to_cpu(iclog->ic_header->h_tail_lsn);
> int blocks;
>
> if (CYCLE_LSN(tail_lsn) == log->l_prev_cycle) {
> @@ -3309,7 +3309,7 @@ xlog_verify_iclog(
> struct xlog_in_core *iclog,
> int count)
> {
> - struct xlog_rec_header *rhead = &iclog->ic_header;
> + struct xlog_rec_header *rhead = iclog->ic_header;
> xlog_in_core_t *icptr;
> void *base_ptr, *ptr;
> ptrdiff_t field_offset;
> @@ -3507,7 +3507,7 @@ xlog_iclogs_empty(
> /* endianness does not matter here, zero is zero in
> * any language.
> */
> - if (iclog->ic_header.h_num_logops)
> + if (iclog->ic_header->h_num_logops)
> return 0;
> iclog = iclog->ic_next;
> } while (iclog != log->l_iclog);
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index f443757e93c2..778ac47adb8c 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -940,7 +940,7 @@ xlog_cil_set_ctx_write_state(
> struct xlog_in_core *iclog)
> {
> struct xfs_cil *cil = ctx->cil;
> - xfs_lsn_t lsn = be64_to_cpu(iclog->ic_header.h_lsn);
> + xfs_lsn_t lsn = be64_to_cpu(iclog->ic_header->h_lsn);
>
> ASSERT(!ctx->commit_lsn);
> if (!ctx->start_lsn) {
> @@ -1458,9 +1458,9 @@ xlog_cil_push_work(
> */
> spin_lock(&log->l_icloglock);
> if (ctx->start_lsn != ctx->commit_lsn) {
> - xfs_lsn_t plsn;
> + xfs_lsn_t plsn = be64_to_cpu(
> + ctx->commit_iclog->ic_prev->ic_header->h_lsn);
>
> - plsn = be64_to_cpu(ctx->commit_iclog->ic_prev->ic_header.h_lsn);
> if (plsn && XFS_LSN_CMP(plsn, ctx->commit_lsn) < 0) {
> /*
> * Waiting on ic_force_wait orders the completion of
> diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> index d2f17691ecca..f1aed6e8f747 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -158,10 +158,8 @@ struct xlog_ticket {
> };
>
> /*
> - * - A log record header is 512 bytes. There is plenty of room to grow the
> - * xlog_rec_header_t into the reserved space.
> - * - ic_data follows, so a write to disk can start at the beginning of
> - * the iclog.
> + * In-core log structure.
> + *
> * - ic_forcewait is used to implement synchronous forcing of the iclog to disk.
> * - ic_next is the pointer to the next iclog in the ring.
> * - ic_log is a pointer back to the global log structure.
> @@ -198,8 +196,7 @@ typedef struct xlog_in_core {
>
> /* reference counts need their own cacheline */
> atomic_t ic_refcnt ____cacheline_aligned_in_smp;
> - xlog_in_core_2_t *ic_data;
> -#define ic_header ic_data->hic_header
> + struct xlog_rec_header *ic_header;
> #ifdef DEBUG
> bool ic_fail_crc : 1;
> #endif
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 79b8641880ab..aa3a3870f894 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -4934,7 +4934,7 @@ DECLARE_EVENT_CLASS(xlog_iclog_class,
> __entry->refcount = atomic_read(&iclog->ic_refcnt);
> __entry->offset = iclog->ic_offset;
> __entry->flags = iclog->ic_flags;
> - __entry->lsn = be64_to_cpu(iclog->ic_header.h_lsn);
> + __entry->lsn = be64_to_cpu(iclog->ic_header->h_lsn);
> __entry->caller_ip = caller_ip;
> ),
> TP_printk("dev %d:%d state %s refcnt %d offset %u lsn 0x%llx flags %s caller %pS",
> --
> 2.47.3
>
>
next prev parent reply other threads:[~2025-10-14 21:47 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-10-13 2:42 kill xlog_in_core_2_t v2 Christoph Hellwig
2025-10-13 2:42 ` [PATCH 1/9] xfs: add a XLOG_CYCLE_DATA_SIZE constant Christoph Hellwig
2025-10-14 21:16 ` Darrick J. Wong
2025-10-13 2:42 ` [PATCH 2/9] xfs: add a on-disk log header cycle array accessor Christoph Hellwig
2025-10-14 21:27 ` Darrick J. Wong
2025-10-15 4:32 ` Christoph Hellwig
2025-10-15 20:25 ` Darrick J. Wong
2025-10-13 2:42 ` [PATCH 3/9] xfs: don't use xlog_in_core_2_t in struct xlog_in_core Christoph Hellwig
2025-10-14 21:47 ` Darrick J. Wong [this message]
2025-10-15 4:34 ` Christoph Hellwig
2025-10-15 20:26 ` Darrick J. Wong
2025-10-13 2:42 ` [PATCH 4/9] xfs: cleanup xlog_alloc_log a bit Christoph Hellwig
2025-10-14 21:48 ` Darrick J. Wong
2025-10-13 2:42 ` [PATCH 5/9] xfs: remove a very outdated comment from xlog_alloc_log Christoph Hellwig
2025-10-14 21:52 ` Darrick J. Wong
2025-10-15 4:37 ` Christoph Hellwig
2025-10-13 2:42 ` [PATCH 6/9] xfs: remove xlog_in_core_2_t Christoph Hellwig
2025-10-14 22:07 ` Darrick J. Wong
2025-10-15 4:41 ` Christoph Hellwig
2025-10-15 20:27 ` Darrick J. Wong
2025-10-13 2:42 ` [PATCH 7/9] xfs: remove the xlog_rec_header_t typedef Christoph Hellwig
2025-10-14 22:08 ` Darrick J. Wong
2025-10-13 2:42 ` [PATCH 8/9] xfs: remove l_iclog_heads Christoph Hellwig
2025-10-14 22:12 ` Darrick J. Wong
2025-10-13 2:42 ` [PATCH 9/9] xfs: remove the xlog_in_core_t typedef Christoph Hellwig
2025-10-14 22:12 ` Darrick J. Wong
-- strict thread matches above, loose matches on Subject: below --
2025-10-27 7:05 kill xlog_in_core_2_t v3 Christoph Hellwig
2025-10-27 7:05 ` [PATCH 3/9] xfs: don't use xlog_in_core_2_t in struct xlog_in_core Christoph Hellwig
2025-10-31 13:42 ` Carlos Maiolino
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20251014214742.GI6188@frogsfrogsfrogs \
--to=djwong@kernel.org \
--cc=cem@kernel.org \
--cc=hch@lst.de \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).