Re: [PATCH v2 15/54] fs: maintain a list of pinned inodes

On Wed, Aug 27, 2025 at 12:07:56PM -0400, Josef Bacik wrote:

On Wed, Aug 27, 2025 at 05:20:17PM +0200, Christian Brauner wrote:

quoted

On Tue, Aug 26, 2025 at 11:39:15AM -0400, Josef Bacik wrote:

quoted

Currently we have relied on dirty inodes and inodes with cache on them
to simply be left hanging around on the system outside of an LRU. The
only way to make sure these inodes are eventually reclaimed is because
dirty writeback will grab a reference on the inode and then iput it when
it's done, potentially getting it on the LRU. For the cached case the
page cache deletion path will call inode_add_lru when the inode no
longer has cached pages in order to make sure the inode object can be
freed eventually.  In the unmount case we walk all inodes and free them
so this all works out fine.

But we want to eliminate 0 i_count objects as a concept, so we need a
mechanism to hold a reference on these pinned inodes. To that end, add a
list to the super block that contains any inodes that are cached for one
reason or another.

When we call inode_add_lru(), if the inode falls into one of these
categories, we will add it to the cached inode list and hold an
i_obj_count reference.  If the inode does not fall into one of these
categories it will be moved to the normal LRU, which is already holds an
i_obj_count reference.

The dirty case we will delete it from the LRU if it is on one, and then
the iput after the writeout will make sure it's placed onto the correct
list at that point.

The page cache case will migrate it when it calls inode_add_lru() when
deleting pages from the page cache.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---

Ok, I'm trying to wrap my head around the justification for this new
list. Currently we have inodes with a zero reference counts that aren't
on any LRU. They just appear on sb->i_sb_list and are e.g., dealt with
during umount (sync_filesystem() followed by evict_inodes()).

So they're either dealt with by writeback or by the page cache and are
eventually put on the regular LRU or the filesystem shuts down before
that happens.

They're easy to handle and recognize because their inode->i_count is
zero.

Now you make the LRUs hold a full reference so it can be grabbed from
the LRU again avoiding the zombie resurrection from zero. So to
recognize inodes that are pinned internally due to being dirty or having
pagecache pages attached to it you need to track them in a new list
otherwise you can't really differentiate them and when to move them onto
the LRU after writeback and pagecache is done with them.

Exactly. We need to put them somewhere so we can account for their reference.

We could technically just use a flag and not have a list for this, and just use
the flag to indicate that the inode is pinned and the flag has a full reference
associated with it.

I did it this way because if I had a nickel for every time I needed to figure
out where a zombie inode was and had to do the most grotesque drgn magic to find
it, I'd have like 15 cents, which isn't a lot but weird that it's happened 3
times. Having a list makes it easier from a debugging perspective.

But again, we have ->s_inodes, and I can just scan that list and look for
I_LRU_CACHED. We'd still need to hold a full reference for that, but it would
eliminate the need for another list if that's more preferable?  Thanks,

I don't mind the additional list and the sb struct is not very size
sensitive anyway.