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

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

[Ran Hongyun]

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

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.

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?

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?

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

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

Thanks,
Ran Hongyun

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

[Dave Chinner]

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

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@kernel.org
---end quoted text---

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

[Chuck Lever]

On Wed, Sep 02, 2026 at 09:35:56AM +0200, Christoph Hellwig wrote:
> On Mon, Aug 31, 2026 at 07:56:27AM +1000, Dave Chinner wrote:
> > 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.

Agreed.

So, I've been chasing the other end of this trade-off. On the NFS client,
rpciod, nfsiod, and xprtiod see sustained completion traffic from many
CPUs, and with the default WQ_AFFN_CACHE_SHARD scope they lose a large
fraction of their cycles to the pool lock on enqueue and dequeue. The
fix there is a finer affinity scope (WQ_AFFN_SMT), so more pools and
fewer CPUs per lock:

  https://lore.kernel.org/linux-nfs/20260831-performance-v1-0-8d9fd9b67f96@kernel.org/

That is the converse of Ran's problem. flush_workqueue_prep_pwqs()
walks every pwq and takes the pool lock once per pool change along the
way, so a finer scope makes each flush take more locks, and a coarser
scope makes the producers contend more. The pwq walk itself is
per-CPU at every scope, which is the part no scope setting removes.

For Ran's immediate purpose, patch 5/8 of that series may be useful:

  https://lore.kernel.org/linux-nfs/20260831-performance-v1-5-8d9fd9b67f96@kernel.org/

It exports alloc_workqueue_attrs(), apply_workqueue_attrs(), and
free_workqueue_attrs(), so a module can set affn_scope on its own
workqueue after alloc_workqueue().

With that, xfs_cil could set WQ_AFFN_SYSTEM on xc_push_wq and get the
single global pool Dave describes, without touching the boot-time
default or relying on WQ_SYSFS, which XFS_WQFLAGS() adds only in
DEBUG builds. That would let Ran measure the flush cost against a
one-pool walk directly rather than through the per-NUMA proxy in the
table.


-- 
Chuck Lever (Come to NFS bake-a-thon! https://nfsv4bat.org)

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

[Tejun Heo]

Hello,

On Wed, Sep 02, 2026 at 10:23:14AM -0400, Chuck Lever wrote:
> 
> On Wed, Sep 02, 2026 at 09:35:56AM +0200, Christoph Hellwig wrote:
> > On Mon, Aug 31, 2026 at 07:56:27AM +1000, Dave Chinner wrote:
> > > 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.
> 
> Agreed.

Well, the changes aren't gratuituous. They're mostly driven by the
increasing number of cpus in systems. Hardware has been scaling pretty fast
and workqueue needs to keep up. Sure, there may be things that could have
gone smoother but you'd need to solve the same problems no matter what.

> So, I've been chasing the other end of this trade-off. On the NFS client,
> rpciod, nfsiod, and xprtiod see sustained completion traffic from many
> CPUs, and with the default WQ_AFFN_CACHE_SHARD scope they lose a large
> fraction of their cycles to the pool lock on enqueue and dequeue. The
> fix there is a finer affinity scope (WQ_AFFN_SMT), so more pools and
> fewer CPUs per lock:
> 
>   https://lore.kernel.org/linux-nfs/20260831-performance-v1-0-8d9fd9b67f96@kernel.org/

And this was worse before because we had a single pool per node for all
unbound workqueues. You're using exactly the new feature that's made
available to solve scalability problems.

> That is the converse of Ran's problem. flush_workqueue_prep_pwqs()
> walks every pwq and takes the pool lock once per pool change along the
> way, so a finer scope makes each flush take more locks, and a coarser
> scope makes the producers contend more. The pwq walk itself is
> per-CPU at every scope, which is the part no scope setting removes.
> 
> For Ran's immediate purpose, patch 5/8 of that series may be useful:
> 
>   https://lore.kernel.org/linux-nfs/20260831-performance-v1-5-8d9fd9b67f96@kernel.org/
> 
> It exports alloc_workqueue_attrs(), apply_workqueue_attrs(), and
> free_workqueue_attrs(), so a module can set affn_scope on its own
> workqueue after alloc_workqueue().
> 
> With that, xfs_cil could set WQ_AFFN_SYSTEM on xc_push_wq and get the
> single global pool Dave describes, without touching the boot-time
> default or relying on WQ_SYSFS, which XFS_WQFLAGS() adds only in
> DEBUG builds. That would let Ran measure the flush cost against a
> one-pool walk directly rather than through the per-NUMA proxy in the
> table.

Try http://lkml.kernel.org/r/20260901210929.3092513-1-tj@kernel.org first?

Thanks.

-- 
tejun