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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250828-chorkonzert-forschen-0203eae65f7f@brauner>
Date: Thu, 28 Aug 2025 10:24:03 +0200
From: Christian Brauner <brauner@...nel.org>
To: Josef Bacik <josef@...icpanda.com>
Cc: linux-fsdevel@...r.kernel.org, linux-btrfs@...r.kernel.org, 
	kernel-team@...com, linux-ext4@...r.kernel.org, linux-xfs@...r.kernel.org, 
	viro@...iv.linux.org.uk, amir73il@...il.com
Subject: 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:
> > 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@...icpanda.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.