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

From: Chuck Lever III <chuck.lever@oracle.com>
To: Bruce Fields <bfields@fieldses.org>
Cc: Bruce Fields <bfields@redhat.com>,
	Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 2/5] nfsd: hash nfs4_files by inode number
Date: Mon, 19 Apr 2021 20:42:21 +0000	[thread overview]
Message-ID: <0788B2C4-B2A5-4430-AE63-0E9DA82A7118@oracle.com> (raw)
In-Reply-To: <20210419195315.GA17661@fieldses.org>

> 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