[PATCH] nfsd: switch nfsd4_open() to use vfs_lookup_open()

[PATCH] nfsd: switch nfsd4_open() to use vfs_lookup_open()

[NeilBrown]

From: NeilBrown <neil@brown.name>

The functionality that was recently gathered into do_lookup_open() is
now available from the vfs as vfs_lookup_open().

This implementation shares more code with syscall open paths and so uses
some filesystem interfaces slightly more correctly.  It also takes the
responsibility for locking out of nfsd so that planned changes can
happen entirely in VFS code.

One difference is that do_lookup_open() would use nfsd_check_obj_isreg()
to get an error when the name exists but is not a regular file.
vfs_lookup_open() uses slightly different error code, particularly
returning -ENODEV when a device-special file is found.  So we need
to map that error to -EFTYPE.

Unfortunately vfs_lookup_open() doesn't allow O_LARGEFILE so we need
to patch it to avoid a warning.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/namei.c         |  2 +-
 fs/nfsd/nfs4proc.c | 66 ++++++++++------------------------------------
 2 files changed, 15 insertions(+), 53 deletions(-)

This patch is against nfsd-test.  I would like it to land there so it
best good nfsd testing over the coming weeks.
Thanks,
NeilBrown


diff --git a/fs/namei.c b/fs/namei.c
index d95249dd527c..85fd5e8221e2 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4632,7 +4632,7 @@ struct file *vfs_lookup_open(struct path *parent, struct qstr *last,
 	int error = 0;
 
 	WARN_ONCE(mode & ~S_IALLUGO, "mode must only have permission bits");
-	WARN_ONCE(open_flag & ~(O_ACCMODE|O_CREAT|O_EXCL|O_TRUNC|__O_REGULAR),
+	WARN_ONCE(open_flag & ~(O_ACCMODE|O_CREAT|O_EXCL|O_TRUNC|__O_REGULAR|O_LARGEFILE),
 		  "open_flag has unsupported flags");
 
 	mode |= S_IFREG;
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 33dc92d48ce0..ae70f7489d95 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -213,52 +213,6 @@ static inline bool nfsd4_create_is_exclusive(int createmode)
 		createmode == NFS4_CREATE_EXCLUSIVE4_1;
 }
 
-static struct file *do_lookup_open(struct path *parent,
-				   struct qstr *name,
-				   unsigned int oflags,
-				   umode_t mode)
-{
-	struct file *filp = NULL;
-	struct path path;
-	struct dentry *child;
-	int want_write_err = 0;
-
-	want_write_err = mnt_want_write(parent->mnt);
-
-	child = start_creating(&nop_mnt_idmap, parent->dentry, name);
-	if (IS_ERR(child)) {
-		filp = ERR_CAST(child);
-		goto out;
-	}
-	path.mnt = parent->mnt;
-	path.dentry = child;
-
-	if (d_really_is_positive(child)) {
-		/*
-		 * open the file so that we consistently have a valid
-		 * op_filp and consequently a valid ->f_path.dentry.
-		 */
-		int err = nfsd_check_obj_isreg(child);
-
-		if (err)
-			filp = ERR_PTR(err);
-		else
-			filp = dentry_open(&path, oflags, current_cred());
-	} else if (!(oflags & O_CREAT)) {
-		filp = ERR_PTR(-ENOENT);
-	} else if (want_write_err) {
-		filp = ERR_PTR(want_write_err);
-	} else {
-		filp = dentry_create(&path, oflags, mode, current_cred());
-		child = path.dentry;
-	}
-	end_creating(child);
-out:
-	if (!want_write_err)
-		mnt_drop_write(parent->mnt);
-	return filp;
-}
-
 /*
  * Implement NFSv4's unchecked, guarded, and exclusive create
  * semantics for regular files. Open state for this new file is
@@ -390,13 +344,21 @@ 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);
+	open->op_filp = vfs_lookup_open(&parent,
+					&QSTR_LEN(open->op_fname,
+						  open->op_fnamelen),
+					oflags,
+					open->op_iattr.ia_mode);
 	if (IS_ERR(open->op_filp)) {
-		status = nfserrno(PTR_ERR(open->op_filp));
+		int hosterr = PTR_ERR(open->op_filp);
+
+		/*
+		 * NFS doesn't differentiate between device files and
+		 * sock/fifo when reporting an error.
+		 */
+		if (hosterr == -ENODEV)
+			hosterr = -EFTYPE;
+		status = nfserrno(hosterr);
 		open->op_filp = NULL;
 		if (status == nfserr_noent && create_status)
 			status = create_status;

base-commit: f5dc2038906bb0c9627c99bea06dd7786b5ac2d1
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH] nfsd: switch nfsd4_open() to use vfs_lookup_open()

[Jeff Layton]

On Wed, 2026-09-02 at 12:30 +1000, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> The functionality that was recently gathered into do_lookup_open() is
> now available from the vfs as vfs_lookup_open().
> 
> This implementation shares more code with syscall open paths and so uses
> some filesystem interfaces slightly more correctly.  It also takes the
> responsibility for locking out of nfsd so that planned changes can
> happen entirely in VFS code.
> 
> One difference is that do_lookup_open() would use nfsd_check_obj_isreg()
> to get an error when the name exists but is not a regular file.
> vfs_lookup_open() uses slightly different error code, particularly
> returning -ENODEV when a device-special file is found.  So we need
> to map that error to -EFTYPE.
> 
> Unfortunately vfs_lookup_open() doesn't allow O_LARGEFILE so we need
> to patch it to avoid a warning.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/namei.c         |  2 +-
>  fs/nfsd/nfs4proc.c | 66 ++++++++++------------------------------------
>  2 files changed, 15 insertions(+), 53 deletions(-)
> 
> This patch is against nfsd-test.  I would like it to land there so it
> best good nfsd testing over the coming weeks.
> Thanks,
> NeilBrown
> 
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index d95249dd527c..85fd5e8221e2 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4632,7 +4632,7 @@ struct file *vfs_lookup_open(struct path *parent, struct qstr *last,
>  	int error = 0;
>  
>  	WARN_ONCE(mode & ~S_IALLUGO, "mode must only have permission bits");
> -	WARN_ONCE(open_flag & ~(O_ACCMODE|O_CREAT|O_EXCL|O_TRUNC|__O_REGULAR),
> +	WARN_ONCE(open_flag & ~(O_ACCMODE|O_CREAT|O_EXCL|O_TRUNC|__O_REGULAR|O_LARGEFILE),
>  		  "open_flag has unsupported flags");
>  
>  	mode |= S_IFREG;
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 33dc92d48ce0..ae70f7489d95 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -213,52 +213,6 @@ static inline bool nfsd4_create_is_exclusive(int createmode)
>  		createmode == NFS4_CREATE_EXCLUSIVE4_1;
>  }
>  
> -static struct file *do_lookup_open(struct path *parent,
> -				   struct qstr *name,
> -				   unsigned int oflags,
> -				   umode_t mode)
> -{
> -	struct file *filp = NULL;
> -	struct path path;
> -	struct dentry *child;
> -	int want_write_err = 0;
> -
> -	want_write_err = mnt_want_write(parent->mnt);
> -
> -	child = start_creating(&nop_mnt_idmap, parent->dentry, name);
> -	if (IS_ERR(child)) {
> -		filp = ERR_CAST(child);
> -		goto out;
> -	}
> -	path.mnt = parent->mnt;
> -	path.dentry = child;
> -
> -	if (d_really_is_positive(child)) {
> -		/*
> -		 * open the file so that we consistently have a valid
> -		 * op_filp and consequently a valid ->f_path.dentry.
> -		 */
> -		int err = nfsd_check_obj_isreg(child);
> -
> -		if (err)
> -			filp = ERR_PTR(err);
> -		else
> -			filp = dentry_open(&path, oflags, current_cred());
> -	} else if (!(oflags & O_CREAT)) {
> -		filp = ERR_PTR(-ENOENT);
> -	} else if (want_write_err) {
> -		filp = ERR_PTR(want_write_err);
> -	} else {
> -		filp = dentry_create(&path, oflags, mode, current_cred());
> -		child = path.dentry;
> -	}
> -	end_creating(child);
> -out:
> -	if (!want_write_err)
> -		mnt_drop_write(parent->mnt);
> -	return filp;
> -}
> -
>  /*
>   * Implement NFSv4's unchecked, guarded, and exclusive create
>   * semantics for regular files. Open state for this new file is
> @@ -390,13 +344,21 @@ 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);
> +	open->op_filp = vfs_lookup_open(&parent,
> +					&QSTR_LEN(open->op_fname,
> +						  open->op_fnamelen),
> +					oflags,
> +					open->op_iattr.ia_mode);
>  	if (IS_ERR(open->op_filp)) {
> -		status = nfserrno(PTR_ERR(open->op_filp));
> +		int hosterr = PTR_ERR(open->op_filp);
> +
> +		/*
> +		 * NFS doesn't differentiate between device files and
> +		 * sock/fifo when reporting an error.
> +		 */
> +		if (hosterr == -ENODEV)
> +			hosterr = -EFTYPE;
> +		status = nfserrno(hosterr);
>  		open->op_filp = NULL;
>  		if (status == nfserr_noent && create_status)
>  			status = create_status;
> 
> base-commit: f5dc2038906bb0c9627c99bea06dd7786b5ac2d1


This looks good to me, but sashiko noted a lockdep issue you probably
want to address:

    https://sashiko.dev/#/patchset/178831622346.3510150.10735958308429473076%40noble.neil.brown.name

Other than that though, this looks good to me. Much less "fiddly".

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH] nfsd: switch nfsd4_open() to use vfs_lookup_open()

[NeilBrown]

On Wed, 02 Sep 2026, Jeff Layton wrote:
> On Wed, 2026-09-02 at 12:30 +1000, NeilBrown wrote:
> > From: NeilBrown <neil@brown.name>
> > 
> > The functionality that was recently gathered into do_lookup_open() is
> > now available from the vfs as vfs_lookup_open().
> > 
> > This implementation shares more code with syscall open paths and so uses
> > some filesystem interfaces slightly more correctly.  It also takes the
> > responsibility for locking out of nfsd so that planned changes can
> > happen entirely in VFS code.
> > 
> > One difference is that do_lookup_open() would use nfsd_check_obj_isreg()
> > to get an error when the name exists but is not a regular file.
> > vfs_lookup_open() uses slightly different error code, particularly
> > returning -ENODEV when a device-special file is found.  So we need
> > to map that error to -EFTYPE.
> > 
> > Unfortunately vfs_lookup_open() doesn't allow O_LARGEFILE so we need
> > to patch it to avoid a warning.
> > 
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> >  fs/namei.c         |  2 +-
> >  fs/nfsd/nfs4proc.c | 66 ++++++++++------------------------------------
> >  2 files changed, 15 insertions(+), 53 deletions(-)
> > 
> > This patch is against nfsd-test.  I would like it to land there so it
> > best good nfsd testing over the coming weeks.
> > Thanks,
> > NeilBrown
> > 
> > 
> > diff --git a/fs/namei.c b/fs/namei.c
> > index d95249dd527c..85fd5e8221e2 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -4632,7 +4632,7 @@ struct file *vfs_lookup_open(struct path *parent, struct qstr *last,
> >  	int error = 0;
> >  
> >  	WARN_ONCE(mode & ~S_IALLUGO, "mode must only have permission bits");
> > -	WARN_ONCE(open_flag & ~(O_ACCMODE|O_CREAT|O_EXCL|O_TRUNC|__O_REGULAR),
> > +	WARN_ONCE(open_flag & ~(O_ACCMODE|O_CREAT|O_EXCL|O_TRUNC|__O_REGULAR|O_LARGEFILE),
> >  		  "open_flag has unsupported flags");
> >  
> >  	mode |= S_IFREG;
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index 33dc92d48ce0..ae70f7489d95 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -213,52 +213,6 @@ static inline bool nfsd4_create_is_exclusive(int createmode)
> >  		createmode == NFS4_CREATE_EXCLUSIVE4_1;
> >  }
> >  
> > -static struct file *do_lookup_open(struct path *parent,
> > -				   struct qstr *name,
> > -				   unsigned int oflags,
> > -				   umode_t mode)
> > -{
> > -	struct file *filp = NULL;
> > -	struct path path;
> > -	struct dentry *child;
> > -	int want_write_err = 0;
> > -
> > -	want_write_err = mnt_want_write(parent->mnt);
> > -
> > -	child = start_creating(&nop_mnt_idmap, parent->dentry, name);
> > -	if (IS_ERR(child)) {
> > -		filp = ERR_CAST(child);
> > -		goto out;
> > -	}
> > -	path.mnt = parent->mnt;
> > -	path.dentry = child;
> > -
> > -	if (d_really_is_positive(child)) {
> > -		/*
> > -		 * open the file so that we consistently have a valid
> > -		 * op_filp and consequently a valid ->f_path.dentry.
> > -		 */
> > -		int err = nfsd_check_obj_isreg(child);
> > -
> > -		if (err)
> > -			filp = ERR_PTR(err);
> > -		else
> > -			filp = dentry_open(&path, oflags, current_cred());
> > -	} else if (!(oflags & O_CREAT)) {
> > -		filp = ERR_PTR(-ENOENT);
> > -	} else if (want_write_err) {
> > -		filp = ERR_PTR(want_write_err);
> > -	} else {
> > -		filp = dentry_create(&path, oflags, mode, current_cred());
> > -		child = path.dentry;
> > -	}
> > -	end_creating(child);
> > -out:
> > -	if (!want_write_err)
> > -		mnt_drop_write(parent->mnt);
> > -	return filp;
> > -}
> > -
> >  /*
> >   * Implement NFSv4's unchecked, guarded, and exclusive create
> >   * semantics for regular files. Open state for this new file is
> > @@ -390,13 +344,21 @@ 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);
> > +	open->op_filp = vfs_lookup_open(&parent,
> > +					&QSTR_LEN(open->op_fname,
> > +						  open->op_fnamelen),
> > +					oflags,
> > +					open->op_iattr.ia_mode);
> >  	if (IS_ERR(open->op_filp)) {
> > -		status = nfserrno(PTR_ERR(open->op_filp));
> > +		int hosterr = PTR_ERR(open->op_filp);
> > +
> > +		/*
> > +		 * NFS doesn't differentiate between device files and
> > +		 * sock/fifo when reporting an error.
> > +		 */
> > +		if (hosterr == -ENODEV)
> > +			hosterr = -EFTYPE;
> > +		status = nfserrno(hosterr);
> >  		open->op_filp = NULL;
> >  		if (status == nfserr_noent && create_status)
> >  			status = create_status;
> > 
> > base-commit: f5dc2038906bb0c9627c99bea06dd7786b5ac2d1
> 
> 
> This looks good to me, but sashiko noted a lockdep issue you probably
> want to address:
> 
>     https://sashiko.dev/#/patchset/178831622346.3510150.10735958308429473076%40noble.neil.brown.name

This is of no consequence.
start_creating() doesn't really need I_MUTEX_PARENT as the child will
never be locked.  Only start_removing() and start_renamed() need that.
But it was simpler to use the same subclass for all cases.

lookup_open() could use I_MUTEX_PARENT but doesn't need to as the opened
inode will not be locked, whether it is created or not.

> 
> Other than that though, this looks good to me. Much less "fiddly".
> 
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> 

Thanks,
NeilBrown

Re: [PATCH] nfsd: switch nfsd4_open() to use vfs_lookup_open()

[Chuck Lever]

“nfsd: switch nfsd4_open() …”

IIUC the patch modifies nfsd4_create_file(), not nfsd4_open() ?

Several more issues below.

On Tue, Sep 1, 2026, at 10:30 PM, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
>
> The functionality that was recently gathered into do_lookup_open() is
> now available from the vfs as vfs_lookup_open().
>
> This implementation shares more code with syscall open paths and so uses
> some filesystem interfaces slightly more correctly.  It also takes the
> responsibility for locking out of nfsd so that planned changes can
> happen entirely in VFS code.
>
> One difference is that do_lookup_open() would use nfsd_check_obj_isreg()
> to get an error when the name exists but is not a regular file.
> vfs_lookup_open() uses slightly different error code, particularly
> returning -ENODEV when a device-special file is found.  So we need
> to map that error to -EFTYPE.
>
> Unfortunately vfs_lookup_open() doesn't allow O_LARGEFILE so we need
> to patch it to avoid a warning.
>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/namei.c         |  2 +-
>  fs/nfsd/nfs4proc.c | 66 ++++++++++------------------------------------
>  2 files changed, 15 insertions(+), 53 deletions(-)
>
> This patch is against nfsd-test.  I would like it to land there so it
> best good nfsd testing over the coming weeks.

Since it touches fs/namei.c, Christian will want to take this through
the VFS tree. To get the testing you requested, I’ll have to merge the
VFS tree into nfsd-next / nfsd-testing when it is ready for exposure.
That might be a while.


> Thanks,
> NeilBrown
>
>
> diff --git a/fs/namei.c b/fs/namei.c
> index d95249dd527c..85fd5e8221e2 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4632,7 +4632,7 @@ struct file *vfs_lookup_open(struct path *parent, 
> struct qstr *last,
>  	int error = 0;
> 
>  	WARN_ONCE(mode & ~S_IALLUGO, "mode must only have permission bits");
> -	WARN_ONCE(open_flag & ~(O_ACCMODE|O_CREAT|O_EXCL|O_TRUNC|__O_REGULAR),
> +	WARN_ONCE(open_flag & 
> ~(O_ACCMODE|O_CREAT|O_EXCL|O_TRUNC|__O_REGULAR|O_LARGEFILE),
>  		  "open_flag has unsupported flags");

Two subtle behavior changes here. Were these intentional?

A client sending mode 0040755 gets an inode created with S_IFSOCK|0755 and
trips this WARN_ONCE. The old code clamped the incoming mode with S_IALLUGO.

Looks like a GUARDED4 create over a non-regular name now returns NFS4ERR_EXIST.
The old code returned ISDIR or SYMLINK. Does this match NFSv3’s behavior? Should
the commit message provide a rational that this is intended, or is a type
check needed?


>  	mode |= S_IFREG;
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 33dc92d48ce0..ae70f7489d95 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -213,52 +213,6 @@ static inline bool nfsd4_create_is_exclusive(int 
> createmode)
>  		createmode == NFS4_CREATE_EXCLUSIVE4_1;
>  }
> 
> -static struct file *do_lookup_open(struct path *parent,
> -				   struct qstr *name,
> -				   unsigned int oflags,
> -				   umode_t mode)
> -{
> -	struct file *filp = NULL;
> -	struct path path;
> -	struct dentry *child;
> -	int want_write_err = 0;
> -
> -	want_write_err = mnt_want_write(parent->mnt);
> -
> -	child = start_creating(&nop_mnt_idmap, parent->dentry, name);
> -	if (IS_ERR(child)) {
> -		filp = ERR_CAST(child);
> -		goto out;
> -	}
> -	path.mnt = parent->mnt;
> -	path.dentry = child;
> -
> -	if (d_really_is_positive(child)) {
> -		/*
> -		 * open the file so that we consistently have a valid
> -		 * op_filp and consequently a valid ->f_path.dentry.
> -		 */
> -		int err = nfsd_check_obj_isreg(child);
> -
> -		if (err)
> -			filp = ERR_PTR(err);
> -		else
> -			filp = dentry_open(&path, oflags, current_cred());
> -	} else if (!(oflags & O_CREAT)) {
> -		filp = ERR_PTR(-ENOENT);
> -	} else if (want_write_err) {
> -		filp = ERR_PTR(want_write_err);
> -	} else {
> -		filp = dentry_create(&path, oflags, mode, current_cred());
> -		child = path.dentry;
> -	}
> -	end_creating(child);
> -out:
> -	if (!want_write_err)
> -		mnt_drop_write(parent->mnt);
> -	return filp;
> -}
> -
>  /*
>   * Implement NFSv4's unchecked, guarded, and exclusive create
>   * semantics for regular files. Open state for this new file is
> @@ -390,13 +344,21 @@ 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);
> +	open->op_filp = vfs_lookup_open(&parent,
> +					&QSTR_LEN(open->op_fname,
> +						  open->op_fnamelen),
> +					oflags,
> +					open->op_iattr.ia_mode);
>  	if (IS_ERR(open->op_filp)) {
> -		status = nfserrno(PTR_ERR(open->op_filp));
> +		int hosterr = PTR_ERR(open->op_filp);
> +
> +		/*
> +		 * NFS doesn't differentiate between device files and
> +		 * sock/fifo when reporting an error.
> +		 */
> +		if (hosterr == -ENODEV)
> +			hosterr = -EFTYPE;
> +		status = nfserrno(hosterr);
>  		open->op_filp = NULL;
>  		if (status == nfserr_noent && create_status)
>  			status = create_status;
>

Since NFSD threads now sleep on directory delegation recalls, if lookup_open()
passes a real delegated_inode, it waits in break_deleg_wait() and retries for
up to lease_break_time.

The old path called vfs_create(..., NULL), so the client got NFS4ERR_DELAY
instead. NFSD needs to continue to work this way, otherwise a client can
cause all the NFSD threads to block.


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