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

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4121 bytes --]

CC: kbuild-all(a)lists.01.org
In-Reply-To: <1618596018-9899-2-git-send-email-bfields@redhat.com>
References: <1618596018-9899-2-git-send-email-bfields@redhat.com>
TO: "J. Bruce Fields" <bfields@redhat.com>
TO: Chuck Lever <chuck.lever@oracle.com>
CC: linux-nfs(a)vger.kernel.org
CC: "J. Bruce Fields" <bfields@redhat.com>

Hi Bruce,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on nfsd/nfsd-next v5.12-rc8 next-20210416]
[cannot apply to cel/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/J-Bruce-Fields/nfsd-ensure-new-clients-break-delegations/20210417-020217
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 151501160401e2dc669ea7dac2c599b53f220c33
:::::: branch date: 3 days ago
:::::: commit date: 3 days ago
config: ia64-randconfig-s031-20210419 (attached as .config)
compiler: ia64-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-330-g09ec74f6-dirty
        # https://github.com/0day-ci/linux/commit/c988dd4d08fdbff8c957f1cb0dcc44247c618af3
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review J-Bruce-Fields/nfsd-ensure-new-clients-break-delegations/20210417-020217
        git checkout c988dd4d08fdbff8c957f1cb0dcc44247c618af3
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' W=1 ARCH=ia64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
   fs/nfsd/nfs4state.c: note: in included file (through include/linux/dcache.h, include/linux/fs.h):
   include/linux/rculist.h:515:9: sparse: sparse: context imbalance in 'put_nfs4_file' - unexpected unlock
   fs/nfsd/nfs4state.c: note: in included file (through include/linux/stringhash.h, include/linux/dcache.h, include/linux/fs.h):
>> include/linux/hash.h:81:45: sparse: sparse: shift too big (4294967104) for type unsigned long long
   fs/nfsd/nfs4state.c: note: in included file (through include/linux/wait.h, include/linux/wait_bit.h, include/linux/fs.h):
   include/linux/list.h:135:19: sparse: sparse: context imbalance in 'put_clnt_odstate' - unexpected unlock
   fs/nfsd/nfs4state.c:977:9: sparse: sparse: context imbalance in 'nfs4_put_stid' - unexpected unlock

vim +81 include/linux/hash.h

ef703f49a6c5b90 George Spelvin 2016-05-26  73  
468a9428521e7d0 George Spelvin 2016-05-26  74  #ifndef HAVE_ARCH_HASH_64
468a9428521e7d0 George Spelvin 2016-05-26  75  #define hash_64 hash_64_generic
468a9428521e7d0 George Spelvin 2016-05-26  76  #endif
468a9428521e7d0 George Spelvin 2016-05-26  77  static __always_inline u32 hash_64_generic(u64 val, unsigned int bits)
ef703f49a6c5b90 George Spelvin 2016-05-26  78  {
ef703f49a6c5b90 George Spelvin 2016-05-26  79  #if BITS_PER_LONG == 64
ef703f49a6c5b90 George Spelvin 2016-05-26  80  	/* 64x64-bit multiply is efficient on all 64-bit processors */
ef703f49a6c5b90 George Spelvin 2016-05-26 @81  	return val * GOLDEN_RATIO_64 >> (64 - bits);
ef703f49a6c5b90 George Spelvin 2016-05-26  82  #else
ef703f49a6c5b90 George Spelvin 2016-05-26  83  	/* Hash 64 bits using only 32x32-bit multiply. */
ef703f49a6c5b90 George Spelvin 2016-05-26  84  	return hash_32((u32)val ^ __hash_32(val >> 32), bits);
ef703f49a6c5b90 George Spelvin 2016-05-26  85  #endif
^1da177e4c3f415 Linus Torvalds 2005-04-16  86  }
^1da177e4c3f415 Linus Torvalds 2005-04-16  87  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 28172 bytes --]

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

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 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

[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