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.
>
> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
> ---
> fs/nfsd/nfs4state.c | 23 ++++++++++++++++++++++-
> fs/nfsd/state.h | 1 +
> 2 files changed, 23 insertions(+), 1 deletion(-)
>
> 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_fh *fh)
>
> static struct hlist_head file_hashtbl[FILE_HASH_SIZE];
>
> +static struct rhltable nfs4_file_rhltable ____cacheline_aligned_in_smp;
> +
> +static const struct rhashtable_params nfs4_file_rhash_params = {
> + .key_len = sizeof_field(struct nfs4_file, fi_inode),
> + .key_offset = offsetof(struct nfs4_file, fi_inode),
> + .head_offset = offsetof(struct nfs4_file, fi_rlist),
> +
> + /* Reduce resizing churn on light workloads */
> + .min_size = 256, /* buckets */
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.
Where did 256 come from? Would PAGE_SIZE/sizeof(void*) make more sense
(though that is 512).
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.
If we choose 64, then at 49 we grow to 128 and at 39 we go back to 64.
The margin seems rather narrow. May 30% is too high - 15% might be a
lot better. But changing that might be difficult.
So I don't have a good recommendation, but I don't like magic numbers.
Maybe PAGE_SIZE/sizeof(void*) ??
But either way
Reviewed-by: NeilBrown <neilb@xxxxxxx>
Thanks,
NeilBrown
> + .automatic_shrinking = true,
> +};
> +
> /*
> * Check if courtesy clients have conflicting access and resolve it if possible
> *
> @@ -8023,10 +8037,16 @@ nfs4_state_start(void)
> {
> int ret;
>
> - ret = nfsd4_create_callback_queue();
> + ret = rhltable_init(&nfs4_file_rhltable, &nfs4_file_rhash_params);
> if (ret)
> return ret;
>
> + ret = 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)
>
> 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;
>
>
>
