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

[Dave Chinner <dgc@kernel.org>]

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.

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

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

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.

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

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

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