On Thu, Aug 21, 2025 at 04:18:32PM -0400, Josef Bacik wrote:
> We are going to use igrab everywhere we want to acquire a live inode.
> Update it to do a refcount_inc_not_zero on the i_count, and if
> successful grab an reference to i_obj_count. Add a comment explaining
> why we do this and the safety.
>
> Signed-off-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
> ---
> fs/inode.c | 26 +++++++++++++-------------
> include/linux/fs.h | 27 +++++++++++++++++++++++++++
> 2 files changed, 40 insertions(+), 13 deletions(-)
>
> diff --git a/fs/inode.c b/fs/inode.c
> index 28d197731914..b9122c1eee1d 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1648,20 +1648,20 @@ EXPORT_SYMBOL(iunique);
>
> struct inode *igrab(struct inode *inode)
> {
> + lockdep_assert_not_held(&inode->i_lock);
> +
> + inode = inode_tryget(inode);
> + if (!inode)
> + return NULL;
> +
> + /*
> + * If this inode is on the LRU, take it off so that we can re-run the
> + * LRU logic on the next iput().
> + */
> spin_lock(&inode->i_lock);
> - if (!(inode->i_state & (I_FREEING|I_WILL_FREE))) {
> - __iget(inode);
> - inode_lru_list_del(inode);
> - spin_unlock(&inode->i_lock);
> - } else {
> - spin_unlock(&inode->i_lock);
> - /*
> - * Handle the case where s_op->clear_inode is not been
> - * called yet, and somebody is calling igrab
> - * while the inode is getting freed.
> - */
> - inode = NULL;
> - }
> + inode_lru_list_del(inode);
> + spin_unlock(&inode->i_lock);
> +
> return inode;
> }
> EXPORT_SYMBOL(igrab);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 34fb40ba8a94..b731224708be 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -3393,6 +3393,33 @@ static inline void iobj_get(struct inode *inode)
> refcount_inc(&inode->i_obj_count);
> }
>
> +static inline struct inode *inode_tryget(struct inode *inode)
> +{
> + /*
> + * We are using inode_tryget() because we're interested in getting a
> + * live reference to the inode, which is ->i_count. Normally we would
> + * grab i_obj_count first, as it is the highe priority reference.
> + * However we're only interested in making sure we have a live inode,
> + * and we know that if we get a reference for i_count then we can safely
> + * acquire i_obj_count because we always drop i_obj_count after dropping
> + * an i_count reference.
> + *
> + * This is meant to be used either in a place where we have an existing
> + * i_obj_count reference on the inode, or under rcu_read_lock() so we
> + * know we're safe in accessing this inode still.
Maybe add a debug assert to that effect?
VFS_WAR_ON_ONCE(!icount_read(inode) && !rcu_read_lock_held());
> + */
> + if (!refcount_inc_not_zero(&inode->i_count)) {
> + /*
> + * If we failed to increment the reference count, then the
> + * inode is being freed or has been freed. We return NULL
> + * in this case.
> + */
> + return NULL;
> + }
I would invert the logic here?
if (refcount_inc_not_zero()) {
iobj_get(inode);
return inode;
}
/*
* If we failed to increment the reference count, then the
* inode is being freed or has been freed. We return NULL
* in this case.
*/
return NULL;
