[PATCH 1/5] nfsd: ensure new clients break delegations

[PATCH 1/5] nfsd: ensure new clients break delegations

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

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 <bfields@redhat.com>
---
 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

[PATCH 2/5] nfsd: hash nfs4_files by inode number

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

The nfs4_file structure is per-filehandle, not per-inode, because the
spec requires open and other state to be per filehandle.

But it will turn out to be convenient for nfs4_files associated with the
same inode to be hashed to the same bucket, so let's hash on the inode
instead of the filehandle.

Filehandle aliasing is rare, so that shouldn't have much performance
impact.

(If you have a ton of exported filesystems, though, and all of them have
a root with inode number 2, could that get you an overlong has chain?
Perhaps this (and the v4 open file cache) should be hashed on the inode
pointer instead.)

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/nfs4state.c | 27 ++++++++++++---------------
 fs/nfsd/state.h     |  1 -
 2 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 886e50ed07c2..b0c74dbde07b 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -542,14 +542,12 @@ static unsigned int ownerstr_hashval(struct xdr_netobj *ownername)
 #define FILE_HASH_BITS                   8
 #define FILE_HASH_SIZE                  (1 << FILE_HASH_BITS)
 
-static unsigned int nfsd_fh_hashval(struct knfsd_fh *fh)
+static unsigned int file_hashval(struct svc_fh *fh)
 {
-	return jhash2(fh->fh_base.fh_pad, XDR_QUADLEN(fh->fh_size), 0);
-}
+	struct inode *inode = d_inode(fh->fh_dentry);
 
-static unsigned int file_hashval(struct knfsd_fh *fh)
-{
-	return nfsd_fh_hashval(fh) & (FILE_HASH_SIZE - 1);
+	/* XXX: why not (here & in file cache) use inode? */
+	return (unsigned int)hash_long(inode->i_ino, FILE_HASH_SIZE);
 }
 
 static struct hlist_head file_hashtbl[FILE_HASH_SIZE];
@@ -4061,7 +4059,7 @@ static struct nfs4_file *nfsd4_alloc_file(void)
 }
 
 /* OPEN Share state helper functions */
-static void nfsd4_init_file(struct knfsd_fh *fh, unsigned int hashval,
+static void nfsd4_init_file(struct svc_fh *fh, unsigned int hashval,
 				struct nfs4_file *fp)
 {
 	lockdep_assert_held(&state_lock);
@@ -4071,7 +4069,7 @@ static void nfsd4_init_file(struct knfsd_fh *fh, unsigned int hashval,
 	INIT_LIST_HEAD(&fp->fi_stateids);
 	INIT_LIST_HEAD(&fp->fi_delegations);
 	INIT_LIST_HEAD(&fp->fi_clnt_odstate);
-	fh_copy_shallow(&fp->fi_fhandle, fh);
+	fh_copy_shallow(&fp->fi_fhandle, &fh->fh_handle);
 	fp->fi_deleg_file = NULL;
 	fp->fi_had_conflict = false;
 	fp->fi_share_deny = 0;
@@ -4415,13 +4413,13 @@ move_to_close_lru(struct nfs4_ol_stateid *s, struct net *net)
 
 /* search file_hashtbl[] for file */
 static struct nfs4_file *
-find_file_locked(struct knfsd_fh *fh, unsigned int hashval)
+find_file_locked(struct svc_fh *fh, unsigned int hashval)
 {
 	struct nfs4_file *fp;
 
 	hlist_for_each_entry_rcu(fp, &file_hashtbl[hashval], fi_hash,
 				lockdep_is_held(&state_lock)) {
-		if (fh_match(&fp->fi_fhandle, fh)) {
+		if (fh_match(&fp->fi_fhandle, &fh->fh_handle)) {
 			if (refcount_inc_not_zero(&fp->fi_ref))
 				return fp;
 		}
@@ -4429,8 +4427,7 @@ find_file_locked(struct knfsd_fh *fh, unsigned int hashval)
 	return NULL;
 }
 
-struct nfs4_file *
-find_file(struct knfsd_fh *fh)
+static struct nfs4_file * find_file(struct svc_fh *fh)
 {
 	struct nfs4_file *fp;
 	unsigned int hashval = file_hashval(fh);
@@ -4442,7 +4439,7 @@ find_file(struct knfsd_fh *fh)
 }
 
 static struct nfs4_file *
-find_or_add_file(struct nfs4_file *new, struct knfsd_fh *fh)
+find_or_add_file(struct nfs4_file *new, struct svc_fh *fh)
 {
 	struct nfs4_file *fp;
 	unsigned int hashval = file_hashval(fh);
@@ -4474,7 +4471,7 @@ nfs4_share_conflict(struct svc_fh *current_fh, unsigned int deny_type)
 	struct nfs4_file *fp;
 	__be32 ret = nfs_ok;
 
-	fp = find_file(&current_fh->fh_handle);
+	fp = find_file(current_fh);
 	if (!fp)
 		return ret;
 	/* Check for conflicting share reservations */
@@ -5155,7 +5152,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 	 * and check for delegations in the process of being recalled.
 	 * If not found, create the nfs4_file struct
 	 */
-	fp = find_or_add_file(open->op_file, &current_fh->fh_handle);
+	fp = find_or_add_file(open->op_file, current_fh);
 	if (fp != open->op_file) {
 		status = nfs4_check_deleg(cl, open, &dp);
 		if (status)
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 73deea353169..af1d9f2e373e 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -665,7 +665,6 @@ extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(struct xdr_netobj name
 				struct xdr_netobj princhash, struct nfsd_net *nn);
 extern bool nfs4_has_reclaimed_state(struct xdr_netobj name, struct nfsd_net *nn);
 
-struct nfs4_file *find_file(struct knfsd_fh *fh);
 void put_nfs4_file(struct nfs4_file *fi);
 extern void nfs4_put_copy(struct nfsd4_copy *copy);
 extern struct nfsd4_copy *
-- 
2.30.2

Re: [PATCH 2/5] nfsd: hash nfs4_files by inode number

[Chuck Lever III]

> On Apr 16, 2021, at 2:00 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> 
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> The nfs4_file structure is per-filehandle, not per-inode, because the
> spec requires open and other state to be per filehandle.
> 
> But it will turn out to be convenient for nfs4_files associated with the
> same inode to be hashed to the same bucket, so let's hash on the inode
> instead of the filehandle.
> 
> Filehandle aliasing is rare, so that shouldn't have much performance
> impact.
> 
> (If you have a ton of exported filesystems, though, and all of them have
> a root with inode number 2, could that get you an overlong has chain?

^has ^hash

Also, I'm getting this new warning:

/home/cel/src/linux/linux/include/linux/hash.h:81:38: warning: shift too big (4294967104) for type unsigned long long


> Perhaps this (and the v4 open file cache) should be hashed on the inode
> pointer instead.)
> 
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
> fs/nfsd/nfs4state.c | 27 ++++++++++++---------------
> fs/nfsd/state.h     |  1 -
> 2 files changed, 12 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 886e50ed07c2..b0c74dbde07b 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -542,14 +542,12 @@ static unsigned int ownerstr_hashval(struct xdr_netobj *ownername)
> #define FILE_HASH_BITS                   8
> #define FILE_HASH_SIZE                  (1 << FILE_HASH_BITS)
> 
> -static unsigned int nfsd_fh_hashval(struct knfsd_fh *fh)
> +static unsigned int file_hashval(struct svc_fh *fh)
> {
> -	return jhash2(fh->fh_base.fh_pad, XDR_QUADLEN(fh->fh_size), 0);
> -}
> +	struct inode *inode = d_inode(fh->fh_dentry);
> 
> -static unsigned int file_hashval(struct knfsd_fh *fh)
> -{
> -	return nfsd_fh_hashval(fh) & (FILE_HASH_SIZE - 1);
> +	/* XXX: why not (here & in file cache) use inode? */
> +	return (unsigned int)hash_long(inode->i_ino, FILE_HASH_SIZE);
> }
> 
> static struct hlist_head file_hashtbl[FILE_HASH_SIZE];
> @@ -4061,7 +4059,7 @@ static struct nfs4_file *nfsd4_alloc_file(void)
> }
> 
> /* OPEN Share state helper functions */
> -static void nfsd4_init_file(struct knfsd_fh *fh, unsigned int hashval,
> +static void nfsd4_init_file(struct svc_fh *fh, unsigned int hashval,
> 				struct nfs4_file *fp)
> {
> 	lockdep_assert_held(&state_lock);
> @@ -4071,7 +4069,7 @@ static void nfsd4_init_file(struct knfsd_fh *fh, unsigned int hashval,
> 	INIT_LIST_HEAD(&fp->fi_stateids);
> 	INIT_LIST_HEAD(&fp->fi_delegations);
> 	INIT_LIST_HEAD(&fp->fi_clnt_odstate);
> -	fh_copy_shallow(&fp->fi_fhandle, fh);
> +	fh_copy_shallow(&fp->fi_fhandle, &fh->fh_handle);
> 	fp->fi_deleg_file = NULL;
> 	fp->fi_had_conflict = false;
> 	fp->fi_share_deny = 0;
> @@ -4415,13 +4413,13 @@ move_to_close_lru(struct nfs4_ol_stateid *s, struct net *net)
> 
> /* search file_hashtbl[] for file */
> static struct nfs4_file *
> -find_file_locked(struct knfsd_fh *fh, unsigned int hashval)
> +find_file_locked(struct svc_fh *fh, unsigned int hashval)
> {
> 	struct nfs4_file *fp;
> 
> 	hlist_for_each_entry_rcu(fp, &file_hashtbl[hashval], fi_hash,
> 				lockdep_is_held(&state_lock)) {
> -		if (fh_match(&fp->fi_fhandle, fh)) {
> +		if (fh_match(&fp->fi_fhandle, &fh->fh_handle)) {
> 			if (refcount_inc_not_zero(&fp->fi_ref))
> 				return fp;
> 		}
> @@ -4429,8 +4427,7 @@ find_file_locked(struct knfsd_fh *fh, unsigned int hashval)
> 	return NULL;
> }
> 
> -struct nfs4_file *
> -find_file(struct knfsd_fh *fh)
> +static struct nfs4_file * find_file(struct svc_fh *fh)
> {
> 	struct nfs4_file *fp;
> 	unsigned int hashval = file_hashval(fh);
> @@ -4442,7 +4439,7 @@ find_file(struct knfsd_fh *fh)
> }
> 
> static struct nfs4_file *
> -find_or_add_file(struct nfs4_file *new, struct knfsd_fh *fh)
> +find_or_add_file(struct nfs4_file *new, struct svc_fh *fh)
> {
> 	struct nfs4_file *fp;
> 	unsigned int hashval = file_hashval(fh);
> @@ -4474,7 +4471,7 @@ nfs4_share_conflict(struct svc_fh *current_fh, unsigned int deny_type)
> 	struct nfs4_file *fp;
> 	__be32 ret = nfs_ok;
> 
> -	fp = find_file(&current_fh->fh_handle);
> +	fp = find_file(current_fh);
> 	if (!fp)
> 		return ret;
> 	/* Check for conflicting share reservations */
> @@ -5155,7 +5152,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
> 	 * and check for delegations in the process of being recalled.
> 	 * If not found, create the nfs4_file struct
> 	 */
> -	fp = find_or_add_file(open->op_file, &current_fh->fh_handle);
> +	fp = find_or_add_file(open->op_file, current_fh);
> 	if (fp != open->op_file) {
> 		status = nfs4_check_deleg(cl, open, &dp);
> 		if (status)
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 73deea353169..af1d9f2e373e 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -665,7 +665,6 @@ extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(struct xdr_netobj name
> 				struct xdr_netobj princhash, struct nfsd_net *nn);
> extern bool nfs4_has_reclaimed_state(struct xdr_netobj name, struct nfsd_net *nn);
> 
> -struct nfs4_file *find_file(struct knfsd_fh *fh);
> void put_nfs4_file(struct nfs4_file *fi);
> extern void nfs4_put_copy(struct nfsd4_copy *copy);
> extern struct nfsd4_copy *
> -- 
> 2.30.2
> 

--
Chuck Lever

Re: [PATCH 2/5] nfsd: hash nfs4_files by inode number

[J. Bruce Fields]

On Fri, Apr 16, 2021 at 07:21:16PM +0000, Chuck Lever III wrote:
> 
> 
> > On Apr 16, 2021, at 2:00 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> > 
> > From: "J. Bruce Fields" <bfields@redhat.com>
> > 
> > The nfs4_file structure is per-filehandle, not per-inode, because the
> > spec requires open and other state to be per filehandle.
> > 
> > But it will turn out to be convenient for nfs4_files associated with the
> > same inode to be hashed to the same bucket, so let's hash on the inode
> > instead of the filehandle.
> > 
> > Filehandle aliasing is rare, so that shouldn't have much performance
> > impact.
> > 
> > (If you have a ton of exported filesystems, though, and all of them have
> > a root with inode number 2, could that get you an overlong has chain?
> 
> ^has ^hash
> 
> Also, I'm getting this new warning:
> 
> /home/cel/src/linux/linux/include/linux/hash.h:81:38: warning: shift too big (4294967104) for type unsigned long long

Whoops; it needs this: would you like me to resend?--b.

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index b0c74dbde07b..47a76284b47c 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -547,7 +547,7 @@ static unsigned int file_hashval(struct svc_fh *fh)
 	struct inode *inode = d_inode(fh->fh_dentry);
 
 	/* XXX: why not (here & in file cache) use inode? */
-	return (unsigned int)hash_long(inode->i_ino, FILE_HASH_SIZE);
+	return (unsigned int)hash_long(inode->i_ino, FILE_HASH_BITS);
 }
 
 static struct hlist_head file_hashtbl[FILE_HASH_SIZE];

Re: [PATCH 2/5] nfsd: hash nfs4_files by inode number

[Chuck Lever III]

> On Apr 19, 2021, at 3:53 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> 
> On Fri, Apr 16, 2021 at 07:21:16PM +0000, Chuck Lever III wrote:
>> 
>> 
>>> On Apr 16, 2021, at 2:00 PM, J. Bruce Fields <bfields@redhat.com> wrote:
>>> 
>>> From: "J. Bruce Fields" <bfields@redhat.com>
>>> 
>>> The nfs4_file structure is per-filehandle, not per-inode, because the
>>> spec requires open and other state to be per filehandle.
>>> 
>>> But it will turn out to be convenient for nfs4_files associated with the
>>> same inode to be hashed to the same bucket, so let's hash on the inode
>>> instead of the filehandle.
>>> 
>>> Filehandle aliasing is rare, so that shouldn't have much performance
>>> impact.
>>> 
>>> (If you have a ton of exported filesystems, though, and all of them have
>>> a root with inode number 2, could that get you an overlong has chain?
>> 
>> ^has ^hash
>> 
>> Also, I'm getting this new warning:
>> 
>> /home/cel/src/linux/linux/include/linux/hash.h:81:38: warning: shift too big (4294967104) for type unsigned long long
> 
> Whoops; it needs this: would you like me to resend?--b.

No, thanks. I'll squash this in.


> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index b0c74dbde07b..47a76284b47c 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -547,7 +547,7 @@ static unsigned int file_hashval(struct svc_fh *fh)
> 	struct inode *inode = d_inode(fh->fh_dentry);
> 
> 	/* XXX: why not (here & in file cache) use inode? */
> -	return (unsigned int)hash_long(inode->i_ino, FILE_HASH_SIZE);
> +	return (unsigned int)hash_long(inode->i_ino, FILE_HASH_BITS);
> }
> 
> static struct hlist_head file_hashtbl[FILE_HASH_SIZE];

--
Chuck Lever

[PATCH 3/5] nfsd: track filehandle aliasing in nfs4_files

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

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 <bfields@redhat.com>
---
 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

[PATCH 4/5] nfsd: reshuffle some code

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

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 <bfields@redhat.com>
---
 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

[PATCH 5/5] nfsd: grant read delegations to clients holding writes

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

It's OK to grant a read delegation to a client that holds a write, as
long as it's the only client holding the write.

We originally tried to do this in 94415b06eb8a "nfsd4: a client's own
opens needn't prevent delegations", which had to be reverted in
6ee65a773096.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/locks.c          |  3 ++
 fs/nfsd/nfs4state.c | 82 +++++++++++++++++++++++++++++++++++++--------
 2 files changed, 71 insertions(+), 14 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 6125d2de39b8..bcc71c469ede 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1808,6 +1808,9 @@ check_conflicting_open(struct file *filp, const long arg, int flags)
 
 	if (flags & FL_LAYOUT)
 		return 0;
+	if (flags & FL_DELEG)
+		/* We leave these checks to the caller */
+		return 0;
 
 	if (arg == F_RDLCK)
 		return inode_is_open_for_write(inode) ? -EAGAIN : 0;
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index dfe3e39c891c..5c4896773bdd 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4976,6 +4976,65 @@ static struct file_lock *nfs4_alloc_init_lease(struct nfs4_delegation *dp,
 	return fl;
 }
 
+static int nfsd4_check_conflicting_opens(struct nfs4_client *clp,
+					 struct nfs4_file *fp)
+{
+	struct nfs4_ol_stateid *st;
+	struct file *f = fp->fi_deleg_file->nf_file;
+	struct inode *ino = locks_inode(f);
+	int writes;
+
+	writes = atomic_read(&ino->i_writecount);
+	if (!writes)
+		return 0;
+	/*
+	 * There could be multiple filehandles (hence multiple
+	 * nfs4_files) referencing this file, but that's not too
+	 * common; let's just give up in that case rather than
+	 * trying to go look up all the clients using that other
+	 * nfs4_file as well:
+	 */
+	if (fp->fi_aliased)
+		return -EAGAIN;
+	/*
+	 * If there's a close in progress, make sure that we see it
+	 * clear any fi_fds[] entries before we see it decrement
+	 * i_writecount:
+	 */
+	smp_mb__after_atomic();
+
+	if (fp->fi_fds[O_WRONLY])
+		writes--;
+	if (fp->fi_fds[O_RDWR])
+		writes--;
+	if (writes > 0)
+		return -EAGAIN; /* There may be non-NFSv4 writers */
+	/*
+	 * It's possible there are non-NFSv4 write opens in progress,
+	 * but if they haven't incremented i_writecount yet then they
+	 * also haven't called break lease yet; so, they'll break this
+	 * lease soon enough.  So, all that's left to check for is NFSv4
+	 * opens:
+	 */
+	spin_lock(&fp->fi_lock);
+	list_for_each_entry(st, &fp->fi_stateids, st_perfile) {
+		if (st->st_openstp == NULL /* it's an open */ &&
+		    access_permit_write(st) &&
+		    st->st_stid.sc_client != clp) {
+			spin_unlock(&fp->fi_lock);
+			return -EAGAIN;
+		}
+	}
+	spin_unlock(&fp->fi_lock);
+	/*
+	 * There's a small chance that we could be racing with another
+	 * NFSv4 open.  However, any open that hasn't added itself to
+	 * the fi_stateids list also hasn't called break_lease yet; so,
+	 * they'll break this lease soon enough.
+	 */
+	return 0;
+}
+
 static struct nfs4_delegation *
 nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh,
 		    struct nfs4_file *fp, struct nfs4_clnt_odstate *odstate)
@@ -4995,9 +5054,12 @@ nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh,
 
 	nf = find_readable_file(fp);
 	if (!nf) {
-		/* We should always have a readable file here */
-		WARN_ON_ONCE(1);
-		return ERR_PTR(-EBADF);
+		/*
+		 * We probably could attempt another open and get a read
+		 * delegation, but for now, don't bother until the
+		 * client actually sends us one.
+		 */
+		return ERR_PTR(-EAGAIN);
 	}
 	spin_lock(&state_lock);
 	spin_lock(&fp->fi_lock);
@@ -5032,6 +5094,9 @@ nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh,
 		locks_free_lock(fl);
 	if (status)
 		goto out_clnt_odstate;
+	status = nfsd4_check_conflicting_opens(clp, fp);
+	if (status)
+		goto out_unlock;
 
 	spin_lock(&state_lock);
 	spin_lock(&fp->fi_lock);
@@ -5113,17 +5178,6 @@ nfs4_open_delegation(struct svc_fh *fh, struct nfsd4_open *open,
 				goto out_no_deleg;
 			if (!cb_up || !(oo->oo_flags & NFS4_OO_CONFIRMED))
 				goto out_no_deleg;
-			/*
-			 * Also, if the file was opened for write or
-			 * create, there's a good chance the client's
-			 * about to write to it, resulting in an
-			 * immediate recall (since we don't support
-			 * write delegations):
-			 */
-			if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE)
-				goto out_no_deleg;
-			if (open->op_create == NFS4_OPEN_CREATE)
-				goto out_no_deleg;
 			break;
 		default:
 			goto out_no_deleg;
-- 
2.30.2

Re: [PATCH 1/5] nfsd: ensure new clients break delegations

[Chuck Lever III]

> On Apr 16, 2021, at 2:00 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> 
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> 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 <bfields@redhat.com>
> ---
> 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

Re: [PATCH 1/5] nfsd: ensure new clients break delegations

[J. Bruce Fields]

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 <bfields@redhat.com> wrote:
> > 
> > From: "J. Bruce Fields" <bfields@redhat.com>
> > 
> > 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 <bfields@redhat.com>
> > ---
> > 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
> 
>