| Message ID | 20230911183027.11372-1-trond.myklebust@hammerspace.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfsd: Handle EOPENSTALE correctly in the filecache | expand |
On Mon, Sep 11, 2023 at 02:30:27PM -0400, trondmy@gmail.com wrote: > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > The nfsd_open code handles EOPENSTALE correctly, by retrying the call to > fh_verify() and __nfsd_open(). However the filecache just drops the > error on the floor, and immediately returns nfserr_stale to the caller. > > This patch ensures that we propagate the EOPENSTALE code back to > nfsd_file_do_acquire, and that we handle it correctly. > > Fixes: 65294c1f2c5e ("nfsd: add a new struct file caching facility to nfsd") > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> > --- > fs/nfsd/filecache.c | 27 +++++++++++++++++++-------- > fs/nfsd/vfs.c | 28 +++++++++++++--------------- > fs/nfsd/vfs.h | 2 +- > 3 files changed, 33 insertions(+), 24 deletions(-) > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > index ee9c923192e0..07bf219f9ae4 100644 > --- a/fs/nfsd/filecache.c > +++ b/fs/nfsd/filecache.c > @@ -989,22 +989,21 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > unsigned char need = may_flags & NFSD_FILE_MAY_MASK; > struct net *net = SVC_NET(rqstp); > struct nfsd_file *new, *nf; > - const struct cred *cred; > + bool stale_retry = true; > bool open_retry = true; > struct inode *inode; > __be32 status; > int ret; > > +retry: > status = fh_verify(rqstp, fhp, S_IFREG, > may_flags|NFSD_MAY_OWNER_OVERRIDE); > if (status != nfs_ok) > return status; > inode = d_inode(fhp->fh_dentry); > - cred = get_current_cred(); If memory serves, we have to hold a reference to the current cred through the acquire process. Otherwise, the cred can be released while nfsd_file_do_acquire() is still running, which results in an oops. What's the reason to replace get_current_cred() with current_cred()? > -retry: > rcu_read_lock(); > - nf = nfsd_file_lookup_locked(net, cred, inode, need, want_gc); > + nf = nfsd_file_lookup_locked(net, current_cred(), inode, need, want_gc); > rcu_read_unlock(); > > if (nf) { > @@ -1026,7 +1025,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > > rcu_read_lock(); > spin_lock(&inode->i_lock); > - nf = nfsd_file_lookup_locked(net, cred, inode, need, want_gc); > + nf = nfsd_file_lookup_locked(net, current_cred(), inode, need, want_gc); > if (unlikely(nf)) { > spin_unlock(&inode->i_lock); > rcu_read_unlock(); > @@ -1058,6 +1057,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > goto construction_err; > } > open_retry = false; > + fh_put(fhp); > goto retry; > } > this_cpu_inc(nfsd_file_cache_hits); > @@ -1074,7 +1074,6 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > nfsd_file_check_write_error(nf); > *pnf = nf; > } > - put_cred(cred); > trace_nfsd_file_acquire(rqstp, inode, may_flags, nf, status); > return status; > > @@ -1088,8 +1087,20 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > status = nfs_ok; > trace_nfsd_file_opened(nf, status); > } else { > - status = nfsd_open_verified(rqstp, fhp, may_flags, > - &nf->nf_file); > + ret = nfsd_open_verified(rqstp, fhp, may_flags, > + &nf->nf_file); > + if (ret == -EOPENSTALE && stale_retry) { > + stale_retry = false; > + nfsd_file_unhash(nf); > + clear_and_wake_up_bit(NFSD_FILE_PENDING, > + &nf->nf_flags); > + if (refcount_dec_and_test(&nf->nf_ref)) > + nfsd_file_free(nf); > + nf = NULL; > + fh_put(fhp); > + goto retry; > + } > + status = nfserrno(ret); > trace_nfsd_file_open(nf, status); > } > } else > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > index 2c9074ab2315..98fa4fd0556d 100644 > --- a/fs/nfsd/vfs.c > +++ b/fs/nfsd/vfs.c > @@ -823,7 +823,7 @@ int nfsd_open_break_lease(struct inode *inode, int access) > * and additional flags. > * N.B. After this call fhp needs an fh_put > */ > -static __be32 > +static int > __nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, > int may_flags, struct file **filp) > { > @@ -831,14 +831,12 @@ __nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, > struct inode *inode; > struct file *file; > int flags = O_RDONLY|O_LARGEFILE; > - __be32 err; > - int host_err = 0; > + int host_err = -EPERM; > > path.mnt = fhp->fh_export->ex_path.mnt; > path.dentry = fhp->fh_dentry; > inode = d_inode(path.dentry); > > - err = nfserr_perm; > if (IS_APPEND(inode) && (may_flags & NFSD_MAY_WRITE)) > goto out; > > @@ -847,7 +845,7 @@ __nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, > > host_err = nfsd_open_break_lease(inode, may_flags); > if (host_err) /* NOMEM or WOULDBLOCK */ > - goto out_nfserr; > + goto out; > > if (may_flags & NFSD_MAY_WRITE) { > if (may_flags & NFSD_MAY_READ) > @@ -859,13 +857,13 @@ __nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, > file = dentry_open(&path, flags, current_cred()); > if (IS_ERR(file)) { > host_err = PTR_ERR(file); > - goto out_nfserr; > + goto out; > } > > host_err = ima_file_check(file, may_flags); > if (host_err) { > fput(file); > - goto out_nfserr; > + goto out; > } > > if (may_flags & NFSD_MAY_64BIT_COOKIE) > @@ -874,10 +872,8 @@ __nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, > file->f_mode |= FMODE_32BITHASH; > > *filp = file; > -out_nfserr: > - err = nfserrno(host_err); > out: > - return err; > + return host_err; > } > > __be32 > @@ -885,6 +881,7 @@ nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, > int may_flags, struct file **filp) > { > __be32 err; > + int host_err; > bool retried = false; > > validate_process_creds(); > @@ -904,12 +901,13 @@ nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, > retry: > err = fh_verify(rqstp, fhp, type, may_flags); > if (!err) { > - err = __nfsd_open(rqstp, fhp, type, may_flags, filp); > - if (err == nfserr_stale && !retried) { > + host_err = __nfsd_open(rqstp, fhp, type, may_flags, filp); > + if (host_err == -EOPENSTALE && !retried) { > retried = true; > fh_put(fhp); > goto retry; > } > + err = nfserrno(host_err); > } > validate_process_creds(); > return err; > @@ -922,13 +920,13 @@ nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, > * @may_flags: internal permission flags > * @filp: OUT: open "struct file *" > * > - * Returns an nfsstat value in network byte order. > + * Returns a posix error. > */ > -__be32 > +int > nfsd_open_verified(struct svc_rqst *rqstp, struct svc_fh *fhp, int may_flags, > struct file **filp) > { > - __be32 err; > + int err; > > validate_process_creds(); > err = __nfsd_open(rqstp, fhp, S_IFREG, may_flags, filp); > diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h > index a6890ea7b765..e4b7207ef2e0 100644 > --- a/fs/nfsd/vfs.h > +++ b/fs/nfsd/vfs.h > @@ -104,7 +104,7 @@ __be32 nfsd_setxattr(struct svc_rqst *rqstp, struct svc_fh *fhp, > int nfsd_open_break_lease(struct inode *, int); > __be32 nfsd_open(struct svc_rqst *, struct svc_fh *, umode_t, > int, struct file **); > -__be32 nfsd_open_verified(struct svc_rqst *, struct svc_fh *, > +int nfsd_open_verified(struct svc_rqst *, struct svc_fh *, > int, struct file **); > __be32 nfsd_splice_read(struct svc_rqst *rqstp, struct svc_fh *fhp, > struct file *file, loff_t offset, > -- > 2.41.0 >
On Mon, 2023-09-11 at 18:26 -0400, Chuck Lever wrote: > On Mon, Sep 11, 2023 at 02:30:27PM -0400, trondmy@gmail.com wrote: > > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > > > The nfsd_open code handles EOPENSTALE correctly, by retrying the > > call to > > fh_verify() and __nfsd_open(). However the filecache just drops the > > error on the floor, and immediately returns nfserr_stale to the > > caller. > > > > This patch ensures that we propagate the EOPENSTALE code back to > > nfsd_file_do_acquire, and that we handle it correctly. > > > > Fixes: 65294c1f2c5e ("nfsd: add a new struct file caching facility > > to nfsd") > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> > > --- > > fs/nfsd/filecache.c | 27 +++++++++++++++++++-------- > > fs/nfsd/vfs.c | 28 +++++++++++++--------------- > > fs/nfsd/vfs.h | 2 +- > > 3 files changed, 33 insertions(+), 24 deletions(-) > > > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > > index ee9c923192e0..07bf219f9ae4 100644 > > --- a/fs/nfsd/filecache.c > > +++ b/fs/nfsd/filecache.c > > @@ -989,22 +989,21 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, > > struct svc_fh *fhp, > > unsigned char need = may_flags & NFSD_FILE_MAY_MASK; > > struct net *net = SVC_NET(rqstp); > > struct nfsd_file *new, *nf; > > - const struct cred *cred; > > + bool stale_retry = true; > > bool open_retry = true; > > struct inode *inode; > > __be32 status; > > int ret; > > > > +retry: > > status = fh_verify(rqstp, fhp, S_IFREG, > > may_flags|NFSD_MAY_OWNER_OVERRIDE); > > if (status != nfs_ok) > > return status; > > inode = d_inode(fhp->fh_dentry); > > - cred = get_current_cred(); > > If memory serves, we have to hold a reference to the current cred > through the acquire process. Otherwise, the cred can be released > while nfsd_file_do_acquire() is still running, which results in an > oops. > > What's the reason to replace get_current_cred() with current_cred()? In both cases, you're holding the rcu_read_lock() across the entire call, so it is quite safe to use current_cred(). > > > > -retry: > > rcu_read_lock(); > > - nf = nfsd_file_lookup_locked(net, cred, inode, need, > > want_gc); > > + nf = nfsd_file_lookup_locked(net, current_cred(), inode, > > need, want_gc); > > rcu_read_unlock(); > > > > if (nf) { > > @@ -1026,7 +1025,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, > > struct svc_fh *fhp, > > > > rcu_read_lock(); > > spin_lock(&inode->i_lock); > > - nf = nfsd_file_lookup_locked(net, cred, inode, need, > > want_gc); > > + nf = nfsd_file_lookup_locked(net, current_cred(), inode, > > need, want_gc); > > if (unlikely(nf)) { > > spin_unlock(&inode->i_lock); > > rcu_read_unlock(); > > @@ -1058,6 +1057,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, > > struct svc_fh *fhp, > > goto construction_err; > > } > > open_retry = false; > > + fh_put(fhp); > > goto retry; > > } > > this_cpu_inc(nfsd_file_cache_hits); > > @@ -1074,7 +1074,6 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, > > struct svc_fh *fhp, > > nfsd_file_check_write_error(nf); > > *pnf = nf; > > } > > - put_cred(cred); > > trace_nfsd_file_acquire(rqstp, inode, may_flags, nf, > > status); > > return status; > > > > @@ -1088,8 +1087,20 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, > > struct svc_fh *fhp, > > status = nfs_ok; > > trace_nfsd_file_opened(nf, status); > > } else { > > - status = nfsd_open_verified(rqstp, fhp, > > may_flags, > > - &nf->nf_file); > > + ret = nfsd_open_verified(rqstp, fhp, > > may_flags, > > + &nf->nf_file); > > + if (ret == -EOPENSTALE && stale_retry) { > > + stale_retry = false; > > + nfsd_file_unhash(nf); > > + clear_and_wake_up_bit(NFSD_FILE_PEN > > DING, > > + &nf- > > >nf_flags); > > + if (refcount_dec_and_test(&nf- > > >nf_ref)) > > + nfsd_file_free(nf); > > + nf = NULL; > > + fh_put(fhp); > > + goto retry; > > + } > > + status = nfserrno(ret); > > trace_nfsd_file_open(nf, status); > > } > > } else > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > > index 2c9074ab2315..98fa4fd0556d 100644 > > --- a/fs/nfsd/vfs.c > > +++ b/fs/nfsd/vfs.c > > @@ -823,7 +823,7 @@ int nfsd_open_break_lease(struct inode *inode, > > int access) > > * and additional flags. > > * N.B. After this call fhp needs an fh_put > > */ > > -static __be32 > > +static int > > __nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t > > type, > > int may_flags, struct file **filp) > > { > > @@ -831,14 +831,12 @@ __nfsd_open(struct svc_rqst *rqstp, struct > > svc_fh *fhp, umode_t type, > > struct inode *inode; > > struct file *file; > > int flags = O_RDONLY|O_LARGEFILE; > > - __be32 err; > > - int host_err = 0; > > + int host_err = -EPERM; > > > > path.mnt = fhp->fh_export->ex_path.mnt; > > path.dentry = fhp->fh_dentry; > > inode = d_inode(path.dentry); > > > > - err = nfserr_perm; > > if (IS_APPEND(inode) && (may_flags & NFSD_MAY_WRITE)) > > goto out; > > > > @@ -847,7 +845,7 @@ __nfsd_open(struct svc_rqst *rqstp, struct > > svc_fh *fhp, umode_t type, > > > > host_err = nfsd_open_break_lease(inode, may_flags); > > if (host_err) /* NOMEM or WOULDBLOCK */ > > - goto out_nfserr; > > + goto out; > > > > if (may_flags & NFSD_MAY_WRITE) { > > if (may_flags & NFSD_MAY_READ) > > @@ -859,13 +857,13 @@ __nfsd_open(struct svc_rqst *rqstp, struct > > svc_fh *fhp, umode_t type, > > file = dentry_open(&path, flags, current_cred()); > > if (IS_ERR(file)) { > > host_err = PTR_ERR(file); > > - goto out_nfserr; > > + goto out; > > } > > > > host_err = ima_file_check(file, may_flags); > > if (host_err) { > > fput(file); > > - goto out_nfserr; > > + goto out; > > } > > > > if (may_flags & NFSD_MAY_64BIT_COOKIE) > > @@ -874,10 +872,8 @@ __nfsd_open(struct svc_rqst *rqstp, struct > > svc_fh *fhp, umode_t type, > > file->f_mode |= FMODE_32BITHASH; > > > > *filp = file; > > -out_nfserr: > > - err = nfserrno(host_err); > > out: > > - return err; > > + return host_err; > > } > > > > __be32 > > @@ -885,6 +881,7 @@ nfsd_open(struct svc_rqst *rqstp, struct svc_fh > > *fhp, umode_t type, > > int may_flags, struct file **filp) > > { > > __be32 err; > > + int host_err; > > bool retried = false; > > > > validate_process_creds(); > > @@ -904,12 +901,13 @@ nfsd_open(struct svc_rqst *rqstp, struct > > svc_fh *fhp, umode_t type, > > retry: > > err = fh_verify(rqstp, fhp, type, may_flags); > > if (!err) { > > - err = __nfsd_open(rqstp, fhp, type, may_flags, > > filp); > > - if (err == nfserr_stale && !retried) { > > + host_err = __nfsd_open(rqstp, fhp, type, may_flags, > > filp); > > + if (host_err == -EOPENSTALE && !retried) { > > retried = true; > > fh_put(fhp); > > goto retry; > > } > > + err = nfserrno(host_err); > > } > > validate_process_creds(); > > return err; > > @@ -922,13 +920,13 @@ nfsd_open(struct svc_rqst *rqstp, struct > > svc_fh *fhp, umode_t type, > > * @may_flags: internal permission flags > > * @filp: OUT: open "struct file *" > > * > > - * Returns an nfsstat value in network byte order. > > + * Returns a posix error. > > */ > > -__be32 > > +int > > nfsd_open_verified(struct svc_rqst *rqstp, struct svc_fh *fhp, int > > may_flags, > > struct file **filp) > > { > > - __be32 err; > > + int err; > > > > validate_process_creds(); > > err = __nfsd_open(rqstp, fhp, S_IFREG, may_flags, filp); > > diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h > > index a6890ea7b765..e4b7207ef2e0 100644 > > --- a/fs/nfsd/vfs.h > > +++ b/fs/nfsd/vfs.h > > @@ -104,7 +104,7 @@ __be32 nfsd_setxattr(struct > > svc_rqst *rqstp, struct svc_fh *fhp, > > int nfsd_open_break_lease(struct inode *, int); > > __be32 nfsd_open(struct svc_rqst *, struct svc_fh *, > > umode_t, > > int, struct file **); > > -__be32 nfsd_open_verified(struct svc_rqst *, struct svc_fh > > *, > > +int nfsd_open_verified(struct svc_rqst *, struct svc_fh > > *, > > int, struct file **); > > __be32 nfsd_splice_read(struct svc_rqst *rqstp, struct > > svc_fh *fhp, > > struct file *file, loff_t offset, > > -- > > 2.41.0 > > >
On Mon, 2023-09-11 at 14:30 -0400, trondmy@gmail.com wrote: > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > The nfsd_open code handles EOPENSTALE correctly, by retrying the call to > fh_verify() and __nfsd_open(). However the filecache just drops the > error on the floor, and immediately returns nfserr_stale to the caller. > > This patch ensures that we propagate the EOPENSTALE code back to > nfsd_file_do_acquire, and that we handle it correctly. > > Fixes: 65294c1f2c5e ("nfsd: add a new struct file caching facility to nfsd") > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> > --- > fs/nfsd/filecache.c | 27 +++++++++++++++++++-------- > fs/nfsd/vfs.c | 28 +++++++++++++--------------- > fs/nfsd/vfs.h | 2 +- > 3 files changed, 33 insertions(+), 24 deletions(-) > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > index ee9c923192e0..07bf219f9ae4 100644 > --- a/fs/nfsd/filecache.c > +++ b/fs/nfsd/filecache.c > @@ -989,22 +989,21 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > unsigned char need = may_flags & NFSD_FILE_MAY_MASK; > struct net *net = SVC_NET(rqstp); > struct nfsd_file *new, *nf; > - const struct cred *cred; > + bool stale_retry = true; > bool open_retry = true; > struct inode *inode; > __be32 status; > int ret; > > +retry: > status = fh_verify(rqstp, fhp, S_IFREG, > may_flags|NFSD_MAY_OWNER_OVERRIDE); > if (status != nfs_ok) > return status; > inode = d_inode(fhp->fh_dentry); > - cred = get_current_cred(); > > -retry: > rcu_read_lock(); > - nf = nfsd_file_lookup_locked(net, cred, inode, need, want_gc); > + nf = nfsd_file_lookup_locked(net, current_cred(), inode, need, want_gc); > rcu_read_unlock(); > > if (nf) { > @@ -1026,7 +1025,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > > rcu_read_lock(); > spin_lock(&inode->i_lock); > - nf = nfsd_file_lookup_locked(net, cred, inode, need, want_gc); > + nf = nfsd_file_lookup_locked(net, current_cred(), inode, need, want_gc); > if (unlikely(nf)) { > spin_unlock(&inode->i_lock); > rcu_read_unlock(); > @@ -1058,6 +1057,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > goto construction_err; > } > open_retry = false; > + fh_put(fhp); > goto retry; > } > this_cpu_inc(nfsd_file_cache_hits); > @@ -1074,7 +1074,6 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > nfsd_file_check_write_error(nf); > *pnf = nf; > } > - put_cred(cred); > trace_nfsd_file_acquire(rqstp, inode, may_flags, nf, status); > return status; > > @@ -1088,8 +1087,20 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > status = nfs_ok; > trace_nfsd_file_opened(nf, status); > } else { > - status = nfsd_open_verified(rqstp, fhp, may_flags, > - &nf->nf_file); > + ret = nfsd_open_verified(rqstp, fhp, may_flags, > + &nf->nf_file); > + if (ret == -EOPENSTALE && stale_retry) { > + stale_retry = false; > + nfsd_file_unhash(nf); > + clear_and_wake_up_bit(NFSD_FILE_PENDING, > + &nf->nf_flags); > + if (refcount_dec_and_test(&nf->nf_ref)) > + nfsd_file_free(nf); > + nf = NULL; > + fh_put(fhp); > + goto retry; > + } > + status = nfserrno(ret); > trace_nfsd_file_open(nf, status); > } > } else > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > index 2c9074ab2315..98fa4fd0556d 100644 > --- a/fs/nfsd/vfs.c > +++ b/fs/nfsd/vfs.c > @@ -823,7 +823,7 @@ int nfsd_open_break_lease(struct inode *inode, int access) > * and additional flags. > * N.B. After this call fhp needs an fh_put > */ > -static __be32 > +static int > __nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, > int may_flags, struct file **filp) > { > @@ -831,14 +831,12 @@ __nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, > struct inode *inode; > struct file *file; > int flags = O_RDONLY|O_LARGEFILE; > - __be32 err; > - int host_err = 0; > + int host_err = -EPERM; > > path.mnt = fhp->fh_export->ex_path.mnt; > path.dentry = fhp->fh_dentry; > inode = d_inode(path.dentry); > > - err = nfserr_perm; > if (IS_APPEND(inode) && (may_flags & NFSD_MAY_WRITE)) > goto out; > > @@ -847,7 +845,7 @@ __nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, > > host_err = nfsd_open_break_lease(inode, may_flags); > if (host_err) /* NOMEM or WOULDBLOCK */ > - goto out_nfserr; > + goto out; > > if (may_flags & NFSD_MAY_WRITE) { > if (may_flags & NFSD_MAY_READ) > @@ -859,13 +857,13 @@ __nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, > file = dentry_open(&path, flags, current_cred()); > if (IS_ERR(file)) { > host_err = PTR_ERR(file); > - goto out_nfserr; > + goto out; > } > > host_err = ima_file_check(file, may_flags); > if (host_err) { > fput(file); > - goto out_nfserr; > + goto out; > } > > if (may_flags & NFSD_MAY_64BIT_COOKIE) > @@ -874,10 +872,8 @@ __nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, > file->f_mode |= FMODE_32BITHASH; > > *filp = file; > -out_nfserr: > - err = nfserrno(host_err); > out: > - return err; > + return host_err; > } > > __be32 > @@ -885,6 +881,7 @@ nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, > int may_flags, struct file **filp) > { > __be32 err; > + int host_err; > bool retried = false; > > validate_process_creds(); > @@ -904,12 +901,13 @@ nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, > retry: > err = fh_verify(rqstp, fhp, type, may_flags); > if (!err) { > - err = __nfsd_open(rqstp, fhp, type, may_flags, filp); > - if (err == nfserr_stale && !retried) { > + host_err = __nfsd_open(rqstp, fhp, type, may_flags, filp); > + if (host_err == -EOPENSTALE && !retried) { > retried = true; > fh_put(fhp); > goto retry; > } > + err = nfserrno(host_err); > } > validate_process_creds(); > return err; > @@ -922,13 +920,13 @@ nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, > * @may_flags: internal permission flags > * @filp: OUT: open "struct file *" > * > - * Returns an nfsstat value in network byte order. > + * Returns a posix error. > */ > -__be32 > +int > nfsd_open_verified(struct svc_rqst *rqstp, struct svc_fh *fhp, int may_flags, > struct file **filp) > { > - __be32 err; > + int err; > > validate_process_creds(); > err = __nfsd_open(rqstp, fhp, S_IFREG, may_flags, filp); > diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h > index a6890ea7b765..e4b7207ef2e0 100644 > --- a/fs/nfsd/vfs.h > +++ b/fs/nfsd/vfs.h > @@ -104,7 +104,7 @@ __be32 nfsd_setxattr(struct svc_rqst *rqstp, struct svc_fh *fhp, > int nfsd_open_break_lease(struct inode *, int); > __be32 nfsd_open(struct svc_rqst *, struct svc_fh *, umode_t, > int, struct file **); > -__be32 nfsd_open_verified(struct svc_rqst *, struct svc_fh *, > +int nfsd_open_verified(struct svc_rqst *, struct svc_fh *, > int, struct file **); > __be32 nfsd_splice_read(struct svc_rqst *rqstp, struct svc_fh *fhp, > struct file *file, loff_t offset, Looks reasonable. Reviewed-by: Jeff Layton <jlayton@kernel.org>
On Tue, Sep 12, 2023 at 06:44:40AM -0400, Jeff Layton wrote: > On Mon, 2023-09-11 at 14:30 -0400, trondmy@gmail.com wrote: > > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > > > The nfsd_open code handles EOPENSTALE correctly, by retrying the call to > > fh_verify() and __nfsd_open(). However the filecache just drops the > > error on the floor, and immediately returns nfserr_stale to the caller. > > > > This patch ensures that we propagate the EOPENSTALE code back to > > nfsd_file_do_acquire, and that we handle it correctly. > > > > Fixes: 65294c1f2c5e ("nfsd: add a new struct file caching facility to nfsd") > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> > > --- > > fs/nfsd/filecache.c | 27 +++++++++++++++++++-------- > > fs/nfsd/vfs.c | 28 +++++++++++++--------------- > > fs/nfsd/vfs.h | 2 +- > > 3 files changed, 33 insertions(+), 24 deletions(-) > > > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > > index ee9c923192e0..07bf219f9ae4 100644 > > --- a/fs/nfsd/filecache.c > > +++ b/fs/nfsd/filecache.c > > @@ -989,22 +989,21 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > > unsigned char need = may_flags & NFSD_FILE_MAY_MASK; > > struct net *net = SVC_NET(rqstp); > > struct nfsd_file *new, *nf; > > - const struct cred *cred; > > + bool stale_retry = true; > > bool open_retry = true; > > struct inode *inode; > > __be32 status; > > int ret; > > > > +retry: > > status = fh_verify(rqstp, fhp, S_IFREG, > > may_flags|NFSD_MAY_OWNER_OVERRIDE); > > if (status != nfs_ok) > > return status; > > inode = d_inode(fhp->fh_dentry); > > - cred = get_current_cred(); > > > > -retry: > > rcu_read_lock(); > > - nf = nfsd_file_lookup_locked(net, cred, inode, need, want_gc); > > + nf = nfsd_file_lookup_locked(net, current_cred(), inode, need, want_gc); > > rcu_read_unlock(); > > > > if (nf) { > > @@ -1026,7 +1025,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > > > > rcu_read_lock(); > > spin_lock(&inode->i_lock); > > - nf = nfsd_file_lookup_locked(net, cred, inode, need, want_gc); > > + nf = nfsd_file_lookup_locked(net, current_cred(), inode, need, want_gc); > > if (unlikely(nf)) { > > spin_unlock(&inode->i_lock); > > rcu_read_unlock(); > > @@ -1058,6 +1057,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > > goto construction_err; > > } > > open_retry = false; > > + fh_put(fhp); > > goto retry; > > } > > this_cpu_inc(nfsd_file_cache_hits); > > @@ -1074,7 +1074,6 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > > nfsd_file_check_write_error(nf); > > *pnf = nf; > > } > > - put_cred(cred); > > trace_nfsd_file_acquire(rqstp, inode, may_flags, nf, status); > > return status; > > > > @@ -1088,8 +1087,20 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > > status = nfs_ok; > > trace_nfsd_file_opened(nf, status); > > } else { > > - status = nfsd_open_verified(rqstp, fhp, may_flags, > > - &nf->nf_file); > > + ret = nfsd_open_verified(rqstp, fhp, may_flags, > > + &nf->nf_file); > > + if (ret == -EOPENSTALE && stale_retry) { > > + stale_retry = false; > > + nfsd_file_unhash(nf); > > + clear_and_wake_up_bit(NFSD_FILE_PENDING, > > + &nf->nf_flags); > > + if (refcount_dec_and_test(&nf->nf_ref)) > > + nfsd_file_free(nf); > > + nf = NULL; > > + fh_put(fhp); > > + goto retry; > > + } > > + status = nfserrno(ret); > > trace_nfsd_file_open(nf, status); > > } > > } else > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > > index 2c9074ab2315..98fa4fd0556d 100644 > > --- a/fs/nfsd/vfs.c > > +++ b/fs/nfsd/vfs.c > > @@ -823,7 +823,7 @@ int nfsd_open_break_lease(struct inode *inode, int access) > > * and additional flags. > > * N.B. After this call fhp needs an fh_put > > */ > > -static __be32 > > +static int > > __nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, > > int may_flags, struct file **filp) > > { > > @@ -831,14 +831,12 @@ __nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, > > struct inode *inode; > > struct file *file; > > int flags = O_RDONLY|O_LARGEFILE; > > - __be32 err; > > - int host_err = 0; > > + int host_err = -EPERM; > > > > path.mnt = fhp->fh_export->ex_path.mnt; > > path.dentry = fhp->fh_dentry; > > inode = d_inode(path.dentry); > > > > - err = nfserr_perm; > > if (IS_APPEND(inode) && (may_flags & NFSD_MAY_WRITE)) > > goto out; > > > > @@ -847,7 +845,7 @@ __nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, > > > > host_err = nfsd_open_break_lease(inode, may_flags); > > if (host_err) /* NOMEM or WOULDBLOCK */ > > - goto out_nfserr; > > + goto out; > > > > if (may_flags & NFSD_MAY_WRITE) { > > if (may_flags & NFSD_MAY_READ) > > @@ -859,13 +857,13 @@ __nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, > > file = dentry_open(&path, flags, current_cred()); > > if (IS_ERR(file)) { > > host_err = PTR_ERR(file); > > - goto out_nfserr; > > + goto out; > > } > > > > host_err = ima_file_check(file, may_flags); > > if (host_err) { > > fput(file); > > - goto out_nfserr; > > + goto out; > > } > > > > if (may_flags & NFSD_MAY_64BIT_COOKIE) > > @@ -874,10 +872,8 @@ __nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, > > file->f_mode |= FMODE_32BITHASH; > > > > *filp = file; > > -out_nfserr: > > - err = nfserrno(host_err); > > out: > > - return err; > > + return host_err; > > } > > > > __be32 > > @@ -885,6 +881,7 @@ nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, > > int may_flags, struct file **filp) > > { > > __be32 err; > > + int host_err; > > bool retried = false; > > > > validate_process_creds(); > > @@ -904,12 +901,13 @@ nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, > > retry: > > err = fh_verify(rqstp, fhp, type, may_flags); > > if (!err) { > > - err = __nfsd_open(rqstp, fhp, type, may_flags, filp); > > - if (err == nfserr_stale && !retried) { > > + host_err = __nfsd_open(rqstp, fhp, type, may_flags, filp); > > + if (host_err == -EOPENSTALE && !retried) { > > retried = true; > > fh_put(fhp); > > goto retry; > > } > > + err = nfserrno(host_err); > > } > > validate_process_creds(); > > return err; > > @@ -922,13 +920,13 @@ nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, > > * @may_flags: internal permission flags > > * @filp: OUT: open "struct file *" > > * > > - * Returns an nfsstat value in network byte order. > > + * Returns a posix error. > > */ > > -__be32 > > +int > > nfsd_open_verified(struct svc_rqst *rqstp, struct svc_fh *fhp, int may_flags, > > struct file **filp) > > { > > - __be32 err; > > + int err; > > > > validate_process_creds(); > > err = __nfsd_open(rqstp, fhp, S_IFREG, may_flags, filp); > > diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h > > index a6890ea7b765..e4b7207ef2e0 100644 > > --- a/fs/nfsd/vfs.h > > +++ b/fs/nfsd/vfs.h > > @@ -104,7 +104,7 @@ __be32 nfsd_setxattr(struct svc_rqst *rqstp, struct svc_fh *fhp, > > int nfsd_open_break_lease(struct inode *, int); > > __be32 nfsd_open(struct svc_rqst *, struct svc_fh *, umode_t, > > int, struct file **); > > -__be32 nfsd_open_verified(struct svc_rqst *, struct svc_fh *, > > +int nfsd_open_verified(struct svc_rqst *, struct svc_fh *, > > int, struct file **); > > __be32 nfsd_splice_read(struct svc_rqst *rqstp, struct svc_fh *fhp, > > struct file *file, loff_t offset, > > Looks reasonable. > > Reviewed-by: Jeff Layton <jlayton@kernel.org> Thanks, Jeff. Applied to nfsd-next.
diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c index ee9c923192e0..07bf219f9ae4 100644 --- a/fs/nfsd/filecache.c +++ b/fs/nfsd/filecache.c @@ -989,22 +989,21 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, unsigned char need = may_flags & NFSD_FILE_MAY_MASK; struct net *net = SVC_NET(rqstp); struct nfsd_file *new, *nf; - const struct cred *cred; + bool stale_retry = true; bool open_retry = true; struct inode *inode; __be32 status; int ret; +retry: status = fh_verify(rqstp, fhp, S_IFREG, may_flags|NFSD_MAY_OWNER_OVERRIDE); if (status != nfs_ok) return status; inode = d_inode(fhp->fh_dentry); - cred = get_current_cred(); -retry: rcu_read_lock(); - nf = nfsd_file_lookup_locked(net, cred, inode, need, want_gc); + nf = nfsd_file_lookup_locked(net, current_cred(), inode, need, want_gc); rcu_read_unlock(); if (nf) { @@ -1026,7 +1025,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, rcu_read_lock(); spin_lock(&inode->i_lock); - nf = nfsd_file_lookup_locked(net, cred, inode, need, want_gc); + nf = nfsd_file_lookup_locked(net, current_cred(), inode, need, want_gc); if (unlikely(nf)) { spin_unlock(&inode->i_lock); rcu_read_unlock(); @@ -1058,6 +1057,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, goto construction_err; } open_retry = false; + fh_put(fhp); goto retry; } this_cpu_inc(nfsd_file_cache_hits); @@ -1074,7 +1074,6 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, nfsd_file_check_write_error(nf); *pnf = nf; } - put_cred(cred); trace_nfsd_file_acquire(rqstp, inode, may_flags, nf, status); return status; @@ -1088,8 +1087,20 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, status = nfs_ok; trace_nfsd_file_opened(nf, status); } else { - status = nfsd_open_verified(rqstp, fhp, may_flags, - &nf->nf_file); + ret = nfsd_open_verified(rqstp, fhp, may_flags, + &nf->nf_file); + if (ret == -EOPENSTALE && stale_retry) { + stale_retry = false; + nfsd_file_unhash(nf); + clear_and_wake_up_bit(NFSD_FILE_PENDING, + &nf->nf_flags); + if (refcount_dec_and_test(&nf->nf_ref)) + nfsd_file_free(nf); + nf = NULL; + fh_put(fhp); + goto retry; + } + status = nfserrno(ret); trace_nfsd_file_open(nf, status); } } else diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c index 2c9074ab2315..98fa4fd0556d 100644 --- a/fs/nfsd/vfs.c +++ b/fs/nfsd/vfs.c @@ -823,7 +823,7 @@ int nfsd_open_break_lease(struct inode *inode, int access) * and additional flags. * N.B. After this call fhp needs an fh_put */ -static __be32 +static int __nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, int may_flags, struct file **filp) { @@ -831,14 +831,12 @@ __nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, struct inode *inode; struct file *file; int flags = O_RDONLY|O_LARGEFILE; - __be32 err; - int host_err = 0; + int host_err = -EPERM; path.mnt = fhp->fh_export->ex_path.mnt; path.dentry = fhp->fh_dentry; inode = d_inode(path.dentry); - err = nfserr_perm; if (IS_APPEND(inode) && (may_flags & NFSD_MAY_WRITE)) goto out; @@ -847,7 +845,7 @@ __nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, host_err = nfsd_open_break_lease(inode, may_flags); if (host_err) /* NOMEM or WOULDBLOCK */ - goto out_nfserr; + goto out; if (may_flags & NFSD_MAY_WRITE) { if (may_flags & NFSD_MAY_READ) @@ -859,13 +857,13 @@ __nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, file = dentry_open(&path, flags, current_cred()); if (IS_ERR(file)) { host_err = PTR_ERR(file); - goto out_nfserr; + goto out; } host_err = ima_file_check(file, may_flags); if (host_err) { fput(file); - goto out_nfserr; + goto out; } if (may_flags & NFSD_MAY_64BIT_COOKIE) @@ -874,10 +872,8 @@ __nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, file->f_mode |= FMODE_32BITHASH; *filp = file; -out_nfserr: - err = nfserrno(host_err); out: - return err; + return host_err; } __be32 @@ -885,6 +881,7 @@ nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, int may_flags, struct file **filp) { __be32 err; + int host_err; bool retried = false; validate_process_creds(); @@ -904,12 +901,13 @@ nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, retry: err = fh_verify(rqstp, fhp, type, may_flags); if (!err) { - err = __nfsd_open(rqstp, fhp, type, may_flags, filp); - if (err == nfserr_stale && !retried) { + host_err = __nfsd_open(rqstp, fhp, type, may_flags, filp); + if (host_err == -EOPENSTALE && !retried) { retried = true; fh_put(fhp); goto retry; } + err = nfserrno(host_err); } validate_process_creds(); return err; @@ -922,13 +920,13 @@ nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, * @may_flags: internal permission flags * @filp: OUT: open "struct file *" * - * Returns an nfsstat value in network byte order. + * Returns a posix error. */ -__be32 +int nfsd_open_verified(struct svc_rqst *rqstp, struct svc_fh *fhp, int may_flags, struct file **filp) { - __be32 err; + int err; validate_process_creds(); err = __nfsd_open(rqstp, fhp, S_IFREG, may_flags, filp); diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h index a6890ea7b765..e4b7207ef2e0 100644 --- a/fs/nfsd/vfs.h +++ b/fs/nfsd/vfs.h @@ -104,7 +104,7 @@ __be32 nfsd_setxattr(struct svc_rqst *rqstp, struct svc_fh *fhp, int nfsd_open_break_lease(struct inode *, int); __be32 nfsd_open(struct svc_rqst *, struct svc_fh *, umode_t, int, struct file **); -__be32 nfsd_open_verified(struct svc_rqst *, struct svc_fh *, +int nfsd_open_verified(struct svc_rqst *, struct svc_fh *, int, struct file **); __be32 nfsd_splice_read(struct svc_rqst *rqstp, struct svc_fh *fhp, struct file *file, loff_t offset,