[PATCH v5 00/18] nfsd: refactor nfs4_create_file()

[PATCH v5 00/18] nfsd: refactor nfs4_create_file()

[NeilBrown]

For for another version so soon - I hadn't noticed Jeff's LLM results.
Most of the issue raised there were already addressed I think, but the
using  nfserr_noent instead of NFSERR_NOENT, and adding a condition on
the call to mnt_want_write() in the final patch were still needed.

Thanks,
NeilBrown


 [PATCH v5 01/18] nfsd: honour client-provided attributes for
 [PATCH v5 02/18] nfsd: move check_nfsd_access() call into
 [PATCH v5 03/18] nfsd: correctly handle CREATE of mounted-on files
 [PATCH v5 04/18] nfsd: replace fh_fill_both_attrs() with
 [PATCH v5 05/18] nfsd: move fh_want_write() after preamble in
 [PATCH v5 06/18] nfsd: move more nfs-specific code into preamble of
 [PATCH v5 07/18] nfsd: remove subtlety from nfsd4_create_file()
 [PATCH v5 08/18] nfsd: in nfsd4_create_file() let VFS report if file
 [PATCH v5 09/18] nfsd: nfsd4_create_file(): Move NFSD_MAY_CREATE
 [PATCH v5 10/18] nfsd: fh_want_write) failure need not be immediately
 [PATCH v5 11/18] nfsd: (almost) always open file in
 [PATCH v5 12/18] nfsd: reduce range of directory lock in
 [PATCH v5 13/18] nfsd: open-code nfsd4_vfs_create() into
 [PATCH v5 14/18] nfsd: move some code out of the
 [PATCH v5 15/18] nfsd: reduce want-write range in nfsd4_create_file()
 [PATCH v5 16/18] nfsd: move v0 checking out of nfsd_check_obj_isreg()
 [PATCH v5 17/18] nfsd: separate out VFS-specific code from
 [PATCH v5 18/18] nfsd: use do_lookup_open() for non-creating open

[PATCH v5 01/18] 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")
Reviewed-by: Jeff Layton <jlayton@kernel.org>
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 13292f38cf3d..bb99ab6b78b5 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -394,8 +394,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

[PATCH v5 02/18] nfsd: move check_nfsd_access() call into nfsd_cross_mnt()

[NeilBrown]

From: NeilBrown <neil@brown.name>

Whenever we cross a mount point, we need to check_nfsd_access() for v4.
So move the call into nfsd_cross_mnt() in the place where we actually do
cross.  This avoids the possibility of calling nfsd_cross_mnt() without
the required check_nfsd_access().

Also remove the last arg from check_nfsd_access(), which is always false.

nfsd_cross_mnt() now returns an nfserr rather than an errno.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/export.c   |  6 ++----
 fs/nfsd/export.h   |  3 +--
 fs/nfsd/nfs4proc.c |  2 +-
 fs/nfsd/nfs4xdr.c  |  9 +-------
 fs/nfsd/vfs.c      | 54 +++++++++++++++++++++++++---------------------
 fs/nfsd/vfs.h      |  4 ++--
 6 files changed, 36 insertions(+), 42 deletions(-)

diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
index a47c90f40422..5aefb388cc27 100644
--- a/fs/nfsd/export.c
+++ b/fs/nfsd/export.c
@@ -1890,21 +1890,19 @@ __be32 check_security_flavor(struct svc_export *exp, struct svc_rqst *rqstp,
  * check_nfsd_access - check if access to export is allowed.
  * @exp: svc_export that is being accessed.
  * @rqstp: svc_rqst attempting to access @exp.
- * @may_bypass_gss: reduce strictness of authorization check
  *
  * Return values:
  *   %nfs_ok if access is granted, or
  *   %nfserr_wrongsec if access is denied
  */
-__be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
-			 bool may_bypass_gss)
+__be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp)
 {
 	__be32 status;
 
 	status = check_xprtsec_policy(exp, rqstp);
 	if (status != nfs_ok)
 		return status;
-	return check_security_flavor(exp, rqstp, may_bypass_gss);
+	return check_security_flavor(exp, rqstp, false);
 }
 
 /*
diff --git a/fs/nfsd/export.h b/fs/nfsd/export.h
index d2b09cd76145..117fb28db1e0 100644
--- a/fs/nfsd/export.h
+++ b/fs/nfsd/export.h
@@ -104,8 +104,7 @@ int nfsexp_flags(struct svc_cred *cred, struct svc_export *exp);
 __be32 check_xprtsec_policy(struct svc_export *exp, struct svc_rqst *rqstp);
 __be32 check_security_flavor(struct svc_export *exp, struct svc_rqst *rqstp,
 			     bool may_bypass_gss);
-__be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
-			 bool may_bypass_gss);
+__be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp);
 
 /*
  * Function declarations
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index bb99ab6b78b5..0317ff3f1c39 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -3328,7 +3328,7 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
 
 			if (current_fh->fh_export &&
 					need_wrongsec_check(rqstp))
-				op->status = check_nfsd_access(current_fh->fh_export, rqstp, false);
+				op->status = check_nfsd_access(current_fh->fh_export, rqstp);
 		}
 encode_op:
 		if (op->status == nfserr_replay_me) {
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 606ddcb085c0..04755c41d871 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -4574,8 +4574,6 @@ nfsd4_encode_entry4_fattr(struct nfsd4_readdir *cd, const char *name,
 	 * directly from the mountpoint dentry.
 	 */
 	if (nfsd_mountpoint(dentry, exp)) {
-		int err;
-
 		if (!(exp->ex_flags & NFSEXP_V4ROOT)
 				&& !attributes_need_mount(cd->rd_bmval)) {
 			ignore_crossmnt = 1;
@@ -4586,12 +4584,7 @@ nfsd4_encode_entry4_fattr(struct nfsd4_readdir *cd, const char *name,
 		 * Different "."/".." handling?  Something else?
 		 * At least, add a comment here to explain....
 		 */
-		err = nfsd_cross_mnt(cd->rd_rqstp, &dentry, &exp);
-		if (err) {
-			nfserr = nfserrno(err);
-			goto out_put;
-		}
-		nfserr = check_nfsd_access(exp, cd->rd_rqstp, false);
+		nfserr = nfsd_cross_mnt(cd->rd_rqstp, &dentry, &exp);
 		if (nfserr)
 			goto out_put;
 		crossed = true;
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 8923a9910a08..7898af35874b 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -118,15 +118,15 @@ nfserrno (int errno)
 	return nfserr_io;
 }
 
-/* 
- * Called from nfsd_lookup and encode_dirent. Check if we have crossed 
+/*
+ * Called from nfsd_lookup and encode_dirent. Check if we have crossed
  * a mount point.
- * Returns -EAGAIN or -ETIMEDOUT leaving *dpp and *expp unchanged,
+ * Returns an nfs error leaving *dpp and *expp unchanged,
  *  or nfs_ok having possibly changed *dpp and *expp
  */
-int
-nfsd_cross_mnt(struct svc_rqst *rqstp, struct dentry **dpp, 
-		        struct svc_export **expp)
+__be32
+nfsd_cross_mnt(struct svc_rqst *rqstp, struct dentry **dpp,
+	       struct svc_export **expp)
 {
 	struct svc_export *exp = *expp, *exp2 = NULL;
 	struct dentry *dentry = *dpp;
@@ -134,6 +134,7 @@ nfsd_cross_mnt(struct svc_rqst *rqstp, struct dentry **dpp,
 			    .dentry = dget(dentry)};
 	unsigned int follow_flags = 0;
 	int err = 0;
+	__be32 nfserr = nfs_ok;
 
 	if (exp->ex_flags & NFSEXP_CROSSMOUNT)
 		follow_flags = LOOKUP_AUTOMOUNT;
@@ -163,23 +164,28 @@ nfsd_cross_mnt(struct svc_rqst *rqstp, struct dentry **dpp,
 			err = 0;
 	} else if (nfsd_v4client(rqstp) ||
 		(exp->ex_flags & NFSEXP_CROSSMOUNT) || EX_NOHIDE(exp2)) {
-		/* successfully crossed mount point */
-		/*
-		 * This is subtle: path.dentry is *not* on path.mnt
-		 * at this point.  The only reason we are safe is that
-		 * original mnt is pinned down by exp, so we should
-		 * put path *before* putting exp
-		 */
-		*dpp = path.dentry;
-		path.dentry = dentry;
-		*expp = exp2;
-		exp2 = exp;
+		nfserr = check_nfsd_access(exp, rqstp);
+		if (nfserr == nfs_ok) {
+			/* successfully crossed mount point */
+			/*
+			 * This is subtle: path.dentry is *not* on path.mnt
+			 * at this point.  The only reason we are safe is that
+			 * original mnt is pinned down by exp, so we should
+			 * put path *before* putting exp
+			 */
+			*dpp = path.dentry;
+			path.dentry = dentry;
+			*expp = exp2;
+			exp2 = exp;
+		}
 	}
 out:
 	path_put(&path);
 	if (exp2)
 		exp_put(exp2);
-	return err;
+	if (nfserr)
+		return nfserr;
+	return nfserrno(err);
 }
 
 static void follow_to_parent(struct path *path)
@@ -277,10 +283,11 @@ nfsd_lookup_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		if (IS_ERR(dentry))
 			goto out_nfserr;
 		if (nfsd_mountpoint(dentry, exp)) {
-			host_err = nfsd_cross_mnt(rqstp, &dentry, &exp);
-			if (host_err) {
+			int nfserr = nfsd_cross_mnt(rqstp, &dentry, &exp);
+			if (nfserr) {
 				dput(dentry);
-				goto out_nfserr;
+				exp_put(exp);
+				return nfserr;
 			}
 		}
 	}
@@ -327,9 +334,6 @@ nfsd_lookup(struct svc_rqst *rqstp, struct svc_fh *fhp, const char *name,
 	err = nfsd_lookup_dentry(rqstp, fhp, name, len, &exp, &dentry);
 	if (err)
 		return err;
-	err = check_nfsd_access(exp, rqstp, false);
-	if (err)
-		goto out;
 	/*
 	 * Note: we compose the file handle now, but as the
 	 * dentry may be negative, it may need to be updated.
@@ -337,7 +341,7 @@ nfsd_lookup(struct svc_rqst *rqstp, struct svc_fh *fhp, const char *name,
 	err = fh_compose(resfh, exp, dentry, fhp);
 	if (!err && d_really_is_negative(dentry))
 		err = nfserr_noent;
-out:
+
 	dput(dentry);
 	exp_put(exp);
 	return err;
diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
index 4af2ff9e9dfe..5554878781f4 100644
--- a/fs/nfsd/vfs.h
+++ b/fs/nfsd/vfs.h
@@ -76,8 +76,8 @@ static inline bool nfsd_attrs_valid(struct nfsd_attrs *attrs)
 }
 
 __be32		nfserrno (int errno);
-int		nfsd_cross_mnt(struct svc_rqst *rqstp, struct dentry **dpp,
-		                struct svc_export **expp);
+__be32		nfsd_cross_mnt(struct svc_rqst *rqstp, struct dentry **dpp,
+			       struct svc_export **expp);
 __be32		nfsd_lookup(struct svc_rqst *, struct svc_fh *,
 				const char *, unsigned int, struct svc_fh *);
 __be32		 nfsd_lookup_dentry(struct svc_rqst *, struct svc_fh *,
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v5 03/18] nfsd: correctly handle CREATE of mounted-on files

[NeilBrown]

From: NeilBrown <neil@brown.name>

Linux allows a file (non-directory) to be mounted on a file.  nfsd
mostly supports this if the crossmnt option is in effect.  However if
CREATE is used on an existing mounted-on file, the filehandle for the
underlying file is returns.  The client will then continue to use that
filehandle.

So
  cat /mnt/file
will show the contents of the mounted file as expected, but if
the dcache is flushed with "drop_caches" or similar, then
  >> /mnt/file
  cat /mnt/file
will show the mounted-on file.

For exclusive or checked creates this is not a problem as the creation
will fail no matter which file is seen. For unchecked creates we need to
see if the name is in the dcache, and if it is mounted.  If so, we
simply provide that filehandle, possibly truncating.

This probably has always existed since before the git history.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs3proc.c | 19 ++++++++++++++++++-
 fs/nfsd/nfs4proc.c | 27 +++++++++++++++++++++++++++
 fs/nfsd/nfsproc.c  | 17 ++++++++++++++++-
 3 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
index bbaef884f893..61272a6d2211 100644
--- a/fs/nfsd/nfs3proc.c
+++ b/fs/nfsd/nfs3proc.c
@@ -282,6 +282,7 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	struct nfsd_attrs attrs = {
 		.na_iattr	= iap,
 	};
+	struct svc_export *exp;
 	__u32 v_mtime, v_atime;
 	struct inode *inode;
 	__be32 status;
@@ -320,7 +321,23 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 			goto out;
 	}
 
-	status = fh_compose(resfhp, fhp->fh_export, child, fhp);
+	exp = exp_get(fhp->fh_export);
+	if (argp->createmode == NFS3_CREATE_UNCHECKED) {
+		/*
+		 * If name is already in dcache we need to check for mountpoints
+		 */
+		if (d_is_reg(child) &&
+		    unlikely(nfsd_mountpoint(child, exp))) {
+			status = nfsd_cross_mnt(rqstp, &child, &exp);
+			if (status != nfs_ok) {
+				exp_put(exp);
+				goto out;
+			}
+		}
+	}
+
+	status = fh_compose(resfhp, exp, child, fhp);
+	exp_put(exp);
 	if (status != nfs_ok)
 		goto out;
 
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 0317ff3f1c39..a13dc1756b1b 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -271,6 +271,33 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	parent = fhp->fh_dentry;
 	inode = d_inode(parent);
 
+	if (open->op_createmode == NFS4_CREATE_UNCHECKED) {
+		/*
+		 * If name is already in dcache we need to check for mountpoints
+		 */
+		child = try_lookup_noperm(&QSTR_LEN(open->op_fname,
+						    open->op_fnamelen),
+					  parent);
+		if (child && !IS_ERR(child) && d_is_reg(child) &&
+		    unlikely(nfsd_mountpoint(child, fhp->fh_export))) {
+			struct svc_export *exp = exp_get(fhp->fh_export);
+			status = nfsd_cross_mnt(rqstp, &child, &exp);
+			if (status == nfs_ok)
+				status = fh_compose(resfhp, exp,
+						    child, fhp);
+			if (status == nfs_ok)
+				status = fh_fill_both_attrs(fhp);
+			open->op_truncate =
+				(iap->ia_valid & ATTR_SIZE) &&
+				!iap->ia_size;
+			dput(child);
+			exp_put(exp);
+			return status;
+		}
+		if (!IS_ERR(child))
+			dput(child);
+	}
+
 	host_err = fh_want_write(fhp);
 	if (host_err)
 		return nfserrno(host_err);
diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
index f60043632575..20fe4411f666 100644
--- a/fs/nfsd/nfsproc.c
+++ b/fs/nfsd/nfsproc.c
@@ -291,6 +291,7 @@ nfsd_proc_create(struct svc_rqst *rqstp)
 	struct nfsd_attrs attrs = {
 		.na_iattr	= attr,
 	};
+	struct svc_export *exp;
 	struct inode	*inode;
 	struct dentry	*dchild;
 	int		type, mode;
@@ -319,8 +320,22 @@ nfsd_proc_create(struct svc_rqst *rqstp)
 		resp->status = nfserrno(PTR_ERR(dchild));
 		goto out_write;
 	}
+	/*
+	 * If name exists we need to check for mountpoints
+	 */
+	exp = exp_get(dirfhp->fh_export);
+	if (d_is_reg(dchild) &&
+	    unlikely(nfsd_mountpoint(dchild, exp))) {
+		resp->status = nfsd_cross_mnt(rqstp, &dchild, &exp);
+		if (resp->status != nfs_ok) {
+			exp_put(exp);
+			goto out_unlock;
+		}
+	}
+
 	fh_init(newfhp, NFS_FHSIZE);
-	resp->status = fh_compose(newfhp, dirfhp->fh_export, dchild, dirfhp);
+	resp->status = fh_compose(newfhp, exp, dchild, dirfhp);
+	exp_put(exp);
 	if (!resp->status && d_really_is_negative(dchild))
 		resp->status = nfserr_noent;
 	if (resp->status) {
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v5 04/18] 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.

Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4proc.c | 23 +++++++---------
 fs/nfsd/nfsfh.c    | 69 +++++++++++++++++++++++-----------------------
 fs/nfsd/nfsfh.h    | 14 +++++++++-
 3 files changed, 57 insertions(+), 49 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index a13dc1756b1b..6e02976484d9 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -285,8 +285,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 			if (status == nfs_ok)
 				status = fh_compose(resfhp, exp,
 						    child, fhp);
-			if (status == nfs_ok)
-				status = fh_fill_both_attrs(fhp);
+			fh_fill_post_noop(fhp);
 			open->op_truncate =
 				(iap->ia_valid & ATTR_SIZE) &&
 				!iap->ia_size;
@@ -355,9 +354,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)
@@ -404,9 +401,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;
@@ -492,6 +486,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:
@@ -523,11 +520,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 c7c60c35bdfc..c0a46784d525 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -782,34 +782,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
@@ -826,6 +845,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;
@@ -837,29 +859,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

[PATCH v5 05/18] 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.

Reviewed-by: Jeff Layton <jlayton@kernel.org>
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 6e02976484d9..d5c5f440481e 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -297,22 +297,18 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 			dput(child);
 	}
 
-	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;
@@ -320,6 +316,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)) {
@@ -436,8 +438,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

[PATCH v5 06/18] 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.

Reviewed-by: Jeff Layton <jlayton@kernel.org>
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 d5c5f440481e..2b9fad28a9ef 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -297,6 +297,9 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 			dput(child);
 	}
 
+	if (!IS_POSIXACL(inode))
+		iap->ia_mode &= ~current_umask();
+
 	if (!is_create_with_attrs(open)) {
 		/* No attrs to check */
 	} else if (open->op_acl) {
@@ -316,6 +319,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);
@@ -335,23 +362,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.
@@ -400,9 +410,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;
@@ -416,14 +423,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

[PATCH v5 07/18] 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.

Reviewed-by: Jeff Layton <jlayton@kernel.org>
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 2b9fad28a9ef..a311810abfde 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -390,22 +390,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

[PATCH v5 08/18] 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 19ce43c9a6e6..9af1d5bc89fc 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -5077,6 +5077,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 a311810abfde..8840f6dd8a0a 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -211,7 +211,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) {
@@ -360,22 +364,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
@@ -383,41 +395,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

[PATCH v5 09/18] 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.

Reviewed-by: Jeff Layton <jlayton@kernel.org>
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 8840f6dd8a0a..5e56bf6e6bd9 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -261,7 +261,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))
@@ -347,6 +347,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);
@@ -360,16 +362,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

[PATCH v5 10/18] nfsd: fh_want_write) failure need not be immediately fatal for nfsd4_create_file()

[NeilBrown]

From: NeilBrown <neil@brown.name>

If nfsd4_create_file() is asked to create a file, then failure to get
write access to the mount need not be fatal if the file already exists.
So we can delay handling the error until it is known if creation was
needed, just like with the error from testing for write permission in
parent.

This is similar to want_write error handling in lookup_open() in
fs/namei.c.

Note that getting mnt write access to support O_RDWR is handled
separately in do_dentry_open(), and op_truncate is handled in
do_open_permission(), so  nfsd doesn't need to be concerned
with these.  It only needs to be concerned with creation, and setattr.

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

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 5e56bf6e6bd9..a4838d36885b 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -262,7 +262,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	__u32 v_mtime, v_atime;
 	struct inode *inode;
 	__be32 status, create_status;
-	int host_err;
+	int want_write_err;
 
 	if (name_is_dot_dotdot(open->op_fname, open->op_fnamelen))
 		return nfserr_exist;
@@ -349,11 +349,10 @@ 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_free;
-	}
+	want_write_err = fh_want_write(fhp);
+	if (want_write_err)
+		/* Might still succeed if no create is needed */
+		create_status = nfserrno(want_write_err);
 
 	child = start_creating(&nop_mnt_idmap, parent,
 			       &QSTR_LEN(open->op_fname, open->op_fnamelen));
@@ -424,8 +423,8 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		open->op_bmval[2] &= ~FATTR4_WORD2_POSIX_ACCESS_ACL;
 out:
 	end_creating(child);
-	fh_drop_write(fhp);
-out_free:
+	if (!want_write_err)
+		fh_drop_write(fhp);
 	nfsd_attrs_free(&attrs);
 	return status;
 }
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v5 11/18] nfsd: (almost) 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, and providing a consistently valid
op_filp will simplify future changes.

To simplify this, change nfsd_check_obj_isreg() to take a dentry.

This doesn't apply in the case where the file was found in the dcache to
be mounted-on.  That takes a different path and doesn't require an early
open.

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

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index a4838d36885b..0b3b358679f3 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -169,9 +169,9 @@ do_open_permission(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfs
 	return fh_verify(rqstp, current_fh, S_IFREG, accmode);
 }
 
-static __be32 nfsd_check_obj_isreg(struct svc_fh *fh, u32 minor_version)
+static __be32 nfsd_check_obj_isreg(struct dentry *child, u32 minor_version)
 {
-	umode_t mode = d_inode(fh->fh_dentry)->i_mode;
+	umode_t mode = d_inode(child)->i_mode;
 
 	if (S_ISREG(mode))
 		return nfs_ok;
@@ -253,6 +253,8 @@ static __be32
 nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		  struct svc_fh *resfhp, struct nfsd4_open *open)
 {
+	struct nfsd4_compoundres *resp = rqstp->rq_resp;
+	struct nfsd4_compound_state *cstate = &resp->cstate;
 	struct iattr *iap = &open->op_iattr;
 	struct nfsd_attrs attrs = {
 		.na_iattr	= iap,
@@ -362,7 +364,35 @@ 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;
+		}
+
+		status = nfsd_check_obj_isreg(child, cstate->minorversion);
+		if (status == nfs_ok) {
+			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 {
@@ -516,7 +546,8 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
 	}
 	if (status)
 		goto out;
-	status = nfsd_check_obj_isreg(*resfh, cstate->minorversion);
+	status = nfsd_check_obj_isreg((*resfh)->fh_dentry,
+				      cstate->minorversion);
 	if (status)
 		goto out;
 
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v5 12/18] 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 | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 0b3b358679f3..c04a021d9719 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -366,7 +366,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if (d_really_is_positive(child)) {
 		/*
 		 * open the file so that we consistently have a valid
-		 * op_filp.
+		 * op_filp and consequently a valid ->f_path.dentry.
 		 */
 		struct path path = {.mnt = fhp->fh_export->ex_path.mnt,
 				    .dentry = child,
@@ -400,9 +400,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;
@@ -452,7 +455,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);
 	if (!want_write_err)
 		fh_drop_write(fhp);
 	nfsd_attrs_free(&attrs);
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v5 13/18] 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.

Reviewed-by: Jeff Layton <jlayton@kernel.org>
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 c04a021d9719..c3bec447a43e 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -202,46 +202,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
@@ -396,9 +356,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

[PATCH v5 14/18] 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.

Reviewed-by: Jeff Layton <jlayton@kernel.org>
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 c3bec447a43e..7a67e1385b76 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -220,7 +220,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;
@@ -266,6 +270,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) {
@@ -322,27 +344,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 and consequently a valid ->f_path.dentry.
 		 */
-		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;
-		}
 
 		status = nfsd_check_obj_isreg(child, cstate->minorversion);
 		if (status == nfs_ok) {
@@ -356,39 +364,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

[PATCH v5 15/18] 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 (or pre-existing) file.

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 to cover setattr.  If we failed to
get write access, it is only fatal if there were attrs to set.

We call nfsd_create_setattr() if at all possible, even when no attrs, as
it also calls commit_metadata and we need to be certain that the file
creation has been synced.  If the mount became read-only since the
creation happened, we can safely assume that the sync happened as part
of that.

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

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 7a67e1385b76..ec925f60b2ae 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -342,6 +342,8 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 			       &QSTR_LEN(open->op_fname, open->op_fnamelen));
 	if (IS_ERR(child)) {
 		status = nfserrno(PTR_ERR(child));
+		if (!want_write_err)
+			fh_drop_write(fhp);
 		goto out;
 	}
 	path.dentry = child;
@@ -376,6 +378,8 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		}
 	}
 	end_creating(child);
+	if (!want_write_err)
+		fh_drop_write(fhp);
 	if (status != nfs_ok)
 		goto out;
 
@@ -419,7 +423,16 @@ 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);
+	/* We will need write access to set the attrs */
+	want_write_err = fh_want_write(fhp);
+	if (!want_write_err) {
+		status = nfsd_create_setattr(rqstp, fhp,
+					     resfhp, &attrs);
+		fh_drop_write(fhp);
+	} else if (nfsd_attrs_valid(&attrs)) {
+		/* Needed write access */
+		status = nfserrno(want_write_err);
+	}
 
 	if (attrs.na_labelerr)
 		open->op_bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
@@ -430,8 +443,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:
-	if (!want_write_err)
-		fh_drop_write(fhp);
 	nfsd_attrs_free(&attrs);
 	return status;
 }
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v5 16/18] nfsd: move v0 checking out of nfsd_check_obj_isreg()

[NeilBrown]

From: NeilBrown <neil@brown.name>

A future patch will use nfsd_check_obj_isreg() in a context where the
protocol version is not easily available.  So move the version check out
and put it at the end of do_open_lookup().

Also change to return errno error code and use nfserrno() to convert to
nfs error codes.  Use -ELOOP for nfserr_symlink, which is an error
indication a problem with symlinks.  -EFTYPE is a good match for
nfserr_wrong_type.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4proc.c | 29 ++++++++++++-----------------
 fs/nfsd/vfs.c      |  4 +++-
 2 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index ec925f60b2ae..760354818189 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -169,23 +169,17 @@ do_open_permission(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfs
 	return fh_verify(rqstp, current_fh, S_IFREG, accmode);
 }
 
-static __be32 nfsd_check_obj_isreg(struct dentry *child, u32 minor_version)
+static __be32 nfsd_check_obj_isreg(struct dentry *child)
 {
 	umode_t mode = d_inode(child)->i_mode;
 
 	if (S_ISREG(mode))
-		return nfs_ok;
+		return 0;
 	if (S_ISDIR(mode))
-		return nfserr_isdir;
+		return -EISDIR;
 	if (S_ISLNK(mode))
-		return nfserr_symlink;
-
-	/* RFC 7530 - 16.16.6 */
-	if (minor_version == 0)
-		return nfserr_symlink;
-	else
-		return nfserr_wrong_type;
-
+		return -ELOOP;
+	return -EFTYPE;
 }
 
 static void nfsd4_set_open_owner_reply_cache(struct nfsd4_compound_state *cstate, struct nfsd4_open *open, struct svc_fh *resfh)
@@ -213,8 +207,6 @@ static __be32
 nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		  struct svc_fh *resfhp, struct nfsd4_open *open)
 {
-	struct nfsd4_compoundres *resp = rqstp->rq_resp;
-	struct nfsd4_compound_state *cstate = &resp->cstate;
 	struct iattr *iap = &open->op_iattr;
 	struct nfsd_attrs attrs = {
 		.na_iattr	= iap,
@@ -354,8 +346,8 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		 * op_filp and consequently a valid ->f_path.dentry.
 		 */
 
-		status = nfsd_check_obj_isreg(child, cstate->minorversion);
-		if (status == nfs_ok) {
+		status = nfserrno(nfsd_check_obj_isreg(child));
+		if (!status) {
 			open->op_filp = dentry_open(&path, oflags,
 						    current_cred());
 			if (IS_ERR(open->op_filp)) {
@@ -534,8 +526,7 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
 	}
 	if (status)
 		goto out;
-	status = nfsd_check_obj_isreg((*resfh)->fh_dentry,
-				      cstate->minorversion);
+	status = nfserrno(nfsd_check_obj_isreg((*resfh)->fh_dentry));
 	if (status)
 		goto out;
 
@@ -547,6 +538,10 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
 	status = do_open_permission(rqstp, *resfh, open, accmode);
 	set_change_info(&open->op_cinfo, current_fh);
 out:
+	if (status == nfserr_wrong_type && cstate->minorversion == 0)
+		/* RFC 7530 - 16.16.6 */
+		return nfserr_symlink;
+
 	return status;
 }
 
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 7898af35874b..b346f683143e 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -63,7 +63,7 @@ u64 nfsd_io_cache_write __read_mostly = NFSD_IO_BUFFERED;
  * it's an error we don't expect, log it once and return nfserr_io.
  */
 __be32
-nfserrno (int errno)
+nfserrno(int errno)
 {
 	static struct {
 		__be32	nfserr;
@@ -107,6 +107,8 @@ nfserrno (int errno)
 		{ nfserr_perm, -ENOKEY },
 		{ nfserr_no_grace, -ENOGRACE},
 		{ nfserr_io, -EBADMSG },
+		{ nfserr_symlink, -ELOOP },
+		{ nfserr_wrong_type, -EFTYPE },
 	};
 	int	i;
 
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v5 17/18] nfsd: separate out VFS-specific code 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 | 118 ++++++++++++++++++++++++---------------------
 1 file changed, 64 insertions(+), 54 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 760354818189..b7fdc75c4b81 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -196,6 +196,51 @@ 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 want_write_err = 0;
+
+	want_write_err = mnt_want_write(parent->mnt);
+
+	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 we consistently have a valid
+		 * op_filp and consequently a valid ->f_path.dentry.
+		 */
+		int err = nfsd_check_obj_isreg(child);
+		if (err)
+			filp = ERR_PTR(err);
+		else
+			filp = dentry_open(&path, oflags, current_cred());
+	} else if (!(oflags & O_CREAT)) {
+		filp = ERR_PTR(-ENOENT);
+	} else if (want_write_err) {
+		filp = ERR_PTR(want_write_err);
+	} else {
+		filp = dentry_create(&path, oflags, mode, current_cred());
+		child = path.dentry;
+	}
+	end_creating(child);
+out:
+	if (!want_write_err)
+		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
@@ -213,12 +258,12 @@ 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 want_write_err;
 
@@ -230,8 +275,6 @@ 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 (open->op_createmode == NFS4_CREATE_UNCHECKED) {
 		/*
@@ -239,7 +282,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		 */
 		child = try_lookup_noperm(&QSTR_LEN(open->op_fname,
 						    open->op_fnamelen),
-					  parent);
+					  parent.dentry);
 		if (child && !IS_ERR(child) && d_is_reg(child) &&
 		    unlikely(nfsd_mountpoint(child, fhp->fh_export))) {
 			struct svc_export *exp = exp_get(fhp->fh_export);
@@ -259,7 +302,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 			dput(child);
 	}
 
-	if (!IS_POSIXACL(inode))
+	if (!IS_POSIXACL(d_inode(parent.dentry)))
 		iap->ia_mode &= ~current_umask();
 
 	/*
@@ -324,58 +367,25 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	}
 
 	create_status = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE);
-
-	want_write_err = fh_want_write(fhp);
-	if (want_write_err)
+	if (create_status)
 		/* Might still succeed if no create is needed */
-		create_status = nfserrno(want_write_err);
-
-	child = start_creating(&nop_mnt_idmap, parent,
-			       &QSTR_LEN(open->op_fname, open->op_fnamelen));
-	if (IS_ERR(child)) {
-		status = nfserrno(PTR_ERR(child));
-		if (!want_write_err)
-			fh_drop_write(fhp);
+		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 we consistently have a valid
-		 * op_filp and consequently a valid ->f_path.dentry.
-		 */
-
-		status = nfserrno(nfsd_check_obj_isreg(child));
-		if (!status) {
-			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);
-	if (!want_write_err)
-		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

[PATCH v5 18/18] 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 prepares for switching to vfs_lookup_open() once the VFS provides
that.  This will ensure consistent code and fs-interaction with VFS 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.

The non-create path now doesn't use nfsd_lookup().  As mount-point
crossing including nfsd_check_access() is already included for existing
names, this does not lose us anything.

We now only call mnt_want_write() is there is a flag which indicates we
might want write access - previously O_CREAT was always set.

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

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index b7fdc75c4b81..11d0411dec08 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -204,9 +204,10 @@ static struct file *do_lookup_open(struct path *parent,
 	struct file *filp = NULL;
 	struct path path;
 	struct dentry *child;
-	int want_write_err = 0;
+	int want_write_err = -ENOENT;
 
-	want_write_err = mnt_want_write(parent->mnt);
+	if (oflags & (O_CREAT|O_RDONLY|O_RDWR|O_TRUNC))
+		want_write_err = mnt_want_write(parent->mnt);
 
 	child = start_creating(&nop_mnt_idmap, parent->dentry, name);
 	if (IS_ERR(child)) {
@@ -242,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;
 	int want_write_err;
 
 	if (name_is_dot_dotdot(open->op_fname, open->op_fnamelen))
@@ -276,6 +278,10 @@ 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 (open->op_createmode == NFS4_CREATE_UNCHECKED) {
 		/*
 		 * If name is already in dcache we need to check for mountpoints
@@ -305,11 +311,15 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	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) {
@@ -344,7 +354,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;
 
 		/*
@@ -366,11 +376,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)
-		/* Might still succeed if no create is needed */
-		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),
@@ -392,14 +402,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.
 			 */
@@ -494,46 +505,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 = nfserrno(nfsd_check_obj_isreg((*resfh)->fh_dentry));
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH v5 00/18] nfsd: refactor nfs4_create_file()

[Jeff Layton]

On Fri, 2026-07-17 at 19:27 +1000, NeilBrown wrote:
> For for another version so soon - I hadn't noticed Jeff's LLM results.
> Most of the issue raised there were already addressed I think, but the
> using  nfserr_noent instead of NFSERR_NOENT, and adding a condition on
> the call to mnt_want_write() in the final patch were still needed.
> 
> Thanks,
> NeilBrown
> 
> 
>  [PATCH v5 01/18] nfsd: honour client-provided attributes for
>  [PATCH v5 02/18] nfsd: move check_nfsd_access() call into
>  [PATCH v5 03/18] nfsd: correctly handle CREATE of mounted-on files
>  [PATCH v5 04/18] nfsd: replace fh_fill_both_attrs() with
>  [PATCH v5 05/18] nfsd: move fh_want_write() after preamble in
>  [PATCH v5 06/18] nfsd: move more nfs-specific code into preamble of
>  [PATCH v5 07/18] nfsd: remove subtlety from nfsd4_create_file()
>  [PATCH v5 08/18] nfsd: in nfsd4_create_file() let VFS report if file
>  [PATCH v5 09/18] nfsd: nfsd4_create_file(): Move NFSD_MAY_CREATE
>  [PATCH v5 10/18] nfsd: fh_want_write) failure need not be immediately
>  [PATCH v5 11/18] nfsd: (almost) always open file in
>  [PATCH v5 12/18] nfsd: reduce range of directory lock in
>  [PATCH v5 13/18] nfsd: open-code nfsd4_vfs_create() into
>  [PATCH v5 14/18] nfsd: move some code out of the
>  [PATCH v5 15/18] nfsd: reduce want-write range in nfsd4_create_file()
>  [PATCH v5 16/18] nfsd: move v0 checking out of nfsd_check_obj_isreg()
>  [PATCH v5 17/18] nfsd: separate out VFS-specific code from
>  [PATCH v5 18/18] nfsd: use do_lookup_open() for non-creating open

Nice work, Neil. You can add this to any that don't already have it:

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

Re: [PATCH v5 00/18] nfsd: refactor nfs4_create_file()

[Chuck Lever]

On Fri, 17 Jul 2026 19:27:48 +1000, NeilBrown wrote:
> For for another version so soon - I hadn't noticed Jeff's LLM results.
> Most of the issue raised there were already addressed I think, but the
> using  nfserr_noent instead of NFSERR_NOENT, and adding a condition on
> the call to mnt_want_write() in the final patch were still needed.
> 
> Thanks,
> NeilBrown
> 
> [...]

Applied to nfsd-testing, thanks!

[01/18] nfsd: honour client-provided attributes for NFS4_CREATE_EXCLUSIVE4_1
        commit: 0ac10ed2eadf239b8322acdcf9e6b1560c28789c
[02/18] nfsd: move check_nfsd_access() call into nfsd_cross_mnt()
        commit: c4fcf4eb1c1c96519959585f5fbec5ef38dfab4d
[03/18] nfsd: correctly handle CREATE of mounted-on files
        commit: d50cf4faa7098f1e86e9300e71f81ec543daae3f
[04/18] nfsd: replace fh_fill_both_attrs() with fh_fill_post_noop()
        commit: f2f9c31c20a94f7d99da2629667bfb85ad0876a8
[05/18] nfsd: move fh_want_write() after preamble in nfsd4_create_file()
        commit: 130835c9487dfaff092761d6df1129b7a962c111
[06/18] nfsd: move more nfs-specific code into preamble of nfsd4_create_file()
        commit: d2d28891fbfad5c68a3de28fad87ed7ab3eaa915
[07/18] nfsd: remove subtlety from nfsd4_create_file()
        commit: 5c40d90d8bd3bd552b4ac2a11a81418d209fc2ef
[08/18] nfsd: in nfsd4_create_file() let VFS report if file was created.
        commit: b47e34213946cb6210010648fd9e598291b649bb
[09/18] nfsd: nfsd4_create_file(): Move NFSD_MAY_CREATE check earlier
        commit: 0dff1ebde230f9bcc7abc6cbf6b7539e3ea3f616
[10/18] nfsd: fh_want_write) failure need not be immediately fatal for nfsd4_create_file()
        commit: f7f56f21ff446625dbcccefaeab95d2fda4a3ada
[11/18] nfsd: (almost) always open file in nfsd4_create_file()
        commit: a2b9f05a9aa76978ce4cc0131ff2691baa872b01
[12/18] nfsd: reduce range of directory lock in nfsd4_create_file()
        commit: afe7358ca88eeb3292a8933a139acf1c9e7040b1
[13/18] nfsd: open-code nfsd4_vfs_create() into nfsd4_create_file()
        commit: 1233ae38bcc77a4107b0121004de2e3c6340f2b6
[14/18] nfsd: move some code out of the d_really_is_negative() branch in nfsd4_create_file()
        commit: e6bd2126f078bd0fe3dbe7b804d5c3d3a6fdc23f
[15/18] nfsd: reduce want-write range in nfsd4_create_file()
        commit: dfe0dd04a90fd1c6a2d1db2b605d7da8068a7934
[16/18] nfsd: move v0 checking out of nfsd_check_obj_isreg()
        commit: acf95a02c3d3d32364b91f6e5f938638addc7a68
[17/18] nfsd: separate out VFS-specific code from nfsd4_create_file()
        commit: 385074f4868b4ada9c5cd72a53e24e445b97a5e3
[18/18] nfsd: use do_lookup_open() for non-creating open requests too.
        commit: 71349ee933edf67b4dae8a2b68398da2471284c2

--
Chuck Lever

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

[Jeff Layton]

On Fri, 2026-07-17 at 19:28 +1000, NeilBrown wrote:
> 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 prepares for switching to vfs_lookup_open() once the VFS provides
> that.  This will ensure consistent code and fs-interaction with VFS 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.
> 
> The non-create path now doesn't use nfsd_lookup().  As mount-point
> crossing including nfsd_check_access() is already included for existing
> names, this does not lose us anything.
> 
> We now only call mnt_want_write() is there is a flag which indicates we
> might want write access - previously O_CREAT was always set.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4proc.c | 113 ++++++++++++++++++++++-----------------------
>  1 file changed, 56 insertions(+), 57 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index b7fdc75c4b81..11d0411dec08 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -204,9 +204,10 @@ static struct file *do_lookup_open(struct path *parent,
>  	struct file *filp = NULL;
>  	struct path path;
>  	struct dentry *child;
> -	int want_write_err = 0;
> +	int want_write_err = -ENOENT;
>  
> -	want_write_err = mnt_want_write(parent->mnt);
> +	if (oflags & (O_CREAT|O_RDONLY|O_RDWR|O_TRUNC))
> +		want_write_err = mnt_want_write(parent->mnt);
>  
>  	child = start_creating(&nop_mnt_idmap, parent->dentry, name);
>  	if (IS_ERR(child)) {
> @@ -242,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;
>  	int want_write_err;
>  
>  	if (name_is_dot_dotdot(open->op_fname, open->op_fnamelen))
> @@ -276,6 +278,10 @@ 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 (open->op_createmode == NFS4_CREATE_UNCHECKED) {
>  		/*
>  		 * If name is already in dcache we need to check for mountpoints
> @@ -305,11 +311,15 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	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) {
> @@ -344,7 +354,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;
>  
>  		/*
> @@ -366,11 +376,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)
> -		/* Might still succeed if no create is needed */
> -		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),
> @@ -392,14 +402,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.
>  			 */
> @@ -494,46 +505,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 = nfserrno(nfsd_check_obj_isreg((*resfh)->fh_dentry));

This patch is making the pynfs DELEG8 test fail. It seems like this
patch breaks the behavior where the server sends back NFS4ERR_DELAY on
an OPEN while waiting for a DELEGRETURN. The server just doesn't send a
reply until the delegation times out with it.

-- 
Jeff Layton <jlayton@kernel.org>