Re: [PATCH RESEND] nfs: force drop_nlink if we have a delegation during REMOVE

[Trond Myklebust <trondmy@kernel.org>]

On Mon, 2026-04-20 at 13:43 +0200, Roberto Bergantinos Corpas wrote:
> commit bd4928ec799b ("NFS: Avoid changing nlink when file removes and
> attribute updates race") avoids races on attribute cache with any
> inflight RPC that may modify inode attributes (and gencount) during
> i.e. REMOVE.
> 
> However it does not take into account that REMOVE may trigger
> a delegation return which also advances gencount (via the attribute
> refresh during delegation return), and in that case the gencount
> mismatch is not due to an inflight RPC that already reflected the
> removal.
> 
> If nlink is 1 and we have a delegation before REMOVE, we should force
> the drop to ensure the VFS delivers the expected FS_DELETE_SELF
> notification.
> 
> This fixes LTP/inotify04 failure after bd4928ec799b.
> 
> Fixes: bd4928ec799b ("NFS: Avoid changing nlink when file removes and
> attribute updates race")
> Reviewed-by: Olga Kornievskaia <okorniev@redhat.com>
> Signed-off-by: Roberto Bergantinos Corpas <rbergant@redhat.com>
> ---
>  fs/nfs/dir.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index 2402f57c8e7d..bc6bbf434a21 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -1937,13 +1937,13 @@ static int nfs_dentry_delete(const struct
> dentry *dentry)
>  }
>  
>  /* Ensure that we revalidate inode->i_nlink */
> -static void nfs_drop_nlink(struct inode *inode, unsigned long
> gencount)
> +static void nfs_drop_nlink(struct inode *inode, unsigned long
> gencount, bool force)
>  {
>  	struct nfs_inode *nfsi = NFS_I(inode);
>  
>  	spin_lock(&inode->i_lock);
>  	/* drop the inode if we're reasonably sure this is the last
> link */
> -	if (inode->i_nlink > 0 && gencount == nfsi->attr_gencount)
> +	if (inode->i_nlink > 0 && (force || gencount == nfsi-
> >attr_gencount))
>  		drop_nlink(inode);
>  	nfsi->attr_gencount = nfs_inc_attr_generation_counter();
>  	nfs_set_cache_invalid(
> @@ -1961,7 +1961,7 @@ static void nfs_dentry_iput(struct dentry
> *dentry, struct inode *inode)
>  	if (dentry->d_flags & DCACHE_NFSFS_RENAMED) {
>  		unsigned long gencount = READ_ONCE(NFS_I(inode)-
> >attr_gencount);
>  		nfs_complete_unlink(dentry, inode);
> -		nfs_drop_nlink(inode, gencount);
> +		nfs_drop_nlink(inode, gencount, false);
>  	}
>  	iput(inode);
>  }
> @@ -2556,10 +2556,11 @@ static int nfs_safe_remove(struct dentry
> *dentry)
>  	trace_nfs_remove_enter(dir, dentry);
>  	if (inode != NULL) {
>  		unsigned long gencount = READ_ONCE(NFS_I(inode)-
> >attr_gencount);
> +		bool force_drop =
> nfs_have_read_or_write_delegation(inode) && inode->i_nlink == 1;
>  
>  		error = NFS_PROTO(dir)->remove(dir, dentry);
>  		if (error == 0)
> -			nfs_drop_nlink(inode, gencount);
> +			nfs_drop_nlink(inode, gencount, force_drop);
>  	} else
>  		error = NFS_PROTO(dir)->remove(dir, dentry);
>  	if (error == -ENOENT)
> @@ -2852,7 +2853,7 @@ int nfs_rename(struct mnt_idmap *idmap, struct
> inode *old_dir,
>  			new_dir, new_dentry, error);
>  	if (!error) {
>  		if (new_inode != NULL)
> -			nfs_drop_nlink(new_inode, new_gencount);
> +			nfs_drop_nlink(new_inode, new_gencount,
> false);
>  		/*
>  		 * The d_move() should be here instead of in an
> async RPC completion
>  		 * handler because we need the proper locks to move
> the dentry.  If

The right way to go about this is to push the setting of gencount from
nfs_safe_remove() down into the version-specific layer so that you can
order it correctly with the delegation return and call to
_nfs4_proc_remove().

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trondmy@kernel.org, trond.myklebust@hammerspace.com