Re: [PATCH] xfs: snapshot current CIL sequence under xc_push_lock

[Dave Chinner <dgc@kernel.org>]

On Wed, May 06, 2026 at 09:58:11AM +0800, Cen Zhang wrote:
> xlog_cil_force() and xlog_cil_flush() both use the current CIL
> checkpoint sequence to request an immediate push.  They currently read
> xc_current_sequence before xlog_cil_push_now() takes xc_push_lock.
> 
> The CIL push worker advances xc_current_sequence under xc_push_lock while
> switching to a new checkpoint context.  If a current-checkpoint force reads
> an old sequence and then publishes it as xc_push_seq after the worker has
> moved to the next context, xlog_cil_push_work() can treat the request as a
> previously pushed sequence and skip it via the push_seq < ctx->sequence
> check.  That can leave the current dirty checkpoint unqueued by a
> force/flush request.

Which does not matter at all.

Integrity in XFS is defined by completion-to-submission semantics.
IOWs, the only thing that a CIL -force- guarantees is that it will
ensure the current CIL context containing fully completed
transactions when it is called.

See, for example, __xfs_trans_commit() -> xfs_log_force() for
XFS_TRANS_SYNC transactions.

So if xlog_cil_force() races with a push switching to a new sequence
number, it means that all the transactions that were completed at
the time xfs_log_force() was called are -already being pushed-.
Hence all we need to do is wait for the CIL context at that sequence
to be committed. The new sequence number, by definition, contains
modifications that completed -after- the log force was started, and
so are outside the scope of the current force operation.

IOWs, we can safely skip the new sequence number in teh case of a
push/force race, and the force will still correctly wait on the old
sequence number being forced to disk. The race is benign, and not a
bug.

As for xlog_cil_flush(), it is a latency reduction mechanism and not
a integrity mechanism. We just don't care about racing, because the
caller will call it again if sufficient progress wasn't made by the
previous call.

Hence I don't think there is a bug that needs fixing here. Yes, the
fact that deducing it is ok requires understanding
competion-to-submission integrity semantics, but that's kinda
assumed knowledge because it's exactly the same semantics as we
relyon for data/metadata ordering and integrity w/ O_SYNC/O_DSYNC
operations...

> Make xlog_cil_push_now() resolve push_seq == 0 to the current sequence
> while holding xc_push_lock, and return the resolved sequence for tracing
> and waiting.  Route xlog_cil_force() and xlog_cil_flush() through that path
> so the current-sequence snapshot and xc_push_seq publication are serialized
> with the context switch.

So the change is likely correct, but somewhat nasty in places.
Regardless, I'm inclined to reject it based on "if it isn't broken,
don't fix it" principles.

In future, when reporting problems and fixes like this, it is very
important that you say how the issue was found, whether it can be
reproduced, hardware configs, etc. Presenting multiple fixes in a
short period for a wide variety of different conditions in extremely
complex code without any context of how/when they occur or what
impact they have on systems smells like AI slop more than
anything....

So, please document what tools/AI are you using to find these
problems and how they can be reproduced. I haven't commented on the
other two "fixes" you've just posted because I haven't got hours to
dig into them right now. You also posted a fix a month ago for a
race condition that wasn't a race condition in buf log item
handling, so something is clearly directing you at them....

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