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

["Chuck Lever" <cel@kernel.org>]

On Mon, Aug 24, 2026, at 5:55 PM, NeilBrown wrote:
> On Tue, 18 Aug 2026, NeilBrown wrote:
>> 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@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.
>> > 
>> > 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.
>
> Hi Chuck,
>  I notice this series wasn't in your recent pull request and they seem
>  to have disappeared from your tree.

I see them in nfsd-testing, where they’ve been since August 10.


>  What do you understand the status of these patches to be?
>  I was hoping they would land this merge window.

Jeff and I discussed this, and we’d like these to get another month
or two in linux-next because they modify common paths. When v7.3-rc1
opens, they will be at the first patches in nfsd-next for v7.4.


-- 
Chuck Lever