Re: [PATCH v2 52/54] fs: remove I_REFERENCED

[Christian Brauner <brauner@kernel.org>]

On Tue, Aug 26, 2025 at 11:39:52AM -0400, Josef Bacik wrote:
> Because we have referenced inodes on the LRU we've had to change the
> behavior to make sure we remove the inode from the LRU when we reference
> it.

All for this patch but I'm confused by this sentence. Maybe:

"Because inodes must hold a full refcount while on the LRU we've had to
change the behavior to make sure we remove the inode from the LRU when
someone takes another full refcount to it."?

Basically, I_REFERENCED was a way to sidestep the problem that if there
was another access to the inode and it was already on the LRU we had to
do something to acknowledge that.

This is now completely put on it's head by just taking them always off
in such cases and afaict we only do that when we take another full
reference (or I guess when it's actually evicted).

> 
> We do this to account for the fact that we may end up with an inode on
> the LRU list, and then unlink the inode. We want the last iput() in the
> unlink() to actually evict the inode ideally, so we don't want it to
> stick around on the LRU and be evicted that way.
> 
> With that behavior change we no longer need I_REFERENCED, as we're
> always removing the inode from the LRU list on a subsequent access if
> it's on the LRU.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/inode.c                       | 36 +++++++-------------------------
>  include/linux/fs.h               | 22 +++++++++----------
>  include/trace/events/writeback.h |  1 -
>  3 files changed, 17 insertions(+), 42 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 8f61761ca021..4f77db7aca75 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -591,7 +591,12 @@ static bool inode_del_cached_lru(struct inode *inode)
>  	return false;
>  }
>  
> -static void __inode_add_lru(struct inode *inode, bool rotate)
> +/*
> + * Add inode to LRU if needed (inode is unused and clean).
> + *
> + * Needs inode->i_lock held.
> + */
> +void inode_add_lru(struct inode *inode)
>  {
>  	bool need_ref = true;
>  
> @@ -614,8 +619,6 @@ static void __inode_add_lru(struct inode *inode, bool rotate)
>  		if (need_ref)
>  			__iget(inode);
>  		this_cpu_inc(nr_unused);
> -	} else if (rotate) {
> -		inode->i_state |= I_REFERENCED;
>  	}
>  }
>  
> @@ -630,16 +633,6 @@ struct wait_queue_head *inode_bit_waitqueue(struct wait_bit_queue_entry *wqe,
>  }
>  EXPORT_SYMBOL(inode_bit_waitqueue);
>  
> -/*
> - * Add inode to LRU if needed (inode is unused and clean).
> - *
> - * Needs inode->i_lock held.
> - */
> -void inode_add_lru(struct inode *inode)
> -{
> -	__inode_add_lru(inode, false);
> -}
> -
>  /*
>   * Caller must be holding it's own i_count reference on this inode in order to
>   * prevent this being the final iput.
> @@ -1001,14 +994,6 @@ EXPORT_SYMBOL_GPL(evict_inodes);
>  
>  /*
>   * Isolate the inode from the LRU in preparation for freeing it.
> - *
> - * If the inode has the I_REFERENCED flag set, then it means that it has been
> - * used recently - the flag is set in iput_final(). When we encounter such an
> - * inode, clear the flag and move it to the back of the LRU so it gets another
> - * pass through the LRU before it gets reclaimed. This is necessary because of
> - * the fact we are doing lazy LRU updates to minimise lock contention so the
> - * LRU does not have strict ordering. Hence we don't want to reclaim inodes
> - * with this flag set because they are the inodes that are out of order.
>   */
>  static enum lru_status inode_lru_isolate(struct list_head *item,
>  		struct list_lru_one *lru, void *arg)
> @@ -1039,13 +1024,6 @@ static enum lru_status inode_lru_isolate(struct list_head *item,
>  		return LRU_REMOVED;
>  	}
>  
> -	/* Recently referenced inodes get one more pass */
> -	if (inode->i_state & I_REFERENCED) {
> -		inode->i_state &= ~I_REFERENCED;
> -		spin_unlock(&inode->i_lock);
> -		return LRU_ROTATE;
> -	}
> -
>  	/*
>  	 * On highmem systems, mapping_shrinkable() permits dropping
>  	 * page cache in order to free up struct inodes: lowmem might
> @@ -1995,7 +1973,7 @@ static bool maybe_add_lru(struct inode *inode, bool skip_lru)
>  	if (!(sb->s_flags & SB_ACTIVE))
>  		return drop;
>  
> -	__inode_add_lru(inode, true);
> +	inode_add_lru(inode);
>  	return drop;
>  }
>  
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 2a7e7fc96431..39cde53c1b3b 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -715,7 +715,6 @@ is_uncached_acl(struct posix_acl *acl)
>   *			address once it is done. The bit is also used to pin
>   *			the inode in memory for flusher thread.
>   *
> - * I_REFERENCED		Marks the inode as recently references on the LRU list.
>   *
>   * I_WB_SWITCH		Cgroup bdi_writeback switching in progress.  Used to
>   *			synchronize competing switching instances and to tell
> @@ -764,17 +763,16 @@ enum inode_state_flags_t {
>  	I_DIRTY_DATASYNC	= (1U << 4),
>  	I_DIRTY_PAGES		= (1U << 5),
>  	I_CLEAR			= (1U << 6),
> -	I_REFERENCED		= (1U << 7),
> -	I_LINKABLE		= (1U << 8),
> -	I_DIRTY_TIME		= (1U << 9),
> -	I_WB_SWITCH		= (1U << 10),
> -	I_OVL_INUSE		= (1U << 11),
> -	I_CREATING		= (1U << 12),
> -	I_DONTCACHE		= (1U << 13),
> -	I_SYNC_QUEUED		= (1U << 14),
> -	I_PINNING_NETFS_WB	= (1U << 15),
> -	I_LRU			= (1U << 16),
> -	I_CACHED_LRU		= (1U << 17)
> +	I_LINKABLE		= (1U << 7),
> +	I_DIRTY_TIME		= (1U << 8),
> +	I_WB_SWITCH		= (1U << 9),
> +	I_OVL_INUSE		= (1U << 10),
> +	I_CREATING		= (1U << 11),
> +	I_DONTCACHE		= (1U << 12),
> +	I_SYNC_QUEUED		= (1U << 13),
> +	I_PINNING_NETFS_WB	= (1U << 14),
> +	I_LRU			= (1U << 15),
> +	I_CACHED_LRU		= (1U << 16)
>  };
>  
>  #define I_DIRTY_INODE (I_DIRTY_SYNC | I_DIRTY_DATASYNC)
> diff --git a/include/trace/events/writeback.h b/include/trace/events/writeback.h
> index 58ee61f3d91d..b419b8060dda 100644
> --- a/include/trace/events/writeback.h
> +++ b/include/trace/events/writeback.h
> @@ -18,7 +18,6 @@
>  		{I_CLEAR,		"I_CLEAR"},		\
>  		{I_SYNC,		"I_SYNC"},		\
>  		{I_DIRTY_TIME,		"I_DIRTY_TIME"},	\
> -		{I_REFERENCED,		"I_REFERENCED"},	\
>  		{I_LINKABLE,		"I_LINKABLE"},		\
>  		{I_WB_SWITCH,		"I_WB_SWITCH"},		\
>  		{I_OVL_INUSE,		"I_OVL_INUSE"},		\
> -- 
> 2.49.0
>