On Mon, Aug 25, 2025 at 12:54:01PM +0200, Christian Brauner wrote:
> 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.
Not necsessarily, maybe we had some delayed iput (*cough* btrfs *cough*) that
didn't run until umount time and now we have true coming from ->drop_inode()
with SB_ACTIVE turned off. That would be completely valid. Thanks,
Josef
