From 3af4974eb2c7867d6e160977195dfde586d0e564 Mon Sep 17 00:00:00 2001 From: NeilBrown Date: Wed, 3 Feb 2010 17:31:31 +1100 Subject: sunrpc: don't keep expired entries in the auth caches. currently expired entries remain in the auth caches as long as there is a reference. This was needed long ago when the auth_domain cache used the same cache infrastructure. But since that (being a very different sort of cache) was separated, this test is no longer needed. So remove the test on refcnt and tidy up the surrounding code. This allows the cache_dequeue call (which needed to be there to drop a potentially awkward reference) can be moved outside of the spinlock which is a better place for it. Signed-off-by: NeilBrown Signed-off-by: J. Bruce Fields --- net/sunrpc/cache.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) (limited to 'net') diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c index 39bddba53ba1..83592e012585 100644 --- a/net/sunrpc/cache.c +++ b/net/sunrpc/cache.c @@ -397,31 +397,28 @@ static int cache_clean(void) /* Ok, now to clean this strand */ cp = & current_detail->hash_table[current_index]; - ch = *cp; - for (; ch; cp= & ch->next, ch= *cp) { + for (ch = *cp ; ch ; cp = & ch->next, ch = *cp) { if (current_detail->nextcheck > ch->expiry_time) current_detail->nextcheck = ch->expiry_time+1; if (ch->expiry_time >= get_seconds() && ch->last_refresh >= current_detail->flush_time) continue; - if (test_and_clear_bit(CACHE_PENDING, &ch->flags)) - cache_dequeue(current_detail, ch); - if (atomic_read(&ch->ref.refcount) == 1) - break; - } - if (ch) { *cp = ch->next; ch->next = NULL; current_detail->entries--; rv = 1; + break; } + write_unlock(¤t_detail->hash_lock); d = current_detail; if (!ch) current_index ++; spin_unlock(&cache_list_lock); if (ch) { + if (test_and_clear_bit(CACHE_PENDING, &ch->flags)) + cache_dequeue(current_detail, ch); cache_revisit_request(ch); cache_put(ch, d); } -- cgit v1.2.2 From 2f50d8b63dd6e5320a9d223298df19df3502da29 Mon Sep 17 00:00:00 2001 From: NeilBrown Date: Wed, 3 Feb 2010 17:31:31 +1100 Subject: sunrpc/cache: factor out cache_is_expired This removes a tiny bit of code duplication, but more important prepares for following patch which will perform the expiry check in cache_lookup and the rest of the validity check in cache_check. Signed-off-by: NeilBrown Signed-off-by: J. Bruce Fields --- net/sunrpc/cache.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) (limited to 'net') diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c index 83592e012585..9826c5ceb995 100644 --- a/net/sunrpc/cache.c +++ b/net/sunrpc/cache.c @@ -49,6 +49,12 @@ static void cache_init(struct cache_head *h) h->last_refresh = now; } +static inline int cache_is_expired(struct cache_detail *detail, struct cache_head *h) +{ + return (h->expiry_time < get_seconds()) || + (detail->flush_time > h->last_refresh); +} + struct cache_head *sunrpc_cache_lookup(struct cache_detail *detail, struct cache_head *key, int hash) { @@ -184,9 +190,7 @@ static int cache_make_upcall(struct cache_detail *cd, struct cache_head *h) static inline int cache_is_valid(struct cache_detail *detail, struct cache_head *h) { if (!test_bit(CACHE_VALID, &h->flags) || - h->expiry_time < get_seconds()) - return -EAGAIN; - else if (detail->flush_time > h->last_refresh) + cache_is_expired(detail, h)) return -EAGAIN; else { /* entry is valid */ @@ -400,8 +404,7 @@ static int cache_clean(void) for (ch = *cp ; ch ; cp = & ch->next, ch = *cp) { if (current_detail->nextcheck > ch->expiry_time) current_detail->nextcheck = ch->expiry_time+1; - if (ch->expiry_time >= get_seconds() && - ch->last_refresh >= current_detail->flush_time) + if (!cache_is_expired(current_detail, ch)) continue; *cp = ch->next; -- cgit v1.2.2 From d202cce8963d9268ff355a386e20243e8332b308 Mon Sep 17 00:00:00 2001 From: NeilBrown Date: Wed, 3 Feb 2010 17:31:31 +1100 Subject: sunrpc: never return expired entries in sunrpc_cache_lookup If sunrpc_cache_lookup finds an expired entry, remove it from the cache and return a freshly created non-VALID entry instead. This ensures that we only ever get a usable entry, or an entry that will become usable once an update arrives. i.e. we will never need to repeat the lookup. This allows us to remove the 'is_expired' test from cache_check (i.e. from cache_is_valid). cache_check should never get an expired entry as 'lookup' will never return one. If it does happen - due to inconvenient timing - then just accept it as still valid, it won't be very much past it's use-by date. Signed-off-by: NeilBrown Signed-off-by: J. Bruce Fields --- net/sunrpc/cache.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) (limited to 'net') diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c index 9826c5ceb995..3e1ef8bf4dc2 100644 --- a/net/sunrpc/cache.c +++ b/net/sunrpc/cache.c @@ -59,7 +59,7 @@ struct cache_head *sunrpc_cache_lookup(struct cache_detail *detail, struct cache_head *key, int hash) { struct cache_head **head, **hp; - struct cache_head *new = NULL; + struct cache_head *new = NULL, *freeme = NULL; head = &detail->hash_table[hash]; @@ -68,6 +68,9 @@ struct cache_head *sunrpc_cache_lookup(struct cache_detail *detail, for (hp=head; *hp != NULL ; hp = &(*hp)->next) { struct cache_head *tmp = *hp; if (detail->match(tmp, key)) { + if (cache_is_expired(detail, tmp)) + /* This entry is expired, we will discard it. */ + break; cache_get(tmp); read_unlock(&detail->hash_lock); return tmp; @@ -92,6 +95,13 @@ struct cache_head *sunrpc_cache_lookup(struct cache_detail *detail, for (hp=head; *hp != NULL ; hp = &(*hp)->next) { struct cache_head *tmp = *hp; if (detail->match(tmp, key)) { + if (cache_is_expired(detail, tmp)) { + *hp = tmp->next; + tmp->next = NULL; + detail->entries --; + freeme = tmp; + break; + } cache_get(tmp); write_unlock(&detail->hash_lock); cache_put(new, detail); @@ -104,6 +114,8 @@ struct cache_head *sunrpc_cache_lookup(struct cache_detail *detail, cache_get(new); write_unlock(&detail->hash_lock); + if (freeme) + cache_put(freeme, detail); return new; } EXPORT_SYMBOL_GPL(sunrpc_cache_lookup); @@ -189,8 +201,7 @@ static int cache_make_upcall(struct cache_detail *cd, struct cache_head *h) static inline int cache_is_valid(struct cache_detail *detail, struct cache_head *h) { - if (!test_bit(CACHE_VALID, &h->flags) || - cache_is_expired(detail, h)) + if (!test_bit(CACHE_VALID, &h->flags)) return -EAGAIN; else { /* entry is valid */ -- cgit v1.2.2 From a5990ea1254cd186b38744507aeec3136a0c1c95 Mon Sep 17 00:00:00 2001 From: Li Zefan Date: Thu, 11 Mar 2010 14:08:10 -0800 Subject: sunrpc/cache: fix module refcnt leak in a failure path Don't forget to release the module refcnt if seq_open() returns failure. Signed-off-by: Li Zefan Cc: J. Bruce Fields Cc: Neil Brown Cc: Trond Myklebust Signed-off-by: Andrew Morton Signed-off-by: J. Bruce Fields --- net/sunrpc/cache.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'net') diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c index 3e1ef8bf4dc2..a3f340c8b79a 100644 --- a/net/sunrpc/cache.c +++ b/net/sunrpc/cache.c @@ -1244,8 +1244,10 @@ static int content_open(struct inode *inode, struct file *file, if (!cd || !try_module_get(cd->owner)) return -EACCES; han = __seq_open_private(file, &cache_content_op, sizeof(*han)); - if (han == NULL) + if (han == NULL) { + module_put(cd->owner); return -ENOMEM; + } han->cd = cd; return 0; -- cgit v1.2.2 From 788e69e548cc8d127b90f0de1f7b7e983d1d587a Mon Sep 17 00:00:00 2001 From: "J. Bruce Fields" Date: Mon, 29 Mar 2010 21:02:31 -0400 Subject: svcrpc: don't hold sv_lock over svc_xprt_put() svc_xprt_put() can call tcp_close(), which can sleep, so we shouldn't be holding this lock. In fact, only the xpt_list removal and the sv_tmpcnt decrement should need the sv_lock here. Reported-by: Mi Jinlong Signed-off-by: J. Bruce Fields --- net/sunrpc/svc_xprt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'net') diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c index 8f0f1fb3dc52..c334f5413c60 100644 --- a/net/sunrpc/svc_xprt.c +++ b/net/sunrpc/svc_xprt.c @@ -892,12 +892,12 @@ void svc_delete_xprt(struct svc_xprt *xprt) */ if (test_bit(XPT_TEMP, &xprt->xpt_flags)) serv->sv_tmpcnt--; + spin_unlock_bh(&serv->sv_lock); while ((dr = svc_deferred_dequeue(xprt)) != NULL) kfree(dr); svc_xprt_put(xprt); - spin_unlock_bh(&serv->sv_lock); } void svc_close_xprt(struct svc_xprt *xprt) -- cgit v1.2.2 From b48fa6b99100dc7772af3cd276035fcec9719ceb Mon Sep 17 00:00:00 2001 From: Neil Brown Date: Mon, 1 Mar 2010 16:51:14 +1100 Subject: sunrpc: centralise most calls to svc_xprt_received svc_xprt_received must be called when ->xpo_recvfrom has finished receiving a message, so that the XPT_BUSY flag will be cleared and if necessary, requeued for further work. This call is currently made in each ->xpo_recvfrom function, often from multiple different points. In each case it is the earliest point on a particular path where it is known that the protection provided by XPT_BUSY is no longer needed. However there are (still) some error paths which do not call svc_xprt_received, and requiring each ->xpo_recvfrom to make the call does not encourage robustness. So: move the svc_xprt_received call to be made just after the call to ->xpo_recvfrom(), and move it of the various ->xpo_recvfrom methods. This means that it may not be called at the earliest possible instant, but this is unlikely to be a measurable performance issue. Note that there are still other calls to svc_xprt_received as it is also needed when an xprt is newly created. Signed-off-by: NeilBrown Signed-off-by: J. Bruce Fields --- net/sunrpc/svc_xprt.c | 4 +++- net/sunrpc/svcsock.c | 15 +-------------- net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 3 --- 3 files changed, 4 insertions(+), 18 deletions(-) (limited to 'net') diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c index c334f5413c60..75f9aa2eb089 100644 --- a/net/sunrpc/svc_xprt.c +++ b/net/sunrpc/svc_xprt.c @@ -743,8 +743,10 @@ int svc_recv(struct svc_rqst *rqstp, long timeout) if (rqstp->rq_deferred) { svc_xprt_received(xprt); len = svc_deferred_recv(rqstp); - } else + } else { len = xprt->xpt_ops->xpo_recvfrom(rqstp); + svc_xprt_received(xprt); + } dprintk("svc: got len=%d\n", len); } diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c index a29f259204e6..a33892733643 100644 --- a/net/sunrpc/svcsock.c +++ b/net/sunrpc/svcsock.c @@ -547,7 +547,6 @@ static int svc_udp_recvfrom(struct svc_rqst *rqstp) dprintk("svc: recvfrom returned error %d\n", -err); set_bit(XPT_DATA, &svsk->sk_xprt.xpt_flags); } - svc_xprt_received(&svsk->sk_xprt); return -EAGAIN; } len = svc_addr_len(svc_addr(rqstp)); @@ -562,11 +561,6 @@ static int svc_udp_recvfrom(struct svc_rqst *rqstp) svsk->sk_sk->sk_stamp = skb->tstamp; set_bit(XPT_DATA, &svsk->sk_xprt.xpt_flags); /* there may be more data... */ - /* - * Maybe more packets - kick another thread ASAP. - */ - svc_xprt_received(&svsk->sk_xprt); - len = skb->len - sizeof(struct udphdr); rqstp->rq_arg.len = len; @@ -917,7 +911,6 @@ static int svc_tcp_recv_record(struct svc_sock *svsk, struct svc_rqst *rqstp) if (len < want) { dprintk("svc: short recvfrom while reading record " "length (%d of %d)\n", len, want); - svc_xprt_received(&svsk->sk_xprt); goto err_again; /* record header not complete */ } @@ -953,7 +946,6 @@ static int svc_tcp_recv_record(struct svc_sock *svsk, struct svc_rqst *rqstp) if (len < svsk->sk_reclen) { dprintk("svc: incomplete TCP record (%d of %d)\n", len, svsk->sk_reclen); - svc_xprt_received(&svsk->sk_xprt); goto err_again; /* record not complete */ } len = svsk->sk_reclen; @@ -961,14 +953,11 @@ static int svc_tcp_recv_record(struct svc_sock *svsk, struct svc_rqst *rqstp) return len; error: - if (len == -EAGAIN) { + if (len == -EAGAIN) dprintk("RPC: TCP recv_record got EAGAIN\n"); - svc_xprt_received(&svsk->sk_xprt); - } return len; err_delete: set_bit(XPT_CLOSE, &svsk->sk_xprt.xpt_flags); - svc_xprt_received(&svsk->sk_xprt); err_again: return -EAGAIN; } @@ -1110,7 +1099,6 @@ out: svsk->sk_tcplen = 0; svc_xprt_copy_addrs(rqstp, &svsk->sk_xprt); - svc_xprt_received(&svsk->sk_xprt); if (serv->sv_stats) serv->sv_stats->nettcpcnt++; @@ -1119,7 +1107,6 @@ out: err_again: if (len == -EAGAIN) { dprintk("RPC: TCP recvfrom got EAGAIN\n"); - svc_xprt_received(&svsk->sk_xprt); return len; } error: diff --git a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c index f92e37eb413c..0194de814933 100644 --- a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c +++ b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c @@ -566,7 +566,6 @@ static int rdma_read_complete(struct svc_rqst *rqstp, ret, rqstp->rq_arg.len, rqstp->rq_arg.head[0].iov_base, rqstp->rq_arg.head[0].iov_len); - svc_xprt_received(rqstp->rq_xprt); return ret; } @@ -665,7 +664,6 @@ int svc_rdma_recvfrom(struct svc_rqst *rqstp) rqstp->rq_arg.head[0].iov_len); rqstp->rq_prot = IPPROTO_MAX; svc_xprt_copy_addrs(rqstp, xprt); - svc_xprt_received(xprt); return ret; close_out: @@ -678,6 +676,5 @@ int svc_rdma_recvfrom(struct svc_rqst *rqstp) */ set_bit(XPT_CLOSE, &xprt->xpt_flags); defer: - svc_xprt_received(xprt); return 0; } -- cgit v1.2.2