Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Bruce Fields <bfields@redhat.com>
CC:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 2/5] nfsd: hash nfs4_files by inode number
Thread-Topic: [PATCH 2/5] nfsd: hash nfs4_files by inode number
Thread-Index: AQHXMupixIvrGePep02kzvU78UvPJ6q3hYIA
Date:   Fri, 16 Apr 2021 19:21:16 +0000
Message-ID: <7342A4CD-0B93-4C13-AD32-4DADC26CD8ED@oracle.com>
References: <1618596018-9899-1-git-send-email-bfields@redhat.com>
 <1618596018-9899-2-git-send-email-bfields@redhat.com>
In-Reply-To: <1618596018-9899-2-git-send-email-bfields@redhat.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <CA844D0CC7416845B79EE8051BE45FD9@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 9775e60f-549e-42ed-a2ff-08d9010cce99
X-MS-Exchange-CrossTenant-originalarrivaltime: 16 Apr 2021 19:21:16.5861
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: iXnkZsaOGLCLl9h0X6tMAmmRAcQLQ/+w7tMG/oHFxewZXc9SxsR3FEykRo8G65KeKkWPSJgxbWi5WrVFs7NtNA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB3112
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9956 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 bulkscore=0 malwarescore=0
 spamscore=0 adultscore=0 phishscore=0 mlxlogscore=999 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2104060000
 definitions=main-2104160136
X-Proofpoint-GUID: CKyopCa-raYPuEEqQur5nd8zp2qOUTt3
X-Proofpoint-ORIG-GUID: CKyopCa-raYPuEEqQur5nd8zp2qOUTt3
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9956 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 mlxlogscore=999 spamscore=0
 impostorscore=0 priorityscore=1501 lowpriorityscore=0 adultscore=0
 bulkscore=0 phishscore=0 suspectscore=0 malwarescore=0 clxscore=1015
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2104060000
 definitions=main-2104160136
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Apr 16, 2021, at 2:00 PM, J. Bruce Fields <bfields@redhat.com> wrote:
>=20
> From: "J. Bruce Fields" <bfields@redhat.com>
>=20
> The nfs4_file structure is per-filehandle, not per-inode, because the
> spec requires open and other state to be per filehandle.
>=20
> 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.
>=20
> Filehandle aliasing is rare, so that shouldn't have much performance
> impact.
>=20
> (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 bi=
g (4294967104) for type unsigned long long


> Perhaps this (and the v4 open file cache) should be hashed on the inode
> pointer instead.)
>=20
> 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(-)
>=20
> 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_net=
obj *ownername)
> #define FILE_HASH_BITS                   8
> #define FILE_HASH_SIZE                  (1 << FILE_HASH_BITS)
>=20
> -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 =3D d_inode(fh->fh_dentry);
>=20
> -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);
> }
>=20
> static struct hlist_head file_hashtbl[FILE_HASH_SIZE];
> @@ -4061,7 +4059,7 @@ static struct nfs4_file *nfsd4_alloc_file(void)
> }
>=20
> /* 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, un=
signed 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 =3D NULL;
> 	fp->fi_had_conflict =3D false;
> 	fp->fi_share_deny =3D 0;
> @@ -4415,13 +4413,13 @@ move_to_close_lru(struct nfs4_ol_stateid *s, stru=
ct net *net)
>=20
> /* 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;
>=20
> 	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;
> }
>=20
> -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 =3D file_hashval(fh);
> @@ -4442,7 +4439,7 @@ find_file(struct knfsd_fh *fh)
> }
>=20
> 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 =3D file_hashval(fh);
> @@ -4474,7 +4471,7 @@ nfs4_share_conflict(struct svc_fh *current_fh, unsi=
gned int deny_type)
> 	struct nfs4_file *fp;
> 	__be32 ret =3D nfs_ok;
>=20
> -	fp =3D find_file(&current_fh->fh_handle);
> +	fp =3D 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 =3D find_or_add_file(open->op_file, &current_fh->fh_handle);
> +	fp =3D find_or_add_file(open->op_file, current_fh);
> 	if (fp !=3D open->op_file) {
> 		status =3D 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_rec=
laim(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);
>=20
> -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 *
> --=20
> 2.30.2
>=20

--
Chuck Lever