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

On Tue, 18 Aug 2026, Chuck Lever wrote:
> 
> 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?
> 

Hi,
 sorry for not getting to this sooner.
 I can see the problem.  To fix it we need to somehow get logic similar
 to nfsd_open_break_lease() into the vfs_lookup_open() path.  I think
 that'll be possible though not trivial.

 It is safe to drop just that patch for now.  I don't need it for the
 locking work that I am doing.  I think it is still good to have and I
 will try to rehabilitate it, but not in a hurry.

 I'm confident the rest of the series is not implicated in this problem
 so there is no need to drop the whole series.

Thanks,
NeilBrown