* Re: [PATCH] nfsd: switch nfsd4_open() to use vfs_lookup_open()
2026-09-02 2:30 [PATCH] nfsd: switch nfsd4_open() to use vfs_lookup_open() NeilBrown
@ 2026-09-02 12:40 ` Jeff Layton
2026-09-02 22:38 ` NeilBrown
2026-09-03 3:04 ` Chuck Lever
1 sibling, 1 reply; 4+ messages in thread
From: Jeff Layton @ 2026-09-02 12:40 UTC (permalink / raw)
To: NeilBrown, Chuck Lever; +Cc: Christian Brauner, linux-nfs
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>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] nfsd: switch nfsd4_open() to use vfs_lookup_open()
2026-09-02 2:30 [PATCH] nfsd: switch nfsd4_open() to use vfs_lookup_open() NeilBrown
2026-09-02 12:40 ` Jeff Layton
@ 2026-09-03 3:04 ` Chuck Lever
1 sibling, 0 replies; 4+ messages in thread
From: Chuck Lever @ 2026-09-03 3:04 UTC (permalink / raw)
To: NeilBrown, Chuck Lever, Jeff Layton; +Cc: Christian Brauner, linux-nfs
“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)
^ permalink raw reply [flat|nested] 4+ messages in thread