On Thu, Aug 21, 2025 at 04:18:29PM -0400, Josef Bacik wrote:
> Now that we take a full reference for inodes on the LRU, move the logic
> to add the inode to the LRU to before we drop our last reference. This
> allows us to ensure that if the inode has a reference count it can be
> used, and we no longer hold onto inodes that have a 0 reference count.
>
> Signed-off-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
> ---
> fs/inode.c | 53 +++++++++++++++++++++++++++++++++--------------------
> 1 file changed, 33 insertions(+), 20 deletions(-)
>
> diff --git a/fs/inode.c b/fs/inode.c
> index de0ec791f9a3..b4145ddbaf8e 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -614,7 +614,7 @@ static void __inode_add_lru(struct inode *inode, bool rotate)
>
> if (inode->i_state & (I_FREEING | I_WILL_FREE))
> return;
> - if (atomic_read(&inode->i_count))
> + if (atomic_read(&inode->i_count) != 1)
> return;
> if (inode->__i_nlink == 0)
> return;
> @@ -1966,28 +1966,11 @@ EXPORT_SYMBOL(generic_delete_inode);
> * in cache if fs is alive, sync and evict if fs is
> * shutting down.
> */
> -static void iput_final(struct inode *inode, bool skip_lru)
> +static void iput_final(struct inode *inode, bool drop)
> {
> - struct super_block *sb = inode->i_sb;
> - const struct super_operations *op = inode->i_sb->s_op;
> unsigned long state;
> - int drop;
>
> WARN_ON(inode->i_state & I_NEW);
> -
> - if (op->drop_inode)
> - drop = op->drop_inode(inode);
> - else
> - drop = generic_drop_inode(inode);
> -
> - if (!drop && !skip_lru &&
> - !(inode->i_state & I_DONTCACHE) &&
> - (sb->s_flags & SB_ACTIVE)) {
> - __inode_add_lru(inode, true);
> - spin_unlock(&inode->i_lock);
> - return;
> - }
> -
> WARN_ON(!list_empty(&inode->i_lru));
>
> state = inode->i_state;
> @@ -2009,8 +1992,29 @@ static void iput_final(struct inode *inode, bool skip_lru)
> evict(inode);
> }
>
> +static bool maybe_add_lru(struct inode *inode, bool skip_lru)
> +{
> + const struct super_operations *op = inode->i_sb->s_op;
> + struct super_block *sb = inode->i_sb;
> + bool drop = false;
> +
> + if (op->drop_inode)
> + drop = op->drop_inode(inode);
> + else
> + drop = generic_drop_inode(inode);
> +
> + if (!drop && !skip_lru &&
> + !(inode->i_state & I_DONTCACHE) &&
> + (sb->s_flags & SB_ACTIVE))
> + __inode_add_lru(inode, true);
> +
> + return drop;
> +}
Can we rewrite this as:
static bool maybe_add_lru(struct inode *inode, bool skip_lru)
{
const struct super_operations *op = inode->i_sb->s_op;
const struct super_block *sb = inode->i_sb;
bool drop = false;
if (op->drop_inode)
drop = op->drop_inode(inode);
else
drop = generic_drop_inode(inode);
if (drop)
return drop;
if (skip_lru)
return drop;
if (inode->i_state & I_DONTCACHE)
return drop;
if (!(sb->s_flags & SB_ACTIVE))
return drop;
__inode_add_lru(inode, true);
return drop;
}
so it's a lot easier to follow. I really dislike munging conditions
together with a bunch of ands and negations mixed in.
And btw for both I_DONTCACHE and !SB_ACTIVE it seems that returning
anything other than false from op->drop_inode() would be a bug probably
a technicality but I find it pretty odd.
Maybe we add a VFS_WARN_ON_ONCE() at least in your local testing to see
whether you see anything that ever hits this case.
> +
> static void __iput(struct inode *inode, bool skip_lru)
> {
> + bool drop;
> +
> if (!inode)
> return;
> BUG_ON(inode->i_state & I_CLEAR);
> @@ -2026,8 +2030,17 @@ static void __iput(struct inode *inode, bool skip_lru)
> }
>
> spin_lock(&inode->i_lock);
> +
> + /*
> + * If we want to keep the inode around on an LRU we will grab a ref to
> + * the inode when we add it to the LRU list, so we can safely drop the
> + * callers reference after this. If we didn't add the inode to the LRU
> + * then the refcount will still be 1 and we can do the final iput.
> + */
> + drop = maybe_add_lru(inode, skip_lru);
> +
> if (atomic_dec_and_test(&inode->i_count))
> - iput_final(inode, skip_lru);
> + iput_final(inode, drop);
> else
> spin_unlock(&inode->i_lock);
>
> --
> 2.49.0
>
