Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Neil Brown <neilb@suse.de>
CC:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v5 12/13] NFSD: Allocate an rhashtable for nfs4_file
 objects
Thread-Topic: [PATCH v5 12/13] NFSD: Allocate an rhashtable for nfs4_file
 objects
Thread-Index: AQHY6AGdu/UYtY1O+kiAsof/0iyDy64ePVcA
Date:   Tue, 25 Oct 2022 00:13:59 +0000
Message-ID: <C1C6B958-A138-42A0-B39E-D8CCEB3D79F7@oracle.com>
References: <166664935937.50761.7812494396457965637.stgit@klimt.1015granger.net>
 <166665108857.50761.11874625810370383309.stgit@klimt.1015granger.net>
 <166665465494.12462.9078997979555811271@noble.neil.brown.name>
In-Reply-To: <166665465494.12462.9078997979555811271@noble.neil.brown.name>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?hVrNADLzKDWbjI7HZcDjz8DZO+p5HbLJ/oVMWy+OgPsuMQWSXEjYrxFfte36?=
 =?us-ascii?Q?3jz699TvYaE1fBiHEIxfVURilxXxeUwDidzryedbZNt89CECXSKkQhWWINFb?=
 =?us-ascii?Q?eTvmVSG85Fb22rFnJSwp+J9f3O+iR8TFXFITmBXd3cKvD+oxvEvQJMRTHCHl?=
 =?us-ascii?Q?lWInVFiYjXn0bfi03UdAP3J9TPYpMZTyl6vR+YNoFr+50DyyhQmgqBmd/OUz?=
 =?us-ascii?Q?R76zsh4dI7wrDfyHOW3s259zq0BK2SDGSgquMRAEZvXsEfVeT93CxhuGE9vN?=
 =?us-ascii?Q?i7I27HbDwDd0l2IW3dQ67Pf60l6vReOLzglJgPud9YpsD6H2t3nW/5AdnkQ+?=
 =?us-ascii?Q?un8R3Zynw03VQpP86QUiTvy1eyzfGE3MJxkG8zXechvg7JSs01ZJvNbO+Ys+?=
 =?us-ascii?Q?y9kNi+H4oMOJ7YRrBuiMFzw3aw9SYBpRSVxLbUBnb5dpj5RYseCVqQT8vgsW?=
 =?us-ascii?Q?g+05RVL1uDeY9k1moSZ5kw4v0CfPCcKIzGfDh+FllopZ1XDTEgQn0YEzlhrF?=
 =?us-ascii?Q?yDe+rOWsqBGqIcfUoq0oicXosYsEWhnhw4UM2TyGGMle4BssylNyrHDOmkCt?=
 =?us-ascii?Q?P3xiw2dhWXpP8mNCfiPhplJrDn7UzkBCvjqMPG/VRThlC00sVo8OJLPdpzO5?=
 =?us-ascii?Q?twrU6ZlA/N6Kkfs2k4tL4Hu/Pw8cL4tRlSZDJf2MWW4Y9ObpXzFm3LPGTuJB?=
 =?us-ascii?Q?0Yzk4w3lDpVh1rUYB0TTPBvp7qNxjHkdYPW8UTmSUsS8oC+oXat7dDdDH0K+?=
 =?us-ascii?Q?LlE1A6PA+khmC/Xz7z3JhBacmKzHvLvp4OdnAKtnbZSzvPIWGLmS52PTMu+8?=
 =?us-ascii?Q?a8wga6IJBr1PXPC+U6kJgRKUER71qNQ99NXOtpKC+5nJHQbnuqGxZD4m0jU/?=
 =?us-ascii?Q?SOiUjQeZM+VFvgl9q/VCvZy8yQrUs18KN2fsI9O5J+uvHSSyitLxLJiuED4B?=
 =?us-ascii?Q?rBvzuG/+603yG8JKaZsVqYvlQ+UJoEmG16a7XOgYHXMMpKZJTkpurHFuvtso?=
 =?us-ascii?Q?G+1xQohjyW3HrGIO87H6nU1mh8d3POtL58sBzVWeJBpFNGC0hme7DlmykMTO?=
 =?us-ascii?Q?93+ccSOIej4W27LrRkKHff44aIgqvmUYhXp32Ff1nC/cML7FDAzzfZDZNlAJ?=
 =?us-ascii?Q?evX5fYLvU0Cd4ftrKlRHI1YpMt7gUXqid656RdL6pfGoowYg8yV7pFHd7rWa?=
 =?us-ascii?Q?ImM+EmejMRHXCyFKSKZonEZz28CBYzPvZML0eTOUhVgWPLSTxMK7jxQcL5Jx?=
 =?us-ascii?Q?OpQkUn4U7o2DaPtd0Y9qunSf+8/JTpCxk/gJfeqxzHJcjuxuP0oRSBW22jL/?=
 =?us-ascii?Q?kOzF/DFG6XDkfxHMa5NqtEYfsaGwIFfelYZtYDecTZu7dtWP31W2eerfEQMJ?=
 =?us-ascii?Q?Kx7ab2MntCLEcBfTNvfwKJtoHteP1bTXmr64gv5vQACdTR1K3YGZf9zxM2t7?=
 =?us-ascii?Q?0SPA1azQ5wM5AG57uJXeiuRPai1Zl2Bg3YrXV8XL5sNqy/GqEmdP4wyecQq2?=
 =?us-ascii?Q?9P2/4ix2tF21+l+s7p/e3AUOK8BOOVDYI82+lw3UKXXemL1ZLUipQkvfBVox?=
 =?us-ascii?Q?c6TRax3Og9jr8ws6Q8NwmPIdNy0BwVWZuobDStg8Ue1zQL1WCJZbiZ/aFsKV?=
 =?us-ascii?Q?hQ=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <85F84A3F7FD9184DA47A0431B03B5C48@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: fd067c63-2fe4-4521-30b2-08dab61dd060
X-MS-Exchange-CrossTenant-originalarrivaltime: 25 Oct 2022 00:13:59.1993
 (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: +m5/pyf8CFwiJLHpa5mqskBj8PJtTOlRuqJB96uPKEsZhbEPiSk+DpD6l7XvEYCVOdXu+xNALdOYrk5lnQ8jUw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB5645
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2022-10-24_08,2022-10-21_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 phishscore=0
 spamscore=0 suspectscore=0 bulkscore=0 malwarescore=0 mlxscore=0
 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2209130000 definitions=main-2210250000
X-Proofpoint-GUID: WLXb_s7xNhRsKkOy7Tf5OmeSDpfDdgQN
X-Proofpoint-ORIG-GUID: WLXb_s7xNhRsKkOy7Tf5OmeSDpfDdgQN
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Oct 24, 2022, at 7:37 PM, NeilBrown <neilb@suse.de> wrote:
>=20
> On Tue, 25 Oct 2022, Chuck Lever wrote:
>> Introduce the infrastructure for managing nfs4_file objects in an
>> rhashtable. This infrastructure will be used by the next patch.
>>=20
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> fs/nfsd/nfs4state.c |   23 ++++++++++++++++++++++-
>> fs/nfsd/state.h     |    1 +
>> 2 files changed, 23 insertions(+), 1 deletion(-)
>>=20
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index abed795bb4ec..681cb2daa843 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -44,7 +44,9 @@
>> #include <linux/jhash.h>
>> #include <linux/string_helpers.h>
>> #include <linux/fsnotify.h>
>> +#include <linux/rhashtable.h>
>> #include <linux/nfs_ssc.h>
>> +
>> #include "xdr4.h"
>> #include "xdr4cb.h"
>> #include "vfs.h"
>> @@ -721,6 +723,18 @@ static unsigned int file_hashval(const struct svc_f=
h *fh)
>>=20
>> static struct hlist_head file_hashtbl[FILE_HASH_SIZE];
>>=20
>> +static struct rhltable nfs4_file_rhltable ____cacheline_aligned_in_smp;
>> +
>> +static const struct rhashtable_params nfs4_file_rhash_params =3D {
>> +	.key_len		=3D sizeof_field(struct nfs4_file, fi_inode),
>> +	.key_offset		=3D offsetof(struct nfs4_file, fi_inode),
>> +	.head_offset		=3D offsetof(struct nfs4_file, fi_rlist),
>> +
>> +	/* Reduce resizing churn on light workloads */
>> +	.min_size		=3D 256,		/* buckets */
>=20
> Every time I see this line a groan a little bit.  Probably I'm groaning
> at rhashtable - you shouldn't need to have to worry about these sorts of
> details when using an API...  but I agree that avoiding churn is likely
> a good idea.
>=20
> Where did 256 come from?

Here's the current file_hashtbl definition:

 710 /* hash table for nfs4_file */
 711 #define FILE_HASH_BITS                   8
 712 #define FILE_HASH_SIZE                  (1 << FILE_HASH_BITS)
 713=20
 714 static unsigned int file_hashval(const struct svc_fh *fh)
 715 {
 716         struct inode *inode =3D d_inode(fh->fh_dentry);
 717=20
 718         /* XXX: why not (here & in file cache) use inode? */
 719         return (unsigned int)hash_long(inode->i_ino, FILE_HASH_BITS);
 720 }
 721=20
 722 static struct hlist_head file_hashtbl[FILE_HASH_SIZE];

256 buckets is the size of the existing file_hashtbl.


>  Would PAGE_SIZE/sizeof(void*) make more sense
> (though that is 512).

For rhashtable, you need to account for sizeof(struct bucket_table),
if I'm reading nested_bucket_table_alloc() correctly.

256 is 2048 bytes + sizeof(struct bucket_table). 512 buckets would
push us over a page.


> How much churn is too much?  The default is 4 and we grow at >75% and
> shrink at <30%, so at 4 entries the table would resize to 8, and that 2
> entries it  would shrink back.  That does sound like churn.
> If we choose 8, then at 7 we grow to 16 and at 4 we go back to 8.
> If we choose 16, then at 13 we grow to 32 and at 9 we go back to 16.
>=20
> If we choose 64, then at 49 we grow to 128 and at 39 we go back to 64.
>=20
> The margin seems rather narrow.  May 30% is too high - 15% might be a
> lot better.  But changing that might be difficult.

I could go a little smaller. Basically table resizing is OK when we're
talking about a lot of buckets because that overhead is very likely to
be amortized over many insertions and removals.


> So I don't have a good recommendation, but I don't like magic numbers.
> Maybe PAGE_SIZE/sizeof(void*) ??

The only thing I can think of would be

#define NFS4_FILE_HASH_SIZE  (some number or constant calculation)

which to me isn't much better than

	.size	=3D 256,	/* buckets */

I will ponder some more.


> But either way
>  Reviewed-by: NeilBrown <neilb@suse.de>
>=20
> Thanks,
> NeilBrown
>=20
>=20
>> +	.automatic_shrinking	=3D true,
>> +};
>> +
>> /*
>>  * Check if courtesy clients have conflicting access and resolve it if p=
ossible
>>  *
>> @@ -8023,10 +8037,16 @@ nfs4_state_start(void)
>> {
>> 	int ret;
>>=20
>> -	ret =3D nfsd4_create_callback_queue();
>> +	ret =3D rhltable_init(&nfs4_file_rhltable, &nfs4_file_rhash_params);
>> 	if (ret)
>> 		return ret;
>>=20
>> +	ret =3D nfsd4_create_callback_queue();
>> +	if (ret) {
>> +		rhltable_destroy(&nfs4_file_rhltable);
>> +		return ret;
>> +	}
>> +
>> 	set_max_delegations();
>> 	return 0;
>> }
>> @@ -8057,6 +8077,7 @@ nfs4_state_shutdown_net(struct net *net)
>>=20
>> 	nfsd4_client_tracking_exit(net);
>> 	nfs4_state_destroy_net(net);
>> +	rhltable_destroy(&nfs4_file_rhltable);
>> #ifdef CONFIG_NFSD_V4_2_INTER_SSC
>> 	nfsd4_ssc_shutdown_umount(nn);
>> #endif
>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
>> index e2daef3cc003..190fc7e418a4 100644
>> --- a/fs/nfsd/state.h
>> +++ b/fs/nfsd/state.h
>> @@ -546,6 +546,7 @@ struct nfs4_file {
>> 	bool			fi_aliased;
>> 	spinlock_t		fi_lock;
>> 	struct hlist_node       fi_hash;	/* hash on fi_fhandle */
>> +	struct rhlist_head	fi_rlist;
>> 	struct list_head        fi_stateids;
>> 	union {
>> 		struct list_head	fi_delegations;
>>=20
>>=20
>>=20

--
Chuck Lever