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

On Wed, Aug 12, 2026, at 1:12 PM, Jeff Layton wrote:
> On Fri, 2026-07-17 at 19:28 +1000, NeilBrown wrote:
>> From: NeilBrown <neil-+NVA1uvv1dVBDLzU/[email protected]>
>> 
>> 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-+NVA1uvv1dVBDLzU/[email protected]>
>> ---
>>  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.

Neil, I need a response from you. How soon do you intend to address this
issue? Can this patch be dropped safely from the series, or do I need to
drop the whole series?


-- 
Chuck Lever