[PATCH 0/2] prep patches for my mkdir series

[PATCH 0/2] prep patches for my mkdir series

[NeilBrown]

These two patches are cleanup are dependencies for my mkdir changes and
subsequence directory locking changes.  Both have been discussed with
the maintainers.  I would like these to land via the VFS tree to smooth
the way for landing the mkdir series which I plan to repost after these
are in vfs.all.

The patches are against 6.14-rc4

Thanks,
NeilBrown

 [PATCH 1/2] nfs/vfs: discard d_exact_alias()
 [PATCH 2/2] nfsd: drop fh_update() from S_IFDIR branch of

[PATCH 1/2] nfs/vfs: discard d_exact_alias()

[NeilBrown]

d_exact_alias() is a descendent of d_add_unique() which was introduced
20 years ago mostly likely to work around problems with NFS servers of
the time.  It is now not used in several situations were it was
originally needed and there have been no reports of problems -
presumably the old NFS servers have been improved.  This only place it
is now use is in NFSv4 code and the old problematic servers are thought
to have been v2/v3 only.

There is no clear benefit in reusing a unhashed() dentry which happens
to have the same name as the dentry we are adding.

So this patch removes d_exact_alias() and the one place that it is used.

Cc: Trond Myklebust <trond.myklebust@hammerspace.com>
Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/dcache.c            | 46 ------------------------------------------
 fs/nfs/nfs4proc.c      |  4 +---
 include/linux/dcache.h |  1 -
 3 files changed, 1 insertion(+), 50 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index e3634916ffb9..726a5be2747b 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2687,52 +2687,6 @@ void d_add(struct dentry *entry, struct inode *inode)
 }
 EXPORT_SYMBOL(d_add);
 
-/**
- * d_exact_alias - find and hash an exact unhashed alias
- * @entry: dentry to add
- * @inode: The inode to go with this dentry
- *
- * If an unhashed dentry with the same name/parent and desired
- * inode already exists, hash and return it.  Otherwise, return
- * NULL.
- *
- * Parent directory should be locked.
- */
-struct dentry *d_exact_alias(struct dentry *entry, struct inode *inode)
-{
-	struct dentry *alias;
-	unsigned int hash = entry->d_name.hash;
-
-	spin_lock(&inode->i_lock);
-	hlist_for_each_entry(alias, &inode->i_dentry, d_u.d_alias) {
-		/*
-		 * Don't need alias->d_lock here, because aliases with
-		 * d_parent == entry->d_parent are not subject to name or
-		 * parent changes, because the parent inode i_mutex is held.
-		 */
-		if (alias->d_name.hash != hash)
-			continue;
-		if (alias->d_parent != entry->d_parent)
-			continue;
-		if (!d_same_name(alias, entry->d_parent, &entry->d_name))
-			continue;
-		spin_lock(&alias->d_lock);
-		if (!d_unhashed(alias)) {
-			spin_unlock(&alias->d_lock);
-			alias = NULL;
-		} else {
-			dget_dlock(alias);
-			__d_rehash(alias);
-			spin_unlock(&alias->d_lock);
-		}
-		spin_unlock(&inode->i_lock);
-		return alias;
-	}
-	spin_unlock(&inode->i_lock);
-	return NULL;
-}
-EXPORT_SYMBOL(d_exact_alias);
-
 static void swap_names(struct dentry *dentry, struct dentry *target)
 {
 	if (unlikely(dname_external(target))) {
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index df9669d4ded7..0a46b193f18e 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -3153,9 +3153,7 @@ static int _nfs4_open_and_get_state(struct nfs4_opendata *opendata,
 	if (d_really_is_negative(dentry)) {
 		struct dentry *alias;
 		d_drop(dentry);
-		alias = d_exact_alias(dentry, state->inode);
-		if (!alias)
-			alias = d_splice_alias(igrab(state->inode), dentry);
+		alias = d_splice_alias(igrab(state->inode), dentry);
 		/* d_splice_alias() can't fail here - it's a non-directory */
 		if (alias) {
 			dput(ctx->dentry);
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 4afb60365675..8a63978187a4 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -253,7 +253,6 @@ extern struct dentry * d_splice_alias(struct inode *, struct dentry *);
 extern struct dentry * d_add_ci(struct dentry *, struct inode *, struct qstr *);
 extern bool d_same_name(const struct dentry *dentry, const struct dentry *parent,
 			const struct qstr *name);
-extern struct dentry * d_exact_alias(struct dentry *, struct inode *);
 extern struct dentry *d_find_any_alias(struct inode *inode);
 extern struct dentry * d_obtain_alias(struct inode *);
 extern struct dentry * d_obtain_root(struct inode *);
-- 
2.48.1

Re: [PATCH 1/2] nfs/vfs: discard d_exact_alias()

[Jeff Layton]

On Wed, 2025-02-26 at 17:18 +1100, NeilBrown wrote:
> d_exact_alias() is a descendent of d_add_unique() which was introduced
> 20 years ago mostly likely to work around problems with NFS servers of
> the time.  It is now not used in several situations were it was
> originally needed and there have been no reports of problems -
> presumably the old NFS servers have been improved.  This only place it
> is now use is in NFSv4 code and the old problematic servers are thought
> to have been v2/v3 only.
> 
> There is no clear benefit in reusing a unhashed() dentry which happens
> to have the same name as the dentry we are adding.
> 
> So this patch removes d_exact_alias() and the one place that it is used.
> 
> Cc: Trond Myklebust <trond.myklebust@hammerspace.com>
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/dcache.c            | 46 ------------------------------------------
>  fs/nfs/nfs4proc.c      |  4 +---
>  include/linux/dcache.h |  1 -
>  3 files changed, 1 insertion(+), 50 deletions(-)
> 
> diff --git a/fs/dcache.c b/fs/dcache.c
> index e3634916ffb9..726a5be2747b 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -2687,52 +2687,6 @@ void d_add(struct dentry *entry, struct inode *inode)
>  }
>  EXPORT_SYMBOL(d_add);
>  
> -/**
> - * d_exact_alias - find and hash an exact unhashed alias
> - * @entry: dentry to add
> - * @inode: The inode to go with this dentry
> - *
> - * If an unhashed dentry with the same name/parent and desired
> - * inode already exists, hash and return it.  Otherwise, return
> - * NULL.
> - *
> - * Parent directory should be locked.
> - */
> -struct dentry *d_exact_alias(struct dentry *entry, struct inode *inode)
> -{
> -	struct dentry *alias;
> -	unsigned int hash = entry->d_name.hash;
> -
> -	spin_lock(&inode->i_lock);
> -	hlist_for_each_entry(alias, &inode->i_dentry, d_u.d_alias) {
> -		/*
> -		 * Don't need alias->d_lock here, because aliases with
> -		 * d_parent == entry->d_parent are not subject to name or
> -		 * parent changes, because the parent inode i_mutex is held.
> -		 */
> -		if (alias->d_name.hash != hash)
> -			continue;
> -		if (alias->d_parent != entry->d_parent)
> -			continue;
> -		if (!d_same_name(alias, entry->d_parent, &entry->d_name))
> -			continue;
> -		spin_lock(&alias->d_lock);
> -		if (!d_unhashed(alias)) {
> -			spin_unlock(&alias->d_lock);
> -			alias = NULL;
> -		} else {
> -			dget_dlock(alias);
> -			__d_rehash(alias);
> -			spin_unlock(&alias->d_lock);
> -		}
> -		spin_unlock(&inode->i_lock);
> -		return alias;
> -	}
> -	spin_unlock(&inode->i_lock);
> -	return NULL;
> -}
> -EXPORT_SYMBOL(d_exact_alias);
> -
>  static void swap_names(struct dentry *dentry, struct dentry *target)
>  {
>  	if (unlikely(dname_external(target))) {
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index df9669d4ded7..0a46b193f18e 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -3153,9 +3153,7 @@ static int _nfs4_open_and_get_state(struct nfs4_opendata *opendata,
>  	if (d_really_is_negative(dentry)) {
>  		struct dentry *alias;
>  		d_drop(dentry);
> -		alias = d_exact_alias(dentry, state->inode);
> -		if (!alias)
> -			alias = d_splice_alias(igrab(state->inode), dentry);
> +		alias = d_splice_alias(igrab(state->inode), dentry);
>  		/* d_splice_alias() can't fail here - it's a non-directory */
>  		if (alias) {
>  			dput(ctx->dentry);
> diff --git a/include/linux/dcache.h b/include/linux/dcache.h
> index 4afb60365675..8a63978187a4 100644
> --- a/include/linux/dcache.h
> +++ b/include/linux/dcache.h
> @@ -253,7 +253,6 @@ extern struct dentry * d_splice_alias(struct inode *, struct dentry *);
>  extern struct dentry * d_add_ci(struct dentry *, struct inode *, struct qstr *);
>  extern bool d_same_name(const struct dentry *dentry, const struct dentry *parent,
>  			const struct qstr *name);
> -extern struct dentry * d_exact_alias(struct dentry *, struct inode *);
>  extern struct dentry *d_find_any_alias(struct inode *inode);
>  extern struct dentry * d_obtain_alias(struct inode *);
>  extern struct dentry * d_obtain_root(struct inode *);

Reviewed-by: Jeff Layton <jlayton@kernel.org>

[PATCH 2/2] nfsd: drop fh_update() from S_IFDIR branch of nfsd_create_locked()

[NeilBrown]

nfsd_create_locked() doesn't need to explicitly call fh_update().
On success (which is the only time that fh_update() matters at all),
nfsd_create_setattr() will be called and it will call fh_update().

This extra call is not harmful, but is not necessary.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/vfs.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 29cb7b812d71..1035010f1198 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1505,11 +1505,8 @@ nfsd_create_locked(struct svc_rqst *rqstp, struct svc_fh *fhp,
 			}
 			dput(resfhp->fh_dentry);
 			resfhp->fh_dentry = dget(d);
-			err = fh_update(resfhp);
 			dput(dchild);
 			dchild = d;
-			if (err)
-				goto out;
 		}
 		break;
 	case S_IFCHR:
-- 
2.48.1

Re: [PATCH 2/2] nfsd: drop fh_update() from S_IFDIR branch of nfsd_create_locked()

[Jeff Layton]

On Wed, 2025-02-26 at 17:18 +1100, NeilBrown wrote:
> nfsd_create_locked() doesn't need to explicitly call fh_update().
> On success (which is the only time that fh_update() matters at all),
> nfsd_create_setattr() will be called and it will call fh_update().
> 
> This extra call is not harmful, but is not necessary.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/vfs.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 29cb7b812d71..1035010f1198 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1505,11 +1505,8 @@ nfsd_create_locked(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  			}
>  			dput(resfhp->fh_dentry);
>  			resfhp->fh_dentry = dget(d);
> -			err = fh_update(resfhp);
>  			dput(dchild);
>  			dchild = d;
> -			if (err)
> -				goto out;
>  		}
>  		break;
>  	case S_IFCHR:

Not really related to 1/2 but ok.

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH 2/2] nfsd: drop fh_update() from S_IFDIR branch of nfsd_create_locked()

[Chuck Lever]

On 2/26/25 1:18 AM, NeilBrown wrote:
> nfsd_create_locked() doesn't need to explicitly call fh_update().
> On success (which is the only time that fh_update() matters at all),
> nfsd_create_setattr() will be called and it will call fh_update().
> 
> This extra call is not harmful, but is not necessary.
> > Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/vfs.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 29cb7b812d71..1035010f1198 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1505,11 +1505,8 @@ nfsd_create_locked(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  			}
>  			dput(resfhp->fh_dentry);
>  			resfhp->fh_dentry = dget(d);
> -			err = fh_update(resfhp);
>  			dput(dchild);
>  			dchild = d;
> -			if (err)
> -				goto out;
>  		}
>  		break;
>  	case S_IFCHR:

Thanks for splitting this out!

I was hoping to see a reference to commit 3819bb0d79f5 ("nfsd:
vfs_mkdir() might succeed leaving dentry negative unhashed") in the
patch description... but reviewers can figure that out themselves.

Acked-by: Chuck Lever <chuck.lever@oracle.com>


-- 
Chuck Lever

Re: [PATCH 0/2] prep patches for my mkdir series

[Christian Brauner]

On Wed, 26 Feb 2025 17:18:30 +1100, NeilBrown wrote:
> These two patches are cleanup are dependencies for my mkdir changes and
> subsequence directory locking changes.  Both have been discussed with
> the maintainers.  I would like these to land via the VFS tree to smooth
> the way for landing the mkdir series which I plan to repost after these
> are in vfs.all.
> 
> The patches are against 6.14-rc4
> 
> [...]

Put onto the same stable branch as your other preparatory changes.

---

Applied to the vfs-6.15.async.dir branch of the vfs/vfs.git tree.
Patches in the vfs-6.15.async.dir branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs-6.15.async.dir

[1/2] nfs/vfs: discard d_exact_alias()
      https://git.kernel.org/vfs/vfs/c/3ff6c8707c9a
[2/2] nfsd: drop fh_update() from S_IFDIR branch of nfsd_create_locked()
      https://git.kernel.org/vfs/vfs/c/4cf006b73995