Re: [QUESTION] xfs: Can we remove flush_workqueue from xlog_cil_push_now?

[Dave Chinner <dgc@kernel.org>]

On Thu, Aug 27, 2026 at 11:36:52AM +0800, Ran Hongyun wrote:
> In xlog_cil_push_now(), we call flush_workqueue(cil->xc_push_wq) before
> queuing a new work item in sync mode. The intention is to start any
> pending background push and reduce the wait time for the new sync work.
> However, after commit 636b927eba5b ("workqueue: Make unbound workqueues
> to use per-cpu pool_workqueues"), flush_workqueue() iterates over all
> nr_cpus pool_workqueue entries, instead of per-NUMA nodes. On large SMP
> systems (e.g. 128 CPUs, 4 NUMA nodes), this adds significant overhead.
> Even after 85f0d8e39aff ("workqueue: Reduce expensive locks for unbound
> workqueue") reduced lock cost, the per-CPU iteration remains costly.
> This seems contrary to the original performance intent of the flush.
> 
> Below is a bpftrace test result on a 128-CPU system with 4 NUMA nodes,
> 16 threads doing sequential writes with fsync per write.
> 
> Function                    remove flush          with flush
>                             per-CPU pwqs     per-CPU      per-NUMA
> -------------------------  --------------   ----------   -----------
> xfs_fsync_flush_log          32-256 us      256-1000 us    32-256 us
> flush_workqueue_prep_pwqs      N/A           2-64 us        2-8 us
> 
> With flush_workqueue() removed, xfs_fsync_flush_log latency drops
> back to the per‑NUMA range (32‑256 us).

Ok, but how does the IO performance change? Does this result in a
measurable increase in RWF_OSYNC write throughput? (write + fsync ==
pwritev2(RWF_OSYNC)).

However, behavioural changes like this seem like an infrastructure
regression, and trying to modify callers to hide that regression is
a bandaid. This affects all UNBOUND workqueues, not just this one,
and we do not need per-cpu scheduling for workqueues with such low
work and concurrency counts - there can only be 4 pushs scheduled
and running concurrently at any time on this work queue. Hence we
don't even need NUMA queuing - a single global queue would work just
fine...

Hence it seems to me that this overhead problem should be fixed at
the WQ infra level, not worked around in the caller context...

> The removal relies on the assumption that flush_workqueue is just a
> performance optimization, not a correctness requirement. Without it,
> xlog_cil_force_seq already provides a reliable completion guarantee via
> xlog_wait(&cil->xc_commit_wait), which blocks until the target sequence
> has been committed. The caller will eventually observe the completion
> regardless of whether flush_workqueue was called. In addition, ordering
> is enforced by xlog_cil_order_write, which ensures all records are
> written in strict sequence order. Therefore, removing flush_workqueue
> does not weaken any ordering or completion semantics.

And wait latency goes up because now there is nothing to expedidite
the scheduling of the work we are waiting to complete.

i.e. you might see a drop in CPU time in flush heavy workloads by
removing it, but the trade off is an increase in wait time variance
and long tail latencies, especially under light load.

> If this analysis is correct, the change can be applied broadly to all
> sync force callers through xlog_cil_push_now(), as they all share the
> same completion and ordering guarantees provided by the existing CIL
> infrastructure. The per-CPU iteration overhead would be eliminated on
> every sync log force without affecting correctness.
> 
> But there are several concerns I'd like to confirm:
> 
> Is flush_workqueue just about performance, or does it hide any
> correctness dependency I'm missing?

It may well hide implicit correctness bugs.

> Without it, the "if (!ctx->commit_lsn) { goto restart; }" loop in
> xlog_cil_force_seq may be triggered more often. Is that acceptable,
> or is it a concern?

That's additional context switches and latency. i.e. CPU overhead
that we are trying to avoid...

> Are there any ordering guarantees provided by flush_workqueue that
> xc_push_lock + waitqueues don't already provide?

Removing the flush will change the order in which concurrent push
pipelines order in the journal. This could change the inherent order
of checkpoints in the journal, so there could be visible artifacts
during crash recovery compared to an flushing implementaiton. None
of that would be a correctness issue, but from an application POV it
would be like the concurrent fsync operations completing in a
different order across different kernel versions.

A lakc of flushing could also simply move the ordering latency
bottleneck to journal IO completion where checkpoints have to be
completed in ascending LSN order. Whether that causes a problem or
not is a complete unknown, but it could increase overall wait times
as waiting seqeunces always get woken in ascending order, and hence
everything will wait if a low sequeunce flush submission gets held
up because the kworker isn't scheduled immediately...

> I'd appreciate any thoughts on whether this change is safe.

Probably safe, but there is lots of potential for performance
regressions by removing it. Better would be to provide a low
overhead work queue implementation for callers that don't need
scale-out performance from their unbound workqueues....

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