Re: [PATCH 5/7] nfsd: switch NFS4 OPEN to use vfs_lookup_open()

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

>     This implementation shares more code with syscall open paths and so uses
>     some filesystem interfaces slightly more correctly.  It also takes the

Which interfaces? The concrete difference I can see is that lookup_open()
honors ->atomic_open, which do_lookup_open() never called. Naming that
would be clearer than "slightly more correctly".


>     We need to pass O_NONBLOCK so that that EWOULDBLOCK errors from

Repeated word: "that that".


>     vfs_lookup_open() always returns -EFTYPE if a non-regular-file was
>     found, and provides the dentry in parent.dentry.  We can use
>     nfsd_check_obj_is_reg() to turn this into an error.
>
>     As parent.dentry could be NULL, we enhance nfsd_check_obj_is_reg() to
>     cope with that.

The function is nfsd_check_obj_isreg(), in both places.


>     vfs_lookup_open() will return -EEXIST if required for
>     NFS_CREATE_GUARDED4 as O_EXCL is passed in.  Other checks for
>     and existing object need only test for nfsd4_create_is_exclusive().

s/and existing/an existing/, and the identifier is NFS4_CREATE_GUARDED.

The -EEXIST claim holds only on the path through the tail of
vfs_lookup_open(), where the O_EXCL test runs before the d_is_reg()
test. When lookup_open() itself returns -EFTYPE, vfs_lookup_open()
returns before either test. See below.


> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 3a82af381a8d..0fd5a6411ed3 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c

[ ... ]

> @@ -312,7 +269,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		.na_iattr	= iap,
>  		.na_seclabel	= &open->op_label,
>  	};
> -	int oflags = O_CREAT | O_LARGEFILE;
> +	int oflags = O_CREAT | O_LARGEFILE | O_NONBLOCK;

Nit: do_dentry_open() strips O_CREAT, O_EXCL and O_TRUNC from f_flags
and leaves O_NONBLOCK in place, so op_filp now carries O_NONBLOCK into
the filecache entry that nfsd_file_acquire_opened() builds from it.
Files opened through __nfsd_open() do not. Is that divergence
intended?


>  	struct dentry *child = ERR_PTR(-EINVAL);
>  	struct path parent = {
>  		.mnt = fhp->fh_export->ex_path.mnt,
> @@ -424,28 +381,29 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		/* Might still succeed if no create is needed */
>  		oflags &= ~O_CREAT;
>  
> -	open->op_filp = do_lookup_open(&parent,
> -				       &QSTR_LEN(open->op_fname,
> -						 open->op_fnamelen),
> -				       oflags,
> -				       open->op_iattr.ia_mode);
> +	dget(parent.dentry);
> +	open->op_filp = vfs_lookup_open(&parent,
> +					&QSTR_LEN(open->op_fname,
> +						  open->op_fnamelen),
> +					oflags,
> +					open->op_iattr.ia_mode & S_IALLUGO);

Nit: after this call "parent.dentry" is the child (or NULL), while a
few lines up IS_POSIXACL(d_inode(parent.dentry)) reads the same
expression as the parent directory. The VFS patch renamed its parameter
to @path for this reason. Would renaming the local here avoid the same
trap?


>  	if (IS_ERR(open->op_filp)) {
>  		int hosterr = PTR_ERR(open->op_filp);
>  
> -		if (open->op_createmode != NFS4_CREATE_UNCHECKED) {
> -			switch (hosterr) {
> -			case -EISDIR:
> -			case -ELOOP:
> -			case -EFTYPE:
> +		if (hosterr == -EFTYPE) {
> +			if (nfsd4_create_is_exclusive(open->op_createmode))
>  				hosterr = -EEXIST;
> -			}
> +			else
> +				hosterr = nfsd_check_obj_isreg(parent.dentry);
>  		}

Does this drop the NFS4ERR_EXIST mapping for GUARDED4?

The old test covered GUARDED as well as the two EXCLUSIVE modes.
nfsd4_create_is_exclusive() covers only EXCLUSIVE4 and EXCLUSIVE4_1, so
a GUARDED open that finds a non-regular object now takes the else arm
and returns NFS4ERR_ISDIR, NFS4ERR_WRONG_TYPE, and so on. RFC 8881
section 18.16.3 says a GUARDED4 duplicate is NFS4ERR_EXIST.

Keeping "open->op_createmode != NFS4_CREATE_UNCHECKED" here would
restore the old mapping without depending on the ordering of checks
inside each filesystem's ->atomic_open.

Separately, can this arm return nfs_ok? nfsd_check_obj_isreg() returns
0 for a regular file, and vfs_lookup_open() can leave a regular file in
parent.dentry when the d_lookup() it does after an -EFTYPE races with a
rename (the VFS marks that case with a WARN_ON_ONCE rather than
excluding it). nfserrno(0) is nfs_ok, op_filp is then set to NULL, and
nfsd4_create_file() returns success with resfhp never composed. It only
fails safely today because do_open_lookup() then calls
nfsd_check_obj_isreg() on the NULL fh_dentry. Something like

		if (!hosterr)
			hosterr = -EFTYPE;

would address that.


>  		status = nfserrno(hosterr);
>  		open->op_filp = NULL;
>  		if (status == nfserr_noent && create_status)
>  			status = create_status;
> +		dput(parent.dentry);
>  		goto out;
>  	}
> +	dput(parent.dentry);
>  
>  	child = open->op_filp->f_path.dentry;
>  	open->op_created = open->op_filp->f_mode & FMODE_CREATED;
> @@ -462,7 +420,9 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		open->op_created = true;
>  
>  	if (!open->op_created) {
> -		if (open->op_createmode == NFS4_CREATE_UNCHECKED) {
> +		if (nfsd4_create_is_exclusive(open->op_createmode)) {
> +			status = nfserr_exist;
> +		} else {

Same question here. GUARDED used to get nfserr_exist from this branch
and now gets the "no change happened" arm. It is unreachable for GUARDED
today only because vfs_lookup_open() returns -EEXIST whenever O_EXCL is
set and nothing was created. Should this also stay as
"!= NFS4_CREATE_UNCHECKED" so the protocol rule is enforced here rather
than relying on that ordering?


-- 
Chuck Lever (Come to NFS bake-a-thon! https://nfsv4bat.org)