[PATCH v3 00/17] nfsd: refactor nfs4_create_file()

[PATCH v3 00/17] nfsd: refactor nfs4_create_file()

[NeilBrown]

This v2 incorporates fixes for issues Chuck reported from "Codex
review".  These changes are distributed around the patches a bit, but
02/17 is an interesting new bugfix.

This is against nfs-testing (5f5dc3d855cf).

Thanks,
NeilBrown

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

[PATCH v3 01/17] 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 935408252ace..ca9460e97e2b 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

[PATCH v3 02/17] 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.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs3proc.c | 28 ++++++++++++++++++++++++++++
 fs/nfsd/nfs4proc.c | 30 ++++++++++++++++++++++++++++++
 fs/nfsd/nfsproc.c  | 24 +++++++++++++++++++++++-
 3 files changed, 81 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
index bbaef884f893..20eaf56fa9e7 100644
--- a/fs/nfsd/nfs3proc.c
+++ b/fs/nfsd/nfs3proc.c
@@ -303,6 +303,34 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	parent = fhp->fh_dentry;
 	inode = d_inode(parent);
 
+	if (argp->createmode == NFS3_CREATE_UNCHECKED) {
+		/*
+		 * If name is already in dcache we need to check for mountpoints
+		 */
+		child = try_lookup_noperm(&QSTR_LEN(argp->name,
+						    argp->len),
+					  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);
+			if (nfsd_cross_mnt(rqstp, &child, &exp) == 0) {
+				status = check_nfsd_access(exp, rqstp, false);
+				if (status == nfs_ok)
+					status = fh_compose(resfhp, exp,
+							    child, fhp);
+				if (status == nfs_ok)
+					status = nfsd_create_setattr(
+						rqstp, fhp, resfhp, &attrs);
+				dput(child);
+				exp_put(exp);
+				return status;
+			}
+			exp_put(exp);
+		}
+		if (!IS_ERR(child))
+			dput(child);
+	}
+
 	host_err = fh_want_write(fhp);
 	if (host_err)
 		return nfserrno(host_err);
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index ca9460e97e2b..9a8c1e37cc0f 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -270,6 +270,36 @@ 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);
+			if (nfsd_cross_mnt(rqstp, &child, &exp) == 0) {
+				status = check_nfsd_access(exp, rqstp, false);
+				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;
+			}
+			exp_put(exp);
+		}
+		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..549eed8f2c19 100644
--- a/fs/nfsd/nfsproc.c
+++ b/fs/nfsd/nfsproc.c
@@ -302,11 +302,34 @@ nfsd_proc_create(struct svc_rqst *rqstp)
 	if (resp->status != nfs_ok)
 		goto done; /* must fh_put dirfhp even on error */
 
+	fh_init(newfhp, NFS_FHSIZE);
+
 	/* Check for NFSD_MAY_WRITE in nfsd_create if necessary */
 
 	resp->status = nfserr_exist;
 	if (name_is_dot_dotdot(argp->name, argp->len))
 		goto done;
+
+	/*
+	 * If name is already in dcache we need to check for mountpoints
+	 */
+	dchild = try_lookup_noperm(&QSTR_LEN(argp->name, argp->len),
+				   dirfhp->fh_export);
+	if (dchild && !IS_ERR(dchild) && d_is_reg(child) &&
+	    unlikely(nfsd_mountpoint(dchild, fhp->fh_export))) {
+		struct svc_export *exp = fhp->fh_export;
+		if (nfsd_cross_mnt(rqstp, &dchild, &exp) == 0 &&
+		    d_isreg(dchild)) {
+			resp->status = check_nfsd_access(exp, rqstp, false);
+			if (resp->status == nfs_ok)
+				resp->status = fh_compose(newfhp, dirfhp->fh_export,
+							  dchild, dirfhp);
+			goto done;
+		}
+	}
+	if (!IS_ERR(dchild))
+		dput(dchild);
+
 	hosterr = fh_want_write(dirfhp);
 	if (hosterr) {
 		resp->status = nfserrno(hosterr);
@@ -319,7 +342,6 @@ nfsd_proc_create(struct svc_rqst *rqstp)
 		resp->status = nfserrno(PTR_ERR(dchild));
 		goto out_write;
 	}
-	fh_init(newfhp, NFS_FHSIZE);
 	resp->status = fh_compose(newfhp, dirfhp->fh_export, dchild, dirfhp);
 	if (!resp->status && d_really_is_negative(dchild))
 		resp->status = nfserr_noent;
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH v3 02/17] nfsd: correctly handle CREATE of mounted-on files

[Chuck Lever]

Hi Neil-

On Mon, Jul 13, 2026, at 2:15 AM, NeilBrown wrote:
> 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.
>
> Signed-off-by: NeilBrown <neil@brown.name>

I didn't see issues in the other patches in this series, but this
new one does have some correctness issues. This one doesn't build
here with CONFIG_NFSD_V2=y, and the NFSv2 and NFSv3 create paths
have some refcount and behavior problems. The NFSv4 path looks
good.

Big picture: the three create paths now handle an existing
mounted-on file three different ways. v4 sets op_truncate for
size-zero truncation only, v3 applies the full client iattr, and
v2 applies nothing.  The v4 behavior is the one I prefer, so
bring v2 and v3 into line with it.

Specifics below.


> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> index bbaef884f893..20eaf56fa9e7 100644
> --- a/fs/nfsd/nfs3proc.c
> +++ b/fs/nfsd/nfs3proc.c
> @@ -303,6 +303,34 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct 
> svc_fh *fhp,
>  	parent = fhp->fh_dentry;
>  	inode = d_inode(parent);
> 
> +	if (argp->createmode == NFS3_CREATE_UNCHECKED) {
> +		/*
> +		 * If name is already in dcache we need to check for mountpoints
> +		 */
> +		child = try_lookup_noperm(&QSTR_LEN(argp->name,
> +						    argp->len),
> +					  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);
> +			if (nfsd_cross_mnt(rqstp, &child, &exp) == 0) {
> +				status = check_nfsd_access(exp, rqstp, false);
> +				if (status == nfs_ok)
> +					status = fh_compose(resfhp, exp,
> +							    child, fhp);
> +				if (status == nfs_ok)
> +					status = nfsd_create_setattr(
> +						rqstp, fhp, resfhp, &attrs);
> +				dput(child);
> +				exp_put(exp);
> +				return status;
> +			}
> +			exp_put(exp);
> +		}
> +		if (!IS_ERR(child))
> +			dput(child);
> +	}
> +
>  	host_err = fh_want_write(fhp);
>  	if (host_err)
>  		return nfserrno(host_err);

The ordinary UNCHECKED path masks iap->ia_valid to ATTR_SIZE before
calling nfsd_create_setattr(). This branch passes the full client
iattr, so it applies atime/mtime to the existing mounted-on file
that the ordinary create path drops. Mask to ATTR_SIZE here too.


> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> index f60043632575..549eed8f2c19 100644
> --- a/fs/nfsd/nfsproc.c
> +++ b/fs/nfsd/nfsproc.c
> @@ -302,11 +302,34 @@ nfsd_proc_create(struct svc_rqst *rqstp)
>  	if (resp->status != nfs_ok)
>  		goto done; /* must fh_put dirfhp even on error */
> 
> +	fh_init(newfhp, NFS_FHSIZE);
> +
>  	/* Check for NFSD_MAY_WRITE in nfsd_create if necessary */
> 
>  	resp->status = nfserr_exist;
>  	if (name_is_dot_dotdot(argp->name, argp->len))
>  		goto done;
> +
> +	/*
> +	 * If name is already in dcache we need to check for mountpoints
> +	 */
> +	dchild = try_lookup_noperm(&QSTR_LEN(argp->name, argp->len),
> +				   dirfhp->fh_export);
> +	if (dchild && !IS_ERR(dchild) && d_is_reg(child) &&
> +	    unlikely(nfsd_mountpoint(dchild, fhp->fh_export))) {

This hunk does not compile with CONFIG_NFSD_V2=y.


> +		struct svc_export *exp = fhp->fh_export;
> +		if (nfsd_cross_mnt(rqstp, &dchild, &exp) == 0 &&
> +		    d_isreg(dchild)) {

nfsd_cross_mnt() drops a reference on the export it is given and
returns referenced replacements in dchild and exp. This branch
hands it the filehandle's borrowed fh_export with no exp_get(),
so a successful crossing underflows the export refcount. It then
jumps to done without releasing either replacement, leaking dchild
and exp. The v3 and v4 hunks get this right: exp_get() first,
dput(child) and exp_put(exp) after.


> +			resp->status = check_nfsd_access(exp, rqstp, false);
> +			if (resp->status == nfs_ok)
> +				resp->status = fh_compose(newfhp, dirfhp->fh_export,
> +							  dchild, dirfhp);

After the crossing, dchild is on the mounted filesystem, which exp
describes, not dirfhp->fh_export. Thus fh_compose() must use exp
here.


> +			goto done;

The normal v2 path truncates an existing regular file: it masks to
ATTR_SIZE and calls nfsd_setattr(). This branch returns without
truncating, so an UNCHECKED create with size zero leaves the
mounted-on file's contents intact.


Lastly, should we consider this patch for backporting to LTS? If
so, I'm guessing the issues it fixes were introduced at different
points in the commit history, so this patch would have to be split
accordingly. (If no backporting is necessary, then it can remain
as a single patch).


-- 
Chuck Lever

Re: [PATCH v3 02/17] nfsd: correctly handle CREATE of mounted-on files

[NeilBrown]

On Mon, 13 Jul 2026, Chuck Lever wrote:
> Hi Neil-
> 
> On Mon, Jul 13, 2026, at 2:15 AM, NeilBrown wrote:
> > 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.
> >
> > Signed-off-by: NeilBrown <neil@brown.name>
> 
> I didn't see issues in the other patches in this series, but this
> new one does have some correctness issues. This one doesn't build
> here with CONFIG_NFSD_V2=y, and the NFSv2 and NFSv3 create paths
> have some refcount and behavior problems. The NFSv4 path looks
> good.
> 
> Big picture: the three create paths now handle an existing
> mounted-on file three different ways. v4 sets op_truncate for
> size-zero truncation only, v3 applies the full client iattr, and
> v2 applies nothing.  The v4 behavior is the one I prefer, so
> bring v2 and v3 into line with it.
> 
> Specifics below.
> 
> 
> > diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> > index bbaef884f893..20eaf56fa9e7 100644
> > --- a/fs/nfsd/nfs3proc.c
> > +++ b/fs/nfsd/nfs3proc.c
> > @@ -303,6 +303,34 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct 
> > svc_fh *fhp,
> >  	parent = fhp->fh_dentry;
> >  	inode = d_inode(parent);
> > 
> > +	if (argp->createmode == NFS3_CREATE_UNCHECKED) {
> > +		/*
> > +		 * If name is already in dcache we need to check for mountpoints
> > +		 */
> > +		child = try_lookup_noperm(&QSTR_LEN(argp->name,
> > +						    argp->len),
> > +					  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);
> > +			if (nfsd_cross_mnt(rqstp, &child, &exp) == 0) {
> > +				status = check_nfsd_access(exp, rqstp, false);
> > +				if (status == nfs_ok)
> > +					status = fh_compose(resfhp, exp,
> > +							    child, fhp);
> > +				if (status == nfs_ok)
> > +					status = nfsd_create_setattr(
> > +						rqstp, fhp, resfhp, &attrs);
> > +				dput(child);
> > +				exp_put(exp);
> > +				return status;
> > +			}
> > +			exp_put(exp);
> > +		}
> > +		if (!IS_ERR(child))
> > +			dput(child);
> > +	}
> > +
> >  	host_err = fh_want_write(fhp);
> >  	if (host_err)
> >  		return nfserrno(host_err);
> 
> The ordinary UNCHECKED path masks iap->ia_valid to ATTR_SIZE before
> calling nfsd_create_setattr(). This branch passes the full client
> iattr, so it applies atime/mtime to the existing mounted-on file
> that the ordinary create path drops. Mask to ATTR_SIZE here too.
> 
> 
> > diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> > index f60043632575..549eed8f2c19 100644
> > --- a/fs/nfsd/nfsproc.c
> > +++ b/fs/nfsd/nfsproc.c
> > @@ -302,11 +302,34 @@ nfsd_proc_create(struct svc_rqst *rqstp)
> >  	if (resp->status != nfs_ok)
> >  		goto done; /* must fh_put dirfhp even on error */
> > 
> > +	fh_init(newfhp, NFS_FHSIZE);
> > +
> >  	/* Check for NFSD_MAY_WRITE in nfsd_create if necessary */
> > 
> >  	resp->status = nfserr_exist;
> >  	if (name_is_dot_dotdot(argp->name, argp->len))
> >  		goto done;
> > +
> > +	/*
> > +	 * If name is already in dcache we need to check for mountpoints
> > +	 */
> > +	dchild = try_lookup_noperm(&QSTR_LEN(argp->name, argp->len),
> > +				   dirfhp->fh_export);
> > +	if (dchild && !IS_ERR(dchild) && d_is_reg(child) &&
> > +	    unlikely(nfsd_mountpoint(dchild, fhp->fh_export))) {
> 
> This hunk does not compile with CONFIG_NFSD_V2=y.
> 
> 
> > +		struct svc_export *exp = fhp->fh_export;
> > +		if (nfsd_cross_mnt(rqstp, &dchild, &exp) == 0 &&
> > +		    d_isreg(dchild)) {
> 
> nfsd_cross_mnt() drops a reference on the export it is given and
> returns referenced replacements in dchild and exp. This branch
> hands it the filehandle's borrowed fh_export with no exp_get(),
> so a successful crossing underflows the export refcount. It then
> jumps to done without releasing either replacement, leaking dchild
> and exp. The v3 and v4 hunks get this right: exp_get() first,
> dput(child) and exp_put(exp) after.
> 
> 
> > +			resp->status = check_nfsd_access(exp, rqstp, false);
> > +			if (resp->status == nfs_ok)
> > +				resp->status = fh_compose(newfhp, dirfhp->fh_export,
> > +							  dchild, dirfhp);
> 
> After the crossing, dchild is on the mounted filesystem, which exp
> describes, not dirfhp->fh_export. Thus fh_compose() must use exp
> here.
> 
> 
> > +			goto done;
> 
> The normal v2 path truncates an existing regular file: it masks to
> ATTR_SIZE and calls nfsd_setattr(). This branch returns without
> truncating, so an UNCHECKED create with size zero leaves the
> mounted-on file's contents intact.
> 
> 
> Lastly, should we consider this patch for backporting to LTS? If
> so, I'm guessing the issues it fixes were introduced at different
> points in the commit history, so this patch would have to be split
> accordingly. (If no backporting is necessary, then it can remain
> as a single patch).

Thanks for the review! I'll develop some fixes in a day or 3.
I wouldn't bother back poring.  This is not a regression and is not
exploitable.

This has *never* worked correctly.  CREATE has *always* ignored mounts.
unchecked-create is unique in that it doesn't fail with -EEXIST, but
succeeds without having gone through nfsd_lookup().
Maybe I should try to refactor nfsd_lookup() and use the same code...

Thanks,
NeilBrown

[PATCH v3 03/17] 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 9a8c1e37cc0f..3e5c1fdde57b 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;
@@ -357,9 +356,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)
@@ -406,9 +403,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;
@@ -494,6 +488,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:
@@ -525,11 +522,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

[PATCH v3 04/17] 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 3e5c1fdde57b..443ed535e092 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -299,22 +299,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;
@@ -322,6 +318,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)) {
@@ -438,8 +440,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 v3 05/17] 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 443ed535e092..3568059b0c4a 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -299,6 +299,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) {
@@ -318,6 +321,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);
@@ -337,23 +364,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.
@@ -402,9 +412,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;
@@ -418,14 +425,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 v3 06/17] 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 3568059b0c4a..ec3e31376da4 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -392,22 +392,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 v3 07/17] 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 ec3e31376da4..83ad690a4948 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) {
@@ -362,22 +366,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
@@ -385,41 +397,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 v3 08/17] 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 83ad690a4948..33c112eda4c4 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))
@@ -349,6 +349,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);
@@ -362,16 +364,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 v3 09/17] 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 33c112eda4c4..7fb63d1836ba 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,
 	__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;
@@ -351,11 +351,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));
@@ -426,8 +425,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 v3 10/17] 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 7fb63d1836ba..abe27a4841eb 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -168,9 +168,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;
@@ -252,6 +252,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,
@@ -364,7 +366,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 {
@@ -518,7 +548,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 v3 11/17] 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 abe27a4841eb..a1dfe0a31ad7 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -368,7 +368,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,
@@ -402,9 +402,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;
@@ -454,7 +457,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 v3 12/17] 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 a1dfe0a31ad7..32b6c0e507ea 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
@@ -398,9 +358,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 v3 13/17] 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 32b6c0e507ea..adfc1f5ccd98 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -219,7 +219,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;
@@ -268,6 +272,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) {
@@ -324,27 +346,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) {
@@ -358,39 +366,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 v3 14/17] 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 adfc1f5ccd98..0d1bcb12ecbc 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -344,6 +344,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;
@@ -378,6 +380,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;
 
@@ -421,7 +425,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;
@@ -432,8 +445,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

Re: [PATCH v3 14/17] nfsd: reduce want-write range in nfsd4_create_file()

[Jeff Layton]

On Mon, 2026-07-13 at 16:15 +1000, NeilBrown wrote:
> 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 adfc1f5ccd98..0d1bcb12ecbc 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -344,6 +344,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;
> @@ -378,6 +380,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;
>  
> @@ -421,7 +425,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;
> @@ -432,8 +445,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;
>  }

It sucks that file creation is so fraught with peril and places that
things can go wrong and leave stuff sitting out on the fs. I wonder if
we ought to be using O_TMPFILE where possible to do all of this setup
and only later link it into the namespace. That's a much bigger change
though of course.

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

[PATCH v3 15/17] 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 0d1bcb12ecbc..ffeda7214d66 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -168,23 +168,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)
@@ -212,8 +206,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,
@@ -356,8 +348,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)) {
@@ -536,8 +528,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;
 
@@ -549,6 +540,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 9e05c3949cc1..c0e8c87a5e00 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -61,7 +61,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;
@@ -105,6 +105,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

Re: [PATCH v3 15/17] nfsd: move v0 checking out of nfsd_check_obj_isreg()

[Jeff Layton]

On Mon, 2026-07-13 at 16:15 +1000, NeilBrown wrote:
> 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 0d1bcb12ecbc..ffeda7214d66 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -168,23 +168,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)
> @@ -212,8 +206,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,
> @@ -356,8 +348,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)) {
> @@ -536,8 +528,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;
>  
> @@ -549,6 +540,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 9e05c3949cc1..c0e8c87a5e00 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -61,7 +61,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;
> @@ -105,6 +105,8 @@ nfserrno (int errno)
>  		{ nfserr_perm, -ENOKEY },
>  		{ nfserr_no_grace, -ENOGRACE},
>  		{ nfserr_io, -EBADMSG },
> +		{ nfserr_symlink, -ELOOP },
> +		{ nfserr_wrong_type, -EFTYPE },
>  	};
>  	int	i;
>  

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

[PATCH v3 16/17] 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 ffeda7214d66..6a4bddfc92cc 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -195,6 +195,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_error = 0;
+
+	want_write_error = 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_error) {
+		filp = ERR_PTR(want_write_error);
+	} else {
+		filp = dentry_create(&path, oflags, mode, current_cred());
+		child = path.dentry;
+	}
+	end_creating(child);
+out:
+	if (!want_write_error)
+		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
@@ -212,12 +257,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;
 
@@ -229,8 +274,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) {
 		/*
@@ -238,7 +281,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);
@@ -261,7 +304,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();
 
 	/*
@@ -326,58 +369,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 v3 17/17] 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.

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

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 6a4bddfc92cc..68fa3c7c987f 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -241,29 +241,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))
@@ -275,6 +276,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
@@ -307,11 +312,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) {
@@ -346,7 +355,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;
 
 		/*
@@ -368,11 +377,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),
@@ -394,14 +403,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.
 			 */
@@ -496,46 +506,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 v3 00/17] nfsd: refactor nfs4_create_file()

[Jeff Layton]

On Mon, 2026-07-13 at 16:15 +1000, NeilBrown wrote:
> This v2 incorporates fixes for issues Chuck reported from "Codex
> review".  These changes are distributed around the patches a bit, but
> 02/17 is an interesting new bugfix.
> 
> This is against nfs-testing (5f5dc3d855cf).
> 
> Thanks,
> NeilBrown
> 
>  [PATCH v3 01/17] nfsd: honour client-provided attributes for
>  [PATCH v3 02/17] nfsd: correctly handle CREATE of mounted-on files
>  [PATCH v3 03/17] nfsd: replace fh_fill_both_attrs() with
>  [PATCH v3 04/17] nfsd: move fh_want_write() after preamble in
>  [PATCH v3 05/17] nfsd: move more nfs-specific code into preamble of
>  [PATCH v3 06/17] nfsd: remove subtlety from nfsd4_create_file()
>  [PATCH v3 07/17] nfsd: in nfsd4_create_file() let VFS report if file
>  [PATCH v3 08/17] nfsd: nfsd4_create_file(): Move NFSD_MAY_CREATE
>  [PATCH v3 09/17] nfsd: fh_want_write) failure need not be immediately
>  [PATCH v3 10/17] nfsd: (almost) always open file in
>  [PATCH v3 11/17] nfsd: reduce range of directory lock in
>  [PATCH v3 12/17] nfsd: open-code nfsd4_vfs_create() into
>  [PATCH v3 13/17] nfsd: move some code out of the
>  [PATCH v3 14/17] nfsd: reduce want-write range in nfsd4_create_file()
>  [PATCH v3 15/17] nfsd: move v0 checking out of nfsd_check_obj_isreg()
>  [PATCH v3 16/17] nfsd: separate out VFS-specific code from
>  [PATCH v3 17/17] nfsd: use do_lookup_open() for non-creating open

I ran a review pass over this with an LLM and it found a few issues.
Let me know if you want the more detailed inline reviews:

Regressions found
-----------------
[high] 850097d914d3 "always open file in nfsd4_create_file()"
  dentry_open() now runs on any positive dentry the client name resolves
  to, before the nfsd_check_obj_isreg() type check in do_open_lookup().
  For an existing fifo, the default O_RDONLY (no O_NONBLOCK) open blocks in
  fifo_open() waiting for a writer, and does so while the parent directory
  i_rwsem is held (start_creating..end_creating) - an nfsd thread stall
  with the directory locked. Device nodes invoke the driver ->open. The
  ordering persists to the series head (do_lookup_open). Detail:
  series-review/850097d914d3/review-inline.txt.

[medium] 90a149d88abc "reduce want-write range in nfsd4_create_file()"
  Issue 1: For a write-mode create the setattr now runs without
  sb_start_write() freeze protection. The reduced want_write is dropped
  after end_creating(); only the O_RDONLY branch reacquires fh_want_write()
  around nfsd_create_setattr(). A write-mode open holds the mnt writer
  reference but not freeze protection, and nfsd_setattr() does not take its
  own (resfhp->fh_dentry is set, so get_write_count is false). A concurrent
  freeze_super() can complete in the window, after which notify_change()
  modifies a frozen filesystem.
  Issue 2 (low): commit message says "we add a call to dentry_open()" for
  the already-exists case, but that call pre-exists; only the comment is
  reworded. Detail: series-review/90a149d88abc/review-inline.txt.

[medium] f467dd742c2d "separate out VFS-specific ... do_lookup_open()"
  Issue 1: "if (status == NFSERR_NOENT && create_status)" compares a __be32
  status against the raw host enum NFSERR_NOENT (2). status holds
  nfserr_noent == cpu_to_be32(2) == 0x02000000 on little-endian, so the
  branch never fires and the create-permission error (create_status) is
  masked by NFS4ERR_NOENT. Should be nfserr_noent. Persists to commit 14.
  Issue 2 (trivial): duplicated word in the subject ("from from").
  Detail: series-review/f467dd742c2d/review-inline.txt.

[high] b4f1a9d54d0f "use do_lookup_open() for non-creating open requests too."
  Issue 1: do_lookup_open() begins with an unconditional mnt_want_write().
  Now that non-creating OPEN is routed through it (previously nfsd_lookup(),
  which needs no write access), a plain read-only OPEN of an existing file
  returns NFS4ERR_ROFS when the exported filesystem is mounted read-only or
  has a read-only superblock (squashfs, iso9660, erofs, ro-mounted
  ext4/xfs). mnt_want_write() also calls sb_start_write(), so a read OPEN
  now blocks while the filesystem is frozen.
  Issue 2 (medium/perf): do_lookup_open() uses start_creating(), taking the
  parent i_rwsem exclusively for every non-creating OPEN; nfsd_lookup()
  previously used lookup_one_unlocked() (shared, or none on a dcache hit),
  so concurrent opens in one directory are now serialised.
  Detail: series-review/b4f1a9d54d0f/review-inline.txt.

-- 
Jeff Layton <jlayton@kernel.org>