[PATCH v1 0/3] Allow knfsd to use atomic_open()

[PATCH v1 0/3] Allow knfsd to use atomic_open()

[Benjamin Coddington]

We have workloads that will benefit from allowing knfsd to use atomic_open()
in the open/create path.  There are two benefits; the first is the original
matter of correctness: when knfsd must perform both vfs_create() and
vfs_open() in series there can be races or error results that cause the
caller to receive unexpected results.  The second benefit is that for some
network filesystems, we can reduce the number of remote round-trip
operations by using a single atomic_open() path which provides a performance
benefit. 

I've implemented this with the simplest possible change - by modifying
dentry_create() which has a single user: knfsd.  The changes cause us to
insert ourselves part-way into the previously closed/static atomic_open()
path, so I expect VFS folks to have some good ideas about potentially
superior approaches.

Thanks for any comment and critique.

Benjamin Coddington (3):
  VFS: move dentry_create() from fs/open.c to fs/namei.c
  VFS: Prepare atomic_open() for dentry_create()
  VFS/knfsd: Teach dentry_create() to use atomic_open()

 fs/namei.c         | 84 ++++++++++++++++++++++++++++++++++++++++++----
 fs/nfsd/nfs4proc.c |  8 +++--
 fs/open.c          | 41 ----------------------
 include/linux/fs.h |  2 +-
 4 files changed, 83 insertions(+), 52 deletions(-)

-- 
2.50.1

[PATCH v1 1/3] VFS: move dentry_create() from fs/open.c to fs/namei.c

[Benjamin Coddington]

To prepare knfsd's helper dentry_create(), move it to namei.c so that it
can access static functions within.  Callers of dentry_create() can be
viewed as being mostly done with lookup, but still need to perform a few
final checks.  In order to o use atomic_open() we want dentry_create() to
be able to access:

	- vfs_prepare_mode
	- may_o_create
	- atomic_open

.. all of which have static declarations.

Signed-off-by: Benjamin Coddington <bcodding@hammerspace.com>
---
 fs/namei.c | 41 +++++++++++++++++++++++++++++++++++++++++
 fs/open.c  | 41 -----------------------------------------
 2 files changed, 41 insertions(+), 41 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index cd43ff89fbaa..e2bfd2a73cba 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4191,6 +4191,47 @@ inline struct dentry *user_path_create(int dfd, const char __user *pathname,
 }
 EXPORT_SYMBOL(user_path_create);
 
+/**
+ * dentry_create - Create and open a file
+ * @path: path to create
+ * @flags: O_ flags
+ * @mode: mode bits for new file
+ * @cred: credentials to use
+ *
+ * Caller must hold the parent directory's lock, and have prepared
+ * a negative dentry, placed in @path->dentry, for the new file.
+ *
+ * Caller sets @path->mnt to the vfsmount of the filesystem where
+ * the new file is to be created. The parent directory and the
+ * negative dentry must reside on the same filesystem instance.
+ *
+ * On success, returns a "struct file *". Otherwise a ERR_PTR
+ * is returned.
+ */
+struct file *dentry_create(const struct path *path, int flags, umode_t mode,
+			   const struct cred *cred)
+{
+	struct file *file;
+	int error;
+
+	file = alloc_empty_file(flags, cred);
+	if (IS_ERR(file))
+		return file;
+
+	error = vfs_create(mnt_idmap(path->mnt),
+			   d_inode(path->dentry->d_parent),
+			   path->dentry, mode, true);
+	if (!error)
+		error = vfs_open(path, file);
+
+	if (unlikely(error)) {
+		fput(file);
+		return ERR_PTR(error);
+	}
+	return file;
+}
+EXPORT_SYMBOL(dentry_create);
+
 /**
  * vfs_mknod - create device node or file
  * @idmap:	idmap of the mount the inode was found from
diff --git a/fs/open.c b/fs/open.c
index 9655158c3885..8fdece931f7d 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1142,47 +1142,6 @@ struct file *dentry_open_nonotify(const struct path *path, int flags,
 	return f;
 }
 
-/**
- * dentry_create - Create and open a file
- * @path: path to create
- * @flags: O_ flags
- * @mode: mode bits for new file
- * @cred: credentials to use
- *
- * Caller must hold the parent directory's lock, and have prepared
- * a negative dentry, placed in @path->dentry, for the new file.
- *
- * Caller sets @path->mnt to the vfsmount of the filesystem where
- * the new file is to be created. The parent directory and the
- * negative dentry must reside on the same filesystem instance.
- *
- * On success, returns a "struct file *". Otherwise a ERR_PTR
- * is returned.
- */
-struct file *dentry_create(const struct path *path, int flags, umode_t mode,
-			   const struct cred *cred)
-{
-	struct file *f;
-	int error;
-
-	f = alloc_empty_file(flags, cred);
-	if (IS_ERR(f))
-		return f;
-
-	error = vfs_create(mnt_idmap(path->mnt),
-			   d_inode(path->dentry->d_parent),
-			   path->dentry, mode, true);
-	if (!error)
-		error = vfs_open(path, f);
-
-	if (unlikely(error)) {
-		fput(f);
-		return ERR_PTR(error);
-	}
-	return f;
-}
-EXPORT_SYMBOL(dentry_create);
-
 /**
  * kernel_file_open - open a file for kernel internal use
  * @path:	path of the file to open
-- 
2.50.1

[PATCH v1 2/3] VFS: Prepare atomic_open() for dentry_create()

[Benjamin Coddington]

The next patch allows dentry_create() to call atomic_open(), but it does
not have fabricated nameidata.  Let atomic_open() take a path instead.

Signed-off-by: Benjamin Coddington <bcodding@hammerspace.com>
---
 fs/namei.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index e2bfd2a73cba..9c0aad5bbff7 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3552,19 +3552,16 @@ static int may_o_create(struct mnt_idmap *idmap,
  *
  * Returns an error code otherwise.
  */
-static struct dentry *atomic_open(struct nameidata *nd, struct dentry *dentry,
+static struct dentry *atomic_open(const struct path *path, struct dentry *dentry,
 				  struct file *file,
 				  int open_flag, umode_t mode)
 {
 	struct dentry *const DENTRY_NOT_SET = (void *) -1UL;
-	struct inode *dir =  nd->path.dentry->d_inode;
+	struct inode *dir =  path->dentry->d_inode;
 	int error;
 
-	if (nd->flags & LOOKUP_DIRECTORY)
-		open_flag |= O_DIRECTORY;
-
 	file->f_path.dentry = DENTRY_NOT_SET;
-	file->f_path.mnt = nd->path.mnt;
+	file->f_path.mnt = path->mnt;
 	error = dir->i_op->atomic_open(dir, dentry, file,
 				       open_to_namei_flags(open_flag), mode);
 	d_lookup_done(dentry);
@@ -3676,7 +3673,10 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
 	if (create_error)
 		open_flag &= ~O_CREAT;
 	if (dir_inode->i_op->atomic_open) {
-		dentry = atomic_open(nd, dentry, file, open_flag, mode);
+		if (nd->flags & LOOKUP_DIRECTORY)
+			open_flag |= O_DIRECTORY;
+
+		dentry = atomic_open(&nd->path, dentry, file, open_flag, mode);
 		if (unlikely(create_error) && dentry == ERR_PTR(-ENOENT))
 			dentry = ERR_PTR(create_error);
 		return dentry;
-- 
2.50.1

[PATCH v1 3/3] VFS/knfsd: Teach dentry_create() to use atomic_open()

[Benjamin Coddington]

While knfsd offers combined exclusive create and open results to clients,
on some filesystems those results may not be atomic.  This behavior can be
observed.  For example, an open O_CREAT with mode 0 will succeed in creating
the file but unexpectedly return -EACCES from vfs_open().

Additionally reducing the number of remote RPC calls required for O_CREAT
on network filesystem provides a performance benefit in the open path.

Teach knfsd's helper create_dentry() to use atomic_open() for filesystems
that support it.

Signed-off-by: Benjamin Coddington <bcodding@hammerspace.com>
---
 fs/namei.c         | 43 ++++++++++++++++++++++++++++++++++++-------
 fs/nfsd/nfs4proc.c |  8 +++++---
 include/linux/fs.h |  2 +-
 3 files changed, 42 insertions(+), 11 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 9c0aad5bbff7..70ab74fb5e95 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4208,21 +4208,50 @@ EXPORT_SYMBOL(user_path_create);
  * On success, returns a "struct file *". Otherwise a ERR_PTR
  * is returned.
  */
-struct file *dentry_create(const struct path *path, int flags, umode_t mode,
+struct file *dentry_create(struct path *path, int flags, umode_t mode,
 			   const struct cred *cred)
 {
+	struct dentry *dentry = path->dentry;
+	struct dentry *dir = dentry->d_parent;
+	struct inode *dir_inode = d_inode(dir);
+	struct mnt_idmap *idmap;
 	struct file *file;
-	int error;
+	int error, create_error;
 
 	file = alloc_empty_file(flags, cred);
 	if (IS_ERR(file))
 		return file;
 
-	error = vfs_create(mnt_idmap(path->mnt),
-			   d_inode(path->dentry->d_parent),
-			   path->dentry, mode, true);
-	if (!error)
-		error = vfs_open(path, file);
+	idmap = mnt_idmap(path->mnt);
+
+	if (dir_inode->i_op->atomic_open) {
+		path->dentry = dir;
+		mode = vfs_prepare_mode(idmap, dir_inode, mode, S_IALLUGO, S_IFREG);
+
+		create_error = may_o_create(idmap, path, dentry, mode);
+		if (create_error)
+			flags &= ~O_CREAT;
+
+		dentry = atomic_open(path, dentry, file, flags, mode);
+		error = PTR_ERR_OR_ZERO(dentry);
+
+		if (unlikely(create_error) && error == -ENOENT)
+			error = create_error;
+
+		if (!error) {
+			if (file->f_mode & FMODE_CREATED)
+				fsnotify_create(dir->d_inode, dentry);
+			if (file->f_mode & FMODE_OPENED)
+				fsnotify_open(file);
+		}
+
+		path->dentry = dentry;
+
+	} else {
+		error = vfs_create(idmap, dir_inode, dentry, mode, true);
+		if (!error)
+			error = vfs_open(path, file);
+	}
 
 	if (unlikely(error)) {
 		fput(file);
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 71b428efcbb5..7ff7e5855e58 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -194,7 +194,7 @@ static inline bool nfsd4_create_is_exclusive(int createmode)
 }
 
 static __be32
-nfsd4_vfs_create(struct svc_fh *fhp, struct dentry *child,
+nfsd4_vfs_create(struct svc_fh *fhp, struct dentry **child,
 		 struct nfsd4_open *open)
 {
 	struct file *filp;
@@ -214,9 +214,11 @@ nfsd4_vfs_create(struct svc_fh *fhp, struct dentry *child,
 	}
 
 	path.mnt = fhp->fh_export->ex_path.mnt;
-	path.dentry = child;
+	path.dentry = *child;
 	filp = dentry_create(&path, oflags, open->op_iattr.ia_mode,
 			     current_cred());
+	*child = path.dentry;
+
 	if (IS_ERR(filp))
 		return nfserrno(PTR_ERR(filp));
 
@@ -353,7 +355,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	status = fh_fill_pre_attrs(fhp);
 	if (status != nfs_ok)
 		goto out;
-	status = nfsd4_vfs_create(fhp, child, open);
+	status = nfsd4_vfs_create(fhp, &child, open);
 	if (status != nfs_ok)
 		goto out;
 	open->op_created = true;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 601d036a6c78..772b734477e5 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2878,7 +2878,7 @@ struct file *dentry_open(const struct path *path, int flags,
 			 const struct cred *creds);
 struct file *dentry_open_nonotify(const struct path *path, int flags,
 				  const struct cred *cred);
-struct file *dentry_create(const struct path *path, int flags, umode_t mode,
+struct file *dentry_create(struct path *path, int flags, umode_t mode,
 			   const struct cred *cred);
 struct path *backing_file_user_path(const struct file *f);
 
-- 
2.50.1

Re: [PATCH v1 0/3] Allow knfsd to use atomic_open()

[Chuck Lever]

On 11/18/25 11:33 AM, Benjamin Coddington wrote:
> We have workloads that will benefit from allowing knfsd to use atomic_open()
> in the open/create path.  There are two benefits; the first is the original
> matter of correctness: when knfsd must perform both vfs_create() and
> vfs_open() in series there can be races or error results that cause the
> caller to receive unexpected results.

Commit fb70bf124b05 ("NFSD: Instantiate a struct file when creating a
regular NFSv4 file") was supposed to address this. If there are still
issues, then a Fixes: tag and some explanation of where there are gaps
would be welcome in the commit message or cover letter. We might need
to identify LTS backport requirements, in that case.


> The second benefit is that for some
> network filesystems, we can reduce the number of remote round-trip
> operations by using a single atomic_open() path which provides a performance
> benefit. 
> 
> I've implemented this with the simplest possible change - by modifying
> dentry_create() which has a single user: knfsd.  The changes cause us to
> insert ourselves part-way into the previously closed/static atomic_open()
> path, so I expect VFS folks to have some good ideas about potentially
> superior approaches.
> 
> Thanks for any comment and critique.
> 
> Benjamin Coddington (3):
>   VFS: move dentry_create() from fs/open.c to fs/namei.c
>   VFS: Prepare atomic_open() for dentry_create()
>   VFS/knfsd: Teach dentry_create() to use atomic_open()
> 
>  fs/namei.c         | 84 ++++++++++++++++++++++++++++++++++++++++++----
>  fs/nfsd/nfs4proc.c |  8 +++--
>  fs/open.c          | 41 ----------------------
>  include/linux/fs.h |  2 +-
>  4 files changed, 83 insertions(+), 52 deletions(-)
> 


-- 
Chuck Lever

Re: [PATCH v1 0/3] Allow knfsd to use atomic_open()

[Benjamin Coddington]

On 18 Nov 2025, at 11:58, Chuck Lever wrote:

> On 11/18/25 11:33 AM, Benjamin Coddington wrote:
>> We have workloads that will benefit from allowing knfsd to use atomic_open()
>> in the open/create path.  There are two benefits; the first is the original
>> matter of correctness: when knfsd must perform both vfs_create() and
>> vfs_open() in series there can be races or error results that cause the
>> caller to receive unexpected results.
>
> Commit fb70bf124b05 ("NFSD: Instantiate a struct file when creating a
> regular NFSv4 file") was supposed to address this. If there are still
> issues, then a Fixes: tag and some explanation of where there are gaps
> would be welcome in the commit message or cover letter. We might need
> to identify LTS backport requirements, in that case.

The problem was noticed on a test that did open O_CREAT with mode 0 which
will succeed in creating the file but will return -EACCES from vfs_open() -
this specific test is mentioned in 3/3 description.  Insane case, but I
suppose someone might want it to behave properly.

Thanks for the commit pointer, I will check out the BugLink on it.  I will
add the Fixes tag in the next version if one emerges.

Ben

Re: [PATCH v1 0/3] Allow knfsd to use atomic_open()

[Trond Myklebust]

On Tue, 2025-11-18 at 11:58 -0500, Chuck Lever wrote:
> On 11/18/25 11:33 AM, Benjamin Coddington wrote:
> > We have workloads that will benefit from allowing knfsd to use
> > atomic_open()
> > in the open/create path.  There are two benefits; the first is the
> > original
> > matter of correctness: when knfsd must perform both vfs_create()
> > and
> > vfs_open() in series there can be races or error results that cause
> > the
> > caller to receive unexpected results.
> 
> Commit fb70bf124b05 ("NFSD: Instantiate a struct file when creating a
> regular NFSv4 file") was supposed to address this. If there are still
> issues, then a Fixes: tag and some explanation of where there are
> gaps
> would be welcome in the commit message or cover letter. We might need
> to identify LTS backport requirements, in that case.
> 

That patch only fixes the case where you're creating a local file and
then exporting it over NFSv4.

The case where we see a permissions problem is when creating a file
over NFSv4, and then exporting it over NFSv3.
i.e. it is the re-exporting over NFSv3 case.

Note that independently of the permissions issues, atomic_open also
solves races in open(O_CREAT|O_TRUNC). The NFS client now uses it for
both NFSv4 and NFSv3 for that reason.
See commit 7c6c5249f061 "NFS: add atomic_open for NFSv3 to handle
O_TRUNC correctly."

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

Re: [PATCH v1 3/3] VFS/knfsd: Teach dentry_create() to use atomic_open()

[Mike Snitzer]

On Tue, Nov 18, 2025 at 11:33:59AM -0500, Benjamin Coddington wrote:
> While knfsd offers combined exclusive create and open results to clients,
> on some filesystems those results may not be atomic.  This behavior can be
> observed.  For example, an open O_CREAT with mode 0 will succeed in creating
> the file but unexpectedly return -EACCES from vfs_open().
> 
> Additionally reducing the number of remote RPC calls required for O_CREAT
> on network filesystem provides a performance benefit in the open path.
> 
> Teach knfsd's helper create_dentry() to use atomic_open() for filesystems
> that support it.
> 
> Signed-off-by: Benjamin Coddington <bcodding@hammerspace.com>
> ---
>  fs/namei.c         | 43 ++++++++++++++++++++++++++++++++++++-------
>  fs/nfsd/nfs4proc.c |  8 +++++---
>  include/linux/fs.h |  2 +-
>  3 files changed, 42 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 9c0aad5bbff7..70ab74fb5e95 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4208,21 +4208,50 @@ EXPORT_SYMBOL(user_path_create);
>   * On success, returns a "struct file *". Otherwise a ERR_PTR
>   * is returned.
>   */
> -struct file *dentry_create(const struct path *path, int flags, umode_t mode,
> +struct file *dentry_create(struct path *path, int flags, umode_t mode,
>  			   const struct cred *cred)
>  {
> +	struct dentry *dentry = path->dentry;
> +	struct dentry *dir = dentry->d_parent;
> +	struct inode *dir_inode = d_inode(dir);
> +	struct mnt_idmap *idmap;
>  	struct file *file;
> -	int error;
> +	int error, create_error;
>  
>  	file = alloc_empty_file(flags, cred);
>  	if (IS_ERR(file))
>  		return file;
>  
> -	error = vfs_create(mnt_idmap(path->mnt),
> -			   d_inode(path->dentry->d_parent),
> -			   path->dentry, mode, true);
> -	if (!error)
> -		error = vfs_open(path, file);
> +	idmap = mnt_idmap(path->mnt);
> +
> +	if (dir_inode->i_op->atomic_open) {
> +		path->dentry = dir;
> +		mode = vfs_prepare_mode(idmap, dir_inode, mode, S_IALLUGO, S_IFREG);
> +
> +		create_error = may_o_create(idmap, path, dentry, mode);
> +		if (create_error)
> +			flags &= ~O_CREAT;
> +
> +		dentry = atomic_open(path, dentry, file, flags, mode);
> +		error = PTR_ERR_OR_ZERO(dentry);
> +
> +		if (unlikely(create_error) && error == -ENOENT)
> +			error = create_error;
> +
> +		if (!error) {
> +			if (file->f_mode & FMODE_CREATED)
> +				fsnotify_create(dir->d_inode, dentry);
> +			if (file->f_mode & FMODE_OPENED)
> +				fsnotify_open(file);
> +		}
> +
> +		path->dentry = dentry;
> +
> +	} else {
> +		error = vfs_create(idmap, dir_inode, dentry, mode, true);
> +		if (!error)
> +			error = vfs_open(path, file);
> +	}
>  
>  	if (unlikely(error)) {
>  		fput(file);
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 71b428efcbb5..7ff7e5855e58 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -194,7 +194,7 @@ static inline bool nfsd4_create_is_exclusive(int createmode)
>  }
>  
>  static __be32
> -nfsd4_vfs_create(struct svc_fh *fhp, struct dentry *child,
> +nfsd4_vfs_create(struct svc_fh *fhp, struct dentry **child,
>  		 struct nfsd4_open *open)
>  {
>  	struct file *filp;
> @@ -214,9 +214,11 @@ nfsd4_vfs_create(struct svc_fh *fhp, struct dentry *child,
>  	}
>  
>  	path.mnt = fhp->fh_export->ex_path.mnt;
> -	path.dentry = child;
> +	path.dentry = *child;
>  	filp = dentry_create(&path, oflags, open->op_iattr.ia_mode,
>  			     current_cred());
> +	*child = path.dentry;
> +
>  	if (IS_ERR(filp))
>  		return nfserrno(PTR_ERR(filp));
>  

Given the potential for side-effect due to dentry_create() now using
atomic_open() if available, I think you'd do well to update the
comment block above dentry_create to make it clear that the caller
really should pass along the dentry (regardless of whether
dentry_create returns an ERR_PTR).

> @@ -353,7 +355,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	status = fh_fill_pre_attrs(fhp);
>  	if (status != nfs_ok)
>  		goto out;
> -	status = nfsd4_vfs_create(fhp, child, open);
> +	status = nfsd4_vfs_create(fhp, &child, open);
>  	if (status != nfs_ok)
>  		goto out;
>  	open->op_created = true;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 601d036a6c78..772b734477e5 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2878,7 +2878,7 @@ struct file *dentry_open(const struct path *path, int flags,
>  			 const struct cred *creds);
>  struct file *dentry_open_nonotify(const struct path *path, int flags,
>  				  const struct cred *cred);
> -struct file *dentry_create(const struct path *path, int flags, umode_t mode,
> +struct file *dentry_create(struct path *path, int flags, umode_t mode,
>  			   const struct cred *cred);
>  struct path *backing_file_user_path(const struct file *f);
>  
> -- 
> 2.50.1
> 

Re: [PATCH v1 3/3] VFS/knfsd: Teach dentry_create() to use atomic_open()

[Benjamin Coddington]

On 18 Nov 2025, at 13:01, Mike Snitzer wrote:

> On Tue, Nov 18, 2025 at 11:33:59AM -0500, Benjamin Coddington wrote:
>
>>  	path.mnt = fhp->fh_export->ex_path.mnt;
>> -	path.dentry = child;
>> +	path.dentry = *child;
>>  	filp = dentry_create(&path, oflags, open->op_iattr.ia_mode,
>>  			     current_cred());
>> +	*child = path.dentry;
>> +
>>  	if (IS_ERR(filp))
>>  		return nfserrno(PTR_ERR(filp));
>>
>
> Given the potential for side-effect due to dentry_create() now using
> atomic_open() if available, I think you'd do well to update the
> comment block above dentry_create to make it clear that the caller
> really should pass along the dentry (regardless of whether
> dentry_create returns an ERR_PTR).

I will update the comment block to make it clear that the dentry you sent on
@path may be changed.

Ben

Re: [PATCH v1 0/3] Allow knfsd to use atomic_open()

[Jeff Layton]

On Tue, 2025-11-18 at 11:33 -0500, Benjamin Coddington wrote:
> We have workloads that will benefit from allowing knfsd to use atomic_open()
> in the open/create path.  There are two benefits; the first is the original
> matter of correctness: when knfsd must perform both vfs_create() and
> vfs_open() in series there can be races or error results that cause the
> caller to receive unexpected results.  The second benefit is that for some
> network filesystems, we can reduce the number of remote round-trip
> operations by using a single atomic_open() path which provides a performance
> benefit. 
> 
> I've implemented this with the simplest possible change - by modifying
> dentry_create() which has a single user: knfsd.  The changes cause us to
> insert ourselves part-way into the previously closed/static atomic_open()
> path, so I expect VFS folks to have some good ideas about potentially
> superior approaches.
> 
> Thanks for any comment and critique.
> 
> Benjamin Coddington (3):
>   VFS: move dentry_create() from fs/open.c to fs/namei.c
>   VFS: Prepare atomic_open() for dentry_create()
>   VFS/knfsd: Teach dentry_create() to use atomic_open()
> 
>  fs/namei.c         | 84 ++++++++++++++++++++++++++++++++++++++++++----
>  fs/nfsd/nfs4proc.c |  8 +++--
>  fs/open.c          | 41 ----------------------
>  include/linux/fs.h |  2 +-
>  4 files changed, 83 insertions(+), 52 deletions(-)

Nice work, Ben. This looks pretty reasonable to me, and I agree that
using atomic_open is desirable for preventing races.

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

Re: [PATCH v1 0/3] Allow knfsd to use atomic_open()

[NeilBrown]

On Wed, 19 Nov 2025, Benjamin Coddington wrote:
> We have workloads that will benefit from allowing knfsd to use atomic_open()
> in the open/create path.  There are two benefits; the first is the original
> matter of correctness: when knfsd must perform both vfs_create() and
> vfs_open() in series there can be races or error results that cause the
> caller to receive unexpected results.  The second benefit is that for some
> network filesystems, we can reduce the number of remote round-trip
> operations by using a single atomic_open() path which provides a performance
> benefit. 
> 
> I've implemented this with the simplest possible change - by modifying
> dentry_create() which has a single user: knfsd.  The changes cause us to
> insert ourselves part-way into the previously closed/static atomic_open()
> path, so I expect VFS folks to have some good ideas about potentially
> superior approaches.

I think using atomic_open is important - thanks for doing this.

I think there is another race this fixes.
If the client ends and unchecked v4 OPEN request, nfsd does a lookup and
finds the name doesn't exist, it will then (currently) use vfs_create()
requesting an exclusive create.  If this races with a create happening
from another client, this could result in -EEXIST which is not what the
client would expect.  Using atomic_open would fix this.

However I cannot see that you ever pass O_EXCL to atomic_open (or did I
miss something?).  So I don't think the code is quite right yet.  O_EXCL
should be passed is an exclusive or checked create was requested.

With a VFS hat on, I would rather there were more shared code between
dentry_create() and lookup_open().  I don't know exactly what this would
look like, and I wouldn't want that desire to hold up this patch, but it
might be worth thinking about to see if there are any easy similarities
to exploit.

Thanks,
NeilBrown


> 
> Thanks for any comment and critique.
> 
> Benjamin Coddington (3):
>   VFS: move dentry_create() from fs/open.c to fs/namei.c
>   VFS: Prepare atomic_open() for dentry_create()
>   VFS/knfsd: Teach dentry_create() to use atomic_open()
> 
>  fs/namei.c         | 84 ++++++++++++++++++++++++++++++++++++++++++----
>  fs/nfsd/nfs4proc.c |  8 +++--
>  fs/open.c          | 41 ----------------------
>  include/linux/fs.h |  2 +-
>  4 files changed, 83 insertions(+), 52 deletions(-)
> 
> -- 
> 2.50.1
> 
> 

Re: [PATCH v1 2/3] VFS: Prepare atomic_open() for dentry_create()

[NeilBrown]

On Wed, 19 Nov 2025, Benjamin Coddington wrote:
> The next patch allows dentry_create() to call atomic_open(), but it does
> not have fabricated nameidata.  Let atomic_open() take a path instead.

I think this commit message could usefully be longer and more details.

 atomic_open() currently takes a nameidata of which it only uses the
 path and the flags.  Flags are only used to update open_flags.  That
 update can happen before atomic_open() is called which would mean that
 only the path need be passed to atomic_open() rather than the whole
 nameidata.  This will make it easier for dentry_create() To call
 atomic_open().
	
Thanks,
NeilBrown

> 
> Signed-off-by: Benjamin Coddington <bcodding@hammerspace.com>
> ---
>  fs/namei.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index e2bfd2a73cba..9c0aad5bbff7 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3552,19 +3552,16 @@ static int may_o_create(struct mnt_idmap *idmap,
>   *
>   * Returns an error code otherwise.
>   */
> -static struct dentry *atomic_open(struct nameidata *nd, struct dentry *dentry,
> +static struct dentry *atomic_open(const struct path *path, struct dentry *dentry,
>  				  struct file *file,
>  				  int open_flag, umode_t mode)
>  {
>  	struct dentry *const DENTRY_NOT_SET = (void *) -1UL;
> -	struct inode *dir =  nd->path.dentry->d_inode;
> +	struct inode *dir =  path->dentry->d_inode;
>  	int error;
>  
> -	if (nd->flags & LOOKUP_DIRECTORY)
> -		open_flag |= O_DIRECTORY;
> -
>  	file->f_path.dentry = DENTRY_NOT_SET;
> -	file->f_path.mnt = nd->path.mnt;
> +	file->f_path.mnt = path->mnt;
>  	error = dir->i_op->atomic_open(dir, dentry, file,
>  				       open_to_namei_flags(open_flag), mode);
>  	d_lookup_done(dentry);
> @@ -3676,7 +3673,10 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
>  	if (create_error)
>  		open_flag &= ~O_CREAT;
>  	if (dir_inode->i_op->atomic_open) {
> -		dentry = atomic_open(nd, dentry, file, open_flag, mode);
> +		if (nd->flags & LOOKUP_DIRECTORY)
> +			open_flag |= O_DIRECTORY;
> +
> +		dentry = atomic_open(&nd->path, dentry, file, open_flag, mode);
>  		if (unlikely(create_error) && dentry == ERR_PTR(-ENOENT))
>  			dentry = ERR_PTR(create_error);
>  		return dentry;
> -- 
> 2.50.1
> 
> 

Re: [PATCH v1 3/3] VFS/knfsd: Teach dentry_create() to use atomic_open()

[NeilBrown]

On Wed, 19 Nov 2025, Benjamin Coddington wrote:
> While knfsd offers combined exclusive create and open results to clients,
> on some filesystems those results may not be atomic.  This behavior can be
> observed.  For example, an open O_CREAT with mode 0 will succeed in creating
> the file but unexpectedly return -EACCES from vfs_open().
> 
> Additionally reducing the number of remote RPC calls required for O_CREAT
> on network filesystem provides a performance benefit in the open path.
> 
> Teach knfsd's helper create_dentry() to use atomic_open() for filesystems
> that support it.
> 
> Signed-off-by: Benjamin Coddington <bcodding@hammerspace.com>
> ---
>  fs/namei.c         | 43 ++++++++++++++++++++++++++++++++++++-------
>  fs/nfsd/nfs4proc.c |  8 +++++---
>  include/linux/fs.h |  2 +-
>  3 files changed, 42 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 9c0aad5bbff7..70ab74fb5e95 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4208,21 +4208,50 @@ EXPORT_SYMBOL(user_path_create);
>   * On success, returns a "struct file *". Otherwise a ERR_PTR
>   * is returned.
>   */
> -struct file *dentry_create(const struct path *path, int flags, umode_t mode,
> +struct file *dentry_create(struct path *path, int flags, umode_t mode,

I don't like that you dropped "const" without telling us why.
It is because we not assign to path->dentry, which is because
atomic_open() returns a dentry....  which will only be different for
directories (I think).

But do we need to update path?  The returned file will point to the
correct dentry - isn't that all that matters?

I guess that I'd like an explanation for why the const is being dropped,
and why 'path' is being changed.

Thanks,
NeilBrown



>  			   const struct cred *cred)
>  {
> +	struct dentry *dentry = path->dentry;
> +	struct dentry *dir = dentry->d_parent;
> +	struct inode *dir_inode = d_inode(dir);
> +	struct mnt_idmap *idmap;
>  	struct file *file;
> -	int error;
> +	int error, create_error;
>  
>  	file = alloc_empty_file(flags, cred);
>  	if (IS_ERR(file))
>  		return file;
>  
> -	error = vfs_create(mnt_idmap(path->mnt),
> -			   d_inode(path->dentry->d_parent),
> -			   path->dentry, mode, true);
> -	if (!error)
> -		error = vfs_open(path, file);
> +	idmap = mnt_idmap(path->mnt);
> +
> +	if (dir_inode->i_op->atomic_open) {
> +		path->dentry = dir;
> +		mode = vfs_prepare_mode(idmap, dir_inode, mode, S_IALLUGO, S_IFREG);
> +
> +		create_error = may_o_create(idmap, path, dentry, mode);
> +		if (create_error)
> +			flags &= ~O_CREAT;
> +
> +		dentry = atomic_open(path, dentry, file, flags, mode);
> +		error = PTR_ERR_OR_ZERO(dentry);
> +
> +		if (unlikely(create_error) && error == -ENOENT)
> +			error = create_error;
> +
> +		if (!error) {
> +			if (file->f_mode & FMODE_CREATED)
> +				fsnotify_create(dir->d_inode, dentry);
> +			if (file->f_mode & FMODE_OPENED)
> +				fsnotify_open(file);
> +		}
> +
> +		path->dentry = dentry;
> +
> +	} else {
> +		error = vfs_create(idmap, dir_inode, dentry, mode, true);
> +		if (!error)
> +			error = vfs_open(path, file);
> +	}
>  
>  	if (unlikely(error)) {
>  		fput(file);
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 71b428efcbb5..7ff7e5855e58 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -194,7 +194,7 @@ static inline bool nfsd4_create_is_exclusive(int createmode)
>  }
>  
>  static __be32
> -nfsd4_vfs_create(struct svc_fh *fhp, struct dentry *child,
> +nfsd4_vfs_create(struct svc_fh *fhp, struct dentry **child,
>  		 struct nfsd4_open *open)
>  {
>  	struct file *filp;
> @@ -214,9 +214,11 @@ nfsd4_vfs_create(struct svc_fh *fhp, struct dentry *child,
>  	}
>  
>  	path.mnt = fhp->fh_export->ex_path.mnt;
> -	path.dentry = child;
> +	path.dentry = *child;
>  	filp = dentry_create(&path, oflags, open->op_iattr.ia_mode,
>  			     current_cred());
> +	*child = path.dentry;
> +
>  	if (IS_ERR(filp))
>  		return nfserrno(PTR_ERR(filp));
>  
> @@ -353,7 +355,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	status = fh_fill_pre_attrs(fhp);
>  	if (status != nfs_ok)
>  		goto out;
> -	status = nfsd4_vfs_create(fhp, child, open);
> +	status = nfsd4_vfs_create(fhp, &child, open);
>  	if (status != nfs_ok)
>  		goto out;
>  	open->op_created = true;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 601d036a6c78..772b734477e5 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2878,7 +2878,7 @@ struct file *dentry_open(const struct path *path, int flags,
>  			 const struct cred *creds);
>  struct file *dentry_open_nonotify(const struct path *path, int flags,
>  				  const struct cred *cred);
> -struct file *dentry_create(const struct path *path, int flags, umode_t mode,
> +struct file *dentry_create(struct path *path, int flags, umode_t mode,
>  			   const struct cred *cred);
>  struct path *backing_file_user_path(const struct file *f);
>  
> -- 
> 2.50.1
> 
> 

Re: [PATCH v1 0/3] Allow knfsd to use atomic_open()

[Benjamin Coddington]

On 18 Nov 2025, at 20:23, NeilBrown wrote:

> On Wed, 19 Nov 2025, Benjamin Coddington wrote:
>> We have workloads that will benefit from allowing knfsd to use atomic_open()
>> in the open/create path.  There are two benefits; the first is the original
>> matter of correctness: when knfsd must perform both vfs_create() and
>> vfs_open() in series there can be races or error results that cause the
>> caller to receive unexpected results.  The second benefit is that for some
>> network filesystems, we can reduce the number of remote round-trip
>> operations by using a single atomic_open() path which provides a performance
>> benefit.
>>
>> I've implemented this with the simplest possible change - by modifying
>> dentry_create() which has a single user: knfsd.  The changes cause us to
>> insert ourselves part-way into the previously closed/static atomic_open()
>> path, so I expect VFS folks to have some good ideas about potentially
>> superior approaches.
>
> I think using atomic_open is important - thanks for doing this.
>
> I think there is another race this fixes.
> If the client ends and unchecked v4 OPEN request, nfsd does a lookup and
> finds the name doesn't exist, it will then (currently) use vfs_create()
> requesting an exclusive create.  If this races with a create happening
> from another client, this could result in -EEXIST which is not what the
> client would expect.  Using atomic_open would fix this.
>
> However I cannot see that you ever pass O_EXCL to atomic_open (or did I
> miss something?).  So I don't think the code is quite right yet.  O_EXCL
> should be passed is an exclusive or checked create was requested.

Ah, it's true.  I did not validate knfsd's behaviors, only its interface with
VFS.  IIUC knfsd gets around needing to pass O_EXCL by holding the directory
inode lock over the create, and since it doesn't need to do lookup because
it already has a filehandle, I think O_EXCL is moot.

> With a VFS hat on, I would rather there were more shared code between
> dentry_create() and lookup_open().  I don't know exactly what this would
> look like, and I wouldn't want that desire to hold up this patch, but it
> might be worth thinking about to see if there are any easy similarities
> to exploit.

I agree, that would be nice.  It would definitely be a bigger touch, and I
was going for the minimal change here.

Thanks for looking at this Neil.

Ben

Re: [PATCH v1 3/3] VFS/knfsd: Teach dentry_create() to use atomic_open()

[Benjamin Coddington]

On 18 Nov 2025, at 20:41, NeilBrown wrote:

> On Wed, 19 Nov 2025, Benjamin Coddington wrote:
>> While knfsd offers combined exclusive create and open results to clients,
>> on some filesystems those results may not be atomic.  This behavior can be
>> observed.  For example, an open O_CREAT with mode 0 will succeed in creating
>> the file but unexpectedly return -EACCES from vfs_open().
>>
>> Additionally reducing the number of remote RPC calls required for O_CREAT
>> on network filesystem provides a performance benefit in the open path.
>>
>> Teach knfsd's helper create_dentry() to use atomic_open() for filesystems
>> that support it.
>>
>> Signed-off-by: Benjamin Coddington <bcodding@hammerspace.com>
>> ---
>>  fs/namei.c         | 43 ++++++++++++++++++++++++++++++++++++-------
>>  fs/nfsd/nfs4proc.c |  8 +++++---
>>  include/linux/fs.h |  2 +-
>>  3 files changed, 42 insertions(+), 11 deletions(-)
>>
>> diff --git a/fs/namei.c b/fs/namei.c
>> index 9c0aad5bbff7..70ab74fb5e95 100644
>> --- a/fs/namei.c
>> +++ b/fs/namei.c
>> @@ -4208,21 +4208,50 @@ EXPORT_SYMBOL(user_path_create);
>>   * On success, returns a "struct file *". Otherwise a ERR_PTR
>>   * is returned.
>>   */
>> -struct file *dentry_create(const struct path *path, int flags, umode_t mode,
>> +struct file *dentry_create(struct path *path, int flags, umode_t mode,
>
> I don't like that you dropped "const" without telling us why.
> It is because we not assign to path->dentry, which is because
> atomic_open() returns a dentry....  which will only be different for
> directories (I think).
>
> But do we need to update path?  The returned file will point to the
> correct dentry - isn't that all that matters?
>
> I guess that I'd like an explanation for why the const is being dropped,
> and why 'path' is being changed.

Well, the first reason was that I was embarrassed at all the new local
variables being added and atomic_open() wanted path->parent and
dentry_create() already had path->child, it was convenient to just re-use
it.  Then it became clear that nfsd4_create_file() really wants to clean up
(or not) the reference to its "child" dentry based on whether the dentry had
been consumed or was an error - so passing back that dentry rather than
re-arrange the tail of nfsd4_create_file() seemed nicer.

Its true that we can acquire the dentry from file->f_path.dentry, but only
in the successful case for both the atomic_open() and the
vfs_create()/vfs_open() path.  Atomic_open() does the work of swapping and
fiddling with the dentry refcounts for us so we don't need to check, so that
is the 2nd reason I passed the dentry back on struct path.

I don't understand the cases atomic_open() is handling here:

3555 static struct dentry *atomic_open(const struct path *path, struct dentry *dentry,
...
3568     if (!error) {
3569         if (file->f_mode & FMODE_OPENED) {
3570             if (unlikely(dentry != file->f_path.dentry)) {
3571                 dput(dentry);
3572                 dentry = dget(file->f_path.dentry);
3573             }

You think this can only happen for a directory?  I figured VFS trying to
work around whatever might have happened inside the filesystem.

One thing that's not happening is that if knfsd /does/ get a different
dentry back, its not updating struct svc_fh->fh_dentry.

Ben

Re: [PATCH v1 2/3] VFS: Prepare atomic_open() for dentry_create()

[Benjamin Coddington]

On 18 Nov 2025, at 20:32, NeilBrown wrote:

> On Wed, 19 Nov 2025, Benjamin Coddington wrote:
>> The next patch allows dentry_create() to call atomic_open(), but it does
>> not have fabricated nameidata.  Let atomic_open() take a path instead.
>
> I think this commit message could usefully be longer and more details.
>
>  atomic_open() currently takes a nameidata of which it only uses the
>  path and the flags.  Flags are only used to update open_flags.  That
>  update can happen before atomic_open() is called which would mean that
>  only the path need be passed to atomic_open() rather than the whole
>  nameidata.  This will make it easier for dentry_create() To call
>  atomic_open().

Thank you!  I'll add this.

Ben

Re: [PATCH v1 0/3] Allow knfsd to use atomic_open()

[NeilBrown]

On Wed, 19 Nov 2025, Benjamin Coddington wrote:
> On 18 Nov 2025, at 20:23, NeilBrown wrote:
> 
> > On Wed, 19 Nov 2025, Benjamin Coddington wrote:
> >> We have workloads that will benefit from allowing knfsd to use atomic_open()
> >> in the open/create path.  There are two benefits; the first is the original
> >> matter of correctness: when knfsd must perform both vfs_create() and
> >> vfs_open() in series there can be races or error results that cause the
> >> caller to receive unexpected results.  The second benefit is that for some
> >> network filesystems, we can reduce the number of remote round-trip
> >> operations by using a single atomic_open() path which provides a performance
> >> benefit.
> >>
> >> I've implemented this with the simplest possible change - by modifying
> >> dentry_create() which has a single user: knfsd.  The changes cause us to
> >> insert ourselves part-way into the previously closed/static atomic_open()
> >> path, so I expect VFS folks to have some good ideas about potentially
> >> superior approaches.
> >
> > I think using atomic_open is important - thanks for doing this.
> >
> > I think there is another race this fixes.
> > If the client ends and unchecked v4 OPEN request, nfsd does a lookup and
> > finds the name doesn't exist, it will then (currently) use vfs_create()
> > requesting an exclusive create.  If this races with a create happening
> > from another client, this could result in -EEXIST which is not what the
> > client would expect.  Using atomic_open would fix this.
> >
> > However I cannot see that you ever pass O_EXCL to atomic_open (or did I
> > miss something?).  So I don't think the code is quite right yet.  O_EXCL
> > should be passed is an exclusive or checked create was requested.
> 
> Ah, it's true.  I did not validate knfsd's behaviors, only its interface with
> VFS.  IIUC knfsd gets around needing to pass O_EXCL by holding the directory
> inode lock over the create, and since it doesn't need to do lookup because
> it already has a filehandle, I think O_EXCL is moot.

Holding the directory lock is sufficient for providing O_EXCL for local
filesystems which will be blocked from creating while that lock is held.
It is *not* sufficient for remote filesystems which are precisely those
which provide ->atomic_open.

The fact that you are adding support for atomic_open means that O_EXCL
isn't moot.

I don't know what you mean by "since it doesn't need to do lookup because
it already has a filehandle".  What filehandle does it already have?

Thanks,
NeilBrown


> 
> > With a VFS hat on, I would rather there were more shared code between
> > dentry_create() and lookup_open().  I don't know exactly what this would
> > look like, and I wouldn't want that desire to hold up this patch, but it
> > might be worth thinking about to see if there are any easy similarities
> > to exploit.
> 
> I agree, that would be nice.  It would definitely be a bigger touch, and I
> was going for the minimal change here.
> 
> Thanks for looking at this Neil.
> 
> Ben
> 

Re: [PATCH v1 0/3] Allow knfsd to use atomic_open()

[Benjamin Coddington]

On 20 Nov 2025, at 17:26, NeilBrown wrote:

> On Wed, 19 Nov 2025, Benjamin Coddington wrote:
>
>> Ah, it's true.  I did not validate knfsd's behaviors, only its interface with
>> VFS.  IIUC knfsd gets around needing to pass O_EXCL by holding the directory
>> inode lock over the create, and since it doesn't need to do lookup because
>> it already has a filehandle, I think O_EXCL is moot.
>
> Holding the directory lock is sufficient for providing O_EXCL for local
> filesystems which will be blocked from creating while that lock is held.
> It is *not* sufficient for remote filesystems which are precisely those
> which provide ->atomic_open.
>
> The fact that you are adding support for atomic_open means that O_EXCL
> isn't moot.

I mean to say: knfsd doesn't need to pass O_EXCL because its already taking
care to produce an exclusive open via nfsv4 semantics.

> I don't know what you mean by "since it doesn't need to do lookup because
> it already has a filehandle".  What filehandle does it already have?

The client has sent along the filehandle of the parent directory, and knfsd
has already done lookup_one() on the child name, and we pass along that
negative dentry thet we looked up while holding the directory's inode lock.

Ben

Re: [PATCH v1 0/3] Allow knfsd to use atomic_open()

[NeilBrown]

On Fri, 21 Nov 2025, Benjamin Coddington wrote:
> On 20 Nov 2025, at 17:26, NeilBrown wrote:
> 
> > On Wed, 19 Nov 2025, Benjamin Coddington wrote:
> >
> >> Ah, it's true.  I did not validate knfsd's behaviors, only its interface with
> >> VFS.  IIUC knfsd gets around needing to pass O_EXCL by holding the directory
> >> inode lock over the create, and since it doesn't need to do lookup because
> >> it already has a filehandle, I think O_EXCL is moot.
> >
> > Holding the directory lock is sufficient for providing O_EXCL for local
> > filesystems which will be blocked from creating while that lock is held.
> > It is *not* sufficient for remote filesystems which are precisely those
> > which provide ->atomic_open.
> >
> > The fact that you are adding support for atomic_open means that O_EXCL
> > isn't moot.
> 
> I mean to say: knfsd doesn't need to pass O_EXCL because its already taking
> care to produce an exclusive open via nfsv4 semantics.

Huh?

The interesting circumstance here is an NFS re-export of an NFS
filesystem - is that right?

The only way that an exclusive create can be achieved on the target
filesystem is if an NFS4_CREATE_EXCLUSIVE4_1 (or similar) create request
is sent to the ultimate sever.  There is nothing knfsd can do to
produce exclusive open semantics on a remote NFS serve except to
explicitly request them.

> 
> > I don't know what you mean by "since it doesn't need to do lookup because
> > it already has a filehandle".  What filehandle does it already have?
> 
> The client has sent along the filehandle of the parent directory, and knfsd
> has already done lookup_one() on the child name, and we pass along that
> negative dentry thet we looked up while holding the directory's inode lock.

This (holding the directory's inode lock) works perfectly well for local
filesystems (which don't implement ->atomic_open).  It has no effect on
remote filesystems (which is why we have ->atomic_open).

Thanks,
NeilBrown

Re: [PATCH v1 0/3] Allow knfsd to use atomic_open()

[Benjamin Coddington]

On 26 Nov 2025, at 15:59, NeilBrown wrote:

> On Fri, 21 Nov 2025, Benjamin Coddington wrote:
>> On 20 Nov 2025, at 17:26, NeilBrown wrote:
>>
>>> On Wed, 19 Nov 2025, Benjamin Coddington wrote:
>>>
>>>> Ah, it's true.  I did not validate knfsd's behaviors, only its interface with
>>>> VFS.  IIUC knfsd gets around needing to pass O_EXCL by holding the directory
>>>> inode lock over the create, and since it doesn't need to do lookup because
>>>> it already has a filehandle, I think O_EXCL is moot.
>>>
>>> Holding the directory lock is sufficient for providing O_EXCL for local
>>> filesystems which will be blocked from creating while that lock is held.
>>> It is *not* sufficient for remote filesystems which are precisely those
>>> which provide ->atomic_open.
>>>
>>> The fact that you are adding support for atomic_open means that O_EXCL
>>> isn't moot.
>>
>> I mean to say: knfsd doesn't need to pass O_EXCL because its already taking
>> care to produce an exclusive open via nfsv4 semantics.
>
> Huh?
>
> The interesting circumstance here is an NFS re-export of an NFS
> filesystem - is that right?

That's right.

> The only way that an exclusive create can be achieved on the target
> filesystem is if an NFS4_CREATE_EXCLUSIVE4_1 (or similar) create request
> is sent to the ultimate sever.  There is nothing knfsd can do to
> produce exclusive open semantics on a remote NFS serve except to
> explicitly request them.

True - but I haven't really been worried about that, so I think I see what
you're getting at now - you'd like kNFSD to start using O_EXCL when it
receives NFS4_CREATE_EXCLUSIVE4_1.

I think that's a whole different change on its own, but not necessary
here because these changes are targeting a very specific problem - the
problem where open(O_CREAT) is done in two operations on the remote
filesystem.  That problem is solved by this patchset, and I don't think the
solution is incomplete because we're not passing O_EXCL for the
NFS4_CREATE_EXCLUSIVE{4_1} case.  I think that's a new enhancement - one
that I haven't thought through (yet) or tested.

Up until now, kNFSD has not bothered fiddling with O_EXCL because of the
reasons I listed above - for local filesystems or remote.

Do you disagree that the changes here for the open(O_CREAT) problem is
incomplete without new O_EXCL passing to atomic_open()?  If so, do we also
need to consider passing O_EXCL when kNFSD does vfs_open() for the case when
the filesystem does not have atomic_open()?

Thanks for engaging with me,
Ben

Re: [PATCH v1 0/3] Allow knfsd to use atomic_open()

[NeilBrown]

On Thu, 27 Nov 2025, Benjamin Coddington wrote:
> On 26 Nov 2025, at 15:59, NeilBrown wrote:
> 
> > On Fri, 21 Nov 2025, Benjamin Coddington wrote:
> >> On 20 Nov 2025, at 17:26, NeilBrown wrote:
> >>
> >>> On Wed, 19 Nov 2025, Benjamin Coddington wrote:
> >>>
> >>>> Ah, it's true.  I did not validate knfsd's behaviors, only its interface with
> >>>> VFS.  IIUC knfsd gets around needing to pass O_EXCL by holding the directory
> >>>> inode lock over the create, and since it doesn't need to do lookup because
> >>>> it already has a filehandle, I think O_EXCL is moot.
> >>>
> >>> Holding the directory lock is sufficient for providing O_EXCL for local
> >>> filesystems which will be blocked from creating while that lock is held.
> >>> It is *not* sufficient for remote filesystems which are precisely those
> >>> which provide ->atomic_open.
> >>>
> >>> The fact that you are adding support for atomic_open means that O_EXCL
> >>> isn't moot.
> >>
> >> I mean to say: knfsd doesn't need to pass O_EXCL because its already taking
> >> care to produce an exclusive open via nfsv4 semantics.
> >
> > Huh?
> >
> > The interesting circumstance here is an NFS re-export of an NFS
> > filesystem - is that right?
> 
> That's right.
> 
> > The only way that an exclusive create can be achieved on the target
> > filesystem is if an NFS4_CREATE_EXCLUSIVE4_1 (or similar) create request
> > is sent to the ultimate sever.  There is nothing knfsd can do to
> > produce exclusive open semantics on a remote NFS serve except to
> > explicitly request them.
> 
> True - but I haven't really been worried about that, so I think I see what
> you're getting at now - you'd like kNFSD to start using O_EXCL when it
> receives NFS4_CREATE_EXCLUSIVE4_1.
> 
> I think that's a whole different change on its own, but not necessary
> here because these changes are targeting a very specific problem - the
> problem where open(O_CREAT) is done in two operations on the remote
> filesystem.  That problem is solved by this patchset, and I don't think the
> solution is incomplete because we're not passing O_EXCL for the
> NFS4_CREATE_EXCLUSIVE{4_1} case.  I think that's a new enhancement - one
> that I haven't thought through (yet) or tested.
> 
> Up until now, kNFSD has not bothered fiddling with O_EXCL because of the
> reasons I listed above - for local filesystems or remote.
> 
> Do you disagree that the changes here for the open(O_CREAT) problem is
> incomplete without new O_EXCL passing to atomic_open()? 

It isn't so much that the change is incomplete.  Rather, the change
introduces a regression.

The old code was

-	error = vfs_create(mnt_idmap(path->mnt),
-			   d_inode(path->dentry->d_parent),
-			   path->dentry, mode, true);


Note the "true" at the end.  This instructs nfs_create() to pass O_EXCL
to nfs_do_create() so an over-the-wire exclusive create is performed.

The new code is

+		dentry = atomic_open(path, dentry, file, flags, mode);

Where "flags" is oflags from nfsd4_vfs_create() which is 
   O_CREAT| O_LARGEFILE | O_(read/write/rdwr)
and no O_EXCL.
(When atomic_open is called by lookup_open, "open_flag" is passed which
might contain O_EXCL).

>                                                          If so, do we also
> need to consider passing O_EXCL when kNFSD does vfs_open() for the case when
> the filesystem does not have atomic_open()?

No as vfs_open() doesn't do the create, vfs_create() does that.
And we do need to pass (the equivalent of) O_EXCL when calling
vfs_create().  In fact we do - that 'true' as the large arg means
exactly O_EXCL.
(really we shouldn't be passing 'true' if an exclusive create wasn't
requested, but it only makes a difference for filesystems that support
->atomic_open, so it doesn't actually matter what we pass - and Jeff
has a patch to remove that last arg to vfs_create()).


> 
> Thanks for engaging with me,
> Ben
> 

Thanks,
NeilBrown

Re: [PATCH v1 0/3] Allow knfsd to use atomic_open()

[Benjamin Coddington]

On 26 Nov 2025, at 19:36, NeilBrown wrote:
> It isn't so much that the change is incomplete.  Rather, the change
> introduces a regression.
>
> The old code was
>
> -	error = vfs_create(mnt_idmap(path->mnt),
> -			   d_inode(path->dentry->d_parent),
> -			   path->dentry, mode, true);
>
>
> Note the "true" at the end.  This instructs nfs_create() to pass O_EXCL
> to nfs_do_create() so an over-the-wire exclusive create is performed.
>
> The new code is
>
> +		dentry = atomic_open(path, dentry, file, flags, mode);
>
> Where "flags" is oflags from nfsd4_vfs_create() which is
>    O_CREAT| O_LARGEFILE | O_(read/write/rdwr)
> and no O_EXCL.
> (When atomic_open is called by lookup_open, "open_flag" is passed which
> might contain O_EXCL).

Of course, you're quite right, I should put more effort into trying to
understand your very first reply.

Fixing this up seems simple enough, I think we just need to do this on top
of what's here:

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 7e39234e0649..6990ba92bca1 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -202,6 +202,9 @@ nfsd4_vfs_create(struct svc_fh *fhp, struct dentry **child,
        int oflags;

        oflags = O_CREAT | O_LARGEFILE;
+       if (nfsd4_create_is_exclusive(open->op_createmode))
+               oflags |= O_EXCL;
+
        switch (open->op_share_access & NFS4_SHARE_ACCESS_BOTH) {
        case NFS4_SHARE_ACCESS_WRITE:
                oflags |= O_WRONLY;

I will send this through my re-export testing, but I don't think that its
going to produce different results because we lack a multi-client test to
detect cases for O_EXCL.

Ben