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

[Christoph Hellwig]

On Mon, Aug 31, 2026 at 07:56:27AM +1000, Dave Chinner wrote:
> > 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.

Adding the authors/reviwers and maintainers relevant to this commit and
Chuck as he has been looking into different unbound workqueue overhead
lately.

> > 
> > 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...

Yeah, a lot of the recent workqueue changes had a lot of adverse effects.
We really need to come up with a way to queue work to task context (or a
different task context) that doesn't require reverse engineering "smart"
behavior in the backend.  This has become a never ending whack-a-mole
unfortunately.

Not sure what we can do here, but a simply "just queue things up on a
fixed queue" interface would be really helpful.  Even better with just a
double or even singly linked list as overhead, as the work_struct is
way to big due to the function pointer not even needed in most cases
as typically every item in the queue uses the same one.

[leaving the rest of the context in place for the new readers]


> 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@xxxxxxxxxx
---end quoted text---