Re: [PATCH] xfs: fix skipped flushing items not counted in xfsaild_push()

[Dave Chinner <dgc@kernel.org>]

On Mon, Sep 28, 2026 at 02:51:30PM -0400, Brian Foster wrote:
> On Thu, Sep 24, 2026 at 10:34:01AM +1000, Dave Chinner wrote:
> > On Wed, Sep 23, 2026 at 09:44:32AM -0400, Brian Foster wrote:
> > > On Wed, Sep 23, 2026 at 08:12:46AM +1000, Dave Chinner wrote:
> > > > On Tue, Sep 22, 2026 at 04:14:29PM +0800, MingTao Huang wrote:
> > > > > From: MingTao Huang <mintaohuang@tencent.com>
> > > > > 
> > > > > Commit f3f7ae68a4ea ("xfs: skip flushing log items during push")
> > > > > introduced a fast path in xfsaild_push() that uses
> > > > > test_bit(XFS_LI_FLUSHING) to skip log items already being
> > > > > flushed.  However, the fast path jumps directly to the
> > > > > next_item label, bypassing flushing++, count++, and the
> > > > > ail_last_pushed_lsn update.  This causes three problems:
> > 
> > [....]
> > 
> > > > >    letting the loop scan an order of magnitude more items per round.
> > > > >    Each cluster flush adds a buffer to ail_buf_list, and the resulting
> > > > >    oversized list causes xfs_buf_delwri_submit_nowait() -- which runs
> > > > >    list_sort() plus per-buffer trylock and IO submission -- to take so
> > > > >    long that the watchdog fires.
> > > > 
> > > > How? count bounds the delwri list to 1000 buffers at most. That's
> > > > very intentional, and the number is based on benchmarking results.
> > > > 
> > > 
> > > I'm confused by some of the reasoning here. Maybe I'm missing something,
> > > but that count check looks like it only limits the current pass.  The
> > > flushing value is basically just input into the thread throttling (i.e.
> > > schedule timeout) behavior.
> > 
> > It's much more complex than that.
> > 
> > The count bounds the number of buffers we can add to the list each
> > pass.  We also submit the entire list for IO once per pass, hence we
> > fill and drain the list once per pass. Hence in normal
> > circumstances, the AIL buffer list should never grow very much
> > beyond 1000 buffers.
> > 
> > Further, using xfs_buf_delwri_submit_nowait() doesn't mean that it
> > won't block, it just won't wait for IO completion.  We can still
> > block on IO submission when the request queue fills up.  i.e. the
> > AIL push algorithm is designed to throttle writeback processing at
> > IO submission time, not via the higher level backoff loops.
> > 
> 
> Ok, but it also may not submit I/O at all.

In which case, it is either stuck (and so will backoff) or needs to
continue iterating until it reaches the target or finds something to
submit. In the latter case, we want that search to happen as quickly
as possible to keep disk utilisation up and free up space in the log
for modifications that are blocked waiting for log space.

Pushing items that are already in FLUSHING state and then sleeping
for long periods on 'excessive flushing' heuristics does not acheive
this.

> Also I'm not claiming the AIL push is not designed to throttle on I/O
> submission, but I do think it's also historically implemented to
> throttle its own execution based on push progress and/or list state, for
> whatever variety of external reasons it might need to.

The original design was simply to provide efficient processing and
IO submission of log items without external interruption. i.e there
are no external reasons for things like backoffs existing; they
exist only for efficient management of the AIL contents and IO
submission in isolation from the rest of the filesystem.

> > Hence my request for details about the workload, fs geometry, etc,
> > so I have some idea of how we are getting into this state in the
> > first place.
> > 
> 
> Yeah I mean I'm also interested in more detail here. I'm just not
> convinced the proposed solution is fundamentally wrong.

The patch is definitely wrong. Whether changing the accounting is
needed is unknown, because we don't have any confirmation of what
the actual problem encountered is yet.

> > Hence, from the POV of the AIL control loop, this latter
> > ITEM_FLUSHING metric is pure noise. We did not queue new IO, we did
> > not update a buffer already queued for IO, and we do not know if the
> > item has been recently modified, yet we still accounted it as
> > "flushing".
> > 
> 
> This all makes sense, but I wonder if it would be simpler to just let
> the case of "new inode flushed to pre-queued cluster buffer" return
> ITEM_SUCCESS instead of ITEM_FLUSHING.

That means we lose the 'this inode cluster is being actively
modified' signal from the feedback loop. That makes it behave
differently to buffer items, and so now we have confused and
unbalanced signals being fed to the backoff control decision again.

> > > More of a side note to the discussion and related to the actual patch...
> > > if we did go with something like this ISTM to make more sense to let
> > > xfsaild_push_item() detect and return the flushing state (i.e. similar
> > > to how we handle failed state) so the accounting all exists in one
> > > place, rather than duplicating it for the optimization. Just my .02.
> > 
> > I'm not sure it does - see my comment above about ITEM_FLUSHING
> > being returned by inode items meaning two very different things, and
> > LI_FLUSHING being used to remove the noisy/useless one from the
> > control loop.
> > 
> 
> ISTM this also significantly changed the execution logic in the process.
> For example and if I follow this right, suppose the AIL makes a single
> pass across multiple loop executions (i.e. multiple xfsaild_push()
> calls) and submits multiple batches of I/O in the process. Assume that
> the AIL is inode log item heavy with a good amount of cluster buffer
> overlap.

Yup, and so will likley have thousands to tens of thousands of inode
items per checkpoint LSN in the AIL.

> 
> In this situation prior to the LI_FLUSHING change, we'd account for all
> of the inodes that were batch flushed via xfs_iflush_cluster() by
> tracking them in flushing/count, and we set the last pushed lsn marker
> for successful/flushing items as we go, so each call to xfsaild_push()
> progresses through the list.

We did, but it didn't make progress like you describe because the LSN
would not change very often between iterations because of the sheer
number of items per LSN in the AIL.

That is, we can have tens of thousands of inodes on the same LSN in
the AIL. A CIL checkpoint on a large log is closed off at ~32MB in
size, and an inode core takes up about 300 bytes in the journal. If
we are logging nothing but inode cores (e.g. 'chown -R <somedir>'
over millions of files) then a single checkpoint can contain up to
~100,000 inode log items. These are all inserted into the AIL at the
same LSN.  Hence it is very common for there to be tens of thousands
of inode in the FLUSHING state on the same LSN in the AIL.

This means that the updating the LSN when we hit lots of flushing
inodes doesn't actually move the LSN for the next loop forwards.
We start the next loop at the same LSN in this case, and the only
reason we didn't tend to process the same flushing items a second
time is the backoff allows IO completion to remove the items from
the AIL.

IOWs, any algorithm that requires the LSN to change per iteration to
avoid reprocessing overhead is going to be highly inefficient.  The
backoffs are what allowed the contents at each LSN to change per
iteration in the old code by allowing IO to complete, but the new
code uses LI_FLUSHING instead of backoffs to acheive the same
purpose. The new code has much lower per-iteration latencies and
higher IO submission rates in these situations, which leads to
substantially better modification rates across the entire
filesystem....

> After this change, that first pass no longer tracks those items in
> flushing or count. AFAICT this has a potential side effect of processing
> and submitting more items per delwri submission in this particular
> example, but otherwise doesn't seem all that problematic based on what
> you describe above.

Not a side effect. That was the -intention- of the patch; to be able
to submit more IO, faster, and using less CPU per item that needs
to be processes.

> Once the push loops back (i.e. to the point where it resets last pushed
> lsn), however, we're subject to whatever is on the list by virtue of
> either not yet completing I/O or not submitting or whatever.

The old code was subject to that, too.

> At that
> point it looks like we now may potentially walk through an increased
> number of items in the while loop without any execution backoff because
> we've made batches of the list fully invisible to the scheduling logic.

Which is just fine.

The historical method of handling these sort of "kernel thread has
lots of work to do" cases was to sprinkle cond_resched() through the
code. We've been told not to do that for several years now, and we
also have proper kernel thread preemption now. That means
long-running kernel processing tasks do not need to care about how
long they hold the CPU for as the scheduler can preempt and
reschedule them elsewhere if needed.

IOWs, we just don't care if there is so much work to do that the AIL
just keeps running flat out without backing off. If there's more
work to do, just keep running. It'll either run out of work or block
somewhere eventually, until then it should just keep churning
through the work as fast and efficiency as it can.

> I think you could argue this is actually a loss of "signal" from a task
> scheduling perspective. If this repeats over multiple cycles, it doesn't
> seem all that far fetched for this to spin excessively either due to an
> excessively large AIL list, delwri list, or both.

No, there was no 'task scheduling' signal to begin with. The
original backoffs were purely a mechanism to allow IO to complete
and remove flushing items from the AIL because there was no other
way to efficiently skip them.

The LI_FLUSHING mechanism allowed those items to be efficiently
skipped and ignored by the control loop, so the backoff to manage
the same 'don't reprocess flushing items on the current push lsn'
problem became unnecessary. i.e. there was no 'task management'
consideration in the backoffs at all.

> I could see how the previous behavior is not necessarily the ideal
> design for dealing with this, but just calling this previous state "not
> viable" when it basically restores longstanding historical behavior and
> preserves the optimization of the original change doesn't sound

The proposed fix doesn't restore the old behaviour. It just added
the accounting back in. However, the old behaviour relied on unbound
processing loops and backoffs to function correctly, neither of
which are restored in the proposed patch.

It might behave correctly when it triggers the flushed backoff, but
for sparse flushing items, they will likely get processed and
accounted over and over again until the flushing backoff threshold
is triggered and the aild goes to sleep to allow IO to complete and
remove flushing items from the AIL.

> technically accurate to me. I'm happy to hear more details, but based on
> what we know so far I still suspect the most appropriate thing to do
> would be some combination of restoring the original tracking like this
> patch does, and then revisit these design tweaks in a followup
> patch/series..

No. We need root cause analysis first. There is no general or
widepsread problem with the code as it stands - one bug report
without RCA does not equal "code is broken, must be reverted".

Besides, if the problem really is "we are walking too many
LI_FLUSHING items repeatedly because the LSN is not changing", then
I'm pretty sure there's a relatively straight forward fix for that.
i.e. we need fine grained tracking of the item we are up to across
loop iterations where the AIL lock is not held and the AIL can
change. We already do this within the processing loop itself...

Cheers,

Dave.
-- 
Dave Chinner
dgc@kernel.org