[PATCH v2] xfs: use xfs_trans_ail_copy_lsn for lockless li_lsn read in CIL formatting

[PATCH v2] xfs: use xfs_trans_ail_copy_lsn for lockless li_lsn read in CIL formatting

[Cen Zhang]

xfs_inode_item_format_core() reads lip->li_lsn without holding any lock
to embed the last on-disk LSN into the log dinode during CIL commit:

    xfs_inode_to_log_dinode(ip, dic, ip->i_itemp->ili_item.li_lsn);

Concurrently, xfs_trans_ail_update_bulk() writes lip->li_lsn under
ail_lock when inserting items into the AIL after log IO completion:

    lip->li_lsn = lsn;

The CIL context lock (xc_ctx_lock) and the AIL lock (ail_lock) are
independent and provide no mutual exclusion between these paths.

On 64-bit architectures this is benign since li_lsn monotonically
increases and both old/new values are valid checkpoint LSNs.  On 32-bit
architectures the 64-bit xfs_lsn_t can be torn into two 32-bit loads,
producing a bogus LSN that could cause log recovery to make incorrect
replay decisions.

Use xfs_trans_ail_copy_lsn() to safely snapshot li_lsn, which takes
ail_lock on 32-bit architectures to prevent torn reads.

Fixes: 93f958f9c41f ("xfs: cull unnecessary icdinode fields")
Cc: stable@vger.kernel.org
Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
---
 fs/xfs/xfs_inode_item.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index 8913036b8024..0171f4527f40 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -622,9 +622,12 @@ xfs_inode_item_format_core(
 	struct xlog_format_buf	*lfb)
 {
 	struct xfs_log_dinode	*dic;
+	xfs_lsn_t		lsn;
 
+	xfs_trans_ail_copy_lsn(ip->i_mount->m_ail, &lsn,
+				&ip->i_itemp->ili_item.li_lsn);
 	dic = xlog_format_start(lfb, XLOG_REG_TYPE_ICORE);
-	xfs_inode_to_log_dinode(ip, dic, ip->i_itemp->ili_item.li_lsn);
+	xfs_inode_to_log_dinode(ip, dic, lsn);
 	xlog_format_commit(lfb, xfs_log_dinode_size(ip->i_mount));
 }
 
-- 
2.34.1

Re: [PATCH v2] xfs: use xfs_trans_ail_copy_lsn for lockless li_lsn read in CIL formatting

[Christoph Hellwig]

Looks good to me, even if the additional lock on 32-bit might hurt
that one person or two running performance critical workloads on
32-bit systems:

Reviewed-by: Christoph Hellwig <hch@lst.de>

But I'd really like to have Dave look over this as he's the resident
expert in this area.

Re: [PATCH v2] xfs: use xfs_trans_ail_copy_lsn for lockless li_lsn read in CIL formatting

[Dave Chinner]

On Tue, Mar 24, 2026 at 11:22:42PM -0700, Christoph Hellwig wrote:
> Looks good to me, even if the additional lock on 32-bit might hurt
> that one person or two running performance critical workloads on
> 32-bit systems:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> But I'd really like to have Dave look over this as he's the resident
> expert in this area.

Seen it, haven't had time to look at it in depth. Complex.

In general, updates to lip->li_lsn at AIL insert time do not hold
object locks (buffer locks, dquot locks, inode locks, etc) and so
can race with reading of lip->li_lsn at any other time.  On 64 bit
platforms, this isn't an issue - but on 32 bit platforms any of
these LSN reads could tear if lip->li_lsn is updated at the same
time.

However, I think -all- objects are pinned pinned in memory when
lip->li_lsn is updated in the xlog_cil_ail_insert() path. Hence I
suspect the race condition of concurrent update/read is much more
limited that it possibly could be because being pinned while
updating rules out all the buffer writeback path verifier reads
from racing with writes.

I suspect that inode and dquot relogging are one possible vector.
inode and dquot flushing are another (e.g. xfs_inode_to_disk() in
xfs_iflush() looks suspect), and then it gets complex...

... because we also read lip->li_lsn in various IO completion
routines.  There's the possibility that objects are relogged whilst
IO is in progress (inodes, yes, dquots maybe, buffers no) and so
reads in IO completion processing could also possibly race with
journal completion modifying the lip->li_lsn....

I really don't have time audit all this code, but I a in a quick
scan I do see several locations where there could potentially be
similar torn 64 bit reads and writes...

-Dave.
-- 
Dave Chinner
dgc@kernel.org