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

[Jeff Layton <jlayton@kernel.org>]

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

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

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

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

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

-- 
Jeff Layton <jlayton@kernel.org>