From: "J. Bruce Fields" <[email protected]>
If nfsd already has an open file that it plans to use for IO from
another, it may not need to do another vfs open, but it still may need
to break any delegations in case the existing opens are for another
client.
Symptoms are that we may incorrectly fail to break a delegation on a
write open from a different client, when the delegation-holding client
already has a write open.
Fixes: 28df3d1539de ("nfsd: clients don't need to break their own delegations")
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4state.c | 24 +++++++++++++++++++-----
1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 97447a64bad0..886e50ed07c2 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4869,6 +4869,11 @@ static __be32 nfs4_get_vfs_file(struct svc_rqst *rqstp, struct nfs4_file *fp,
if (nf)
nfsd_file_put(nf);
+ status = nfserrno(nfsd_open_break_lease(cur_fh->fh_dentry->d_inode,
+ access));
+ if (status)
+ goto out_put_access;
+
status = nfsd4_truncate(rqstp, cur_fh, open);
if (status)
goto out_put_access;
@@ -6849,11 +6854,20 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
static __be32 nfsd_test_lock(struct svc_rqst *rqstp, struct svc_fh *fhp, struct file_lock *lock)
{
struct nfsd_file *nf;
- __be32 err = nfsd_file_acquire(rqstp, fhp, NFSD_MAY_READ, &nf);
- if (!err) {
- err = nfserrno(vfs_test_lock(nf->nf_file, lock));
- nfsd_file_put(nf);
- }
+ __be32 err;
+
+ err = nfsd_file_acquire(rqstp, fhp, NFSD_MAY_READ, &nf);
+ if (err)
+ return err;
+ fh_lock(fhp); /* to block new leases till after test_lock: */
+ err = nfserrno(nfsd_open_break_lease(fhp->fh_dentry->d_inode,
+ NFSD_MAY_READ));
+ if (err)
+ goto out;
+ err = nfserrno(vfs_test_lock(nf->nf_file, lock));
+out:
+ fh_unlock(fhp);
+ nfsd_file_put(nf);
return err;
}
--
2.30.2
From: "J. Bruce Fields" <[email protected]>
No change in behavior, I'm just moving some code around to avoid forward
references in a following patch.
(To do someday: figure out how to split up nfs4state.c. It's big and
disorganized.)
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4state.c | 234 ++++++++++++++++++++++----------------------
1 file changed, 117 insertions(+), 117 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index c1ba60db671a..dfe3e39c891c 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -353,6 +353,123 @@ static const struct nfsd4_callback_ops nfsd4_cb_notify_lock_ops = {
.release = nfsd4_cb_notify_lock_release,
};
+/*
+ * We store the NONE, READ, WRITE, and BOTH bits separately in the
+ * st_{access,deny}_bmap field of the stateid, in order to track not
+ * only what share bits are currently in force, but also what
+ * combinations of share bits previous opens have used. This allows us
+ * to enforce the recommendation of rfc 3530 14.2.19 that the server
+ * return an error if the client attempt to downgrade to a combination
+ * of share bits not explicable by closing some of its previous opens.
+ *
+ * XXX: This enforcement is actually incomplete, since we don't keep
+ * track of access/deny bit combinations; so, e.g., we allow:
+ *
+ * OPEN allow read, deny write
+ * OPEN allow both, deny none
+ * DOWNGRADE allow read, deny none
+ *
+ * which we should reject.
+ */
+static unsigned int
+bmap_to_share_mode(unsigned long bmap) {
+ int i;
+ unsigned int access = 0;
+
+ for (i = 1; i < 4; i++) {
+ if (test_bit(i, &bmap))
+ access |= i;
+ }
+ return access;
+}
+
+/* set share access for a given stateid */
+static inline void
+set_access(u32 access, struct nfs4_ol_stateid *stp)
+{
+ unsigned char mask = 1 << access;
+
+ WARN_ON_ONCE(access > NFS4_SHARE_ACCESS_BOTH);
+ stp->st_access_bmap |= mask;
+}
+
+/* clear share access for a given stateid */
+static inline void
+clear_access(u32 access, struct nfs4_ol_stateid *stp)
+{
+ unsigned char mask = 1 << access;
+
+ WARN_ON_ONCE(access > NFS4_SHARE_ACCESS_BOTH);
+ stp->st_access_bmap &= ~mask;
+}
+
+/* test whether a given stateid has access */
+static inline bool
+test_access(u32 access, struct nfs4_ol_stateid *stp)
+{
+ unsigned char mask = 1 << access;
+
+ return (bool)(stp->st_access_bmap & mask);
+}
+
+/* set share deny for a given stateid */
+static inline void
+set_deny(u32 deny, struct nfs4_ol_stateid *stp)
+{
+ unsigned char mask = 1 << deny;
+
+ WARN_ON_ONCE(deny > NFS4_SHARE_DENY_BOTH);
+ stp->st_deny_bmap |= mask;
+}
+
+/* clear share deny for a given stateid */
+static inline void
+clear_deny(u32 deny, struct nfs4_ol_stateid *stp)
+{
+ unsigned char mask = 1 << deny;
+
+ WARN_ON_ONCE(deny > NFS4_SHARE_DENY_BOTH);
+ stp->st_deny_bmap &= ~mask;
+}
+
+/* test whether a given stateid is denying specific access */
+static inline bool
+test_deny(u32 deny, struct nfs4_ol_stateid *stp)
+{
+ unsigned char mask = 1 << deny;
+
+ return (bool)(stp->st_deny_bmap & mask);
+}
+
+static int nfs4_access_to_omode(u32 access)
+{
+ switch (access & NFS4_SHARE_ACCESS_BOTH) {
+ case NFS4_SHARE_ACCESS_READ:
+ return O_RDONLY;
+ case NFS4_SHARE_ACCESS_WRITE:
+ return O_WRONLY;
+ case NFS4_SHARE_ACCESS_BOTH:
+ return O_RDWR;
+ }
+ WARN_ON_ONCE(1);
+ return O_RDONLY;
+}
+
+static inline int
+access_permit_read(struct nfs4_ol_stateid *stp)
+{
+ return test_access(NFS4_SHARE_ACCESS_READ, stp) ||
+ test_access(NFS4_SHARE_ACCESS_BOTH, stp) ||
+ test_access(NFS4_SHARE_ACCESS_WRITE, stp);
+}
+
+static inline int
+access_permit_write(struct nfs4_ol_stateid *stp)
+{
+ return test_access(NFS4_SHARE_ACCESS_WRITE, stp) ||
+ test_access(NFS4_SHARE_ACCESS_BOTH, stp);
+}
+
static inline struct nfs4_stateowner *
nfs4_get_stateowner(struct nfs4_stateowner *sop)
{
@@ -1149,108 +1266,6 @@ static unsigned int clientstr_hashval(struct xdr_netobj name)
return opaque_hashval(name.data, 8) & CLIENT_HASH_MASK;
}
-/*
- * We store the NONE, READ, WRITE, and BOTH bits separately in the
- * st_{access,deny}_bmap field of the stateid, in order to track not
- * only what share bits are currently in force, but also what
- * combinations of share bits previous opens have used. This allows us
- * to enforce the recommendation of rfc 3530 14.2.19 that the server
- * return an error if the client attempt to downgrade to a combination
- * of share bits not explicable by closing some of its previous opens.
- *
- * XXX: This enforcement is actually incomplete, since we don't keep
- * track of access/deny bit combinations; so, e.g., we allow:
- *
- * OPEN allow read, deny write
- * OPEN allow both, deny none
- * DOWNGRADE allow read, deny none
- *
- * which we should reject.
- */
-static unsigned int
-bmap_to_share_mode(unsigned long bmap) {
- int i;
- unsigned int access = 0;
-
- for (i = 1; i < 4; i++) {
- if (test_bit(i, &bmap))
- access |= i;
- }
- return access;
-}
-
-/* set share access for a given stateid */
-static inline void
-set_access(u32 access, struct nfs4_ol_stateid *stp)
-{
- unsigned char mask = 1 << access;
-
- WARN_ON_ONCE(access > NFS4_SHARE_ACCESS_BOTH);
- stp->st_access_bmap |= mask;
-}
-
-/* clear share access for a given stateid */
-static inline void
-clear_access(u32 access, struct nfs4_ol_stateid *stp)
-{
- unsigned char mask = 1 << access;
-
- WARN_ON_ONCE(access > NFS4_SHARE_ACCESS_BOTH);
- stp->st_access_bmap &= ~mask;
-}
-
-/* test whether a given stateid has access */
-static inline bool
-test_access(u32 access, struct nfs4_ol_stateid *stp)
-{
- unsigned char mask = 1 << access;
-
- return (bool)(stp->st_access_bmap & mask);
-}
-
-/* set share deny for a given stateid */
-static inline void
-set_deny(u32 deny, struct nfs4_ol_stateid *stp)
-{
- unsigned char mask = 1 << deny;
-
- WARN_ON_ONCE(deny > NFS4_SHARE_DENY_BOTH);
- stp->st_deny_bmap |= mask;
-}
-
-/* clear share deny for a given stateid */
-static inline void
-clear_deny(u32 deny, struct nfs4_ol_stateid *stp)
-{
- unsigned char mask = 1 << deny;
-
- WARN_ON_ONCE(deny > NFS4_SHARE_DENY_BOTH);
- stp->st_deny_bmap &= ~mask;
-}
-
-/* test whether a given stateid is denying specific access */
-static inline bool
-test_deny(u32 deny, struct nfs4_ol_stateid *stp)
-{
- unsigned char mask = 1 << deny;
-
- return (bool)(stp->st_deny_bmap & mask);
-}
-
-static int nfs4_access_to_omode(u32 access)
-{
- switch (access & NFS4_SHARE_ACCESS_BOTH) {
- case NFS4_SHARE_ACCESS_READ:
- return O_RDONLY;
- case NFS4_SHARE_ACCESS_WRITE:
- return O_WRONLY;
- case NFS4_SHARE_ACCESS_BOTH:
- return O_RDWR;
- }
- WARN_ON_ONCE(1);
- return O_RDONLY;
-}
-
/*
* A stateid that had a deny mode associated with it is being released
* or downgraded. Recalculate the deny mode on the file.
@@ -5507,21 +5522,6 @@ static inline __be32 nfs4_check_fh(struct svc_fh *fhp, struct nfs4_stid *stp)
return nfs_ok;
}
-static inline int
-access_permit_read(struct nfs4_ol_stateid *stp)
-{
- return test_access(NFS4_SHARE_ACCESS_READ, stp) ||
- test_access(NFS4_SHARE_ACCESS_BOTH, stp) ||
- test_access(NFS4_SHARE_ACCESS_WRITE, stp);
-}
-
-static inline int
-access_permit_write(struct nfs4_ol_stateid *stp)
-{
- return test_access(NFS4_SHARE_ACCESS_WRITE, stp) ||
- test_access(NFS4_SHARE_ACCESS_BOTH, stp);
-}
-
static
__be32 nfs4_check_openmode(struct nfs4_ol_stateid *stp, int flags)
{
--
2.30.2
From: "J. Bruce Fields" <[email protected]>
It's unusual but possible for multiple filehandles to point to the same
file. In that case, we may end up with multiple nfs4_files referencing
the same inode.
For delegation purposes it will turn out to be useful to flag those
cases.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4state.c | 37 ++++++++++++++++++++++++++++---------
fs/nfsd/state.h | 2 ++
2 files changed, 30 insertions(+), 9 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index b0c74dbde07b..c1ba60db671a 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4075,6 +4075,8 @@ static void nfsd4_init_file(struct svc_fh *fh, unsigned int hashval,
fp->fi_share_deny = 0;
memset(fp->fi_fds, 0, sizeof(fp->fi_fds));
memset(fp->fi_access, 0, sizeof(fp->fi_access));
+ fp->fi_aliased = false;
+ fp->fi_inode = d_inode(fh->fh_dentry);
#ifdef CONFIG_NFSD_PNFS
INIT_LIST_HEAD(&fp->fi_lo_states);
atomic_set(&fp->fi_lo_recalls, 0);
@@ -4427,6 +4429,31 @@ find_file_locked(struct svc_fh *fh, unsigned int hashval)
return NULL;
}
+static struct nfs4_file *insert_file(struct nfs4_file *new, struct svc_fh *fh,
+ unsigned int hashval)
+{
+ struct nfs4_file *fp;
+ struct nfs4_file *ret = NULL;
+ bool alias_found = false;
+
+ spin_lock(&state_lock);
+ hlist_for_each_entry_rcu(fp, &file_hashtbl[hashval], fi_hash,
+ lockdep_is_held(&state_lock)) {
+ if (fh_match(&fp->fi_fhandle, &fh->fh_handle)) {
+ if (refcount_inc_not_zero(&fp->fi_ref))
+ ret = fp;
+ } else if (d_inode(fh->fh_dentry) == fp->fi_inode)
+ fp->fi_aliased = alias_found = true;
+ }
+ if (likely(ret == NULL)) {
+ nfsd4_init_file(fh, hashval, new);
+ new->fi_aliased = alias_found;
+ ret = new;
+ }
+ spin_unlock(&state_lock);
+ return ret;
+}
+
static struct nfs4_file * find_file(struct svc_fh *fh)
{
struct nfs4_file *fp;
@@ -4450,15 +4477,7 @@ find_or_add_file(struct nfs4_file *new, struct svc_fh *fh)
if (fp)
return fp;
- spin_lock(&state_lock);
- fp = find_file_locked(fh, hashval);
- if (likely(fp == NULL)) {
- nfsd4_init_file(fh, hashval, new);
- fp = new;
- }
- spin_unlock(&state_lock);
-
- return fp;
+ return insert_file(new, fh, hashval);
}
/*
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index af1d9f2e373e..7b10b3eaaa5c 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -512,6 +512,8 @@ struct nfs4_clnt_odstate {
*/
struct nfs4_file {
refcount_t fi_ref;
+ struct inode * fi_inode;
+ bool fi_aliased;
spinlock_t fi_lock;
struct hlist_node fi_hash; /* hash on fi_fhandle */
struct list_head fi_stateids;
--
2.30.2
> On Apr 16, 2021, at 2:00 PM, J. Bruce Fields <[email protected]> wrote:
>
> From: "J. Bruce Fields" <[email protected]>
>
> If nfsd already has an open file that it plans to use for IO from
> another,
from another... client?
> it may not need to do another vfs open, but it still may need
> to break any delegations in case the existing opens are for another
> client.
>
> Symptoms are that we may incorrectly fail to break a delegation on a
> write open from a different client, when the delegation-holding client
> already has a write open.
>
> Fixes: 28df3d1539de ("nfsd: clients don't need to break their own delegations")
> Signed-off-by: J. Bruce Fields <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 24 +++++++++++++++++++-----
> 1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 97447a64bad0..886e50ed07c2 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4869,6 +4869,11 @@ static __be32 nfs4_get_vfs_file(struct svc_rqst *rqstp, struct nfs4_file *fp,
> if (nf)
> nfsd_file_put(nf);
>
> + status = nfserrno(nfsd_open_break_lease(cur_fh->fh_dentry->d_inode,
> + access));
> + if (status)
> + goto out_put_access;
> +
> status = nfsd4_truncate(rqstp, cur_fh, open);
> if (status)
> goto out_put_access;
> @@ -6849,11 +6854,20 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> static __be32 nfsd_test_lock(struct svc_rqst *rqstp, struct svc_fh *fhp, struct file_lock *lock)
> {
> struct nfsd_file *nf;
> - __be32 err = nfsd_file_acquire(rqstp, fhp, NFSD_MAY_READ, &nf);
> - if (!err) {
> - err = nfserrno(vfs_test_lock(nf->nf_file, lock));
> - nfsd_file_put(nf);
> - }
> + __be32 err;
> +
> + err = nfsd_file_acquire(rqstp, fhp, NFSD_MAY_READ, &nf);
> + if (err)
> + return err;
> + fh_lock(fhp); /* to block new leases till after test_lock: */
> + err = nfserrno(nfsd_open_break_lease(fhp->fh_dentry->d_inode,
> + NFSD_MAY_READ));
> + if (err)
> + goto out;
> + err = nfserrno(vfs_test_lock(nf->nf_file, lock));
> +out:
> + fh_unlock(fhp);
> + nfsd_file_put(nf);
> return err;
> }
>
> --
> 2.30.2
>
--
Chuck Lever
On Fri, Apr 16, 2021 at 07:11:36PM +0000, Chuck Lever III wrote:
>
>
> > On Apr 16, 2021, at 2:00 PM, J. Bruce Fields <[email protected]> wrote:
> >
> > From: "J. Bruce Fields" <[email protected]>
> >
> > If nfsd already has an open file that it plans to use for IO from
> > another,
>
> from another... client?
Yes, thanks.--b.
>
>
> > it may not need to do another vfs open, but it still may need
> > to break any delegations in case the existing opens are for another
> > client.
> >
> > Symptoms are that we may incorrectly fail to break a delegation on a
> > write open from a different client, when the delegation-holding client
> > already has a write open.
> >
> > Fixes: 28df3d1539de ("nfsd: clients don't need to break their own delegations")
> > Signed-off-by: J. Bruce Fields <[email protected]>
> > ---
> > fs/nfsd/nfs4state.c | 24 +++++++++++++++++++-----
> > 1 file changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 97447a64bad0..886e50ed07c2 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -4869,6 +4869,11 @@ static __be32 nfs4_get_vfs_file(struct svc_rqst *rqstp, struct nfs4_file *fp,
> > if (nf)
> > nfsd_file_put(nf);
> >
> > + status = nfserrno(nfsd_open_break_lease(cur_fh->fh_dentry->d_inode,
> > + access));
> > + if (status)
> > + goto out_put_access;
> > +
> > status = nfsd4_truncate(rqstp, cur_fh, open);
> > if (status)
> > goto out_put_access;
> > @@ -6849,11 +6854,20 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > static __be32 nfsd_test_lock(struct svc_rqst *rqstp, struct svc_fh *fhp, struct file_lock *lock)
> > {
> > struct nfsd_file *nf;
> > - __be32 err = nfsd_file_acquire(rqstp, fhp, NFSD_MAY_READ, &nf);
> > - if (!err) {
> > - err = nfserrno(vfs_test_lock(nf->nf_file, lock));
> > - nfsd_file_put(nf);
> > - }
> > + __be32 err;
> > +
> > + err = nfsd_file_acquire(rqstp, fhp, NFSD_MAY_READ, &nf);
> > + if (err)
> > + return err;
> > + fh_lock(fhp); /* to block new leases till after test_lock: */
> > + err = nfserrno(nfsd_open_break_lease(fhp->fh_dentry->d_inode,
> > + NFSD_MAY_READ));
> > + if (err)
> > + goto out;
> > + err = nfserrno(vfs_test_lock(nf->nf_file, lock));
> > +out:
> > + fh_unlock(fhp);
> > + nfsd_file_put(nf);
> > return err;
> > }
> >
> > --
> > 2.30.2
> >
>
> --
> Chuck Lever
>
>