On Tue, Aug 26, 2025 at 11:39:02AM -0400, Josef Bacik wrote:
> Instead of doing direct access to ->i_count, add a helper to handle
> this. This will make it easier to convert i_count to a refcount later.
>
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index 079b868552c2..46bfc543f946 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -66,7 +66,7 @@ static void fsnotify_unmount_inodes(struct super_block *sb)
> * removed all zero refcount inodes, in any case. Test to
> * be sure.
> */
> - if (!atomic_read(&inode->i_count)) {
> + if (!icount_read(inode)) {
> spin_unlock(&inode->i_lock);
> continue;
> }
[snip]
> +static inline int icount_read(const struct inode *inode)
> +{
> + return atomic_read(&inode->i_count);
> +}
> +
> extern void iget_failed(struct inode *);
> extern void clear_inode(struct inode *);
> extern void __destroy_inode(struct inode *);
The placement issue I mentioned in another e-mail aside, I would
recommend further error-proofing this.
Above I quoted an example user which treats i_count == 0 as special.
While moving this into helpers is definitely a step in the right
direction, I think having consumer open-code this check is avoidably
error-prone.
Notably, as is there is nothing to indicate whether the consumer expects
the value to remain stable or is perhaps doing a quick check for other
reasons.
As such, specific naming aside, I would create 2 variants:
1. icount_read_unstable() -- the value can change from under you
arbitrarily. I don't there are any consumers for this sucker atm.
2. icount_read() -- the caller expects the transition 0<->1 is
guaranteed to not take place, notably if the value is found to be 0, it
stay at 0. to that end the caller is expected to hold the inode spinlock
*and* the fact that the lock is held is asserted on with lockdep.
All that aside, I think open-coding "is the inode unused" with an
explicit count check is bad form -- a dedicated helper for that would
also be nice.
My 3 CZK.
