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

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

[Roberto Bergantinos Corpas]

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
-- 
2.53.0

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

[Trond Myklebust]

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

[PATCH v2] nfs: set gencount on protocol-specific remove

[Roberto Bergantinos Corpas]

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.

Push the setting of gencount under each version-specific layer,
reordering correctly for nfsv4 case after the delegation returns.

This fixes LTP/inotify04 failure after bd4928ec799b.

Fixes: bd4928ec799b ("NFS: Avoid changing nlink when file removes and attribute updates race")
Signed-off-by: Roberto Bergantinos Corpas <rbergant@redhat.com>
---
v2: Move gencount capture and nfs_drop_nlink into version-specific
remove handlers instead of using force flag (Trond)
---
 fs/nfs/dir.c      | 13 +++----------
 fs/nfs/internal.h |  1 +
 fs/nfs/nfs3proc.c |  8 ++++++++
 fs/nfs/nfs4proc.c |  5 +++++
 fs/nfs/proc.c     |  8 ++++++++
 5 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 2402f57c8e7d..e48e8a837d6f 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1937,7 +1937,7 @@ 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)
+void nfs_drop_nlink(struct inode *inode, unsigned long gencount)
 {
 	struct nfs_inode *nfsi = NFS_I(inode);
 
@@ -1951,6 +1951,7 @@ static void nfs_drop_nlink(struct inode *inode, unsigned long gencount)
 			       NFS_INO_INVALID_NLINK);
 	spin_unlock(&inode->i_lock);
 }
+EXPORT_SYMBOL_GPL(nfs_drop_nlink);
 
 /*
  * Called when the dentry loses inode.
@@ -2542,7 +2543,6 @@ EXPORT_SYMBOL_GPL(nfs_rmdir);
 static int nfs_safe_remove(struct dentry *dentry)
 {
 	struct inode *dir = d_inode(dentry->d_parent);
-	struct inode *inode = d_inode(dentry);
 	int error = -EBUSY;
 		
 	dfprintk(VFS, "NFS: safe_remove(%pd2)\n", dentry);
@@ -2554,14 +2554,7 @@ 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);
-
-		error = NFS_PROTO(dir)->remove(dir, dentry);
-		if (error == 0)
-			nfs_drop_nlink(inode, gencount);
-	} else
-		error = NFS_PROTO(dir)->remove(dir, dentry);
+	error = NFS_PROTO(dir)->remove(dir, dentry);
 	if (error == -ENOENT)
 		nfs_dentry_handle_enoent(dentry);
 	trace_nfs_remove_exit(dir, dentry, error);
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 63e09dfc27a8..84d750de608e 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -390,6 +390,7 @@ extern unsigned long nfs_access_cache_count(struct shrinker *shrink,
 					    struct shrink_control *sc);
 extern unsigned long nfs_access_cache_scan(struct shrinker *shrink,
 					   struct shrink_control *sc);
+extern void nfs_drop_nlink(struct inode *inode, unsigned long gencount);
 struct dentry *nfs_lookup(struct inode *, struct dentry *, unsigned int);
 void nfs_d_prune_case_insensitive_aliases(struct inode *inode);
 int nfs_create(struct mnt_idmap *, struct inode *, struct dentry *,
diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
index 3e2de45c95fe..b077455fb019 100644
--- a/fs/nfs/nfs3proc.c
+++ b/fs/nfs/nfs3proc.c
@@ -442,13 +442,21 @@ nfs3_proc_remove(struct inode *dir, struct dentry *dentry)
 		.rpc_resp = &res,
 	};
 	int status = -ENOMEM;
+	struct inode *inode = d_inode(dentry);
+	unsigned long gencount;
 
 	dprintk("NFS call  remove %pd2\n", dentry);
 	res.dir_attr = nfs_alloc_fattr();
 	if (res.dir_attr == NULL)
 		goto out;
 
+	if (inode)
+		gencount = READ_ONCE(NFS_I(inode)->attr_gencount);
+
 	status = rpc_call_sync(NFS_CLIENT(dir), &msg, 0);
+	if (status == 0 && inode)
+		nfs_drop_nlink(inode, gencount);
+
 	nfs_post_op_update_inode(dir, res.dir_attr);
 	nfs_free_fattr(res.dir_attr);
 out:
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 91bcf67bd743..85901714b4b5 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -4919,12 +4919,14 @@ static int nfs4_proc_remove(struct inode *dir, struct dentry *dentry)
 	};
 	struct inode *inode = d_inode(dentry);
 	int err;
+	unsigned long gencount;
 
 	if (inode) {
 		if (inode->i_nlink == 1)
 			nfs4_inode_return_delegation(inode);
 		else
 			nfs4_inode_make_writeable(inode);
+		gencount = READ_ONCE(NFS_I(inode)->attr_gencount);
 	}
 	do {
 		err = _nfs4_proc_remove(dir, &dentry->d_name, NF4REG);
@@ -4932,6 +4934,9 @@ static int nfs4_proc_remove(struct inode *dir, struct dentry *dentry)
 		err = nfs4_handle_exception(NFS_SERVER(dir), err,
 				&exception);
 	} while (exception.retry);
+
+	if (err == 0 && inode)
+		nfs_drop_nlink(inode, gencount);
 	return err;
 }
 
diff --git a/fs/nfs/proc.c b/fs/nfs/proc.c
index 8c3d2efa2636..a8d8ac421200 100644
--- a/fs/nfs/proc.c
+++ b/fs/nfs/proc.c
@@ -322,9 +322,17 @@ nfs_proc_remove(struct inode *dir, struct dentry *dentry)
 		.rpc_argp = &arg,
 	};
 	int			status;
+	struct inode *inode = d_inode(dentry);
+	unsigned long gencount;
 
 	dprintk("NFS call  remove %pd2\n",dentry);
+	if (inode)
+		gencount = READ_ONCE(NFS_I(inode)->attr_gencount);
+
 	status = rpc_call_sync(NFS_CLIENT(dir), &msg, 0);
+	if (status == 0 && inode)
+		nfs_drop_nlink(inode, gencount);
+
 	nfs_mark_for_revalidate(dir);
 
 	dprintk("NFS reply remove: %d\n", status);
-- 
2.53.0