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: > > On Tue, Aug 26, 2025 at 11:39:15AM -0400, Josef Bacik wrote: > > > 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@xxxxxxxxxxxxxx> > > > --- > > > > 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.
