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

[Jeff Layton <jlayton@kernel.org>]

On Fri, 2026-07-17 at 19:28 +1000, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> Now that we have do_lookup_open() for creating open requests, we can use
> it for non-creating too as do_lookup_open() is already able to do that.
> 
> This prepares for switching to vfs_lookup_open() once the VFS provides
> that.  This will ensure consistent code and fs-interaction with VFS open().
> 
> The resulting simplification allows fh_fill_pre_attrs_unlocked() to be
> moved into nfsd4_open_file() (renamed from nfsd4_create_file()) so it is
> closer to fh_full_post_attrs and fh_fill_post_noop calls.
> 
> As ->op_create_mode isn't defined when op_create is zero, we need a
> local create_mode which is -1 (illegal value) when op_create is zero.
> 
> The non-create path now doesn't use nfsd_lookup().  As mount-point
> crossing including nfsd_check_access() is already included for existing
> names, this does not lose us anything.
> 
> We now only call mnt_want_write() is there is a flag which indicates we
> might want write access - previously O_CREAT was always set.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4proc.c | 113 ++++++++++++++++++++++-----------------------
>  1 file changed, 56 insertions(+), 57 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index b7fdc75c4b81..11d0411dec08 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -204,9 +204,10 @@ static struct file *do_lookup_open(struct path *parent,
>  	struct file *filp = NULL;
>  	struct path path;
>  	struct dentry *child;
> -	int want_write_err = 0;
> +	int want_write_err = -ENOENT;
>  
> -	want_write_err = mnt_want_write(parent->mnt);
> +	if (oflags & (O_CREAT|O_RDONLY|O_RDWR|O_TRUNC))
> +		want_write_err = mnt_want_write(parent->mnt);
>  
>  	child = start_creating(&nop_mnt_idmap, parent->dentry, name);
>  	if (IS_ERR(child)) {
> @@ -242,29 +243,30 @@ static struct file *do_lookup_open(struct path *parent,
>  }
>  
>  /*
> - * Implement NFSv4's unchecked, guarded, and exclusive create
> - * semantics for regular files. Open state for this new file is
> - * subsequently fabricated in nfsd4_process_open2().
> - *
> + * Implement NFSv4's open semantics for regular files.
> + * Both create (unchecked, guarded, and exclusive) and non-create.
> + * Open state for this new file is subsequently fabricated in
> + * nfsd4_process_open2().
>   * Upon return, caller must release @fhp and @resfhp.
>   */
>  static __be32
> -nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> -		  struct svc_fh *resfhp, struct nfsd4_open *open)
> +nfsd4_open_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> +		struct svc_fh *resfhp, struct nfsd4_open *open)
>  {
>  	struct iattr *iap = &open->op_iattr;
>  	struct nfsd_attrs attrs = {
>  		.na_iattr	= iap,
>  		.na_seclabel	= &open->op_label,
>  	};
> -	int oflags = O_CREAT | O_LARGEFILE;
> +	int oflags = O_LARGEFILE;
>  	struct dentry *child = ERR_PTR(-EINVAL);
>  	struct path parent = {
>  		.mnt = fhp->fh_export->ex_path.mnt,
>  		.dentry = fhp->fh_dentry,
>  	};
>  	__u32 v_mtime, v_atime;
> -	__be32 status, create_status;
> +	int createmode = -1;
> +	__be32 status, create_status = 0;
>  	int want_write_err;
>  
>  	if (name_is_dot_dotdot(open->op_fname, open->op_fnamelen))
> @@ -276,6 +278,10 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	if (status != nfs_ok)
>  		return status;
>  
> +	status = fh_fill_pre_attrs_unlocked(fhp);
> +	if (status)
> +		return status;
> +
>  	if (open->op_createmode == NFS4_CREATE_UNCHECKED) {
>  		/*
>  		 * If name is already in dcache we need to check for mountpoints
> @@ -305,11 +311,15 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	if (!IS_POSIXACL(d_inode(parent.dentry)))
>  		iap->ia_mode &= ~current_umask();
>  
> +	if (open->op_create) {
> +		createmode = open->op_createmode;
> +		oflags |= O_CREAT;
> +	}
>  	/*
>  	 * For the EXCLUSIVE modes we do our own uniqueness tests
>  	 * so don't want O_EXCL.
>  	 */
> -	if (open->op_createmode == NFS4_CREATE_GUARDED)
> +	if (createmode == NFS4_CREATE_GUARDED)
>  		oflags |= O_EXCL;
>  
>  	switch (open->op_share_access & NFS4_SHARE_ACCESS_BOTH) {
> @@ -344,7 +354,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  
>  	v_mtime = 0;
>  	v_atime = 0;
> -	if (nfsd4_create_is_exclusive(open->op_createmode)) {
> +	if (nfsd4_create_is_exclusive(createmode)) {
>  		u32 *verifier = (u32 *)open->op_verf.data;
>  
>  		/*
> @@ -366,11 +376,11 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		iap->ia_atime.tv_nsec = 0;
>  	}
>  
> -	create_status = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE);
> -	if (create_status)
> -		/* Might still succeed if no create is needed */
> -		oflags &= ~O_CREAT;
> -
> +	if (oflags & O_CREAT) {
> +		create_status = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE);
> +		if (create_status)
> +			oflags &= ~O_CREAT;
> +	}
>  	open->op_filp = do_lookup_open(&parent,
>  				       &QSTR_LEN(open->op_fname,
>  						 open->op_fnamelen),
> @@ -392,14 +402,15 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		goto out;
>  
>  	if (!open->op_created &&
> -	    nfsd4_create_is_exclusive(open->op_createmode) &&
> +	    nfsd4_create_is_exclusive(createmode) &&
>  	    inode_get_mtime_sec(d_inode(child)) == v_mtime &&
>  	    inode_get_atime_sec(d_inode(child)) == v_atime &&
>  	    d_inode(child)->i_size == 0)
>  		open->op_created = true;
>  
>  	if (!open->op_created) {
> -		if (open->op_createmode == NFS4_CREATE_UNCHECKED) {
> +		if (open->op_create == NFS4_OPEN_NOCREATE ||
> +		    createmode == NFS4_CREATE_UNCHECKED) {
>  			/* NFSv4 protocol requires change attributes
>  			 * even though no change happened.
>  			 */
> @@ -494,46 +505,34 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
>  	fh_init(*resfh, NFS4_FHSIZE);
>  	open->op_truncate = false;
>  
> -	status = fh_fill_pre_attrs_unlocked(current_fh);
> -	if (status)
> -		goto out;
> -	if (open->op_create) {
> -		/* FIXME: check session persistence and pnfs flags.
> -		 * The nfsv4.1 spec requires the following semantics:
> -		 *
> -		 * Persistent   | pNFS   | Server REQUIRED | Client Allowed
> -		 * Reply Cache  | server |                 |
> -		 * -------------+--------+-----------------+--------------------
> -		 * no           | no     | EXCLUSIVE4_1    | EXCLUSIVE4_1
> -		 *              |        |                 | (SHOULD)
> -		 *              |        | and EXCLUSIVE4  | or EXCLUSIVE4
> -		 *              |        |                 | (SHOULD NOT)
> -		 * no           | yes    | EXCLUSIVE4_1    | EXCLUSIVE4_1
> -		 * yes          | no     | GUARDED4        | GUARDED4
> -		 * yes          | yes    | GUARDED4        | GUARDED4
> -		 */
> +	/* FIXME: check session persistence and pnfs flags.
> +	 * The nfsv4.1 spec requires the following semantics:
> +	 *
> +	 * Persistent   | pNFS   | Server REQUIRED | Client Allowed
> +	 * Reply Cache  | server |                 |
> +	 * -------------+--------+-----------------+--------------------
> +	 * no           | no     | EXCLUSIVE4_1    | EXCLUSIVE4_1
> +	 *              |        |                 | (SHOULD)
> +	 *              |        | and EXCLUSIVE4  | or EXCLUSIVE4
> +	 *              |        |                 | (SHOULD NOT)
> +	 * no           | yes    | EXCLUSIVE4_1    | EXCLUSIVE4_1
> +	 * yes          | no     | GUARDED4        | GUARDED4
> +	 * yes          | yes    | GUARDED4        | GUARDED4
> +	 */
>  
> -		current->fs->umask = open->op_umask;
> -		status = nfsd4_create_file(rqstp, current_fh, *resfh, open);
> -		current->fs->umask = 0;
> +	current->fs->umask = open->op_umask;
> +	status = nfsd4_open_file(rqstp, current_fh, *resfh, open);
> +	current->fs->umask = 0;
>  
> -		/*
> -		 * Following rfc 3530 14.2.16, and rfc 5661 18.16.4
> -		 * use the returned bitmask to indicate which attributes
> -		 * we used to store the verifier:
> -		 */
> -		if (nfsd4_create_is_exclusive(open->op_createmode) && status == 0)
> -			open->op_bmval[1] |= (FATTR4_WORD1_TIME_ACCESS |
> -						FATTR4_WORD1_TIME_MODIFY);
> -	} else {
> -		status = nfsd_lookup(rqstp, current_fh,
> -				     open->op_fname, open->op_fnamelen, *resfh);
> -		/*
> -		 * NFSv4 protocol requires change attributes even though
> -		 * no change happened.
> -		 */
> -		fh_fill_post_noop(current_fh);
> -	}
> +	/*
> +	 * Following rfc 3530 14.2.16, and rfc 5661 18.16.4
> +	 * use the returned bitmask to indicate which attributes
> +	 * we used to store the verifier:
> +	 */
> +	if (open->op_create &&
> +	    nfsd4_create_is_exclusive(open->op_createmode) && status == 0)
> +		open->op_bmval[1] |= (FATTR4_WORD1_TIME_ACCESS |
> +				      FATTR4_WORD1_TIME_MODIFY);
>  	if (status)
>  		goto out;
>  	status = nfserrno(nfsd_check_obj_isreg((*resfh)->fh_dentry));

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

-- 
Jeff Layton <jlayton@kernel.org>