diff mbox series

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

Message ID 1618596018-9899-2-git-send-email-bfields@redhat.com (mailing list archive)
State New, archived
Headers show
Series [1/5] nfsd: ensure new clients break delegations | expand

Commit Message

Bruce Fields April 16, 2021, 6 p.m. UTC 
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(-)

Comments

Chuck Lever April 16, 2021, 7:21 p.m. UTC | #1 
> 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
J. Bruce Fields April 19, 2021, 7:53 p.m. UTC | #2 
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];
Chuck Lever April 19, 2021, 8:42 p.m. UTC | #3 
> 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
diff mbox series

Patch

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 *