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

On Fri, 05 Jun 2026, Chuck Lever wrote:
> 
> On Sun, May 31, 2026, at 11:38 PM, NeilBrown wrote:
> 
> > @@ -429,45 +438,35 @@ 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(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 (open->op_create && status == 0 &&
> > +	    nfsd4_create_is_exclusive(open->op_createmode))
> > +		open->op_bmval[1] |= (FATTR4_WORD1_TIME_ACCESS |
> > +				      FATTR4_WORD1_TIME_MODIFY);
> > 
> > -		/*
> > -		 * 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);
> > -	}
> >  	if (status)
> >  		goto out;
> >  	status = nfsd_check_obj_isreg(*resfh, cstate->minorversion);
> 
> Logic bug: Pre-series, the non-create case takes do_open_lookup()'s
> else branch and succeeds; post-series, do_open_lookup() calls
> nfsd4_open_file() unconditionally. Every CLAIM_NULL open of an
> existing file fails with NFS4ERR_EXIST.

yes - that is bad. I think

 	if (!open->op_created &&
-	    createmode == NFS4_CREATE_UNCHECKED) {
+	    (open->op_create == NFS4_OPEN_NOCREATE ||
+	     createmode == NFS4_CREATE_UNCHECKED)) {
 		/* NFSv4 protocol requires change attributes
 		 * even though no change happened.
 		 */

is what I wanted to do.  I've also move the clearing of ATTR_SIZE down
to where we know we did created the file, so no trunc is needed.

Thanks a lot for the review.

NeilBrown