On Thu, 2023-01-05 at 09:58 -0500, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@xxxxxxxxxx>
>
> While we were converting the nfs4_file hashtable to use the kernel's
> resizable hashtable data structure, Neil Brown observed that the
> list variant (rhltable) would be better for managing nfsd_file items
> as well. The nfsd_file hash table will contain multiple entries for
> the same inode -- these should be kept together on a list. And, it
> could be possible for exotic or malicious client behavior to cause
> the hash table to resize itself on every insertion.
>
> A nice simplification is that rhltable_lookup() can return a list
> that contains only nfsd_file items that match a given inode, which
> enables us to eliminate specialized hash table helper functions and
> use the default functions provided by the rhashtable implementation).
>
> Since we are now storing nfsd_file items for the same inode on a
> single list, that effectively reduces the number of hash entries
> that have to be tracked in the hash table. The mininum bucket count
> is therefore lowered.
>
> Suggested-by: Neil Brown <neilb@xxxxxxx>
> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
> ---
> fs/nfsd/filecache.c | 289 +++++++++++++++++++--------------------------------
> fs/nfsd/filecache.h | 9 +-
> 2 files changed, 115 insertions(+), 183 deletions(-)
>
> Just to note that this work is still in the wings. It would need to
> be rebased on Jeff's recent fixes and clean-ups. I'm reluctant to
> apply this until there is more evidence that the instability in v6.0
> has been duly addressed.
>
>
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index 45b2c9e3f636..f04e722bb6bc 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -74,70 +74,9 @@ static struct list_lru nfsd_file_lru;
> static unsigned long nfsd_file_flags;
> static struct fsnotify_group *nfsd_file_fsnotify_group;
> static struct delayed_work nfsd_filecache_laundrette;
> -static struct rhashtable nfsd_file_rhash_tbl
> +static struct rhltable nfsd_file_rhltable
> ____cacheline_aligned_in_smp;
>
> -enum nfsd_file_lookup_type {
> - NFSD_FILE_KEY_INODE,
> - NFSD_FILE_KEY_FULL,
> -};
> -
> -struct nfsd_file_lookup_key {
> - struct inode *inode;
> - struct net *net;
> - const struct cred *cred;
> - unsigned char need;
> - bool gc;
> - enum nfsd_file_lookup_type type;
> -};
> -
> -/*
> - * The returned hash value is based solely on the address of an in-code
> - * inode, a pointer to a slab-allocated object. The entropy in such a
> - * pointer is concentrated in its middle bits.
> - */
> -static u32 nfsd_file_inode_hash(const struct inode *inode, u32 seed)
> -{
> - unsigned long ptr = (unsigned long)inode;
> - u32 k;
> -
> - k = ptr >> L1_CACHE_SHIFT;
> - k &= 0x00ffffff;
> - return jhash2(&k, 1, seed);
> -}
> -
> -/**
> - * nfsd_file_key_hashfn - Compute the hash value of a lookup key
> - * @data: key on which to compute the hash value
> - * @len: rhash table's key_len parameter (unused)
> - * @seed: rhash table's random seed of the day
> - *
> - * Return value:
> - * Computed 32-bit hash value
> - */
> -static u32 nfsd_file_key_hashfn(const void *data, u32 len, u32 seed)
> -{
> - const struct nfsd_file_lookup_key *key = data;
> -
> - return nfsd_file_inode_hash(key->inode, seed);
> -}
> -
> -/**
> - * nfsd_file_obj_hashfn - Compute the hash value of an nfsd_file
> - * @data: object on which to compute the hash value
> - * @len: rhash table's key_len parameter (unused)
> - * @seed: rhash table's random seed of the day
> - *
> - * Return value:
> - * Computed 32-bit hash value
> - */
> -static u32 nfsd_file_obj_hashfn(const void *data, u32 len, u32 seed)
> -{
> - const struct nfsd_file *nf = data;
> -
> - return nfsd_file_inode_hash(nf->nf_inode, seed);
> -}
> -
> static bool
> nfsd_match_cred(const struct cred *c1, const struct cred *c2)
> {
> @@ -158,53 +97,16 @@ nfsd_match_cred(const struct cred *c1, const struct cred *c2)
> return true;
> }
>
> -/**
> - * nfsd_file_obj_cmpfn - Match a cache item against search criteria
> - * @arg: search criteria
> - * @ptr: cache item to check
> - *
> - * Return values:
> - * %0 - Item matches search criteria
> - * %1 - Item does not match search criteria
> - */
> -static int nfsd_file_obj_cmpfn(struct rhashtable_compare_arg *arg,
> - const void *ptr)
> -{
> - const struct nfsd_file_lookup_key *key = arg->key;
> - const struct nfsd_file *nf = ptr;
> -
> - switch (key->type) {
> - case NFSD_FILE_KEY_INODE:
> - if (nf->nf_inode != key->inode)
> - return 1;
> - break;
> - case NFSD_FILE_KEY_FULL:
> - if (nf->nf_inode != key->inode)
> - return 1;
> - if (nf->nf_may != key->need)
> - return 1;
> - if (nf->nf_net != key->net)
> - return 1;
> - if (!nfsd_match_cred(nf->nf_cred, key->cred))
> - return 1;
> - if (!!test_bit(NFSD_FILE_GC, &nf->nf_flags) != key->gc)
> - return 1;
> - if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags) == 0)
> - return 1;
> - break;
> - }
> - return 0;
> -}
> -
> static const struct rhashtable_params nfsd_file_rhash_params = {
> .key_len = sizeof_field(struct nfsd_file, nf_inode),
> .key_offset = offsetof(struct nfsd_file, nf_inode),
> - .head_offset = offsetof(struct nfsd_file, nf_rhash),
> - .hashfn = nfsd_file_key_hashfn,
> - .obj_hashfn = nfsd_file_obj_hashfn,
> - .obj_cmpfn = nfsd_file_obj_cmpfn,
> - /* Reduce resizing churn on light workloads */
> - .min_size = 512, /* buckets */
> + .head_offset = offsetof(struct nfsd_file, nf_rlist),
> +
> + /*
> + * Start with a single page hash table to reduce resizing churn
> + * on light workloads.
> + */
> + .min_size = 256,
> .automatic_shrinking = true,
> };
>
> @@ -307,27 +209,27 @@ nfsd_file_mark_find_or_create(struct nfsd_file *nf, struct inode *inode)
> }
>
> static struct nfsd_file *
> -nfsd_file_alloc(struct nfsd_file_lookup_key *key, unsigned int may)
> +nfsd_file_alloc(struct net *net, struct inode *inode, unsigned char need,
> + bool want_gc)
> {
> struct nfsd_file *nf;
>
> nf = kmem_cache_alloc(nfsd_file_slab, GFP_KERNEL);
> - if (nf) {
> - INIT_LIST_HEAD(&nf->nf_lru);
> - nf->nf_birthtime = ktime_get();
> - nf->nf_file = NULL;
> - nf->nf_cred = get_current_cred();
> - nf->nf_net = key->net;
> - nf->nf_flags = 0;
> - __set_bit(NFSD_FILE_HASHED, &nf->nf_flags);
> - __set_bit(NFSD_FILE_PENDING, &nf->nf_flags);
> - if (key->gc)
> - __set_bit(NFSD_FILE_GC, &nf->nf_flags);
> - nf->nf_inode = key->inode;
> - refcount_set(&nf->nf_ref, 1);
> - nf->nf_may = key->need;
> - nf->nf_mark = NULL;
> - }
> + if (unlikely(!nf))
> + return NULL;
> +
> + INIT_LIST_HEAD(&nf->nf_lru);
> + nf->nf_birthtime = ktime_get();
> + nf->nf_file = NULL;
> + nf->nf_cred = get_current_cred();
> + nf->nf_net = net;
> + nf->nf_flags = want_gc ?
> + BIT(NFSD_FILE_HASHED) | BIT(NFSD_FILE_PENDING) | BIT(NFSD_FILE_GC) :
> + BIT(NFSD_FILE_HASHED) | BIT(NFSD_FILE_PENDING);
> + nf->nf_inode = inode;
> + refcount_set(&nf->nf_ref, 1);
> + nf->nf_may = need;
> + nf->nf_mark = NULL;
> return nf;
> }
>
> @@ -362,8 +264,8 @@ nfsd_file_hash_remove(struct nfsd_file *nf)
>
> if (nfsd_file_check_write_error(nf))
> nfsd_reset_write_verifier(net_generic(nf->nf_net, nfsd_net_id));
> - rhashtable_remove_fast(&nfsd_file_rhash_tbl, &nf->nf_rhash,
> - nfsd_file_rhash_params);
> + rhltable_remove(&nfsd_file_rhltable, &nf->nf_rlist,
> + nfsd_file_rhash_params);
> }
>
> static bool
> @@ -680,21 +582,19 @@ static struct shrinker nfsd_file_shrinker = {
> static void
> nfsd_file_queue_for_close(struct inode *inode, struct list_head *dispose)
> {
> - struct nfsd_file_lookup_key key = {
> - .type = NFSD_FILE_KEY_INODE,
> - .inode = inode,
> - };
> - struct nfsd_file *nf;
> -
> rcu_read_lock();
> do {
> + struct rhlist_head *list;
> + struct nfsd_file *nf;
> int decrement = 1;
>
> - nf = rhashtable_lookup(&nfsd_file_rhash_tbl, &key,
> + list = rhltable_lookup(&nfsd_file_rhltable, &inode,
> nfsd_file_rhash_params);
> - if (!nf)
> + if (!list)
> break;
>
Rather than redriving the lookup multiple times in the loop, would it be
better to just search once and then walk the resulting list with
rhl_for_each_entry_rcu, all while holding the rcu_read_lock?
> + nf = container_of(list, struct nfsd_file, nf_rlist);
> +
> /* If we raced with someone else unhashing, ignore it */
> if (!nfsd_file_unhash(nf))
> continue;
> @@ -836,7 +736,7 @@ nfsd_file_cache_init(void)
> if (test_and_set_bit(NFSD_FILE_CACHE_UP, &nfsd_file_flags) == 1)
> return 0;
>
> - ret = rhashtable_init(&nfsd_file_rhash_tbl, &nfsd_file_rhash_params);
> + ret = rhltable_init(&nfsd_file_rhltable, &nfsd_file_rhash_params);
> if (ret)
> return ret;
>
> @@ -904,7 +804,7 @@ nfsd_file_cache_init(void)
> nfsd_file_mark_slab = NULL;
> destroy_workqueue(nfsd_filecache_wq);
> nfsd_filecache_wq = NULL;
> - rhashtable_destroy(&nfsd_file_rhash_tbl);
> + rhltable_destroy(&nfsd_file_rhltable);
> goto out;
> }
>
> @@ -922,7 +822,7 @@ __nfsd_file_cache_purge(struct net *net)
> struct nfsd_file *nf;
> LIST_HEAD(dispose);
>
> - rhashtable_walk_enter(&nfsd_file_rhash_tbl, &iter);
> + rhltable_walk_enter(&nfsd_file_rhltable, &iter);
> do {
> rhashtable_walk_start(&iter);
>
> @@ -1031,7 +931,7 @@ nfsd_file_cache_shutdown(void)
> nfsd_file_mark_slab = NULL;
> destroy_workqueue(nfsd_filecache_wq);
> nfsd_filecache_wq = NULL;
> - rhashtable_destroy(&nfsd_file_rhash_tbl);
> + rhltable_destroy(&nfsd_file_rhltable);
>
> for_each_possible_cpu(i) {
> per_cpu(nfsd_file_cache_hits, i) = 0;
> @@ -1042,6 +942,36 @@ nfsd_file_cache_shutdown(void)
> }
> }
>
> +static struct nfsd_file *
> +nfsd_file_lookup_locked(const struct net *net, const struct cred *cred,
> + struct inode *inode, unsigned char need,
> + bool want_gc)
> +{
> + struct rhlist_head *tmp, *list;
> + struct nfsd_file *nf;
> +
> + list = rhltable_lookup(&nfsd_file_rhltable, &inode,
> + nfsd_file_rhash_params);
> + rhl_for_each_entry_rcu(nf, tmp, list, nf_rlist) {
> + if (nf->nf_may != need)
> + continue;
> + if (nf->nf_net != net)
> + continue;
> + if (!nfsd_match_cred(nf->nf_cred, cred))
> + continue;
> + if (!!test_bit(NFSD_FILE_GC, &nf->nf_flags) != want_gc)
> + continue;
> + if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags) == 0)
> + continue;
> +
> + /* If it was on the LRU, reuse that reference. */
> + if (nfsd_file_lru_remove(nf))
> + WARN_ON_ONCE(refcount_dec_and_test(&nf->nf_ref));
> + return nf;
> + }
> + return NULL;
> +}
> +
> /**
> * nfsd_file_is_cached - are there any cached open files for this inode?
> * @inode: inode to check
> @@ -1056,15 +986,12 @@ nfsd_file_cache_shutdown(void)
> bool
> nfsd_file_is_cached(struct inode *inode)
> {
> - struct nfsd_file_lookup_key key = {
> - .type = NFSD_FILE_KEY_INODE,
> - .inode = inode,
> - };
> - bool ret = false;
> -
> - if (rhashtable_lookup_fast(&nfsd_file_rhash_tbl, &key,
> - nfsd_file_rhash_params) != NULL)
> - ret = true;
> + bool ret;
> +
> + rcu_read_lock();
> + ret = (rhltable_lookup(&nfsd_file_rhltable, &inode,
> + nfsd_file_rhash_params) != NULL);
> + rcu_read_unlock();
> trace_nfsd_file_is_cached(inode, (int)ret);
> return ret;
> }
> @@ -1074,14 +1001,12 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> unsigned int may_flags, struct nfsd_file **pnf,
> bool open, bool want_gc)
> {
> - struct nfsd_file_lookup_key key = {
> - .type = NFSD_FILE_KEY_FULL,
> - .need = may_flags & NFSD_FILE_MAY_MASK,
> - .net = SVC_NET(rqstp),
> - .gc = want_gc,
> - };
> + unsigned char need = may_flags & NFSD_FILE_MAY_MASK;
> + struct net *net = SVC_NET(rqstp);
> + struct nfsd_file *new, *nf;
> + const struct cred *cred;
> bool open_retry = true;
> - struct nfsd_file *nf;
> + struct inode *inode;
> __be32 status;
> int ret;
>
> @@ -1089,32 +1014,38 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> may_flags|NFSD_MAY_OWNER_OVERRIDE);
> if (status != nfs_ok)
> return status;
> - key.inode = d_inode(fhp->fh_dentry);
> - key.cred = get_current_cred();
> + inode = d_inode(fhp->fh_dentry);
> + cred = get_current_cred();
>
> retry:
> - rcu_read_lock();
> - nf = rhashtable_lookup(&nfsd_file_rhash_tbl, &key,
> - nfsd_file_rhash_params);
> - if (nf)
> - nf = nfsd_file_get(nf);
> - rcu_read_unlock();
> -
> - if (nf) {
> - if (nfsd_file_lru_remove(nf))
> - WARN_ON_ONCE(refcount_dec_and_test(&nf->nf_ref));
> - goto wait_for_construction;
> + if (open) {
> + rcu_read_lock();
> + nf = nfsd_file_lookup_locked(net, cred, inode, need, want_gc);
> + rcu_read_unlock();
> + if (nf)
> + goto wait_for_construction;
> }
>
> - nf = nfsd_file_alloc(&key, may_flags);
> - if (!nf) {
> + new = nfsd_file_alloc(net, inode, need, want_gc);
> + if (!new) {
> status = nfserr_jukebox;
> goto out_status;
> }
> + rcu_read_lock();
> + spin_lock(&inode->i_lock);
> + nf = nfsd_file_lookup_locked(net, cred, inode, need, want_gc);
> + if (unlikely(nf)) {
> + spin_unlock(&inode->i_lock);
> + rcu_read_unlock();
> + nfsd_file_slab_free(&new->nf_rcu);
> + goto wait_for_construction;
> + }
> + nf = new;
> + ret = rhltable_insert(&nfsd_file_rhltable, &nf->nf_rlist,
> + nfsd_file_rhash_params);
> + spin_unlock(&inode->i_lock);
> + rcu_read_unlock();
>
> - ret = rhashtable_lookup_insert_key(&nfsd_file_rhash_tbl,
> - &key, &nf->nf_rhash,
> - nfsd_file_rhash_params);
> if (likely(ret == 0))
> goto open_file;
>
> @@ -1122,7 +1053,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> nf = NULL;
> if (ret == -EEXIST)
> goto retry;
> - trace_nfsd_file_insert_err(rqstp, key.inode, may_flags, ret);
> + trace_nfsd_file_insert_err(rqstp, inode, may_flags, ret);
> status = nfserr_jukebox;
> goto out_status;
>
> @@ -1131,7 +1062,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
>
> /* Did construction of this file fail? */
> if (!test_bit(NFSD_FILE_HASHED, &nf->nf_flags)) {
> - trace_nfsd_file_cons_err(rqstp, key.inode, may_flags, nf);
> + trace_nfsd_file_cons_err(rqstp, inode, may_flags, nf);
> if (!open_retry) {
> status = nfserr_jukebox;
> goto out;
> @@ -1157,14 +1088,14 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> }
>
> out_status:
> - put_cred(key.cred);
> + put_cred(cred);
> if (open)
> - trace_nfsd_file_acquire(rqstp, key.inode, may_flags, nf, status);
> + trace_nfsd_file_acquire(rqstp, inode, may_flags, nf, status);
> return status;
>
> open_file:
> trace_nfsd_file_alloc(nf);
> - nf->nf_mark = nfsd_file_mark_find_or_create(nf, key.inode);
> + nf->nf_mark = nfsd_file_mark_find_or_create(nf, inode);
> if (nf->nf_mark) {
> if (open) {
> status = nfsd_open_verified(rqstp, fhp, may_flags,
> @@ -1178,7 +1109,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> * If construction failed, or we raced with a call to unlink()
> * then unhash.
> */
> - if (status == nfs_ok && key.inode->i_nlink == 0)
> + if (status != nfs_ok || inode->i_nlink == 0)
> status = nfserr_jukebox;
> if (status != nfs_ok)
> nfsd_file_unhash(nf);
> @@ -1273,7 +1204,7 @@ int nfsd_file_cache_stats_show(struct seq_file *m, void *v)
> lru = list_lru_count(&nfsd_file_lru);
>
> rcu_read_lock();
> - ht = &nfsd_file_rhash_tbl;
> + ht = &nfsd_file_rhltable.ht;
> count = atomic_read(&ht->nelems);
> tbl = rht_dereference_rcu(ht->tbl, ht);
> buckets = tbl->size;
> @@ -1289,7 +1220,7 @@ int nfsd_file_cache_stats_show(struct seq_file *m, void *v)
> evictions += per_cpu(nfsd_file_evictions, i);
> }
>
> - seq_printf(m, "total entries: %u\n", count);
> + seq_printf(m, "total inodes: %u\n", count);
> seq_printf(m, "hash buckets: %u\n", buckets);
> seq_printf(m, "lru entries: %lu\n", lru);
> seq_printf(m, "cache hits: %lu\n", hits);
> diff --git a/fs/nfsd/filecache.h b/fs/nfsd/filecache.h
> index b7efb2c3ddb1..7d3b35771565 100644
> --- a/fs/nfsd/filecache.h
> +++ b/fs/nfsd/filecache.h
> @@ -29,9 +29,8 @@ struct nfsd_file_mark {
> * never be dereferenced, only used for comparison.
> */
> struct nfsd_file {
> - struct rhash_head nf_rhash;
> - struct list_head nf_lru;
> - struct rcu_head nf_rcu;
> + struct rhlist_head nf_rlist;
> + void *nf_inode;
> struct file *nf_file;
> const struct cred *nf_cred;
> struct net *nf_net;
> @@ -40,10 +39,12 @@ struct nfsd_file {
> #define NFSD_FILE_REFERENCED (2)
> #define NFSD_FILE_GC (3)
> unsigned long nf_flags;
> - struct inode *nf_inode; /* don't deref */
> refcount_t nf_ref;
> unsigned char nf_may;
> +
> struct nfsd_file_mark *nf_mark;
> + struct list_head nf_lru;
> + struct rcu_head nf_rcu;
> ktime_t nf_birthtime;
> };
>
>
>
--
Jeff Layton <jlayton@xxxxxxxxxx>
