Backport of upstream fixes to NFSD's duplicate reply cache. These have been hand-applied and tested with the same reproducer as was used to create the upstream fixes.
---
Chuck Lever (8): NFSD: Refactor nfsd_reply_cache_free_locked() NFSD: Rename nfsd_reply_cache_alloc() NFSD: Replace nfsd_prune_bucket() NFSD: Refactor the duplicate reply cache shrinker NFSD: Remove svc_rqst::rq_cacherep NFSD: Rename struct svc_cacherep NFSD: Fix "start of NFS reply" pointer passed to nfsd_cache_update() NFSD: Fix checksum mismatches in the duplicate reply cache
fs/nfsd/cache.h | 8 +- fs/nfsd/nfscache.c | 266 ++++++++++++++++++++++++------------- fs/nfsd/nfssvc.c | 20 ++- fs/nfsd/trace.h | 26 +++- include/linux/sunrpc/svc.h | 1 - 5 files changed, 218 insertions(+), 103 deletions(-)
-- Chuck Lever
From: Chuck Lever chuck.lever@oracle.com
[ Upstream commit 35308e7f0fc3942edc87d9c6dc78c4a096428957 ]
To reduce contention on the bucket locks, we must avoid calling kfree() while each bucket lock is held.
Start by refactoring nfsd_reply_cache_free_locked() into a helper that removes an entry from the bucket (and must therefore run under the lock) and a second helper that frees the entry (which does not need to hold the lock).
For readability, rename the helpers nfsd_cacherep_<verb>.
Reviewed-by: Jeff Layton jlayton@kernel.org Signed-off-by: Chuck Lever chuck.lever@oracle.com --- fs/nfsd/nfscache.c | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-)
diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c index 1ad4f30d5f85..e197756b5454 100644 --- a/fs/nfsd/nfscache.c +++ b/fs/nfsd/nfscache.c @@ -110,21 +110,33 @@ nfsd_reply_cache_alloc(struct svc_rqst *rqstp, __wsum csum, return rp; }
+static void nfsd_cacherep_free(struct svc_cacherep *rp) +{ + if (rp->c_type == RC_REPLBUFF) + kfree(rp->c_replvec.iov_base); + kmem_cache_free(drc_slab, rp); +} + static void -nfsd_reply_cache_free_locked(struct nfsd_drc_bucket *b, struct svc_cacherep *rp, - struct nfsd_net *nn) +nfsd_cacherep_unlink_locked(struct nfsd_net *nn, struct nfsd_drc_bucket *b, + struct svc_cacherep *rp) { - if (rp->c_type == RC_REPLBUFF && rp->c_replvec.iov_base) { + if (rp->c_type == RC_REPLBUFF && rp->c_replvec.iov_base) nfsd_stats_drc_mem_usage_sub(nn, rp->c_replvec.iov_len); - kfree(rp->c_replvec.iov_base); - } if (rp->c_state != RC_UNUSED) { rb_erase(&rp->c_node, &b->rb_head); list_del(&rp->c_lru); atomic_dec(&nn->num_drc_entries); nfsd_stats_drc_mem_usage_sub(nn, sizeof(*rp)); } - kmem_cache_free(drc_slab, rp); +} + +static void +nfsd_reply_cache_free_locked(struct nfsd_drc_bucket *b, struct svc_cacherep *rp, + struct nfsd_net *nn) +{ + nfsd_cacherep_unlink_locked(nn, b, rp); + nfsd_cacherep_free(rp); }
static void @@ -132,8 +144,9 @@ nfsd_reply_cache_free(struct nfsd_drc_bucket *b, struct svc_cacherep *rp, struct nfsd_net *nn) { spin_lock(&b->cache_lock); - nfsd_reply_cache_free_locked(b, rp, nn); + nfsd_cacherep_unlink_locked(nn, b, rp); spin_unlock(&b->cache_lock); + nfsd_cacherep_free(rp); }
int nfsd_drc_slab_create(void)
Hi,
Thanks for your patch.
FYI: kernel test robot notices the stable kernel rule is not satisfied.
The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#opti...
Rule: add the tag "Cc: stable@vger.kernel.org" in the sign-off area to have the patch automatically included in the stable tree. Subject: [PATCH 1/8] NFSD: Refactor nfsd_reply_cache_free_locked() Link: https://lore.kernel.org/stable/170120877882.1515.3190174495040446432.stgit%4...
From: Chuck Lever chuck.lever@oracle.com
[ Upstream commit ff0d169329768c1102b7b07eebe5a9839aa1c143 ]
For readability, rename to match the other helpers.
Reviewed-by: Jeff Layton jlayton@kernel.org Signed-off-by: Chuck Lever chuck.lever@oracle.com --- fs/nfsd/nfscache.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c index e197756b5454..40b8bbfc0950 100644 --- a/fs/nfsd/nfscache.c +++ b/fs/nfsd/nfscache.c @@ -85,8 +85,8 @@ nfsd_hashsize(unsigned int limit) }
static struct svc_cacherep * -nfsd_reply_cache_alloc(struct svc_rqst *rqstp, __wsum csum, - struct nfsd_net *nn) +nfsd_cacherep_alloc(struct svc_rqst *rqstp, __wsum csum, + struct nfsd_net *nn) { struct svc_cacherep *rp;
@@ -458,7 +458,7 @@ int nfsd_cache_lookup(struct svc_rqst *rqstp) * preallocate an entry. */ nn = net_generic(SVC_NET(rqstp), nfsd_net_id); - rp = nfsd_reply_cache_alloc(rqstp, csum, nn); + rp = nfsd_cacherep_alloc(rqstp, csum, nn); if (!rp) goto out;
From: Chuck Lever chuck.lever@oracle.com
[ Upstream commit a9507f6af1450ed26a4a36d979af518f5bb21e5d ]
Enable nfsd_prune_bucket() to drop the bucket lock while calling kfree(). Use the same pattern that Jeff recently introduced in the NFSD filecache.
A few percpu operations are moved outside the lock since they temporarily disable local IRQs which is expensive and does not need to be done while the lock is held.
Reviewed-by: Jeff Layton jlayton@kernel.org Signed-off-by: Chuck Lever chuck.lever@oracle.com --- fs/nfsd/nfscache.c | 78 ++++++++++++++++++++++++++++++++++++++++++---------- fs/nfsd/trace.h | 22 +++++++++++++++ 2 files changed, 85 insertions(+), 15 deletions(-)
diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c index 40b8bbfc0950..9dfa2c03b712 100644 --- a/fs/nfsd/nfscache.c +++ b/fs/nfsd/nfscache.c @@ -117,6 +117,21 @@ static void nfsd_cacherep_free(struct svc_cacherep *rp) kmem_cache_free(drc_slab, rp); }
+static unsigned long +nfsd_cacherep_dispose(struct list_head *dispose) +{ + struct svc_cacherep *rp; + unsigned long freed = 0; + + while (!list_empty(dispose)) { + rp = list_first_entry(dispose, struct svc_cacherep, c_lru); + list_del(&rp->c_lru); + nfsd_cacherep_free(rp); + freed++; + } + return freed; +} + static void nfsd_cacherep_unlink_locked(struct nfsd_net *nn, struct nfsd_drc_bucket *b, struct svc_cacherep *rp) @@ -260,6 +275,41 @@ nfsd_cache_bucket_find(__be32 xid, struct nfsd_net *nn) return &nn->drc_hashtbl[hash]; }
+/* + * Remove and return no more than @max expired entries in bucket @b. + * If @max is zero, do not limit the number of removed entries. + */ +static void +nfsd_prune_bucket_locked(struct nfsd_net *nn, struct nfsd_drc_bucket *b, + unsigned int max, struct list_head *dispose) +{ + unsigned long expiry = jiffies - RC_EXPIRE; + struct svc_cacherep *rp, *tmp; + unsigned int freed = 0; + + lockdep_assert_held(&b->cache_lock); + + /* The bucket LRU is ordered oldest-first. */ + list_for_each_entry_safe(rp, tmp, &b->lru_head, c_lru) { + /* + * Don't free entries attached to calls that are still + * in-progress, but do keep scanning the list. + */ + if (rp->c_state == RC_INPROG) + continue; + + if (atomic_read(&nn->num_drc_entries) <= nn->max_drc_entries && + time_before(expiry, rp->c_timestamp)) + break; + + nfsd_cacherep_unlink_locked(nn, b, rp); + list_add(&rp->c_lru, dispose); + + if (max && ++freed > max) + break; + } +} + static long prune_bucket(struct nfsd_drc_bucket *b, struct nfsd_net *nn, unsigned int max) { @@ -283,11 +333,6 @@ static long prune_bucket(struct nfsd_drc_bucket *b, struct nfsd_net *nn, return freed; }
-static long nfsd_prune_bucket(struct nfsd_drc_bucket *b, struct nfsd_net *nn) -{ - return prune_bucket(b, nn, 3); -} - /* * Walk the LRU list and prune off entries that are older than RC_EXPIRE. * Also prune the oldest ones when the total exceeds the max number of entries. @@ -443,6 +488,8 @@ int nfsd_cache_lookup(struct svc_rqst *rqstp) __wsum csum; struct nfsd_drc_bucket *b; int type = rqstp->rq_cachetype; + unsigned long freed; + LIST_HEAD(dispose); int rtn = RC_DOIT;
rqstp->rq_cacherep = NULL; @@ -467,20 +514,18 @@ int nfsd_cache_lookup(struct svc_rqst *rqstp) found = nfsd_cache_insert(b, rp, nn); if (found != rp) goto found_entry; - - nfsd_stats_rc_misses_inc(); rqstp->rq_cacherep = rp; rp->c_state = RC_INPROG; + nfsd_prune_bucket_locked(nn, b, 3, &dispose); + spin_unlock(&b->cache_lock);
+ freed = nfsd_cacherep_dispose(&dispose); + trace_nfsd_drc_gc(nn, freed); + + nfsd_stats_rc_misses_inc(); atomic_inc(&nn->num_drc_entries); nfsd_stats_drc_mem_usage_add(nn, sizeof(*rp)); - - nfsd_prune_bucket(b, nn); - -out_unlock: - spin_unlock(&b->cache_lock); -out: - return rtn; + goto out;
found_entry: /* We found a matching entry which is either in progress or done. */ @@ -518,7 +563,10 @@ int nfsd_cache_lookup(struct svc_rqst *rqstp)
out_trace: trace_nfsd_drc_found(nn, rqstp, rtn); - goto out_unlock; +out_unlock: + spin_unlock(&b->cache_lock); +out: + return rtn; }
/** diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h index 2af74983f146..c06c505d04fb 100644 --- a/fs/nfsd/trace.h +++ b/fs/nfsd/trace.h @@ -1261,6 +1261,28 @@ TRACE_EVENT(nfsd_drc_mismatch, __entry->ingress) );
+TRACE_EVENT_CONDITION(nfsd_drc_gc, + TP_PROTO( + const struct nfsd_net *nn, + unsigned long freed + ), + TP_ARGS(nn, freed), + TP_CONDITION(freed > 0), + TP_STRUCT__entry( + __field(unsigned long long, boot_time) + __field(unsigned long, freed) + __field(int, total) + ), + TP_fast_assign( + __entry->boot_time = nn->boot_time; + __entry->freed = freed; + __entry->total = atomic_read(&nn->num_drc_entries); + ), + TP_printk("boot_time=%16llx total=%d freed=%lu", + __entry->boot_time, __entry->total, __entry->freed + ) +); + TRACE_EVENT(nfsd_cb_args, TP_PROTO( const struct nfs4_client *clp,
From: Chuck Lever chuck.lever@oracle.com
[ Upstream commit c135e1269f34dfdea4bd94c11060c83a3c0b3c12 ]
Avoid holding the bucket lock while freeing cache entries. This change also caps the number of entries that are freed when the shrinker calls to reduce the shrinker's impact on the cache's effectiveness.
Reviewed-by: Jeff Layton jlayton@kernel.org Signed-off-by: Chuck Lever chuck.lever@oracle.com --- fs/nfsd/nfscache.c | 82 +++++++++++++++++++++++++--------------------------- 1 file changed, 39 insertions(+), 43 deletions(-)
diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c index 9dfa2c03b712..8aab82ca798b 100644 --- a/fs/nfsd/nfscache.c +++ b/fs/nfsd/nfscache.c @@ -310,68 +310,64 @@ nfsd_prune_bucket_locked(struct nfsd_net *nn, struct nfsd_drc_bucket *b, } }
-static long prune_bucket(struct nfsd_drc_bucket *b, struct nfsd_net *nn, - unsigned int max) +/** + * nfsd_reply_cache_count - count_objects method for the DRC shrinker + * @shrink: our registered shrinker context + * @sc: garbage collection parameters + * + * Returns the total number of entries in the duplicate reply cache. To + * keep things simple and quick, this is not the number of expired entries + * in the cache (ie, the number that would be removed by a call to + * nfsd_reply_cache_scan). + */ +static unsigned long +nfsd_reply_cache_count(struct shrinker *shrink, struct shrink_control *sc) { - struct svc_cacherep *rp, *tmp; - long freed = 0; + struct nfsd_net *nn = container_of(shrink, + struct nfsd_net, nfsd_reply_cache_shrinker);
- list_for_each_entry_safe(rp, tmp, &b->lru_head, c_lru) { - /* - * Don't free entries attached to calls that are still - * in-progress, but do keep scanning the list. - */ - if (rp->c_state == RC_INPROG) - continue; - if (atomic_read(&nn->num_drc_entries) <= nn->max_drc_entries && - time_before(jiffies, rp->c_timestamp + RC_EXPIRE)) - break; - nfsd_reply_cache_free_locked(b, rp, nn); - if (max && freed++ > max) - break; - } - return freed; + return atomic_read(&nn->num_drc_entries); }
-/* - * Walk the LRU list and prune off entries that are older than RC_EXPIRE. - * Also prune the oldest ones when the total exceeds the max number of entries. +/** + * nfsd_reply_cache_scan - scan_objects method for the DRC shrinker + * @shrink: our registered shrinker context + * @sc: garbage collection parameters + * + * Free expired entries on each bucket's LRU list until we've released + * nr_to_scan freed objects. Nothing will be released if the cache + * has not exceeded it's max_drc_entries limit. + * + * Returns the number of entries released by this call. */ -static long -prune_cache_entries(struct nfsd_net *nn) +static unsigned long +nfsd_reply_cache_scan(struct shrinker *shrink, struct shrink_control *sc) { + struct nfsd_net *nn = container_of(shrink, + struct nfsd_net, nfsd_reply_cache_shrinker); + unsigned long freed = 0; + LIST_HEAD(dispose); unsigned int i; - long freed = 0;
for (i = 0; i < nn->drc_hashsize; i++) { struct nfsd_drc_bucket *b = &nn->drc_hashtbl[i];
if (list_empty(&b->lru_head)) continue; + spin_lock(&b->cache_lock); - freed += prune_bucket(b, nn, 0); + nfsd_prune_bucket_locked(nn, b, 0, &dispose); spin_unlock(&b->cache_lock); - } - return freed; -}
-static unsigned long -nfsd_reply_cache_count(struct shrinker *shrink, struct shrink_control *sc) -{ - struct nfsd_net *nn = container_of(shrink, - struct nfsd_net, nfsd_reply_cache_shrinker); + freed += nfsd_cacherep_dispose(&dispose); + if (freed > sc->nr_to_scan) + break; + }
- return atomic_read(&nn->num_drc_entries); + trace_nfsd_drc_gc(nn, freed); + return freed; }
-static unsigned long -nfsd_reply_cache_scan(struct shrinker *shrink, struct shrink_control *sc) -{ - struct nfsd_net *nn = container_of(shrink, - struct nfsd_net, nfsd_reply_cache_shrinker); - - return prune_cache_entries(nn); -} /* * Walk an xdr_buf and get a CRC for at most the first RC_CSUMLEN bytes */
From: Chuck Lever chuck.lever@oracle.com
[ Upstream commit cb18eca4b86768ec79e847795d1043356c9ee3b0 ]
Over time I'd like to see NFS-specific fields moved out of struct svc_rqst, which is an RPC layer object. These fields are layering violations.
Reviewed-by: Jeff Layton jlayton@kernel.org Signed-off-by: Chuck Lever chuck.lever@oracle.com --- fs/nfsd/cache.h | 6 ++++-- fs/nfsd/nfscache.c | 11 ++++++----- fs/nfsd/nfssvc.c | 10 ++++++---- include/linux/sunrpc/svc.h | 1 - 4 files changed, 16 insertions(+), 12 deletions(-)
diff --git a/fs/nfsd/cache.h b/fs/nfsd/cache.h index 4c9b87850ab1..27610b071880 100644 --- a/fs/nfsd/cache.h +++ b/fs/nfsd/cache.h @@ -84,8 +84,10 @@ int nfsd_net_reply_cache_init(struct nfsd_net *nn); void nfsd_net_reply_cache_destroy(struct nfsd_net *nn); int nfsd_reply_cache_init(struct nfsd_net *); void nfsd_reply_cache_shutdown(struct nfsd_net *); -int nfsd_cache_lookup(struct svc_rqst *); -void nfsd_cache_update(struct svc_rqst *, int, __be32 *); +int nfsd_cache_lookup(struct svc_rqst *rqstp, + struct svc_cacherep **cacherep); +void nfsd_cache_update(struct svc_rqst *rqstp, struct svc_cacherep *rp, + int cachetype, __be32 *statp); int nfsd_reply_cache_stats_show(struct seq_file *m, void *v);
#endif /* NFSCACHE_H */ diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c index 8aab82ca798b..de4f2c6f23dc 100644 --- a/fs/nfsd/nfscache.c +++ b/fs/nfsd/nfscache.c @@ -465,6 +465,7 @@ nfsd_cache_insert(struct nfsd_drc_bucket *b, struct svc_cacherep *key, /** * nfsd_cache_lookup - Find an entry in the duplicate reply cache * @rqstp: Incoming Call to find + * @cacherep: OUT: DRC entry for this request * * Try to find an entry matching the current call in the cache. When none * is found, we try to grab the oldest expired entry off the LRU list. If @@ -477,7 +478,7 @@ nfsd_cache_insert(struct nfsd_drc_bucket *b, struct svc_cacherep *key, * %RC_REPLY: Reply from cache * %RC_DROPIT: Do not process the request further */ -int nfsd_cache_lookup(struct svc_rqst *rqstp) +int nfsd_cache_lookup(struct svc_rqst *rqstp, struct svc_cacherep **cacherep) { struct nfsd_net *nn; struct svc_cacherep *rp, *found; @@ -488,7 +489,6 @@ int nfsd_cache_lookup(struct svc_rqst *rqstp) LIST_HEAD(dispose); int rtn = RC_DOIT;
- rqstp->rq_cacherep = NULL; if (type == RC_NOCACHE) { nfsd_stats_rc_nocache_inc(); goto out; @@ -510,7 +510,7 @@ int nfsd_cache_lookup(struct svc_rqst *rqstp) found = nfsd_cache_insert(b, rp, nn); if (found != rp) goto found_entry; - rqstp->rq_cacherep = rp; + *cacherep = rp; rp->c_state = RC_INPROG; nfsd_prune_bucket_locked(nn, b, 3, &dispose); spin_unlock(&b->cache_lock); @@ -568,6 +568,7 @@ int nfsd_cache_lookup(struct svc_rqst *rqstp) /** * nfsd_cache_update - Update an entry in the duplicate reply cache. * @rqstp: svc_rqst with a finished Reply + * @rp: IN: DRC entry for this request * @cachetype: which cache to update * @statp: pointer to Reply's NFS status code, or NULL * @@ -585,10 +586,10 @@ int nfsd_cache_lookup(struct svc_rqst *rqstp) * nfsd failed to encode a reply that otherwise would have been cached. * In this case, nfsd_cache_update is called with statp == NULL. */ -void nfsd_cache_update(struct svc_rqst *rqstp, int cachetype, __be32 *statp) +void nfsd_cache_update(struct svc_rqst *rqstp, struct svc_cacherep *rp, + int cachetype, __be32 *statp) { struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id); - struct svc_cacherep *rp = rqstp->rq_cacherep; struct kvec *resv = &rqstp->rq_res.head[0], *cachv; struct nfsd_drc_bucket *b; int len; diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c index 2154fa63c5f2..f91fb343313d 100644 --- a/fs/nfsd/nfssvc.c +++ b/fs/nfsd/nfssvc.c @@ -1046,6 +1046,7 @@ int nfsd_dispatch(struct svc_rqst *rqstp) { const struct svc_procedure *proc = rqstp->rq_procinfo; __be32 *statp = rqstp->rq_accept_statp; + struct svc_cacherep *rp;
/* * Give the xdr decoder a chance to change this if it wants @@ -1056,7 +1057,8 @@ int nfsd_dispatch(struct svc_rqst *rqstp) if (!proc->pc_decode(rqstp, &rqstp->rq_arg_stream)) goto out_decode_err;
- switch (nfsd_cache_lookup(rqstp)) { + rp = NULL; + switch (nfsd_cache_lookup(rqstp, &rp)) { case RC_DOIT: break; case RC_REPLY: @@ -1072,7 +1074,7 @@ int nfsd_dispatch(struct svc_rqst *rqstp) if (!proc->pc_encode(rqstp, &rqstp->rq_res_stream)) goto out_encode_err;
- nfsd_cache_update(rqstp, rqstp->rq_cachetype, statp + 1); + nfsd_cache_update(rqstp, rp, rqstp->rq_cachetype, statp + 1); out_cached_reply: return 1;
@@ -1082,13 +1084,13 @@ int nfsd_dispatch(struct svc_rqst *rqstp) return 1;
out_update_drop: - nfsd_cache_update(rqstp, RC_NOCACHE, NULL); + nfsd_cache_update(rqstp, rp, RC_NOCACHE, NULL); out_dropit: return 0;
out_encode_err: trace_nfsd_cant_encode_err(rqstp); - nfsd_cache_update(rqstp, RC_NOCACHE, NULL); + nfsd_cache_update(rqstp, rp, RC_NOCACHE, NULL); *statp = rpc_system_err; return 1; } diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h index f8751118c122..fe1394cc1371 100644 --- a/include/linux/sunrpc/svc.h +++ b/include/linux/sunrpc/svc.h @@ -265,7 +265,6 @@ struct svc_rqst { /* Catering to nfsd */ struct auth_domain * rq_client; /* RPC peer info */ struct auth_domain * rq_gssclient; /* "gss/"-style peer info */ - struct svc_cacherep * rq_cacherep; /* cache info */ struct task_struct *rq_task; /* service thread */ struct net *rq_bc_net; /* pointer to backchannel's * net namespace
From: Chuck Lever chuck.lever@oracle.com
[ Upstream commit e7421ce71437ec8e4d69cc6bdf35b6853adc5050 ]
The svc_ prefix is identified with the SunRPC layer. Although the duplicate reply cache caches RPC replies, it is only for the NFS protocol. Rename the struct to better reflect its purpose.
Reviewed-by: Jeff Layton jlayton@kernel.org Signed-off-by: Chuck Lever chuck.lever@oracle.com --- fs/nfsd/cache.h | 6 +++--- fs/nfsd/nfscache.c | 44 ++++++++++++++++++++++---------------------- fs/nfsd/nfssvc.c | 2 +- fs/nfsd/trace.h | 4 ++-- 4 files changed, 28 insertions(+), 28 deletions(-)
diff --git a/fs/nfsd/cache.h b/fs/nfsd/cache.h index 27610b071880..929248c6ca84 100644 --- a/fs/nfsd/cache.h +++ b/fs/nfsd/cache.h @@ -19,7 +19,7 @@ * typical sockaddr_storage. This is for space reasons, since sockaddr_storage * is much larger than a sockaddr_in6. */ -struct svc_cacherep { +struct nfsd_cacherep { struct { /* Keep often-read xid, csum in the same cache line: */ __be32 k_xid; @@ -85,8 +85,8 @@ void nfsd_net_reply_cache_destroy(struct nfsd_net *nn); int nfsd_reply_cache_init(struct nfsd_net *); void nfsd_reply_cache_shutdown(struct nfsd_net *); int nfsd_cache_lookup(struct svc_rqst *rqstp, - struct svc_cacherep **cacherep); -void nfsd_cache_update(struct svc_rqst *rqstp, struct svc_cacherep *rp, + struct nfsd_cacherep **cacherep); +void nfsd_cache_update(struct svc_rqst *rqstp, struct nfsd_cacherep *rp, int cachetype, __be32 *statp); int nfsd_reply_cache_stats_show(struct seq_file *m, void *v);
diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c index de4f2c6f23dc..abb453be71ca 100644 --- a/fs/nfsd/nfscache.c +++ b/fs/nfsd/nfscache.c @@ -84,11 +84,11 @@ nfsd_hashsize(unsigned int limit) return roundup_pow_of_two(limit / TARGET_BUCKET_SIZE); }
-static struct svc_cacherep * +static struct nfsd_cacherep * nfsd_cacherep_alloc(struct svc_rqst *rqstp, __wsum csum, struct nfsd_net *nn) { - struct svc_cacherep *rp; + struct nfsd_cacherep *rp;
rp = kmem_cache_alloc(drc_slab, GFP_KERNEL); if (rp) { @@ -110,7 +110,7 @@ nfsd_cacherep_alloc(struct svc_rqst *rqstp, __wsum csum, return rp; }
-static void nfsd_cacherep_free(struct svc_cacherep *rp) +static void nfsd_cacherep_free(struct nfsd_cacherep *rp) { if (rp->c_type == RC_REPLBUFF) kfree(rp->c_replvec.iov_base); @@ -120,11 +120,11 @@ static void nfsd_cacherep_free(struct svc_cacherep *rp) static unsigned long nfsd_cacherep_dispose(struct list_head *dispose) { - struct svc_cacherep *rp; + struct nfsd_cacherep *rp; unsigned long freed = 0;
while (!list_empty(dispose)) { - rp = list_first_entry(dispose, struct svc_cacherep, c_lru); + rp = list_first_entry(dispose, struct nfsd_cacherep, c_lru); list_del(&rp->c_lru); nfsd_cacherep_free(rp); freed++; @@ -134,7 +134,7 @@ nfsd_cacherep_dispose(struct list_head *dispose)
static void nfsd_cacherep_unlink_locked(struct nfsd_net *nn, struct nfsd_drc_bucket *b, - struct svc_cacherep *rp) + struct nfsd_cacherep *rp) { if (rp->c_type == RC_REPLBUFF && rp->c_replvec.iov_base) nfsd_stats_drc_mem_usage_sub(nn, rp->c_replvec.iov_len); @@ -147,7 +147,7 @@ nfsd_cacherep_unlink_locked(struct nfsd_net *nn, struct nfsd_drc_bucket *b, }
static void -nfsd_reply_cache_free_locked(struct nfsd_drc_bucket *b, struct svc_cacherep *rp, +nfsd_reply_cache_free_locked(struct nfsd_drc_bucket *b, struct nfsd_cacherep *rp, struct nfsd_net *nn) { nfsd_cacherep_unlink_locked(nn, b, rp); @@ -155,7 +155,7 @@ nfsd_reply_cache_free_locked(struct nfsd_drc_bucket *b, struct svc_cacherep *rp, }
static void -nfsd_reply_cache_free(struct nfsd_drc_bucket *b, struct svc_cacherep *rp, +nfsd_reply_cache_free(struct nfsd_drc_bucket *b, struct nfsd_cacherep *rp, struct nfsd_net *nn) { spin_lock(&b->cache_lock); @@ -167,7 +167,7 @@ nfsd_reply_cache_free(struct nfsd_drc_bucket *b, struct svc_cacherep *rp, int nfsd_drc_slab_create(void) { drc_slab = kmem_cache_create("nfsd_drc", - sizeof(struct svc_cacherep), 0, 0, NULL); + sizeof(struct nfsd_cacherep), 0, 0, NULL); return drc_slab ? 0: -ENOMEM; }
@@ -236,7 +236,7 @@ int nfsd_reply_cache_init(struct nfsd_net *nn)
void nfsd_reply_cache_shutdown(struct nfsd_net *nn) { - struct svc_cacherep *rp; + struct nfsd_cacherep *rp; unsigned int i;
unregister_shrinker(&nn->nfsd_reply_cache_shrinker); @@ -244,7 +244,7 @@ void nfsd_reply_cache_shutdown(struct nfsd_net *nn) for (i = 0; i < nn->drc_hashsize; i++) { struct list_head *head = &nn->drc_hashtbl[i].lru_head; while (!list_empty(head)) { - rp = list_first_entry(head, struct svc_cacherep, c_lru); + rp = list_first_entry(head, struct nfsd_cacherep, c_lru); nfsd_reply_cache_free_locked(&nn->drc_hashtbl[i], rp, nn); } @@ -261,7 +261,7 @@ void nfsd_reply_cache_shutdown(struct nfsd_net *nn) * not already scheduled. */ static void -lru_put_end(struct nfsd_drc_bucket *b, struct svc_cacherep *rp) +lru_put_end(struct nfsd_drc_bucket *b, struct nfsd_cacherep *rp) { rp->c_timestamp = jiffies; list_move_tail(&rp->c_lru, &b->lru_head); @@ -284,7 +284,7 @@ nfsd_prune_bucket_locked(struct nfsd_net *nn, struct nfsd_drc_bucket *b, unsigned int max, struct list_head *dispose) { unsigned long expiry = jiffies - RC_EXPIRE; - struct svc_cacherep *rp, *tmp; + struct nfsd_cacherep *rp, *tmp; unsigned int freed = 0;
lockdep_assert_held(&b->cache_lock); @@ -402,8 +402,8 @@ nfsd_cache_csum(struct svc_rqst *rqstp) }
static int -nfsd_cache_key_cmp(const struct svc_cacherep *key, - const struct svc_cacherep *rp, struct nfsd_net *nn) +nfsd_cache_key_cmp(const struct nfsd_cacherep *key, + const struct nfsd_cacherep *rp, struct nfsd_net *nn) { if (key->c_key.k_xid == rp->c_key.k_xid && key->c_key.k_csum != rp->c_key.k_csum) { @@ -419,11 +419,11 @@ nfsd_cache_key_cmp(const struct svc_cacherep *key, * Must be called with cache_lock held. Returns the found entry or * inserts an empty key on failure. */ -static struct svc_cacherep * -nfsd_cache_insert(struct nfsd_drc_bucket *b, struct svc_cacherep *key, +static struct nfsd_cacherep * +nfsd_cache_insert(struct nfsd_drc_bucket *b, struct nfsd_cacherep *key, struct nfsd_net *nn) { - struct svc_cacherep *rp, *ret = key; + struct nfsd_cacherep *rp, *ret = key; struct rb_node **p = &b->rb_head.rb_node, *parent = NULL; unsigned int entries = 0; @@ -432,7 +432,7 @@ nfsd_cache_insert(struct nfsd_drc_bucket *b, struct svc_cacherep *key, while (*p != NULL) { ++entries; parent = *p; - rp = rb_entry(parent, struct svc_cacherep, c_node); + rp = rb_entry(parent, struct nfsd_cacherep, c_node);
cmp = nfsd_cache_key_cmp(key, rp, nn); if (cmp < 0) @@ -478,10 +478,10 @@ nfsd_cache_insert(struct nfsd_drc_bucket *b, struct svc_cacherep *key, * %RC_REPLY: Reply from cache * %RC_DROPIT: Do not process the request further */ -int nfsd_cache_lookup(struct svc_rqst *rqstp, struct svc_cacherep **cacherep) +int nfsd_cache_lookup(struct svc_rqst *rqstp, struct nfsd_cacherep **cacherep) { struct nfsd_net *nn; - struct svc_cacherep *rp, *found; + struct nfsd_cacherep *rp, *found; __wsum csum; struct nfsd_drc_bucket *b; int type = rqstp->rq_cachetype; @@ -586,7 +586,7 @@ int nfsd_cache_lookup(struct svc_rqst *rqstp, struct svc_cacherep **cacherep) * nfsd failed to encode a reply that otherwise would have been cached. * In this case, nfsd_cache_update is called with statp == NULL. */ -void nfsd_cache_update(struct svc_rqst *rqstp, struct svc_cacherep *rp, +void nfsd_cache_update(struct svc_rqst *rqstp, struct nfsd_cacherep *rp, int cachetype, __be32 *statp) { struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id); diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c index f91fb343313d..97830e28c140 100644 --- a/fs/nfsd/nfssvc.c +++ b/fs/nfsd/nfssvc.c @@ -1046,7 +1046,7 @@ int nfsd_dispatch(struct svc_rqst *rqstp) { const struct svc_procedure *proc = rqstp->rq_procinfo; __be32 *statp = rqstp->rq_accept_statp; - struct svc_cacherep *rp; + struct nfsd_cacherep *rp;
/* * Give the xdr decoder a chance to change this if it wants diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h index c06c505d04fb..2388053eb862 100644 --- a/fs/nfsd/trace.h +++ b/fs/nfsd/trace.h @@ -1240,8 +1240,8 @@ TRACE_EVENT(nfsd_drc_found, TRACE_EVENT(nfsd_drc_mismatch, TP_PROTO( const struct nfsd_net *nn, - const struct svc_cacherep *key, - const struct svc_cacherep *rp + const struct nfsd_cacherep *key, + const struct nfsd_cacherep *rp ), TP_ARGS(nn, key, rp), TP_STRUCT__entry(
From: Chuck Lever chuck.lever@oracle.com
[ Upstream commit 1caf5f61dd8430ae5a0b4538afe4953ce7517cbb ]
The "statp + 1" pointer that is passed to nfsd_cache_update() is supposed to point to the start of the egress NFS Reply header. In fact, it does point there for AUTH_SYS and RPCSEC_GSS_KRB5 requests.
But both krb5i and krb5p add fields between the RPC header's accept_stat field and the start of the NFS Reply header. In those cases, "statp + 1" points at the extra fields instead of the Reply. The result is that nfsd_cache_update() caches what looks to the client like garbage.
A connection break can occur for a number of reasons, but the most common reason when using krb5i/p is a GSS sequence number window underrun. When an underrun is detected, the server is obliged to drop the RPC and the connection to force a retransmit with a fresh GSS sequence number. The client presents the same XID, it hits in the server's DRC, and the server returns the garbage cache entry.
The "statp + 1" argument has been used since the oldest changeset in the kernel history repo, so it has been in nfsd_dispatch() literally since before history began. The problem arose only when the server-side GSS implementation was added twenty years ago.
Reviewed-by: Jeff Layton jlayton@kernel.org Tested-by: Jeff Layton <jlayton@kernel.org Signed-off-by: Chuck Lever chuck.lever@oracle.com --- fs/nfsd/nfssvc.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c index 97830e28c140..3c8a12d1b461 100644 --- a/fs/nfsd/nfssvc.c +++ b/fs/nfsd/nfssvc.c @@ -1047,6 +1047,7 @@ int nfsd_dispatch(struct svc_rqst *rqstp) const struct svc_procedure *proc = rqstp->rq_procinfo; __be32 *statp = rqstp->rq_accept_statp; struct nfsd_cacherep *rp; + __be32 *nfs_reply;
/* * Give the xdr decoder a chance to change this if it wants @@ -1067,6 +1068,7 @@ int nfsd_dispatch(struct svc_rqst *rqstp) goto out_dropit; }
+ nfs_reply = xdr_inline_decode(&rqstp->rq_res_stream, 0); *statp = proc->pc_func(rqstp); if (test_bit(RQ_DROPME, &rqstp->rq_flags)) goto out_update_drop; @@ -1074,7 +1076,7 @@ int nfsd_dispatch(struct svc_rqst *rqstp) if (!proc->pc_encode(rqstp, &rqstp->rq_res_stream)) goto out_encode_err;
- nfsd_cache_update(rqstp, rp, rqstp->rq_cachetype, statp + 1); + nfsd_cache_update(rqstp, rp, rqstp->rq_cachetype, nfs_reply); out_cached_reply: return 1;
From: Chuck Lever chuck.lever@oracle.com
[ Upstream commit bf51c52a1f3c238d72c64e14d5e7702d3a245b82 ]
nfsd_cache_csum() currently assumes that the server's RPC layer has been advancing rq_arg.head[0].iov_base as it decodes an incoming request, because that's the way it used to work. On entry, it expects that buf->head[0].iov_base points to the start of the NFS header, and excludes the already-decoded RPC header.
These days however, head[0].iov_base now points to the start of the RPC header during all processing. It no longer points at the NFS Call header when execution arrives at nfsd_cache_csum().
In a retransmitted RPC the XID and the NFS header are supposed to be the same as the original message, but the contents of the retransmitted RPC header can be different. For example, for krb5, the GSS sequence number will be different between the two. Thus if the RPC header is always included in the DRC checksum computation, the checksum of the retransmitted message might not match the checksum of the original message, even though the NFS part of these messages is identical.
The result is that, even if a matching XID is found in the DRC, the checksum mismatch causes the server to execute the retransmitted RPC transaction again.
Reviewed-by: Jeff Layton jlayton@kernel.org Tested-by: Jeff Layton jlayton@kernel.org Signed-off-by: Chuck Lever chuck.lever@oracle.com --- fs/nfsd/cache.h | 4 ++- fs/nfsd/nfscache.c | 64 +++++++++++++++++++++++++++++++++++----------------- fs/nfsd/nfssvc.c | 10 +++++++- 3 files changed, 54 insertions(+), 24 deletions(-)
diff --git a/fs/nfsd/cache.h b/fs/nfsd/cache.h index 929248c6ca84..4cbe0434cbb8 100644 --- a/fs/nfsd/cache.h +++ b/fs/nfsd/cache.h @@ -84,8 +84,8 @@ int nfsd_net_reply_cache_init(struct nfsd_net *nn); void nfsd_net_reply_cache_destroy(struct nfsd_net *nn); int nfsd_reply_cache_init(struct nfsd_net *); void nfsd_reply_cache_shutdown(struct nfsd_net *); -int nfsd_cache_lookup(struct svc_rqst *rqstp, - struct nfsd_cacherep **cacherep); +int nfsd_cache_lookup(struct svc_rqst *rqstp, unsigned int start, + unsigned int len, struct nfsd_cacherep **cacherep); void nfsd_cache_update(struct svc_rqst *rqstp, struct nfsd_cacherep *rp, int cachetype, __be32 *statp); int nfsd_reply_cache_stats_show(struct seq_file *m, void *v); diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c index abb453be71ca..6cd36af2f97e 100644 --- a/fs/nfsd/nfscache.c +++ b/fs/nfsd/nfscache.c @@ -368,33 +368,52 @@ nfsd_reply_cache_scan(struct shrinker *shrink, struct shrink_control *sc) return freed; }
-/* - * Walk an xdr_buf and get a CRC for at most the first RC_CSUMLEN bytes +/** + * nfsd_cache_csum - Checksum incoming NFS Call arguments + * @buf: buffer containing a whole RPC Call message + * @start: starting byte of the NFS Call header + * @remaining: size of the NFS Call header, in bytes + * + * Compute a weak checksum of the leading bytes of an NFS procedure + * call header to help verify that a retransmitted Call matches an + * entry in the duplicate reply cache. + * + * To avoid assumptions about how the RPC message is laid out in + * @buf and what else it might contain (eg, a GSS MIC suffix), the + * caller passes us the exact location and length of the NFS Call + * header. + * + * Returns a 32-bit checksum value, as defined in RFC 793. */ -static __wsum -nfsd_cache_csum(struct svc_rqst *rqstp) +static __wsum nfsd_cache_csum(struct xdr_buf *buf, unsigned int start, + unsigned int remaining) { + unsigned int base, len; + struct xdr_buf subbuf; + __wsum csum = 0; + void *p; int idx; - unsigned int base; - __wsum csum; - struct xdr_buf *buf = &rqstp->rq_arg; - const unsigned char *p = buf->head[0].iov_base; - size_t csum_len = min_t(size_t, buf->head[0].iov_len + buf->page_len, - RC_CSUMLEN); - size_t len = min(buf->head[0].iov_len, csum_len); + + if (remaining > RC_CSUMLEN) + remaining = RC_CSUMLEN; + if (xdr_buf_subsegment(buf, &subbuf, start, remaining)) + return csum;
/* rq_arg.head first */ - csum = csum_partial(p, len, 0); - csum_len -= len; + if (subbuf.head[0].iov_len) { + len = min_t(unsigned int, subbuf.head[0].iov_len, remaining); + csum = csum_partial(subbuf.head[0].iov_base, len, csum); + remaining -= len; + }
/* Continue into page array */ - idx = buf->page_base / PAGE_SIZE; - base = buf->page_base & ~PAGE_MASK; - while (csum_len) { - p = page_address(buf->pages[idx]) + base; - len = min_t(size_t, PAGE_SIZE - base, csum_len); + idx = subbuf.page_base / PAGE_SIZE; + base = subbuf.page_base & ~PAGE_MASK; + while (remaining) { + p = page_address(subbuf.pages[idx]) + base; + len = min_t(unsigned int, PAGE_SIZE - base, remaining); csum = csum_partial(p, len, csum); - csum_len -= len; + remaining -= len; base = 0; ++idx; } @@ -465,6 +484,8 @@ nfsd_cache_insert(struct nfsd_drc_bucket *b, struct nfsd_cacherep *key, /** * nfsd_cache_lookup - Find an entry in the duplicate reply cache * @rqstp: Incoming Call to find + * @start: starting byte in @rqstp->rq_arg of the NFS Call header + * @len: size of the NFS Call header, in bytes * @cacherep: OUT: DRC entry for this request * * Try to find an entry matching the current call in the cache. When none @@ -478,7 +499,8 @@ nfsd_cache_insert(struct nfsd_drc_bucket *b, struct nfsd_cacherep *key, * %RC_REPLY: Reply from cache * %RC_DROPIT: Do not process the request further */ -int nfsd_cache_lookup(struct svc_rqst *rqstp, struct nfsd_cacherep **cacherep) +int nfsd_cache_lookup(struct svc_rqst *rqstp, unsigned int start, + unsigned int len, struct nfsd_cacherep **cacherep) { struct nfsd_net *nn; struct nfsd_cacherep *rp, *found; @@ -494,7 +516,7 @@ int nfsd_cache_lookup(struct svc_rqst *rqstp, struct nfsd_cacherep **cacherep) goto out; }
- csum = nfsd_cache_csum(rqstp); + csum = nfsd_cache_csum(&rqstp->rq_arg, start, len);
/* * Since the common case is a cache miss followed by an insert, diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c index 3c8a12d1b461..7bfcabe92fc5 100644 --- a/fs/nfsd/nfssvc.c +++ b/fs/nfsd/nfssvc.c @@ -1047,6 +1047,7 @@ int nfsd_dispatch(struct svc_rqst *rqstp) const struct svc_procedure *proc = rqstp->rq_procinfo; __be32 *statp = rqstp->rq_accept_statp; struct nfsd_cacherep *rp; + unsigned int start, len; __be32 *nfs_reply;
/* @@ -1055,11 +1056,18 @@ int nfsd_dispatch(struct svc_rqst *rqstp) */ rqstp->rq_cachetype = proc->pc_cachetype;
+ /* + * ->pc_decode advances the argument stream past the NFS + * Call header, so grab the header's starting location and + * size now for the call to nfsd_cache_lookup(). + */ + start = xdr_stream_pos(&rqstp->rq_arg_stream); + len = xdr_stream_remaining(&rqstp->rq_arg_stream); if (!proc->pc_decode(rqstp, &rqstp->rq_arg_stream)) goto out_decode_err;
rp = NULL; - switch (nfsd_cache_lookup(rqstp, &rp)) { + switch (nfsd_cache_lookup(rqstp, start, len, &rp)) { case RC_DOIT: break; case RC_REPLY:
On Nov 28, 2023, at 4:59 PM, Chuck Lever cel@kernel.org wrote:
Backport of upstream fixes to NFSD's duplicate reply cache. These have been hand-applied and tested with the same reproducer as was used to create the upstream fixes.
After applying patches 1 through 6 cleanly, these applied with fuzz and offset but no rejection -- the same as the 6.6.y patch set. The context changes were due to Lorenzo's new nfsd netlink protocol.
Chuck Lever (8): NFSD: Refactor nfsd_reply_cache_free_locked() NFSD: Rename nfsd_reply_cache_alloc() NFSD: Replace nfsd_prune_bucket() NFSD: Refactor the duplicate reply cache shrinker NFSD: Remove svc_rqst::rq_cacherep NFSD: Rename struct svc_cacherep NFSD: Fix "start of NFS reply" pointer passed to nfsd_cache_update() NFSD: Fix checksum mismatches in the duplicate reply cache
fs/nfsd/cache.h | 8 +- fs/nfsd/nfscache.c | 266 ++++++++++++++++++++++++------------- fs/nfsd/nfssvc.c | 20 ++- fs/nfsd/trace.h | 26 +++- include/linux/sunrpc/svc.h | 1 - 5 files changed, 218 insertions(+), 103 deletions(-)
-- Chuck Lever
-- Chuck Lever
On Tue, Nov 28, 2023 at 10:07:11PM +0000, Chuck Lever III wrote:
On Nov 28, 2023, at 4:59 PM, Chuck Lever cel@kernel.org wrote:
Backport of upstream fixes to NFSD's duplicate reply cache. These have been hand-applied and tested with the same reproducer as was used to create the upstream fixes.
After applying patches 1 through 6 cleanly, these applied with fuzz and offset but no rejection -- the same as the 6.6.y patch set. The context changes were due to Lorenzo's new nfsd netlink protocol.
6.5.y is now end-of-life, sorry.
greg k-h
On Nov 30, 2023, at 8:25 AM, Greg KH gregkh@linuxfoundation.org wrote:
On Tue, Nov 28, 2023 at 10:07:11PM +0000, Chuck Lever III wrote:
On Nov 28, 2023, at 4:59 PM, Chuck Lever cel@kernel.org wrote:
Backport of upstream fixes to NFSD's duplicate reply cache. These have been hand-applied and tested with the same reproducer as was used to create the upstream fixes.
After applying patches 1 through 6 cleanly, these applied with fuzz and offset but no rejection -- the same as the 6.6.y patch set. The context changes were due to Lorenzo's new nfsd netlink protocol.
6.5.y is now end-of-life, sorry.
Whoops, was it marked EOL a few days ago? I missed that.
No big deal, I just wanted to ensure all the stable kernels were covered.
-- Chuck Lever
linux-stable-mirror@lists.linaro.org
-
Chuck Lever -
Chuck Lever III -
Greg KH -
kernel test robot