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

[Brian Foster <bfoster@redhat.com>]

On Tue, Oct 06, 2026 at 03:19:41PM +1100, Dave Chinner wrote:
> On Wed, Sep 30, 2026 at 09:56:21AM -0400, Brian Foster wrote:
> > On Tue, Sep 29, 2026 at 12:13:36PM +1000, Dave Chinner wrote:
> > > > > 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.
> > 
> > What do we do (or not do) with this signal?
> 
> That triggers a larger backoff - the "flushing" part of the
> "if (stuck + flushing > count * 0.9)" of the check. If we get lots
> of newly dirtied inodes pushed to already queued inode buffers,
> then we backoff for 20ms because it implies we are reaching the
> active head of the AIL where journal commits are inserting newly
> committed dirty items.
> 
> This is very similar to when LOCKED starts to show up - the only
> difference between FLUSHING and LOCKED in this situation is that
> FLUSHING means the inode buffer is still queued for IO, whilst
> LOCKED means one of two things:
> 
> 1. It is being actively accessed and maybe modified (i.e. locked in
> a transaction), and so it is about to be relogged and pinned, in
> which case we can't flush it. It is likely "stuck" until some time
> passes.
> 
> 2. it is locked and under writeback IO. In this case, it was likely
> pushed on the previous AIL push iteration. i.e. we are iterating
> recently pushed items that are still under IO. If we get lots of
> locked items, then we are likely 'stuck' needing them to complete IO
> for progress to be made. Hence we need to back off.
> 
> PINNED is very similar LOCKED case 1, only it has been relogged and
> is dirty in the CIL.
> 
> Hence there is a simlar relationship between pinned, locked and
> flushing - the only difference between them is where in the
> 'recently modified and/or submitted for IO' cycle the objects are
> in.
> 
> These all indicate we have push vs access/modification contention on
> the log items, and we give the active modifications processing
> preference by backing off. i.e. we prioritise relogging over
> pushing because it avoids unnecessary writeback IO, and to do that
> we need pushing to back off when contention signals are detected.
> 

Ok. I get this in principle. Does this reflect a realistic workload for
inodes?

According to the current code, flushing inodes are those that have
landed in the AIL since or were otherwise not flushed (i.e. pinned,
locked) during a previous cluster flush, where the cluster was delwri
queued but has not yet submitted.

The only use of this value is as input to the 90% stuck heuristic. For
inodes, this strikes me as rare enough to wonder why we track this at
all..?

> > ITEM_* implies state of the log item, so I don't see that as
> > inconsistent. They behave differently because they are different items.
> > What's the expectation for the push interface? If we flush an ili/dquot
> > and the buffer is already queued, did we push the item "successfully" or
> > was the item "flushing?"
> 
> It's an indication that there is active modification occurring on
> the inodes/dquots on the cluster buffer, and so we have to make a
> decision to push immediately or back off to allow further
> aggregation.
> 
> push immediately (i.e ignore flushing signal) means the inode
> cluster gets submitted for IO, and the next inode we modify on that
> cluster blocks waiting for cluster IO to finish. If we get the
> timing wrong, we end up doing lots of individual inode pushes and
> repeated cluster buffer IO, instead of aggregating them all into a
> single IO. the "flushing" signal is a sign of the push algorithm to
> back off to allow  modification to continue unhindered and hance
> allow the next push of a item on that cluster buffer to sweep all
> the modifications in one go.
> 
> i.e. the decision is "push immediately and repeatedly" or "backoff
> on modification contention and push once". Doing it once is more CPU
> and IO efficient, push immediately causes modification latency
> issues in xfs_inode_item_precommit() locking the cluster
> buffer...
> 
> > > 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.
> > > 
> > 
> > So in the current code what prevents from adding that many cluster
> > buffers to the delwri queue? The current count > 1000 logic only breaks
> > across an LSN change, so ISTM queue length is subject to the level of
> > cluster overlap in the dataset.
> 
> <looks closer>
> 
> Ok, that's probably a real bug. I think it should be an || not an
> &&. i.e. I'd intended it to submit the buffer list when we completed
> flushing a checkpoint (i.e. the LSN changes) or if we'd gathered
> enough items on the buffer list for submission (count > 1000).
> 

Wouldn't that mean we'd only process 1k items per pass, rather than
creating a delwri queue limit? It also seems to me this could lead to
situations we've already discussed like spinning around on the same
subset of items if they happen to land in the same LSN (unless we do the
cursor thing).

> This points out the difference between "knowing the intent" and
> "reading the code". One cannot tell the difference between bug and
> intended behaviour from reading the code....
> 

I'd have expected intent to be documented in the commit log.

> As it is, this could result in longer buffer submission lists, but
> that makes it even less likely to trigger a hangcheck timer. i.e.
> submission to the request queue is typically faster than hardware IO
> completion, hence the more we have to submit, the more likely we are
> to block on a full request queue.
> 

Hm.. not sure about that. I played around with this a bit last week. I
couldn't exactly reproduce what is reported here, but if I run this sort
of workload on a slightly older kernel with preempt=none, I could
definitely reproduce observable multi-second (~3-5s) stalls where the
target system would appear locked up completely. Tracing shows that the
delwri queue can easily grow to ~250k+ buffers and the xfsaild task can
spin in short intervals.

I could corroborate this by reducing the watchdog threshold to a few
seconds and trigger softlockup warnings/stack traces that way, but
obviously this is a little different from a 20+ second lockup using
system defaults.

IIRC this behavior went away with the lazy preempt mode, so perhaps that
is a mitigating factor here. It would be good to know whether these
settings are relevant to the original reproducer or not.

> > > 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.
> > > 
> > 
> > I get that the LSN bookmark is not granular, but the old algorithm only
> > broke the loop if we were stuck or hit the target. It resets the
> > bookmark if the 90% stuck+flushing threshold is met or we hit the
> > target. So the example described above sounds like one where we'd reset
> > the LSN pushed bookmark anyways, or otherwise we are stuck and intend to
> > repush locked/pinned items.
> > 
> > That said, I can see how accounting the flushing state for cluster
> > flushed inodes in the same push cycle inflates this metric into backing
> > off/restarting, which seems like a flaw/inefficiency in the old code.
> 
> It's not just the backoff, it's all the extra code that has to be
> executed to get to the point of realising the inode has already been
> flushed to the cluster buffer. THis change made it a single bit
> check in the outer loop, vs going into ->iop_push and taking a cache
> miss to pull the buffer from the ILI, then another to pull the inode
> and i_flags from the ILI, then another to pull the pincount from the
> inode, then another to pull the pin count from the buffer,
> 

I get that, but we can easily separate iop_push() overhead from the
question of flushing inode accounting.

> IOWs, the LI_FLUSHING check runs hot in cache, whilst iop_push()
> takes multiple dependent cacheline misses for every inode it checks
> for STALE/pinned/flushing....
> 
> <snip>
> 
> > > 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...
> > 
> > I'm curious what you're referring to here, if not the last lsn thing..
> > a cursor enhancement perhaps?
> 
> Yeah, get rid of last_pushed_lsn, and keep the cursor active across
> push iterations. The only reason for last_pushed_lsn existing is to
> re-seed the cursor on the next iteration. It's a bit of a wart, and
> it's quite inefficient because we have to walk the entire list from
> the head or tail to find the LSN we need to start at.
> 
> To replace last_push_lsn, we can store a lsn in the cursor on item
> deletion to act as a reseed value for the next iteration instead of
> starting from the [still valid] item in the cursor...
> 

That sounds reasonable, but perhaps non-trivial due to locking.

So all in all we have a situation where there's an unknown soft lockup
issue, a potential bug in iteration logic, an unclear requirement for
more complicated cursor management, and from my perspective a lack of
documentation on design intent (so we're reverse engineering it
piecemeal).

I still think the right approach here may be to rework the original
optimization in a way that retains prior iteration behavior, and then
revisit whatever algorithmic changes that were intended here in followon
patches with more clear documentation. I don't need to know what the
precise issue being reported here is to reason that there's probably
more complexity to this change than originally anticipated. All this is
saying is that it can't hurt to take a more careful approach. 

I also think this makes life easier for stable kernels where it might
take longer for more conservative users to run into the sort of problems
reported here (particularly with older preemption behavior).

This is just my .02. I cooked up a patch for reference:

https://lore.kernel.org/linux-xfs/20261008123141.358649-1-bfoster@redhat.com/

I forgot to label it RFC, but this is mainly for posterity. I'm
operating under the assumption this is NAK'd, so it is untested unless
there's indication we want to do something with it.

Brian

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