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

[Jeff Layton <jlayton@kernel.org>]

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

My Claude spotted this:

Should this compare against nfserr_noent rather than NFSERR_NOENT?

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


-- 
Jeff Layton <jlayton@kernel.org>