[PATCH v2 00/14] nfsd: refactor nfs4_create_file()

[PATCH v2 00/14] nfsd: refactor nfs4_create_file()

[NeilBrown]

This v2 incorporates changes to address issues reported by sashiko.dev.
Of particular interest is that for open/create we now get pre/post
attributes OUTSIDE of the locked region, so we cannot tell the client
that they are atomic.  This is a slight regression in that it may
require the client to revalidate the directory contents more often.  We
may be able to address it by using directory leases.  It would rather
this were done after this series lands, though I will try to get it done
before making locking changes for other operations (mkdir, link,
symlink, etc).

This series also goes further than the previous series by completing all
the nfsd changes.  Once the planned vfs_lookup_open() becomes available
we can simply switch from the current do_lookup_open() to that.

Original series description was:

nfs4_create_file() duplicates knowledge about opening a file which
exists in the VFS, mostly in lookup_open().  It does use dentry_create()
which shares some code, but there is more code that could be shared.

The nfsd code doesn't get some details quite right, particularly patch
06 shows this.

I hope to introduce a new VFS interface which encapulates more of what
nfsd needs and shares more code with lookup_open().  I particularly want
this as it will simplify some changes to locking rules that I am working
on.

This series re-arranges the nfsd code to get it ready for switching to
the new interface.  Once that interface lands we can then switch over
fairly easily.  Hopefully the series helps clarity even before that
happens.

The series is against nfsd-testing.

Thanks,
NeilBrown

 [PATCH v2 01/14] nfsd: honour client-provided attributes for
 [PATCH v2 02/14] nfsd: replace fh_fill_both_attrs() with
 [PATCH v2 03/14] nfsd: move fh_want_write() after preamble in
 [PATCH v2 04/14] nfsd: move more nfs-specific code into preamble of
 [PATCH v2 05/14] nfsd: remove subtlety from nfsd4_create_file()
 [PATCH v2 06/14] nfsd: in nfsd4_create_file() let VFS report if file
 [PATCH v2 07/14] nfsd: nfsd4_create_file(): Move NFSD_MAY_CREATE
 [PATCH v2 08/14] nfsd: always open file in nfsd4_create_file()
 [PATCH v2 09/14] nfsd: reduce range of directory lock in
 [PATCH v2 10/14] nfsd: open-code nfsd4_vfs_create() into
 [PATCH v2 11/14] nfsd: move some code out of the
 [PATCH v2 12/14] nfsd: reduce want-write range in nfsd4_create_file()
 [PATCH v2 13/14] nfsd: separate out VFS-specific from from
 [PATCH v2 14/14] nfsd: use do_lookup_open() for non-creating open

[PATCH v2 01/14] nfsd: honour client-provided attributes for NFS4_CREATE_EXCLUSIVE4_1

[NeilBrown]

From: NeilBrown <neil@brown.name>

When a file is created with a v4.1 OPEN which requests
NFS4_CREATE_EXCLUSIVE4_1, the request can include attributes to be set.
However when the mtime/atime are set to hold the verifier, the other
ia_valid flags are cleared, so no attributes requested by the client are
used.

This code was originally written for NFSv3 where NFS3_CREATE_EXCLUSIVE
never includes attributes.  When it was updated for v4.1, the fact that an
exclusive create CAN include attributes was not handled properly.

Fixes: ac6721a13e5b ("nfsd41: make sure nfs server process OPEN with EXCLUSIVE4_1 correctly")
Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4proc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 669896be08b6..f8afc356809e 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -393,8 +393,8 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if ((iap->ia_valid & ATTR_SIZE) && (iap->ia_size == 0))
 		iap->ia_valid &= ~ATTR_SIZE;
 	if (nfsd4_create_is_exclusive(open->op_createmode)) {
-		iap->ia_valid = ATTR_MTIME | ATTR_ATIME |
-				ATTR_MTIME_SET|ATTR_ATIME_SET;
+		iap->ia_valid |= ATTR_MTIME | ATTR_ATIME |
+				 ATTR_MTIME_SET|ATTR_ATIME_SET;
 		iap->ia_mtime.tv_sec = v_mtime;
 		iap->ia_atime.tv_sec = v_atime;
 		iap->ia_mtime.tv_nsec = 0;
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH v2 01/14] nfsd: honour client-provided attributes for NFS4_CREATE_EXCLUSIVE4_1

[Jeff Layton]

On Mon, 2026-07-06 at 08:19 +1000, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> When a file is created with a v4.1 OPEN which requests
> NFS4_CREATE_EXCLUSIVE4_1, the request can include attributes to be set.
> However when the mtime/atime are set to hold the verifier, the other
> ia_valid flags are cleared, so no attributes requested by the client are
> used.
> 
> This code was originally written for NFSv3 where NFS3_CREATE_EXCLUSIVE
> never includes attributes.  When it was updated for v4.1, the fact that an
> exclusive create CAN include attributes was not handled properly.
> 
> Fixes: ac6721a13e5b ("nfsd41: make sure nfs server process OPEN with EXCLUSIVE4_1 correctly")
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4proc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 669896be08b6..f8afc356809e 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -393,8 +393,8 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	if ((iap->ia_valid & ATTR_SIZE) && (iap->ia_size == 0))
>  		iap->ia_valid &= ~ATTR_SIZE;
>  	if (nfsd4_create_is_exclusive(open->op_createmode)) {
> -		iap->ia_valid = ATTR_MTIME | ATTR_ATIME |
> -				ATTR_MTIME_SET|ATTR_ATIME_SET;
> +		iap->ia_valid |= ATTR_MTIME | ATTR_ATIME |
> +				 ATTR_MTIME_SET|ATTR_ATIME_SET;
>  		iap->ia_mtime.tv_sec = v_mtime;
>  		iap->ia_atime.tv_sec = v_atime;
>  		iap->ia_mtime.tv_nsec = 0;

Nice catch. I wonder what we've been missing (if anything) all this
time?

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

[PATCH v2 02/14] nfsd: replace fh_fill_both_attrs() with fh_fill_post_noop()

[NeilBrown]

From: NeilBrown <neil@brown.name>

fh_fill_both_attrs() is only needed for open/create and is used in the
case when the target already existed so no creating happens.

As part of refactoring this code it is changed to call
fh_fill_pre_attrs() once early on (so errors only need to be caught in
one place) and then to use a new fh_fill_post_noop() when it is
determined that no creation happened.

fh_fill_pre_attrs() now stores the attrs (which it had to get all of
anyway)_ in ->fh_post_attr.  fh_fill_post_noop() simply marks them as
valid.  fh_fill_post_attrs() replaces them.

This change involves moving fh_fill_pre_attrs() out of the inode_lock on
the directory.  This means that we cannot provide "atomic" wcc data so a
new fh_fill_pre_attrs_unlocked() is provided which marks the attrs as
non-atomic.

This is unfortunate but inevitable if we are ever to allow concurrent
updates in a directory (which can significantly improve performance in
some cases).  To get atomic pre/post attributes we will need to be able
to ask the fs to provide them, or to request a lease on the directory
for the duration of an operation.

Note that we haven't provided pre/post attrs on WRITE requests for a
long time for exactly this reason - we cannot lock the file to get them.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4proc.c | 20 ++++++--------
 fs/nfsd/nfsfh.c    | 69 +++++++++++++++++++++++-----------------------
 fs/nfsd/nfsfh.h    | 14 +++++++++-
 3 files changed, 56 insertions(+), 47 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index f8afc356809e..a93132323b66 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -327,9 +327,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		/* NFSv4 protocol requires change attributes even though
 		 * no change happened.
 		 */
-		status = fh_fill_both_attrs(fhp);
-		if (status != nfs_ok)
-			goto out;
+		fh_fill_post_noop(fhp);
 
 		status = fh_compose(resfhp, fhp->fh_export, child, fhp);
 		if (status != nfs_ok)
@@ -376,9 +374,6 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if (!IS_POSIXACL(inode))
 		iap->ia_mode &= ~current_umask();
 
-	status = fh_fill_pre_attrs(fhp);
-	if (status != nfs_ok)
-		goto out;
 	status = nfsd4_vfs_create(fhp, &child, open);
 	if (status != nfs_ok)
 		goto out;
@@ -464,6 +459,9 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
 	fh_init(*resfh, NFS4_FHSIZE);
 	open->op_truncate = false;
 
+	status = fh_fill_pre_attrs_unlocked(current_fh);
+	if (status)
+		goto out;
 	if (open->op_create) {
 		/* FIXME: check session persistence and pnfs flags.
 		 * The nfsv4.1 spec requires the following semantics:
@@ -495,11 +493,11 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
 	} else {
 		status = nfsd_lookup(rqstp, current_fh,
 				     open->op_fname, open->op_fnamelen, *resfh);
-		if (status == nfs_ok)
-			/* NFSv4 protocol requires change attributes even though
-			 * no change happened.
-			 */
-			status = fh_fill_both_attrs(current_fh);
+		/*
+		 * NFSv4 protocol requires change attributes even though
+		 * no change happened.
+		 */
+		fh_fill_post_noop(current_fh);
 	}
 	if (status)
 		goto out;
diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index 8b1a95e1d058..26980bbb195f 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -780,34 +780,53 @@ __be32 fh_getattr(const struct svc_fh *fhp, struct kstat *stat)
 				    AT_STATX_SYNC_AS_STAT));
 }
 
-/**
- * fh_fill_pre_attrs - Fill in pre-op attributes
- * @fhp: file handle to be updated
- *
- */
-__be32 __must_check fh_fill_pre_attrs(struct svc_fh *fhp)
+static __be32 __must_check __fh_fill_pre_attrs(struct svc_fh *fhp)
 {
 	bool v4 = (fhp->fh_maxsize == NFS4_FHSIZE);
-	struct kstat stat;
 	__be32 err;
 
 	if (fhp->fh_no_wcc || fhp->fh_pre_saved)
 		return nfs_ok;
 
-	err = fh_getattr(fhp, &stat);
+	err = fh_getattr(fhp, &fhp->fh_post_attr);
 	if (err)
 		return err;
 
 	if (v4)
-		fhp->fh_pre_change = nfsd4_change_attribute(&stat);
+		fhp->fh_pre_change = fhp->fh_post_change =
+			nfsd4_change_attribute(&fhp->fh_post_attr);
 
-	fhp->fh_pre_mtime = stat.mtime;
-	fhp->fh_pre_ctime = stat.ctime;
-	fhp->fh_pre_size  = stat.size;
+	fhp->fh_pre_mtime = fhp->fh_post_attr.mtime;
+	fhp->fh_pre_ctime = fhp->fh_post_attr.ctime;
+	fhp->fh_pre_size  = fhp->fh_post_attr.size;
 	fhp->fh_pre_saved = true;
 	return nfs_ok;
 }
 
+/**
+ * fh_fill_pre_attrs - Fill in pre-op attributes
+ * @fhp: file handle to be updated
+ *
+ * Post-op attrs are filled and pre-op attrs are copied
+ * from there.  The post-op attrs can later be replaced by
+ * fh_fill_post_attrs() or activated by fh_fill_post_noop().
+ *
+ * The inode must be locked.
+ *
+ * Returns: error from vfs_getattr() which must be checked.
+ */
+__be32 __must_check fh_fill_pre_attrs(struct svc_fh *fhp)
+{
+	lockdep_assert_held_write(&fhp->fh_dentry->d_inode->i_rwsem);
+	return __fh_fill_pre_attrs(fhp);
+}
+
+__be32 __must_check fh_fill_pre_attrs_unlocked(struct svc_fh *fhp)
+{
+	fhp->fh_no_atomic_attr = true;
+	return __fh_fill_pre_attrs(fhp);
+}
+
 /**
  * fh_fill_post_attrs - Fill in post-op attributes
  * @fhp: file handle to be updated
@@ -824,6 +843,9 @@ __be32 fh_fill_post_attrs(struct svc_fh *fhp)
 	if (fhp->fh_post_saved)
 		printk("nfsd: inode locked twice during operation.\n");
 
+	if (!fhp->fh_no_atomic_attr)
+		lockdep_assert_held_write(&fhp->fh_dentry->d_inode->i_rwsem);
+
 	err = fh_getattr(fhp, &fhp->fh_post_attr);
 	if (err)
 		return err;
@@ -835,29 +857,6 @@ __be32 fh_fill_post_attrs(struct svc_fh *fhp)
 	return nfs_ok;
 }
 
-/**
- * fh_fill_both_attrs - Fill pre-op and post-op attributes
- * @fhp: file handle to be updated
- *
- * This is used when the directory wasn't changed, but wcc attributes
- * are needed anyway.
- */
-__be32 __must_check fh_fill_both_attrs(struct svc_fh *fhp)
-{
-	__be32 err;
-
-	err = fh_fill_post_attrs(fhp);
-	if (err)
-		return err;
-
-	fhp->fh_pre_change = fhp->fh_post_change;
-	fhp->fh_pre_mtime = fhp->fh_post_attr.mtime;
-	fhp->fh_pre_ctime = fhp->fh_post_attr.ctime;
-	fhp->fh_pre_size = fhp->fh_post_attr.size;
-	fhp->fh_pre_saved = true;
-	return nfs_ok;
-}
-
 /*
  * Release a file handle.
  */
diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
index cdeb5eea65a8..ab15b59ac7b3 100644
--- a/fs/nfsd/nfsfh.h
+++ b/fs/nfsd/nfsfh.h
@@ -337,6 +337,18 @@ static inline void fh_clear_pre_post_attrs(struct svc_fh *fhp)
 
 u64 nfsd4_change_attribute(const struct kstat *stat);
 __be32 __must_check fh_fill_pre_attrs(struct svc_fh *fhp);
+__be32 __must_check fh_fill_pre_attrs_unlocked(struct svc_fh *fhp);
 __be32 fh_fill_post_attrs(struct svc_fh *fhp);
-__be32 __must_check fh_fill_both_attrs(struct svc_fh *fhp);
+
+/**
+ * fh_fill_post_noop - Copy pre attrs to post attrs
+ * @fhp: file handle to be updated
+ *
+ * This is used when the directory wasn't changed, but wcc attributes
+ * are needed anyway.
+ */
+static inline void fh_fill_post_noop(struct svc_fh *fhp)
+{
+	fhp->fh_post_saved = true;
+}
 #endif /* _LINUX_NFSD_NFSFH_H */
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH v2 02/14] nfsd: replace fh_fill_both_attrs() with fh_fill_post_noop()

[Jeff Layton]

On Mon, 2026-07-06 at 08:19 +1000, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> fh_fill_both_attrs() is only needed for open/create and is used in the
> case when the target already existed so no creating happens.
> 
> As part of refactoring this code it is changed to call
> fh_fill_pre_attrs() once early on (so errors only need to be caught in
> one place) and then to use a new fh_fill_post_noop() when it is
> determined that no creation happened.
> 
> fh_fill_pre_attrs() now stores the attrs (which it had to get all of
> anyway)_ in ->fh_post_attr.  fh_fill_post_noop() simply marks them as
> valid.  fh_fill_post_attrs() replaces them.
> 
> This change involves moving fh_fill_pre_attrs() out of the inode_lock on
> the directory.  This means that we cannot provide "atomic" wcc data so a
> new fh_fill_pre_attrs_unlocked() is provided which marks the attrs as
> non-atomic.
> 
> This is unfortunate but inevitable if we are ever to allow concurrent
> updates in a directory (which can significantly improve performance in
> some cases).  To get atomic pre/post attributes we will need to be able
> to ask the fs to provide them, or to request a lease on the directory
> for the duration of an operation.
> 
> Note that we haven't provided pre/post attrs on WRITE requests for a
> long time for exactly this reason - we cannot lock the file to get them.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4proc.c | 20 ++++++--------
>  fs/nfsd/nfsfh.c    | 69 +++++++++++++++++++++++-----------------------
>  fs/nfsd/nfsfh.h    | 14 +++++++++-
>  3 files changed, 56 insertions(+), 47 deletions(-)
> 

The loss of atomic post-op attrs is lamentable, but it is in service of
a higher cause!

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

[PATCH v2 03/14] nfsd: move fh_want_write() after preamble in nfsd4_create_file()

[NeilBrown]

From: NeilBrown <neil@brown.name>

As part of separating the nfsd-specific code from the VFS interaction
code in nfsd4_create_file(), move fh_want_write() to just before we need
it.

Consequently errors in the "if" statement that this code is moved over
can now be returned immediately rather than needing to "goto out".

Also restructure that "if" statement to only test is_create_with_attrs()
once.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4proc.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index a93132323b66..17be4f7420fc 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -270,22 +270,18 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	parent = fhp->fh_dentry;
 	inode = d_inode(parent);
 
-	host_err = fh_want_write(fhp);
-	if (host_err)
-		return nfserrno(host_err);
-
-	if (open->op_acl) {
+	if (!is_create_with_attrs(open)) {
+		/* No attrs to check */
+	} else if (open->op_acl) {
 		if (open->op_dpacl || open->op_pacl) {
-			status = nfserr_inval;
-			goto out;
+			/* Cannot specify both NFSv4 and Posix ACLs */
+			return nfserr_inval;
 		}
-		if (is_create_with_attrs(open)) {
-			status = nfsd4_acl_to_attr(NF4REG, open->op_acl,
+		status = nfsd4_acl_to_attr(NF4REG, open->op_acl,
 						   &attrs);
-			if (status)
-				goto out;
-		}
-	} else if (is_create_with_attrs(open)) {
+		if (status)
+			return status;
+	} else {
 		/* The dpacl and pacl will get released by nfsd_attrs_free(). */
 		attrs.na_dpacl = open->op_dpacl;
 		attrs.na_pacl = open->op_pacl;
@@ -293,6 +289,12 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		open->op_pacl = NULL;
 	}
 
+	host_err = fh_want_write(fhp);
+	if (host_err) {
+		status = nfserrno(host_err);
+		goto out_free;
+	}
+
 	child = start_creating(&nop_mnt_idmap, parent,
 			       &QSTR_LEN(open->op_fname, open->op_fnamelen));
 	if (IS_ERR(child)) {
@@ -409,8 +411,9 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		open->op_bmval[2] &= ~FATTR4_WORD2_POSIX_ACCESS_ACL;
 out:
 	end_creating(child);
-	nfsd_attrs_free(&attrs);
 	fh_drop_write(fhp);
+out_free:
+	nfsd_attrs_free(&attrs);
 	return status;
 }
 
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH v2 03/14] nfsd: move fh_want_write() after preamble in nfsd4_create_file()

[Jeff Layton]

On Mon, 2026-07-06 at 08:19 +1000, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> As part of separating the nfsd-specific code from the VFS interaction
> code in nfsd4_create_file(), move fh_want_write() to just before we need
> it.
> 
> Consequently errors in the "if" statement that this code is moved over
> can now be returned immediately rather than needing to "goto out".
> 
> Also restructure that "if" statement to only test is_create_with_attrs()
> once.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4proc.c | 31 +++++++++++++++++--------------
>  1 file changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index a93132323b66..17be4f7420fc 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -270,22 +270,18 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	parent = fhp->fh_dentry;
>  	inode = d_inode(parent);
>  
> -	host_err = fh_want_write(fhp);
> -	if (host_err)
> -		return nfserrno(host_err);
> -
> -	if (open->op_acl) {
> +	if (!is_create_with_attrs(open)) {
> +		/* No attrs to check */
> +	} else if (open->op_acl) {
>  		if (open->op_dpacl || open->op_pacl) {
> -			status = nfserr_inval;
> -			goto out;
> +			/* Cannot specify both NFSv4 and Posix ACLs */
> +			return nfserr_inval;
>  		}
> -		if (is_create_with_attrs(open)) {
> -			status = nfsd4_acl_to_attr(NF4REG, open->op_acl,
> +		status = nfsd4_acl_to_attr(NF4REG, open->op_acl,
>  						   &attrs);
> -			if (status)
> -				goto out;
> -		}
> -	} else if (is_create_with_attrs(open)) {
> +		if (status)
> +			return status;
> +	} else {
>  		/* The dpacl and pacl will get released by nfsd_attrs_free(). */
>  		attrs.na_dpacl = open->op_dpacl;
>  		attrs.na_pacl = open->op_pacl;
> @@ -293,6 +289,12 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		open->op_pacl = NULL;
>  	}
>  
> +	host_err = fh_want_write(fhp);
> +	if (host_err) {
> +		status = nfserrno(host_err);
> +		goto out_free;
> +	}
> +
>  	child = start_creating(&nop_mnt_idmap, parent,
>  			       &QSTR_LEN(open->op_fname, open->op_fnamelen));
>  	if (IS_ERR(child)) {
> @@ -409,8 +411,9 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		open->op_bmval[2] &= ~FATTR4_WORD2_POSIX_ACCESS_ACL;
>  out:
>  	end_creating(child);
> -	nfsd_attrs_free(&attrs);
>  	fh_drop_write(fhp);
> +out_free:
> +	nfsd_attrs_free(&attrs);
>  	return status;
>  }
>  

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

[PATCH v2 04/14] nfsd: move more nfs-specific code into preamble of nfsd4_create_file()

[NeilBrown]

From: NeilBrown <neil@brown.name>

Do NFS-specific prep before interacting with the VFS.

We now add the verifier to iap early so it applies even when an
EXCLUSIVE4_1 replay is detected based on that verifier, so we will set
those attributes again.  This should be harmless even though it will
update ctime and i_version, and so will update the changeid seen by the
client.  It shouldn't matter because the resend implies that the client
hasn't seen the file or its changeid.  If some other client happens to
have noticed the file, it might see an unnecessary changeid up, but that
is of no consequence.

Note that ctime would have been updated anyway if the client has
included other attributes like an ACL.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4proc.c | 55 +++++++++++++++++++++++-----------------------
 1 file changed, 27 insertions(+), 28 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 17be4f7420fc..b723ba08ddaf 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -270,6 +270,9 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	parent = fhp->fh_dentry;
 	inode = d_inode(parent);
 
+	if (!IS_POSIXACL(inode))
+		iap->ia_mode &= ~current_umask();
+
 	if (!is_create_with_attrs(open)) {
 		/* No attrs to check */
 	} else if (open->op_acl) {
@@ -289,6 +292,30 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		open->op_pacl = NULL;
 	}
 
+	v_mtime = 0;
+	v_atime = 0;
+	if (nfsd4_create_is_exclusive(open->op_createmode)) {
+		u32 *verifier = (u32 *)open->op_verf.data;
+
+		/*
+		 * Solaris 7 gets confused (bugid 4218508) if these have
+		 * the high bit set, as do xfs filesystems without the
+		 * "bigtime" feature. So just clear the high bits. If this
+		 * is ever changed to use different attrs for storing the
+		 * verifier, then do_open_lookup() will also need to be
+		 * fixed accordingly.
+		 */
+		v_mtime = verifier[0] & 0x7fffffff;
+		v_atime = verifier[1] & 0x7fffffff;
+
+		iap->ia_valid |= ATTR_MTIME | ATTR_ATIME |
+				 ATTR_MTIME_SET|ATTR_ATIME_SET;
+		iap->ia_mtime.tv_sec = v_mtime;
+		iap->ia_atime.tv_sec = v_atime;
+		iap->ia_mtime.tv_nsec = 0;
+		iap->ia_atime.tv_nsec = 0;
+	}
+
 	host_err = fh_want_write(fhp);
 	if (host_err) {
 		status = nfserrno(host_err);
@@ -308,23 +335,6 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 			goto out;
 	}
 
-	v_mtime = 0;
-	v_atime = 0;
-	if (nfsd4_create_is_exclusive(open->op_createmode)) {
-		u32 *verifier = (u32 *)open->op_verf.data;
-
-		/*
-		 * Solaris 7 gets confused (bugid 4218508) if these have
-		 * the high bit set, as do xfs filesystems without the
-		 * "bigtime" feature. So just clear the high bits. If this
-		 * is ever changed to use different attrs for storing the
-		 * verifier, then do_open_lookup() will also need to be
-		 * fixed accordingly.
-		 */
-		v_mtime = verifier[0] & 0x7fffffff;
-		v_atime = verifier[1] & 0x7fffffff;
-	}
-
 	if (d_really_is_positive(child)) {
 		/* NFSv4 protocol requires change attributes even though
 		 * no change happened.
@@ -373,9 +383,6 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		goto out;
 	}
 
-	if (!IS_POSIXACL(inode))
-		iap->ia_mode &= ~current_umask();
-
 	status = nfsd4_vfs_create(fhp, &child, open);
 	if (status != nfs_ok)
 		goto out;
@@ -389,14 +396,6 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	/* A newly created file already has a file size of zero. */
 	if ((iap->ia_valid & ATTR_SIZE) && (iap->ia_size == 0))
 		iap->ia_valid &= ~ATTR_SIZE;
-	if (nfsd4_create_is_exclusive(open->op_createmode)) {
-		iap->ia_valid |= ATTR_MTIME | ATTR_ATIME |
-				 ATTR_MTIME_SET|ATTR_ATIME_SET;
-		iap->ia_mtime.tv_sec = v_mtime;
-		iap->ia_atime.tv_sec = v_atime;
-		iap->ia_mtime.tv_nsec = 0;
-		iap->ia_atime.tv_nsec = 0;
-	}
 
 set_attr:
 	status = nfsd_create_setattr(rqstp, fhp, resfhp, &attrs);
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH v2 04/14] nfsd: move more nfs-specific code into preamble of nfsd4_create_file()

[Jeff Layton]

On Mon, 2026-07-06 at 08:19 +1000, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> Do NFS-specific prep before interacting with the VFS.
> 
> We now add the verifier to iap early so it applies even when an
> EXCLUSIVE4_1 replay is detected based on that verifier, so we will set
> those attributes again.  This should be harmless even though it will
> update ctime and i_version, and so will update the changeid seen by the
> client.  It shouldn't matter because the resend implies that the client
> hasn't seen the file or its changeid.  If some other client happens to
> have noticed the file, it might see an unnecessary changeid up, but that
> is of no consequence.
> 
> Note that ctime would have been updated anyway if the client has
> included other attributes like an ACL.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4proc.c | 55 +++++++++++++++++++++++-----------------------
>  1 file changed, 27 insertions(+), 28 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 17be4f7420fc..b723ba08ddaf 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -270,6 +270,9 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	parent = fhp->fh_dentry;
>  	inode = d_inode(parent);
>  
> +	if (!IS_POSIXACL(inode))
> +		iap->ia_mode &= ~current_umask();
> +
>  	if (!is_create_with_attrs(open)) {
>  		/* No attrs to check */
>  	} else if (open->op_acl) {
> @@ -289,6 +292,30 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		open->op_pacl = NULL;
>  	}
>  
> +	v_mtime = 0;
> +	v_atime = 0;
> +	if (nfsd4_create_is_exclusive(open->op_createmode)) {
> +		u32 *verifier = (u32 *)open->op_verf.data;
> +
> +		/*
> +		 * Solaris 7 gets confused (bugid 4218508) if these have
> +		 * the high bit set, as do xfs filesystems without the
> +		 * "bigtime" feature. So just clear the high bits. If this
> +		 * is ever changed to use different attrs for storing the
> +		 * verifier, then do_open_lookup() will also need to be
> +		 * fixed accordingly.
> +		 */
> +		v_mtime = verifier[0] & 0x7fffffff;
> +		v_atime = verifier[1] & 0x7fffffff;
> +
> +		iap->ia_valid |= ATTR_MTIME | ATTR_ATIME |
> +				 ATTR_MTIME_SET|ATTR_ATIME_SET;
> +		iap->ia_mtime.tv_sec = v_mtime;
> +		iap->ia_atime.tv_sec = v_atime;
> +		iap->ia_mtime.tv_nsec = 0;
> +		iap->ia_atime.tv_nsec = 0;
> +	}
> +
>  	host_err = fh_want_write(fhp);
>  	if (host_err) {
>  		status = nfserrno(host_err);
> @@ -308,23 +335,6 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  			goto out;
>  	}
>  
> -	v_mtime = 0;
> -	v_atime = 0;
> -	if (nfsd4_create_is_exclusive(open->op_createmode)) {
> -		u32 *verifier = (u32 *)open->op_verf.data;
> -
> -		/*
> -		 * Solaris 7 gets confused (bugid 4218508) if these have
> -		 * the high bit set, as do xfs filesystems without the
> -		 * "bigtime" feature. So just clear the high bits. If this
> -		 * is ever changed to use different attrs for storing the
> -		 * verifier, then do_open_lookup() will also need to be
> -		 * fixed accordingly.
> -		 */
> -		v_mtime = verifier[0] & 0x7fffffff;
> -		v_atime = verifier[1] & 0x7fffffff;
> -	}
> -
>  	if (d_really_is_positive(child)) {
>  		/* NFSv4 protocol requires change attributes even though
>  		 * no change happened.
> @@ -373,9 +383,6 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		goto out;
>  	}
>  
> -	if (!IS_POSIXACL(inode))
> -		iap->ia_mode &= ~current_umask();
> -
>  	status = nfsd4_vfs_create(fhp, &child, open);
>  	if (status != nfs_ok)
>  		goto out;
> @@ -389,14 +396,6 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	/* A newly created file already has a file size of zero. */
>  	if ((iap->ia_valid & ATTR_SIZE) && (iap->ia_size == 0))
>  		iap->ia_valid &= ~ATTR_SIZE;
> -	if (nfsd4_create_is_exclusive(open->op_createmode)) {
> -		iap->ia_valid |= ATTR_MTIME | ATTR_ATIME |
> -				 ATTR_MTIME_SET|ATTR_ATIME_SET;
> -		iap->ia_mtime.tv_sec = v_mtime;
> -		iap->ia_atime.tv_sec = v_atime;
> -		iap->ia_mtime.tv_nsec = 0;
> -		iap->ia_atime.tv_nsec = 0;
> -	}
>  
>  set_attr:
>  	status = nfsd_create_setattr(rqstp, fhp, resfhp, &attrs);

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

[PATCH v2 05/14] nfsd: remove subtlety from nfsd4_create_file()

[NeilBrown]

From: NeilBrown <neil@brown.name>

nfsd4_create_file() has a switch with cases for
NFS4_CREATE_EXCLUSIVE and NFS4_CREATE_EXCLUSIVE4_1 which are identical
except for one line which is marked "subtle" in both cases.

The difference boils down to a "goto".  For the EXCLUSIVE case the
target is "out:" which is after a setattr call.  For EXCLUSIVE4_1
the target is "set_attr:" which is the start of that setattr call.

In the EXCLUSIVE case 'attrs' will only contain the verifier.  Setting
these again is not harmful as discussed in the previous patch.  It will
also call commit_metadata().  In performance terms the cost of an extra
'commit' in the rare case of a replaying exclusive create is negligible.

So we can safely "goto setattr" in both cases and thus simplify the
code.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4proc.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index b723ba08ddaf..69cdbdcde7e9 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -363,22 +363,15 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 			status = nfserr_exist;
 			break;
 		case NFS4_CREATE_EXCLUSIVE:
-			if (inode_get_mtime_sec(d_inode(child)) == v_mtime &&
-			    inode_get_atime_sec(d_inode(child)) == v_atime &&
-			    d_inode(child)->i_size == 0) {
-				open->op_created = true;
-				break;		/* subtle */
-			}
-			status = nfserr_exist;
-			break;
 		case NFS4_CREATE_EXCLUSIVE4_1:
 			if (inode_get_mtime_sec(d_inode(child)) == v_mtime &&
 			    inode_get_atime_sec(d_inode(child)) == v_atime &&
 			    d_inode(child)->i_size == 0) {
 				open->op_created = true;
-				goto set_attr;	/* subtle */
+				goto set_attr;
 			}
 			status = nfserr_exist;
+			break;
 		}
 		goto out;
 	}
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH v2 05/14] nfsd: remove subtlety from nfsd4_create_file()

[Jeff Layton]

On Mon, 2026-07-06 at 08:19 +1000, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> nfsd4_create_file() has a switch with cases for
> NFS4_CREATE_EXCLUSIVE and NFS4_CREATE_EXCLUSIVE4_1 which are identical
> except for one line which is marked "subtle" in both cases.
> 
> The difference boils down to a "goto".  For the EXCLUSIVE case the
> target is "out:" which is after a setattr call.  For EXCLUSIVE4_1
> the target is "set_attr:" which is the start of that setattr call.
> 
> In the EXCLUSIVE case 'attrs' will only contain the verifier.  Setting
> these again is not harmful as discussed in the previous patch.  It will
> also call commit_metadata().  In performance terms the cost of an extra
> 'commit' in the rare case of a replaying exclusive create is negligible.
> 
> So we can safely "goto setattr" in both cases and thus simplify the
> code.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4proc.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index b723ba08ddaf..69cdbdcde7e9 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -363,22 +363,15 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  			status = nfserr_exist;
>  			break;
>  		case NFS4_CREATE_EXCLUSIVE:
> -			if (inode_get_mtime_sec(d_inode(child)) == v_mtime &&
> -			    inode_get_atime_sec(d_inode(child)) == v_atime &&
> -			    d_inode(child)->i_size == 0) {
> -				open->op_created = true;
> -				break;		/* subtle */
> -			}
> -			status = nfserr_exist;
> -			break;
>  		case NFS4_CREATE_EXCLUSIVE4_1:
>  			if (inode_get_mtime_sec(d_inode(child)) == v_mtime &&
>  			    inode_get_atime_sec(d_inode(child)) == v_atime &&
>  			    d_inode(child)->i_size == 0) {
>  				open->op_created = true;
> -				goto set_attr;	/* subtle */
> +				goto set_attr;
>  			}
>  			status = nfserr_exist;
> +			break;
>  		}
>  		goto out;
>  	}

Nice cleanup:

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

[PATCH v2 06/14] nfsd: in nfsd4_create_file() let VFS report if file was created.

[NeilBrown]

From: NeilBrown <neil@brown.name>

nfsd4_create_file() currently assumes that if a lookup failed but then a
create succeeds, then the "create" operation actually created the file.
With atomic_open this may not be the case - some other actor might have
created the file between the lookup and the create.

So we move the call to nfsd4_vfs_create() earlier and set ->op_created
based on the FMODE_CREATED flag that it set.  Then use "!  ->op_created"
to trigger nfserr_exist handling.

The switch statement is split up into two if() statements.
First we check for the possibility of a successful exclusive
create and set ->op_create to true if appropriate.
Then we check for NFS4_CREATE_UNCHECKED to decide if a
pre-existing file means an error or success.

This allows us to combine the two fh_compose() calls to one place.

A subtle difference here is that we now must only pass O_EXCL to
dentry_create() for NFS4_CREATE_GUARDED.  For the EXCLUSIVE create modes
we want a successful open even if the file already exists.  We then
check the verifier after the open succeeded to see if it was exclusive.

The above requires changing dentry_create() to reliably set
FMODE_CREATED when the file was actually created.  Previously it only
sets this flag when atomic_open is used.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/namei.c         |  2 ++
 fs/nfsd/nfs4proc.c | 69 ++++++++++++++++++++--------------------------
 2 files changed, 32 insertions(+), 39 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 5cc9f0f466b8..e0a62198fc60 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -5073,6 +5073,8 @@ struct file *dentry_create(struct path *path, int flags, umode_t mode,
 		error = vfs_create(mnt_idmap(path->mnt), path->dentry, mode, NULL);
 		if (!error)
 			error = vfs_open(path, file);
+		if (!error)
+			file->f_mode |= FMODE_CREATED;
 	}
 	if (unlikely(error))
 		return ERR_PTR(error);
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 69cdbdcde7e9..f59ee074c0c9 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -210,7 +210,11 @@ 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))
+	/*
+	 * For the EXCLUSIVE modes we do our own uniqueness tests
+	 * so don't want O_EXCL.
+	 */
+	if (open->op_createmode == NFS4_CREATE_GUARDED)
 		oflags |= O_EXCL;
 
 	switch (open->op_share_access & NFS4_SHARE_ACCESS_BOTH) {
@@ -333,22 +337,30 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		status = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE);
 		if (status != nfs_ok)
 			goto out;
-	}
 
-	if (d_really_is_positive(child)) {
-		/* NFSv4 protocol requires change attributes even though
-		 * no change happened.
-		 */
-		fh_fill_post_noop(fhp);
-
-		status = fh_compose(resfhp, fhp->fh_export, child, fhp);
+		status = nfsd4_vfs_create(fhp, &child, open);
 		if (status != nfs_ok)
 			goto out;
+		open->op_created = open->op_filp->f_mode & FMODE_CREATED;
+	}
 
-		switch (open->op_createmode) {
-		case NFS4_CREATE_UNCHECKED:
-			if (!d_is_reg(child))
-				break;
+	status = fh_compose(resfhp, fhp->fh_export, child, fhp);
+	if (status != nfs_ok)
+		goto out;
+
+	if (!open->op_created &&
+	    nfsd4_create_is_exclusive(open->op_createmode) &&
+	    inode_get_mtime_sec(d_inode(child)) == v_mtime &&
+	    inode_get_atime_sec(d_inode(child)) == v_atime &&
+	    d_inode(child)->i_size == 0)
+		open->op_created = true;
+
+	if (!open->op_created) {
+		if (open->op_createmode == NFS4_CREATE_UNCHECKED) {
+			/* NFSv4 protocol requires change attributes
+			 * even though no change happened.
+			 */
+			fh_fill_post_noop(fhp);
 
 			/*
 			 * In NFSv4, we don't want to truncate the file
@@ -356,41 +368,20 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 			 * some other reason. Furthermore, if the size is
 			 * nonzero, we should ignore it according to spec!
 			 */
-			open->op_truncate = (iap->ia_valid & ATTR_SIZE) &&
-						!iap->ia_size;
-			break;
-		case NFS4_CREATE_GUARDED:
-			status = nfserr_exist;
-			break;
-		case NFS4_CREATE_EXCLUSIVE:
-		case NFS4_CREATE_EXCLUSIVE4_1:
-			if (inode_get_mtime_sec(d_inode(child)) == v_mtime &&
-			    inode_get_atime_sec(d_inode(child)) == v_atime &&
-			    d_inode(child)->i_size == 0) {
-				open->op_created = true;
-				goto set_attr;
-			}
+			open->op_truncate = (d_is_reg(child) &&
+					     (iap->ia_valid & ATTR_SIZE) &&
+					     !iap->ia_size);
+		} else
 			status = nfserr_exist;
-			break;
-		}
 		goto out;
 	}
-
-	status = nfsd4_vfs_create(fhp, &child, open);
-	if (status != nfs_ok)
-		goto out;
-	open->op_created = true;
+	/* file was created */
 	fh_fill_post_attrs(fhp);
 
-	status = fh_compose(resfhp, fhp->fh_export, child, fhp);
-	if (status != nfs_ok)
-		goto out;
-
 	/* A newly created file already has a file size of zero. */
 	if ((iap->ia_valid & ATTR_SIZE) && (iap->ia_size == 0))
 		iap->ia_valid &= ~ATTR_SIZE;
 
-set_attr:
 	status = nfsd_create_setattr(rqstp, fhp, resfhp, &attrs);
 
 	if (attrs.na_labelerr)
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH v2 06/14] nfsd: in nfsd4_create_file() let VFS report if file was created.

[Jeff Layton]

On Mon, 2026-07-06 at 08:19 +1000, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> nfsd4_create_file() currently assumes that if a lookup failed but then a
> create succeeds, then the "create" operation actually created the file.
> With atomic_open this may not be the case - some other actor might have
> created the file between the lookup and the create.
> 
> So we move the call to nfsd4_vfs_create() earlier and set ->op_created
> based on the FMODE_CREATED flag that it set.  Then use "!  ->op_created"
> to trigger nfserr_exist handling.
> 
> The switch statement is split up into two if() statements.
> First we check for the possibility of a successful exclusive
> create and set ->op_create to true if appropriate.
> Then we check for NFS4_CREATE_UNCHECKED to decide if a
> pre-existing file means an error or success.
> 
> This allows us to combine the two fh_compose() calls to one place.
> 
> A subtle difference here is that we now must only pass O_EXCL to
> dentry_create() for NFS4_CREATE_GUARDED.  For the EXCLUSIVE create modes
> we want a successful open even if the file already exists.  We then
> check the verifier after the open succeeded to see if it was exclusive.
> 

Do we really want a successful open in the EXCLUSIVE cases?

Opens have side effects (notably, that they can cause delegation
recalls). If you have two racing clients creating a file, the first
gets an open and write delegation and then the second ends up
immediately causing a delegrecall for the first, even though it may
never touch the file again after the OPEN fails.

I think we may want to reconsider that logic, if possible: Maybe we
should keep using O_EXCL in those cases and just re-drive the open
without it if it fails and the verifier looks right? That's a bit
uglier, but that may cause fewer delegation recalls.

> The above requires changing dentry_create() to reliably set
> FMODE_CREATED when the file was actually created.  Previously it only
> sets this flag when atomic_open is used.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/namei.c         |  2 ++
>  fs/nfsd/nfs4proc.c | 69 ++++++++++++++++++++--------------------------
>  2 files changed, 32 insertions(+), 39 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 5cc9f0f466b8..e0a62198fc60 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -5073,6 +5073,8 @@ struct file *dentry_create(struct path *path, int flags, umode_t mode,
>  		error = vfs_create(mnt_idmap(path->mnt), path->dentry, mode, NULL);
>  		if (!error)
>  			error = vfs_open(path, file);
> +		if (!error)
> +			file->f_mode |= FMODE_CREATED;
>  	}
>  	if (unlikely(error))
>  		return ERR_PTR(error);
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 69cdbdcde7e9..f59ee074c0c9 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -210,7 +210,11 @@ 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))
> +	/*
> +	 * For the EXCLUSIVE modes we do our own uniqueness tests
> +	 * so don't want O_EXCL.
> +	 */
> +	if (open->op_createmode == NFS4_CREATE_GUARDED)
>  		oflags |= O_EXCL;
>  
>  	switch (open->op_share_access & NFS4_SHARE_ACCESS_BOTH) {
> @@ -333,22 +337,30 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		status = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE);
>  		if (status != nfs_ok)
>  			goto out;
> -	}
>  
> -	if (d_really_is_positive(child)) {
> -		/* NFSv4 protocol requires change attributes even though
> -		 * no change happened.
> -		 */
> -		fh_fill_post_noop(fhp);
> -
> -		status = fh_compose(resfhp, fhp->fh_export, child, fhp);
> +		status = nfsd4_vfs_create(fhp, &child, open);
>  		if (status != nfs_ok)
>  			goto out;
> +		open->op_created = open->op_filp->f_mode & FMODE_CREATED;
> +	}
>  
> -		switch (open->op_createmode) {
> -		case NFS4_CREATE_UNCHECKED:
> -			if (!d_is_reg(child))
> -				break;
> +	status = fh_compose(resfhp, fhp->fh_export, child, fhp);
> +	if (status != nfs_ok)
> +		goto out;
> +
> +	if (!open->op_created &&
> +	    nfsd4_create_is_exclusive(open->op_createmode) &&
> +	    inode_get_mtime_sec(d_inode(child)) == v_mtime &&
> +	    inode_get_atime_sec(d_inode(child)) == v_atime &&
> +	    d_inode(child)->i_size == 0)
> +		open->op_created = true;
> +
> +	if (!open->op_created) {
> +		if (open->op_createmode == NFS4_CREATE_UNCHECKED) {
> +			/* NFSv4 protocol requires change attributes
> +			 * even though no change happened.
> +			 */
> +			fh_fill_post_noop(fhp);
>  
>  			/*
>  			 * In NFSv4, we don't want to truncate the file
> @@ -356,41 +368,20 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  			 * some other reason. Furthermore, if the size is
>  			 * nonzero, we should ignore it according to spec!
>  			 */
> -			open->op_truncate = (iap->ia_valid & ATTR_SIZE) &&
> -						!iap->ia_size;
> -			break;
> -		case NFS4_CREATE_GUARDED:
> -			status = nfserr_exist;
> -			break;
> -		case NFS4_CREATE_EXCLUSIVE:
> -		case NFS4_CREATE_EXCLUSIVE4_1:
> -			if (inode_get_mtime_sec(d_inode(child)) == v_mtime &&
> -			    inode_get_atime_sec(d_inode(child)) == v_atime &&
> -			    d_inode(child)->i_size == 0) {
> -				open->op_created = true;
> -				goto set_attr;
> -			}
> +			open->op_truncate = (d_is_reg(child) &&
> +					     (iap->ia_valid & ATTR_SIZE) &&
> +					     !iap->ia_size);
> +		} else
>  			status = nfserr_exist;
> -			break;
> -		}
>  		goto out;
>  	}
> -
> -	status = nfsd4_vfs_create(fhp, &child, open);
> -	if (status != nfs_ok)
> -		goto out;
> -	open->op_created = true;
> +	/* file was created */
>  	fh_fill_post_attrs(fhp);
>  
> -	status = fh_compose(resfhp, fhp->fh_export, child, fhp);
> -	if (status != nfs_ok)
> -		goto out;
> -
>  	/* A newly created file already has a file size of zero. */
>  	if ((iap->ia_valid & ATTR_SIZE) && (iap->ia_size == 0))
>  		iap->ia_valid &= ~ATTR_SIZE;
>  
> -set_attr:
>  	status = nfsd_create_setattr(rqstp, fhp, resfhp, &attrs);
>  
>  	if (attrs.na_labelerr)

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 06/14] nfsd: in nfsd4_create_file() let VFS report if file was created.

[NeilBrown]

On Tue, 07 Jul 2026, Jeff Layton wrote:
> On Mon, 2026-07-06 at 08:19 +1000, NeilBrown wrote:
> > From: NeilBrown <neil@brown.name>
> > 
> > nfsd4_create_file() currently assumes that if a lookup failed but then a
> > create succeeds, then the "create" operation actually created the file.
> > With atomic_open this may not be the case - some other actor might have
> > created the file between the lookup and the create.
> > 
> > So we move the call to nfsd4_vfs_create() earlier and set ->op_created
> > based on the FMODE_CREATED flag that it set.  Then use "!  ->op_created"
> > to trigger nfserr_exist handling.
> > 
> > The switch statement is split up into two if() statements.
> > First we check for the possibility of a successful exclusive
> > create and set ->op_create to true if appropriate.
> > Then we check for NFS4_CREATE_UNCHECKED to decide if a
> > pre-existing file means an error or success.
> > 
> > This allows us to combine the two fh_compose() calls to one place.
> > 
> > A subtle difference here is that we now must only pass O_EXCL to
> > dentry_create() for NFS4_CREATE_GUARDED.  For the EXCLUSIVE create modes
> > we want a successful open even if the file already exists.  We then
> > check the verifier after the open succeeded to see if it was exclusive.
> > 
> 
> Do we really want a successful open in the EXCLUSIVE cases?
> 
> Opens have side effects (notably, that they can cause delegation
> recalls). If you have two racing clients creating a file, the first
> gets an open and write delegation and then the second ends up
> immediately causing a delegrecall for the first, even though it may
> never touch the file again after the OPEN fails.
> 
> I think we may want to reconsider that logic, if possible: Maybe we
> should keep using O_EXCL in those cases and just re-drive the open
> without it if it fails and the verifier looks right? That's a bit
> uglier, but that may cause fewer delegation recalls.

Thanks for raising this.  My thinking was that next EXCLUSIVE creates
was weird.
If the server is re-exporting NFS, then think about what happens to the
verifier stored on the backend server.
First the verifier from the intermediate server is stored, 
then the nfs client on the intermediate server sends a SETATTR to remove it, 
then the intermediate server sets the verifier from the originating
client.
then the originating client removes it.

So 4 setattrs altogether.

Also if the intermediate server ever gets a retransmit of the OPEN
request, it will send an EXCLUSIVE open to the backend server which will
fail even if the verifier is still correct.

I agree that recalling a delegation needlessly is not good, but will it
happen often?  If the app checks for then name before trying the O_EXCL
open, then the delegation would not get recalled because the OPEN
wouldn't be tried.  I wonder how often that happens....

I think setting O_EXCL in the NFS4_CREATE_GUARDED case is important.
I'm open to discussion around the issues with O_EXCL and the
NFS4_CREATE_EXCLUSIVE case.

Maybe doing a lookup first when the exported fs is NFS would be an OK
compromise.

Thanks,
NeilBrown

[PATCH v2 07/14] nfsd: nfsd4_create_file(): Move NFSD_MAY_CREATE check earlier

[NeilBrown]

From: NeilBrown <neil@brown.name>

We only need NFS_MAY_CREATE check if the file doesn't exist, but it is
nfsd-specific code as it needs to check NFSEXP_READONLY and I want that
to be separate from vfs-specific code, which eventually all be provided
by the VFS.

So move that check earlier, but hold the error status until needed.

The if/else chain here looks a bit clumsy, but it will make a later
patch cleaner.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4proc.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index f59ee074c0c9..95e46c15c5a3 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -260,7 +260,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	struct dentry *parent, *child = ERR_PTR(-EINVAL);
 	__u32 v_mtime, v_atime;
 	struct inode *inode;
-	__be32 status;
+	__be32 status, create_status;
 	int host_err;
 
 	if (name_is_dot_dotdot(open->op_fname, open->op_fnamelen))
@@ -320,6 +320,8 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		iap->ia_atime.tv_nsec = 0;
 	}
 
+	create_status = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE);
+
 	host_err = fh_want_write(fhp);
 	if (host_err) {
 		status = nfserrno(host_err);
@@ -333,16 +335,17 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		goto out;
 	}
 
-	if (d_really_is_negative(child)) {
-		status = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE);
-		if (status != nfs_ok)
-			goto out;
-
+	if (d_really_is_positive(child)) {
+		/* No creation needed */
+	} else if (create_status) {
+		status = create_status;
+	} else {
 		status = nfsd4_vfs_create(fhp, &child, open);
-		if (status != nfs_ok)
-			goto out;
-		open->op_created = open->op_filp->f_mode & FMODE_CREATED;
+		if (status == nfs_ok)
+			open->op_created = open->op_filp->f_mode & FMODE_CREATED;
 	}
+	if (status != nfs_ok)
+		goto out;
 
 	status = fh_compose(resfhp, fhp->fh_export, child, fhp);
 	if (status != nfs_ok)
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH v2 07/14] nfsd: nfsd4_create_file(): Move NFSD_MAY_CREATE check earlier

[Jeff Layton]

On Mon, 2026-07-06 at 08:19 +1000, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> We only need NFS_MAY_CREATE check if the file doesn't exist, but it is
> nfsd-specific code as it needs to check NFSEXP_READONLY and I want that
> to be separate from vfs-specific code, which eventually all be provided
> by the VFS.
> 
> So move that check earlier, but hold the error status until needed.
> 
> The if/else chain here looks a bit clumsy, but it will make a later
> patch cleaner.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4proc.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index f59ee074c0c9..95e46c15c5a3 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -260,7 +260,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	struct dentry *parent, *child = ERR_PTR(-EINVAL);
>  	__u32 v_mtime, v_atime;
>  	struct inode *inode;
> -	__be32 status;
> +	__be32 status, create_status;
>  	int host_err;
>  
>  	if (name_is_dot_dotdot(open->op_fname, open->op_fnamelen))
> @@ -320,6 +320,8 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		iap->ia_atime.tv_nsec = 0;
>  	}
>  
> +	create_status = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE);
> +
>  	host_err = fh_want_write(fhp);
>  	if (host_err) {
>  		status = nfserrno(host_err);
> @@ -333,16 +335,17 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		goto out;
>  	}
>  
> -	if (d_really_is_negative(child)) {
> -		status = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE);
> -		if (status != nfs_ok)
> -			goto out;
> -
> +	if (d_really_is_positive(child)) {
> +		/* No creation needed */
> +	} else if (create_status) {
> +		status = create_status;
> +	} else {
>  		status = nfsd4_vfs_create(fhp, &child, open);
> -		if (status != nfs_ok)
> -			goto out;
> -		open->op_created = open->op_filp->f_mode & FMODE_CREATED;
> +		if (status == nfs_ok)
> +			open->op_created = open->op_filp->f_mode & FMODE_CREATED;
>  	}
> +	if (status != nfs_ok)
> +		goto out;
>  
>  	status = fh_compose(resfhp, fhp->fh_export, child, fhp);
>  	if (status != nfs_ok)

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

[PATCH v2 08/14] nfsd: always open file in nfsd4_create_file()

[NeilBrown]

From: NeilBrown <neil@brown.name>

If the file is found to already exist, open it anyway.  This will
normally be needed eventually anyway, it providing a consistently valid
op_filp will simplify future changes.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4proc.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 95e46c15c5a3..244d5f3975b7 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -336,7 +336,30 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	}
 
 	if (d_really_is_positive(child)) {
-		/* No creation needed */
+		/*
+		 * open the file so that we consistently have a valid
+		 * op_filp.
+		 */
+		struct path path = {.mnt = fhp->fh_export->ex_path.mnt,
+				    .dentry = child,
+		};
+		unsigned int oflags = O_LARGEFILE;
+
+		switch (open->op_share_access & NFS4_SHARE_ACCESS_BOTH) {
+		case NFS4_SHARE_ACCESS_WRITE:
+			oflags |= O_WRONLY;
+			break;
+		case NFS4_SHARE_ACCESS_BOTH:
+			oflags |= O_RDWR;
+			break;
+		default:
+			oflags |= O_RDONLY;
+		}
+		open->op_filp = dentry_open(&path, oflags, current_cred());
+		if (IS_ERR(open->op_filp)) {
+			status = nfserrno(PTR_ERR(open->op_filp));
+			open->op_filp = NULL;
+		}
 	} else if (create_status) {
 		status = create_status;
 	} else {
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v2 09/14] nfsd: reduce range of directory lock in nfsd4_create_file()

[NeilBrown]

From: NeilBrown <neil@brown.name>

We only need to hold the lock taken by start_creating() until the create
has been attempted.  Holding for longer can serve no purpose.

The lock is currently held across the setattr call.  This might be the
intent but it serves no purpose.  Holding the lock prevents the name
from being removed or renamed, but it doesn't prevent a GETATTR or a
racing SETATTR or an OPEN.

Calling end_creating() puts the reference to 'child', but we can still
use the reference that was stored in open->op_filp.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4proc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 244d5f3975b7..973beda7f161 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -367,9 +367,12 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		if (status == nfs_ok)
 			open->op_created = open->op_filp->f_mode & FMODE_CREATED;
 	}
+	end_creating(child);
 	if (status != nfs_ok)
 		goto out;
 
+	child = open->op_filp->f_path.dentry;
+
 	status = fh_compose(resfhp, fhp->fh_export, child, fhp);
 	if (status != nfs_ok)
 		goto out;
@@ -419,7 +422,6 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if (attrs.na_paclerr)
 		open->op_bmval[2] &= ~FATTR4_WORD2_POSIX_ACCESS_ACL;
 out:
-	end_creating(child);
 	fh_drop_write(fhp);
 out_free:
 	nfsd_attrs_free(&attrs);
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH v2 09/14] nfsd: reduce range of directory lock in nfsd4_create_file()

[Jeff Layton]

On Mon, 2026-07-06 at 08:19 +1000, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> We only need to hold the lock taken by start_creating() until the create
> has been attempted.  Holding for longer can serve no purpose.
> 
> The lock is currently held across the setattr call.  This might be the
> intent but it serves no purpose.  Holding the lock prevents the name
> from being removed or renamed, but it doesn't prevent a GETATTR or a
> racing SETATTR or an OPEN.
> 
> Calling end_creating() puts the reference to 'child', but we can still
> use the reference that was stored in open->op_filp.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4proc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 244d5f3975b7..973beda7f161 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -367,9 +367,12 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		if (status == nfs_ok)
>  			open->op_created = open->op_filp->f_mode & FMODE_CREATED;
>  	}
> +	end_creating(child);
>  	if (status != nfs_ok)
>  		goto out;
>  
> +	child = open->op_filp->f_path.dentry;
> +
>  	status = fh_compose(resfhp, fhp->fh_export, child, fhp);
>  	if (status != nfs_ok)
>  		goto out;
> @@ -419,7 +422,6 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	if (attrs.na_paclerr)
>  		open->op_bmval[2] &= ~FATTR4_WORD2_POSIX_ACCESS_ACL;
>  out:
> -	end_creating(child);
>  	fh_drop_write(fhp);
>  out_free:
>  	nfsd_attrs_free(&attrs);

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

[PATCH v2 10/14] nfsd: open-code nfsd4_vfs_create() into nfsd4_create_file()

[NeilBrown]

From: NeilBrown <neil@brown.name>

Having this sub function separate doesn't really add clarity, and merging
allows for some refactoring and ultimately using a different VFS
interface.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4proc.c | 76 +++++++++++++++++++++-------------------------
 1 file changed, 34 insertions(+), 42 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 973beda7f161..a34731e1714f 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -201,46 +201,6 @@ static inline bool nfsd4_create_is_exclusive(int createmode)
 		createmode == NFS4_CREATE_EXCLUSIVE4_1;
 }
 
-static __be32
-nfsd4_vfs_create(struct svc_fh *fhp, struct dentry **child,
-		 struct nfsd4_open *open)
-{
-	struct file *filp;
-	struct path path;
-	int oflags;
-
-	oflags = O_CREAT | O_LARGEFILE;
-	/*
-	 * For the EXCLUSIVE modes we do our own uniqueness tests
-	 * so don't want O_EXCL.
-	 */
-	if (open->op_createmode == NFS4_CREATE_GUARDED)
-		oflags |= O_EXCL;
-
-	switch (open->op_share_access & NFS4_SHARE_ACCESS_BOTH) {
-	case NFS4_SHARE_ACCESS_WRITE:
-		oflags |= O_WRONLY;
-		break;
-	case NFS4_SHARE_ACCESS_BOTH:
-		oflags |= O_RDWR;
-		break;
-	default:
-		oflags |= O_RDONLY;
-	}
-
-	path.mnt = fhp->fh_export->ex_path.mnt;
-	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));
-
-	open->op_filp = filp;
-	return nfs_ok;
-}
-
 /*
  * Implement NFSv4's unchecked, guarded, and exclusive create
  * semantics for regular files. Open state for this new file is
@@ -363,9 +323,41 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	} else if (create_status) {
 		status = create_status;
 	} else {
-		status = nfsd4_vfs_create(fhp, &child, open);
-		if (status == nfs_ok)
+		struct file *filp;
+		struct path path;
+		int oflags;
+
+		oflags = O_CREAT | O_LARGEFILE;
+		/*
+		 * For the EXCLUSIVE modes we do our own uniqueness tests
+		 * so don't want O_EXCL.
+		 */
+		if (open->op_createmode == NFS4_CREATE_GUARDED)
+			oflags |= O_EXCL;
+
+		switch (open->op_share_access & NFS4_SHARE_ACCESS_BOTH) {
+		case NFS4_SHARE_ACCESS_WRITE:
+			oflags |= O_WRONLY;
+			break;
+		case NFS4_SHARE_ACCESS_BOTH:
+			oflags |= O_RDWR;
+			break;
+		default:
+			oflags |= O_RDONLY;
+		}
+
+		path.mnt = fhp->fh_export->ex_path.mnt;
+		path.dentry = child;
+		filp = dentry_create(&path, oflags, open->op_iattr.ia_mode,
+				     current_cred());
+		child = path.dentry;
+
+		if (IS_ERR(filp)) {
+			status = nfserrno(PTR_ERR(filp));
+		} else {
+			open->op_filp = filp;
 			open->op_created = open->op_filp->f_mode & FMODE_CREATED;
+		}
 	}
 	end_creating(child);
 	if (status != nfs_ok)
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH v2 10/14] nfsd: open-code nfsd4_vfs_create() into nfsd4_create_file()

[Jeff Layton]

On Mon, 2026-07-06 at 08:19 +1000, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> Having this sub function separate doesn't really add clarity, and merging
> allows for some refactoring and ultimately using a different VFS
> interface.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4proc.c | 76 +++++++++++++++++++++-------------------------
>  1 file changed, 34 insertions(+), 42 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 973beda7f161..a34731e1714f 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -201,46 +201,6 @@ static inline bool nfsd4_create_is_exclusive(int createmode)
>  		createmode == NFS4_CREATE_EXCLUSIVE4_1;
>  }
>  
> -static __be32
> -nfsd4_vfs_create(struct svc_fh *fhp, struct dentry **child,
> -		 struct nfsd4_open *open)
> -{
> -	struct file *filp;
> -	struct path path;
> -	int oflags;
> -
> -	oflags = O_CREAT | O_LARGEFILE;
> -	/*
> -	 * For the EXCLUSIVE modes we do our own uniqueness tests
> -	 * so don't want O_EXCL.
> -	 */
> -	if (open->op_createmode == NFS4_CREATE_GUARDED)
> -		oflags |= O_EXCL;
> -
> -	switch (open->op_share_access & NFS4_SHARE_ACCESS_BOTH) {
> -	case NFS4_SHARE_ACCESS_WRITE:
> -		oflags |= O_WRONLY;
> -		break;
> -	case NFS4_SHARE_ACCESS_BOTH:
> -		oflags |= O_RDWR;
> -		break;
> -	default:
> -		oflags |= O_RDONLY;
> -	}
> -
> -	path.mnt = fhp->fh_export->ex_path.mnt;
> -	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));
> -
> -	open->op_filp = filp;
> -	return nfs_ok;
> -}
> -
>  /*
>   * Implement NFSv4's unchecked, guarded, and exclusive create
>   * semantics for regular files. Open state for this new file is
> @@ -363,9 +323,41 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	} else if (create_status) {
>  		status = create_status;
>  	} else {
> -		status = nfsd4_vfs_create(fhp, &child, open);
> -		if (status == nfs_ok)
> +		struct file *filp;
> +		struct path path;
> +		int oflags;
> +
> +		oflags = O_CREAT | O_LARGEFILE;
> +		/*
> +		 * For the EXCLUSIVE modes we do our own uniqueness tests
> +		 * so don't want O_EXCL.
> +		 */
> +		if (open->op_createmode == NFS4_CREATE_GUARDED)
> +			oflags |= O_EXCL;
> +
> +		switch (open->op_share_access & NFS4_SHARE_ACCESS_BOTH) {
> +		case NFS4_SHARE_ACCESS_WRITE:
> +			oflags |= O_WRONLY;
> +			break;
> +		case NFS4_SHARE_ACCESS_BOTH:
> +			oflags |= O_RDWR;
> +			break;
> +		default:
> +			oflags |= O_RDONLY;
> +		}
> +
> +		path.mnt = fhp->fh_export->ex_path.mnt;
> +		path.dentry = child;
> +		filp = dentry_create(&path, oflags, open->op_iattr.ia_mode,
> +				     current_cred());
> +		child = path.dentry;
> +
> +		if (IS_ERR(filp)) {
> +			status = nfserrno(PTR_ERR(filp));
> +		} else {
> +			open->op_filp = filp;
>  			open->op_created = open->op_filp->f_mode & FMODE_CREATED;
> +		}
>  	}
>  	end_creating(child);
>  	if (status != nfs_ok)

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

[PATCH v2 11/14] nfsd: move some code out of the d_really_is_negative() branch in nfsd4_create_file()

[NeilBrown]

From: NeilBrown <neil@brown.name>

The benefit of this code movement isn't immediately obvious, but it will
make it easier to switch to using vfs_lookup_open().

One immediate benefit is that common code in the d_is_positive() branch
can be discarded.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4proc.c | 73 ++++++++++++++++++----------------------------
 1 file changed, 28 insertions(+), 45 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index a34731e1714f..7bb476311195 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -217,7 +217,11 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		.na_iattr	= iap,
 		.na_seclabel	= &open->op_label,
 	};
+	int oflags = O_CREAT | O_LARGEFILE;
 	struct dentry *parent, *child = ERR_PTR(-EINVAL);
+	struct path path = {
+		.mnt = fhp->fh_export->ex_path.mnt,
+	};
 	__u32 v_mtime, v_atime;
 	struct inode *inode;
 	__be32 status, create_status;
@@ -237,6 +241,24 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if (!IS_POSIXACL(inode))
 		iap->ia_mode &= ~current_umask();
 
+	/*
+	 * For the EXCLUSIVE modes we do our own uniqueness tests
+	 * so don't want O_EXCL.
+	 */
+	if (open->op_createmode == NFS4_CREATE_GUARDED)
+		oflags |= O_EXCL;
+
+	switch (open->op_share_access & NFS4_SHARE_ACCESS_BOTH) {
+	case NFS4_SHARE_ACCESS_WRITE:
+		oflags |= O_WRONLY;
+		break;
+	case NFS4_SHARE_ACCESS_BOTH:
+		oflags |= O_RDWR;
+		break;
+	default:
+		oflags |= O_RDONLY;
+	}
+
 	if (!is_create_with_attrs(open)) {
 		/* No attrs to check */
 	} else if (open->op_acl) {
@@ -294,27 +316,13 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		status = nfserrno(PTR_ERR(child));
 		goto out;
 	}
+	path.dentry = child;
 
 	if (d_really_is_positive(child)) {
 		/*
 		 * open the file so that we consistently have a valid
 		 * op_filp.
 		 */
-		struct path path = {.mnt = fhp->fh_export->ex_path.mnt,
-				    .dentry = child,
-		};
-		unsigned int oflags = O_LARGEFILE;
-
-		switch (open->op_share_access & NFS4_SHARE_ACCESS_BOTH) {
-		case NFS4_SHARE_ACCESS_WRITE:
-			oflags |= O_WRONLY;
-			break;
-		case NFS4_SHARE_ACCESS_BOTH:
-			oflags |= O_RDWR;
-			break;
-		default:
-			oflags |= O_RDONLY;
-		}
 		open->op_filp = dentry_open(&path, oflags, current_cred());
 		if (IS_ERR(open->op_filp)) {
 			status = nfserrno(PTR_ERR(open->op_filp));
@@ -323,39 +331,14 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	} else if (create_status) {
 		status = create_status;
 	} else {
-		struct file *filp;
-		struct path path;
-		int oflags;
-
-		oflags = O_CREAT | O_LARGEFILE;
-		/*
-		 * For the EXCLUSIVE modes we do our own uniqueness tests
-		 * so don't want O_EXCL.
-		 */
-		if (open->op_createmode == NFS4_CREATE_GUARDED)
-			oflags |= O_EXCL;
-
-		switch (open->op_share_access & NFS4_SHARE_ACCESS_BOTH) {
-		case NFS4_SHARE_ACCESS_WRITE:
-			oflags |= O_WRONLY;
-			break;
-		case NFS4_SHARE_ACCESS_BOTH:
-			oflags |= O_RDWR;
-			break;
-		default:
-			oflags |= O_RDONLY;
-		}
-
-		path.mnt = fhp->fh_export->ex_path.mnt;
-		path.dentry = child;
-		filp = dentry_create(&path, oflags, open->op_iattr.ia_mode,
-				     current_cred());
+		open->op_filp = dentry_create(&path, oflags, open->op_iattr.ia_mode,
+					      current_cred());
 		child = path.dentry;
 
-		if (IS_ERR(filp)) {
-			status = nfserrno(PTR_ERR(filp));
+		if (IS_ERR(open->op_filp)) {
+			status = nfserrno(PTR_ERR(open->op_filp));
+			open->op_filp = NULL;
 		} else {
-			open->op_filp = filp;
 			open->op_created = open->op_filp->f_mode & FMODE_CREATED;
 		}
 	}
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH v2 11/14] nfsd: move some code out of the d_really_is_negative() branch in nfsd4_create_file()

[Jeff Layton]

On Mon, 2026-07-06 at 08:19 +1000, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> The benefit of this code movement isn't immediately obvious, but it will
> make it easier to switch to using vfs_lookup_open().
> 
> One immediate benefit is that common code in the d_is_positive() branch
> can be discarded.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4proc.c | 73 ++++++++++++++++++----------------------------
>  1 file changed, 28 insertions(+), 45 deletions(-)
> 

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

[PATCH v2 12/14] nfsd: reduce want-write range in nfsd4_create_file()

[NeilBrown]

From: NeilBrown <neil@brown.name>

nfsd4_create_file() needs write access to the mount for two purposes:

1/ to create/open the file.
2/ to set attributes on the newly created file.

Normally a file being created would be open for write, and once we have
an active open we have the write access needed for a setattr.
However if a file were created but opened read-only then the setattr
wouldn't necessarily have write access to the mount.

Currently this is all handled by holding the write access across the
open and the setattr.  A subsequent patch will necessarily change how
write access is gained for the open.  So we reduce the range for the
first want_write, and add another one only if setattr is needed on a
read-only open.

Also we need to ensure that we actually do open the file which
previously we didn't if it already existed.  So we add a call to
dentry_open() in that case.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4proc.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 7bb476311195..10323c620b71 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -307,21 +307,23 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	host_err = fh_want_write(fhp);
 	if (host_err) {
 		status = nfserrno(host_err);
-		goto out_free;
+		goto out;
 	}
 
 	child = start_creating(&nop_mnt_idmap, parent,
 			       &QSTR_LEN(open->op_fname, open->op_fnamelen));
 	if (IS_ERR(child)) {
 		status = nfserrno(PTR_ERR(child));
+		fh_drop_write(fhp);
 		goto out;
 	}
 	path.dentry = child;
 
 	if (d_really_is_positive(child)) {
 		/*
-		 * open the file so that we consistently have a valid
-		 * op_filp.
+		 * open the file so that, unless it is O_RDONLY, we
+		 * have write-access to the fs for setattr below.
+		 * Also we can be sure that op_filp->f_path.dentry is valid.
 		 */
 		open->op_filp = dentry_open(&path, oflags, current_cred());
 		if (IS_ERR(open->op_filp)) {
@@ -343,6 +345,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		}
 	}
 	end_creating(child);
+	fh_drop_write(fhp);
 	if (status != nfs_ok)
 		goto out;
 
@@ -386,7 +389,24 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if ((iap->ia_valid & ATTR_SIZE) && (iap->ia_size == 0))
 		iap->ia_valid &= ~ATTR_SIZE;
 
-	status = nfsd_create_setattr(rqstp, fhp, resfhp, &attrs);
+	if (((oflags & O_ACCMODE) == O_RDONLY)) {
+		/*
+		 * We will need write access to set the attrs,
+		 * but a successful open won't have provided
+		 * that.
+		 */
+		int host_err = fh_want_write(fhp);
+		if (host_err) {
+			status = nfserrno(host_err);
+		} else {
+			status = nfsd_create_setattr(rqstp, fhp,
+						     resfhp, &attrs);
+			fh_drop_write(fhp);
+		}
+	} else {
+		status = nfsd_create_setattr(rqstp, fhp,
+					     resfhp, &attrs);
+	}
 
 	if (attrs.na_labelerr)
 		open->op_bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
@@ -397,8 +417,6 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if (attrs.na_paclerr)
 		open->op_bmval[2] &= ~FATTR4_WORD2_POSIX_ACCESS_ACL;
 out:
-	fh_drop_write(fhp);
-out_free:
 	nfsd_attrs_free(&attrs);
 	return status;
 }
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v2 13/14] nfsd: separate out VFS-specific from from nfsd4_create_file()

[NeilBrown]

From: NeilBrown <neil@brown.name>

All the code in nfsd4_create_file() that is VFS manipulation, with now
NFS-specific knowledge, has been localised.  Now we split that out into
a separate function: do_lookup_open().

It is planned to provide a vfs_lookup_open() in vfs code which provides
this functionality.  This will share more code with the syscall open
path, and make it easier to modify locking at the VFS level.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4proc.c | 108 ++++++++++++++++++++++++---------------------
 1 file changed, 58 insertions(+), 50 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 10323c620b71..643cf4302db5 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -201,6 +201,47 @@ static inline bool nfsd4_create_is_exclusive(int createmode)
 		createmode == NFS4_CREATE_EXCLUSIVE4_1;
 }
 
+static struct file *do_lookup_open(struct path *parent,
+				   struct qstr *name,
+				   unsigned int oflags,
+				   umode_t mode)
+{
+	struct file *filp = NULL;
+	struct path path;
+	struct dentry *child;
+	int error = 0;
+
+	error = mnt_want_write(parent->mnt);
+
+	if (error)
+		return ERR_PTR(error);
+
+	child = start_creating(&nop_mnt_idmap, parent->dentry, name);
+	if (IS_ERR(child)) {
+		filp = ERR_CAST(child);
+		goto out;
+	}
+	path.mnt = parent->mnt;
+	path.dentry = child;
+
+	if (d_really_is_positive(child)) {
+		/*
+		 * open the file so that, unless it is O_RDONLY, we
+		 * have write-access to the fs for setattr below.
+		 */
+		filp = dentry_open(&path, oflags, current_cred());
+	} else if (!(oflags & O_CREAT)) {
+		filp = ERR_PTR(-ENOENT);
+	} else {
+		filp = dentry_create(&path, oflags, mode, current_cred());
+		child = path.dentry;
+	}
+	end_creating(child);
+out:
+	mnt_drop_write(parent->mnt);
+	return filp;
+}
+
 /*
  * Implement NFSv4's unchecked, guarded, and exclusive create
  * semantics for regular files. Open state for this new file is
@@ -218,14 +259,13 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		.na_seclabel	= &open->op_label,
 	};
 	int oflags = O_CREAT | O_LARGEFILE;
-	struct dentry *parent, *child = ERR_PTR(-EINVAL);
-	struct path path = {
+	struct dentry *child = ERR_PTR(-EINVAL);
+	struct path parent = {
 		.mnt = fhp->fh_export->ex_path.mnt,
+		.dentry = fhp->fh_dentry,
 	};
 	__u32 v_mtime, v_atime;
-	struct inode *inode;
 	__be32 status, create_status;
-	int host_err;
 
 	if (name_is_dot_dotdot(open->op_fname, open->op_fnamelen))
 		return nfserr_exist;
@@ -235,10 +275,8 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	status = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_EXEC);
 	if (status != nfs_ok)
 		return status;
-	parent = fhp->fh_dentry;
-	inode = d_inode(parent);
 
-	if (!IS_POSIXACL(inode))
+	if (!IS_POSIXACL(d_inode(parent.dentry)))
 		iap->ia_mode &= ~current_umask();
 
 	/*
@@ -303,53 +341,23 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	}
 
 	create_status = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE);
-
-	host_err = fh_want_write(fhp);
-	if (host_err) {
-		status = nfserrno(host_err);
-		goto out;
-	}
-
-	child = start_creating(&nop_mnt_idmap, parent,
-			       &QSTR_LEN(open->op_fname, open->op_fnamelen));
-	if (IS_ERR(child)) {
-		status = nfserrno(PTR_ERR(child));
-		fh_drop_write(fhp);
+	if (create_status)
+		oflags &= ~O_CREAT;
+	open->op_filp = do_lookup_open(&parent,
+				       &QSTR_LEN(open->op_fname,
+						 open->op_fnamelen),
+				       oflags,
+				       open->op_iattr.ia_mode);
+	if (IS_ERR(open->op_filp)) {
+		status = nfserrno(PTR_ERR(open->op_filp));
+		open->op_filp = NULL;
+		if (status == NFSERR_NOENT && create_status)
+			status = create_status;
 		goto out;
 	}
-	path.dentry = child;
-
-	if (d_really_is_positive(child)) {
-		/*
-		 * open the file so that, unless it is O_RDONLY, we
-		 * have write-access to the fs for setattr below.
-		 * Also we can be sure that op_filp->f_path.dentry is valid.
-		 */
-		open->op_filp = dentry_open(&path, oflags, current_cred());
-		if (IS_ERR(open->op_filp)) {
-			status = nfserrno(PTR_ERR(open->op_filp));
-			open->op_filp = NULL;
-		}
-	} else if (create_status) {
-		status = create_status;
-	} else {
-		open->op_filp = dentry_create(&path, oflags, open->op_iattr.ia_mode,
-					      current_cred());
-		child = path.dentry;
-
-		if (IS_ERR(open->op_filp)) {
-			status = nfserrno(PTR_ERR(open->op_filp));
-			open->op_filp = NULL;
-		} else {
-			open->op_created = open->op_filp->f_mode & FMODE_CREATED;
-		}
-	}
-	end_creating(child);
-	fh_drop_write(fhp);
-	if (status != nfs_ok)
-		goto out;
 
 	child = open->op_filp->f_path.dentry;
+	open->op_created = open->op_filp->f_mode & FMODE_CREATED;
 
 	status = fh_compose(resfhp, fhp->fh_export, child, fhp);
 	if (status != nfs_ok)
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH v2 13/14] nfsd: separate out VFS-specific from from nfsd4_create_file()

[Jeff Layton]

On Mon, 2026-07-06 at 08:19 +1000, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> All the code in nfsd4_create_file() that is VFS manipulation, with now
> NFS-specific knowledge, has been localised.  Now we split that out into
> a separate function: do_lookup_open().
> 
> It is planned to provide a vfs_lookup_open() in vfs code which provides
> this functionality.  This will share more code with the syscall open
> path, and make it easier to modify locking at the VFS level.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4proc.c | 108 ++++++++++++++++++++++++---------------------
>  1 file changed, 58 insertions(+), 50 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 10323c620b71..643cf4302db5 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -201,6 +201,47 @@ static inline bool nfsd4_create_is_exclusive(int createmode)
>  		createmode == NFS4_CREATE_EXCLUSIVE4_1;
>  }
>  
> +static struct file *do_lookup_open(struct path *parent,
> +				   struct qstr *name,
> +				   unsigned int oflags,
> +				   umode_t mode)
> +{
> +	struct file *filp = NULL;
> +	struct path path;
> +	struct dentry *child;
> +	int error = 0;
> +
> +	error = mnt_want_write(parent->mnt);
> +
> +	if (error)
> +		return ERR_PTR(error);
> +
> +	child = start_creating(&nop_mnt_idmap, parent->dentry, name);
> +	if (IS_ERR(child)) {
> +		filp = ERR_CAST(child);
> +		goto out;
> +	}
> +	path.mnt = parent->mnt;
> +	path.dentry = child;
> +
> +	if (d_really_is_positive(child)) {
> +		/*
> +		 * open the file so that, unless it is O_RDONLY, we
> +		 * have write-access to the fs for setattr below.
> +		 */
> +		filp = dentry_open(&path, oflags, current_cred());
> +	} else if (!(oflags & O_CREAT)) {
> +		filp = ERR_PTR(-ENOENT);
> +	} else {
> +		filp = dentry_create(&path, oflags, mode, current_cred());
> +		child = path.dentry;
> +	}
> +	end_creating(child);
> +out:
> +	mnt_drop_write(parent->mnt);
> +	return filp;
> +}
> +
>  /*
>   * Implement NFSv4's unchecked, guarded, and exclusive create
>   * semantics for regular files. Open state for this new file is
> @@ -218,14 +259,13 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		.na_seclabel	= &open->op_label,
>  	};
>  	int oflags = O_CREAT | O_LARGEFILE;
> -	struct dentry *parent, *child = ERR_PTR(-EINVAL);
> -	struct path path = {
> +	struct dentry *child = ERR_PTR(-EINVAL);
> +	struct path parent = {
>  		.mnt = fhp->fh_export->ex_path.mnt,
> +		.dentry = fhp->fh_dentry,
>  	};
>  	__u32 v_mtime, v_atime;
> -	struct inode *inode;
>  	__be32 status, create_status;
> -	int host_err;
>  
>  	if (name_is_dot_dotdot(open->op_fname, open->op_fnamelen))
>  		return nfserr_exist;
> @@ -235,10 +275,8 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	status = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_EXEC);
>  	if (status != nfs_ok)
>  		return status;
> -	parent = fhp->fh_dentry;
> -	inode = d_inode(parent);
>  
> -	if (!IS_POSIXACL(inode))
> +	if (!IS_POSIXACL(d_inode(parent.dentry)))
>  		iap->ia_mode &= ~current_umask();
>  
>  	/*
> @@ -303,53 +341,23 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	}
>  
>  	create_status = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE);
> -
> -	host_err = fh_want_write(fhp);
> -	if (host_err) {
> -		status = nfserrno(host_err);
> -		goto out;
> -	}
> -
> -	child = start_creating(&nop_mnt_idmap, parent,
> -			       &QSTR_LEN(open->op_fname, open->op_fnamelen));
> -	if (IS_ERR(child)) {
> -		status = nfserrno(PTR_ERR(child));
> -		fh_drop_write(fhp);
> +	if (create_status)
> +		oflags &= ~O_CREAT;
> +	open->op_filp = do_lookup_open(&parent,
> +				       &QSTR_LEN(open->op_fname,
> +						 open->op_fnamelen),
> +				       oflags,
> +				       open->op_iattr.ia_mode);
> +	if (IS_ERR(open->op_filp)) {
> +		status = nfserrno(PTR_ERR(open->op_filp));
> +		open->op_filp = NULL;
> +		if (status == NFSERR_NOENT && create_status)
> +			status = create_status;
>  		goto out;
>  	}
> 

My Claude spotted this:

Should this compare against nfserr_noent rather than NFSERR_NOENT?

status is __be32, and it is assigned from nfserrno(-ENOENT), which returns
nfserr_noent.


-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 13/14] nfsd: separate out VFS-specific from from nfsd4_create_file()

[NeilBrown]

On Tue, 07 Jul 2026, Jeff Layton wrote:
> 
> My Claude spotted this:
> 
> Should this compare against nfserr_noent rather than NFSERR_NOENT?
> 
> status is __be32, and it is assigned from nfserrno(-ENOENT), which returns
> nfserr_noent.
> 

Thanks Claude :-)

NeilBrown

[PATCH v2 14/14] nfsd: use do_lookup_open() for non-creating open requests too.

[NeilBrown]

From: NeilBrown <neil@brown.name>

Now that we have do_lookup_open() for creating open requests, we can use
it for non-creating too as do_lookup_open() is already able to do that.

This means that when exporting NFS, the lookup and open can be combined
using ->atomic_open.

The resulting simplification allows fh_fill_pre_attrs_unlocked() to be
moved into nfsd4_open_file() (renamed from nfsd4_create_file()) so it is
closer to fh_full_post_attrs and fh_fill_post_noop calls.

As ->op_create_mode isn't defined when op_create is zero, we need a
local create_mode which is -1 (illegal value) when op_create is zero.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4proc.c | 106 ++++++++++++++++++++++-----------------------
 1 file changed, 53 insertions(+), 53 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 643cf4302db5..b244ff8f4e4a 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -243,29 +243,30 @@ static struct file *do_lookup_open(struct path *parent,
 }
 
 /*
- * Implement NFSv4's unchecked, guarded, and exclusive create
- * semantics for regular files. Open state for this new file is
- * subsequently fabricated in nfsd4_process_open2().
- *
+ * Implement NFSv4's open semantics for regular files.
+ * Both create (unchecked, guarded, and exclusive) and non-create.
+ * Open state for this new file is subsequently fabricated in
+ * nfsd4_process_open2().
  * Upon return, caller must release @fhp and @resfhp.
  */
 static __be32
-nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
-		  struct svc_fh *resfhp, struct nfsd4_open *open)
+nfsd4_open_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
+		struct svc_fh *resfhp, struct nfsd4_open *open)
 {
 	struct iattr *iap = &open->op_iattr;
 	struct nfsd_attrs attrs = {
 		.na_iattr	= iap,
 		.na_seclabel	= &open->op_label,
 	};
-	int oflags = O_CREAT | O_LARGEFILE;
+	int oflags = O_LARGEFILE;
 	struct dentry *child = ERR_PTR(-EINVAL);
 	struct path parent = {
 		.mnt = fhp->fh_export->ex_path.mnt,
 		.dentry = fhp->fh_dentry,
 	};
 	__u32 v_mtime, v_atime;
-	__be32 status, create_status;
+	int createmode = -1;
+	__be32 status, create_status = 0;
 
 	if (name_is_dot_dotdot(open->op_fname, open->op_fnamelen))
 		return nfserr_exist;
@@ -276,14 +277,22 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if (status != nfs_ok)
 		return status;
 
+	status = fh_fill_pre_attrs_unlocked(fhp);
+	if (status)
+		return status;
+
 	if (!IS_POSIXACL(d_inode(parent.dentry)))
 		iap->ia_mode &= ~current_umask();
 
+	if (open->op_create) {
+		createmode = open->op_createmode;
+		oflags |= O_CREAT;
+	}
 	/*
 	 * For the EXCLUSIVE modes we do our own uniqueness tests
 	 * so don't want O_EXCL.
 	 */
-	if (open->op_createmode == NFS4_CREATE_GUARDED)
+	if (createmode == NFS4_CREATE_GUARDED)
 		oflags |= O_EXCL;
 
 	switch (open->op_share_access & NFS4_SHARE_ACCESS_BOTH) {
@@ -318,7 +327,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 
 	v_mtime = 0;
 	v_atime = 0;
-	if (nfsd4_create_is_exclusive(open->op_createmode)) {
+	if (nfsd4_create_is_exclusive(createmode)) {
 		u32 *verifier = (u32 *)open->op_verf.data;
 
 		/*
@@ -340,9 +349,11 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		iap->ia_atime.tv_nsec = 0;
 	}
 
-	create_status = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE);
-	if (create_status)
-		oflags &= ~O_CREAT;
+	if (oflags & O_CREAT) {
+		create_status = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE);
+		if (create_status)
+			oflags &= ~O_CREAT;
+	}
 	open->op_filp = do_lookup_open(&parent,
 				       &QSTR_LEN(open->op_fname,
 						 open->op_fnamelen),
@@ -364,14 +375,15 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		goto out;
 
 	if (!open->op_created &&
-	    nfsd4_create_is_exclusive(open->op_createmode) &&
+	    nfsd4_create_is_exclusive(createmode) &&
 	    inode_get_mtime_sec(d_inode(child)) == v_mtime &&
 	    inode_get_atime_sec(d_inode(child)) == v_atime &&
 	    d_inode(child)->i_size == 0)
 		open->op_created = true;
 
 	if (!open->op_created) {
-		if (open->op_createmode == NFS4_CREATE_UNCHECKED) {
+		if (open->op_create == NFS4_OPEN_NOCREATE ||
+		    createmode == NFS4_CREATE_UNCHECKED) {
 			/* NFSv4 protocol requires change attributes
 			 * even though no change happened.
 			 */
@@ -474,46 +486,34 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
 	fh_init(*resfh, NFS4_FHSIZE);
 	open->op_truncate = false;
 
-	status = fh_fill_pre_attrs_unlocked(current_fh);
-	if (status)
-		goto out;
-	if (open->op_create) {
-		/* FIXME: check session persistence and pnfs flags.
-		 * The nfsv4.1 spec requires the following semantics:
-		 *
-		 * Persistent   | pNFS   | Server REQUIRED | Client Allowed
-		 * Reply Cache  | server |                 |
-		 * -------------+--------+-----------------+--------------------
-		 * no           | no     | EXCLUSIVE4_1    | EXCLUSIVE4_1
-		 *              |        |                 | (SHOULD)
-		 *              |        | and EXCLUSIVE4  | or EXCLUSIVE4
-		 *              |        |                 | (SHOULD NOT)
-		 * no           | yes    | EXCLUSIVE4_1    | EXCLUSIVE4_1
-		 * yes          | no     | GUARDED4        | GUARDED4
-		 * yes          | yes    | GUARDED4        | GUARDED4
-		 */
+	/* FIXME: check session persistence and pnfs flags.
+	 * The nfsv4.1 spec requires the following semantics:
+	 *
+	 * Persistent   | pNFS   | Server REQUIRED | Client Allowed
+	 * Reply Cache  | server |                 |
+	 * -------------+--------+-----------------+--------------------
+	 * no           | no     | EXCLUSIVE4_1    | EXCLUSIVE4_1
+	 *              |        |                 | (SHOULD)
+	 *              |        | and EXCLUSIVE4  | or EXCLUSIVE4
+	 *              |        |                 | (SHOULD NOT)
+	 * no           | yes    | EXCLUSIVE4_1    | EXCLUSIVE4_1
+	 * yes          | no     | GUARDED4        | GUARDED4
+	 * yes          | yes    | GUARDED4        | GUARDED4
+	 */
 
-		current->fs->umask = open->op_umask;
-		status = nfsd4_create_file(rqstp, current_fh, *resfh, open);
-		current->fs->umask = 0;
+	current->fs->umask = open->op_umask;
+	status = nfsd4_open_file(rqstp, current_fh, *resfh, open);
+	current->fs->umask = 0;
 
-		/*
-		 * Following rfc 3530 14.2.16, and rfc 5661 18.16.4
-		 * use the returned bitmask to indicate which attributes
-		 * we used to store the verifier:
-		 */
-		if (nfsd4_create_is_exclusive(open->op_createmode) && status == 0)
-			open->op_bmval[1] |= (FATTR4_WORD1_TIME_ACCESS |
-						FATTR4_WORD1_TIME_MODIFY);
-	} else {
-		status = nfsd_lookup(rqstp, current_fh,
-				     open->op_fname, open->op_fnamelen, *resfh);
-		/*
-		 * NFSv4 protocol requires change attributes even though
-		 * no change happened.
-		 */
-		fh_fill_post_noop(current_fh);
-	}
+	/*
+	 * Following rfc 3530 14.2.16, and rfc 5661 18.16.4
+	 * use the returned bitmask to indicate which attributes
+	 * we used to store the verifier:
+	 */
+	if (open->op_create &&
+	    nfsd4_create_is_exclusive(open->op_createmode) && status == 0)
+		open->op_bmval[1] |= (FATTR4_WORD1_TIME_ACCESS |
+				      FATTR4_WORD1_TIME_MODIFY);
 	if (status)
 		goto out;
 	status = nfsd_check_obj_isreg(*resfh, cstate->minorversion);
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH v2 14/14] nfsd: use do_lookup_open() for non-creating open requests too.

[Chuck Lever]

Hi Neil,

Thanks for jumping on the Sashiko findings. Codex review identified
three correctness concerns which I've confirmed with Claude are not
false positives:

1. do_lookup_open() takes write access unconditionally.

	error = mnt_want_write(parent->mnt);
	if (error)
		return ERR_PTR(error);

   Fine for a create. But a read-only OPEN with no create now returns
   -EROFS on anything mounted read-only at the VFS layer: squashfs,
   iso9660, a read-only bind mount, a snapshot. The old path didn't ask
   for write until nfsd_open() actually opened the file, and for
   O_RDONLY it never asked at all, so these opens worked.

   Worth stressing that this isn't the "ro" export option. That keeps
   the underlying mount writable and is enforced in nfsd, so it's fine.
   Only genuinely read-only mounts break, which is probably why it
   slipped past testing. Can we make the mnt_want_write() conditional on
   O_CREAT? The read-only-open setattr path already re-takes write for
   itself, so it shouldn't need the blanket grab.

2. We open the target before checking it's a regular file.

   do_lookup_open() runs dentry_open() on any positive dentry, and
   nfsd_check_obj_isreg() doesn't run until nfsd4_open_file() returns.
   There's no O_NONBLOCK on that open, so an OPEN of an existing FIFO
   sits in fifo_open() waiting for a peer, and it waits with the parent
   directory's i_rwsem still held from start_creating(). One client can
   pin an nfsd thread and the parent directory that way; a handful of
   them and the pool is spent. A device node does the same thing through
   its ->open. We used to know the type before we opened anything, so
   could we check d_is_reg() ahead of the dentry_open()?

   In fairness this isn't new to 14/14. "always open file in
   nfsd4_create_file()" already opened existing files for the UNCHECKED
   case; this patch just extends it to plain reads.

3. Mount crossing and the target export check are gone.

   nfsd_lookup_dentry() went through nfsd_mountpoint() and
   nfsd_cross_mnt() and revalidated the export it crossed into.
   start_creating() is a plain one-component lookup, and we fh_compose()
   against the parent export. Bind-mount one regular file over another
   inside an export and the new path hands back the covered file instead
   of the mounted one, with no check_nfsd_access() on the real target.
   Narrow, since only file-over-file mounts reach it (a directory trips
   the isreg check), but the behavior did change.

Actually some of the suspect logic is added in earlier patches, only to
be exposed by this final switchover patch, so I'll wait for a re-roll.


--
Chuck Lever