* [PATCH 2/2] sunrpc/cache: centralise handling of size limit on deferred list.
2010-10-07 4:29 [PATCH 0/2] revised sunrpc deferral patches NeilBrown
2010-10-07 4:29 ` [PATCH 1/2] sunrpc: Simplify cache_defer_req and related functions NeilBrown
@ 2010-10-07 4:29 ` NeilBrown
1 sibling, 0 replies; 4+ messages in thread
From: NeilBrown @ 2010-10-07 4:29 UTC (permalink / raw
To: J. Bruce Fields; +Cc: linux-nfs
We limit the number of 'defer' requests to DFR_MAX.
The imposition of this limit is spread about a bit - sometime we don't
add new things to the list, sometimes we remove old things.
Also it is currently applied to requests which we are 'waiting' for
rather than 'deferring'. This doesn't seem ideal as 'waiting'
requests are naturally limited by the number of threads.
So gather the DFR_MAX handling code to one place and only apply it to
requests that are actually being deferred.
This means that not all 'cache_deferred_req' structures go on the
'cache_defer_list, so we need to be careful when adding and removing
things.
Signed-off-by: NeilBrown <neilb@suse.de>
---
net/sunrpc/cache.c | 67 +++++++++++++++++++++++++++++++++-------------------
1 files changed, 43 insertions(+), 24 deletions(-)
diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
index a438a9c..e5a1867 100644
--- a/net/sunrpc/cache.c
+++ b/net/sunrpc/cache.c
@@ -513,22 +513,25 @@ static int cache_defer_cnt;
static void __unhash_deferred_req(struct cache_deferred_req *dreq)
{
- list_del_init(&dreq->recent);
hlist_del_init(&dreq->hash);
- cache_defer_cnt--;
+ if (!list_empty(&dreq->recent)) {
+ list_del_init(&dreq->recent);
+ cache_defer_cnt--;
+ }
}
static void __hash_deferred_req(struct cache_deferred_req *dreq, struct cache_head *item)
{
int hash = DFR_HASH(item);
- list_add(&dreq->recent, &cache_defer_list);
+ INIT_LIST_HEAD(&dreq->recent);
hlist_add_head(&dreq->hash, &cache_defer_hash[hash]);
}
-static void setup_deferral(struct cache_deferred_req *dreq, struct cache_head *item)
+static void setup_deferral(struct cache_deferred_req *dreq,
+ struct cache_head *item,
+ int count_me)
{
- struct cache_deferred_req *discard;
dreq->item = item;
@@ -536,18 +539,13 @@ static void setup_deferral(struct cache_deferred_req *dreq, struct cache_head *i
__hash_deferred_req(dreq, item);
- /* it is in, now maybe clean up */
- discard = NULL;
- if (++cache_defer_cnt > DFR_MAX) {
- discard = list_entry(cache_defer_list.prev,
- struct cache_deferred_req, recent);
- __unhash_deferred_req(discard);
+ if (count_me) {
+ cache_defer_cnt++;
+ list_add(&dreq->recent, &cache_defer_list);
}
+
spin_unlock(&cache_defer_lock);
- if (discard)
- /* there was one too many */
- discard->revisit(discard, 1);
}
struct thread_deferred_req {
@@ -570,7 +568,7 @@ static void cache_wait_req(struct cache_req *req, struct cache_head *item)
sleeper.completion = COMPLETION_INITIALIZER_ONSTACK(sleeper.completion);
dreq->revisit = cache_restart_thread;
- setup_deferral(dreq, item);
+ setup_deferral(dreq, item, 0);
if (!test_bit(CACHE_PENDING, &item->flags) ||
wait_for_completion_interruptible_timeout(
@@ -594,19 +592,38 @@ static void cache_wait_req(struct cache_req *req, struct cache_head *item)
}
}
+static void cache_limit_defers(void)
+{
+ /* Make sure we haven't exceed the limit of allowed deferred
+ * requests.
+ */
+ struct cache_deferred_req *discard = NULL;
+
+ if (cache_defer_cnt <= DFR_MAX)
+ return;
+
+ spin_lock(&cache_defer_lock);
+
+ /* Consider removing either the first or the last */
+ if (cache_defer_cnt > DFR_MAX) {
+ if (net_random() & 1)
+ discard = list_entry(cache_defer_list.next,
+ struct cache_deferred_req, recent);
+ else
+ discard = list_entry(cache_defer_list.prev,
+ struct cache_deferred_req, recent);
+ __unhash_deferred_req(discard);
+ }
+ spin_unlock(&cache_defer_lock);
+ if (discard)
+ discard->revisit(discard, 1);
+}
+
static void cache_defer_req(struct cache_req *req, struct cache_head *item)
{
struct cache_deferred_req *dreq;
int timeout;
- if (cache_defer_cnt >= DFR_MAX)
- /* too much in the cache, randomly drop this one,
- * or continue and drop the oldest
- */
- if (net_random()&1)
- return;
-
-
if (req->thread_wait) {
cache_wait_req(req, item, timeout);
if (!test_bit(CACHE_PENDING, &item->flags))
@@ -615,12 +632,14 @@ static void cache_defer_req(struct cache_req *req, struct cache_head *item)
dreq = req->defer(req);
if (dreq == NULL)
return;
- setup_deferral(dreq, item);
+ setup_deferral(dreq, item, 1);
if (!test_bit(CACHE_PENDING, &item->flags))
/* Bit could have been cleared before we managed to
* set up the deferral, so need to revisit just in case
*/
cache_revisit_request(item);
+
+ cache_limit_defers();
}
static void cache_revisit_request(struct cache_head *item)
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 1/2] sunrpc: Simplify cache_defer_req and related functions.
2010-10-07 4:29 [PATCH 0/2] revised sunrpc deferral patches NeilBrown
@ 2010-10-07 4:29 ` NeilBrown
2010-10-12 0:07 ` J. Bruce Fields
2010-10-07 4:29 ` [PATCH 2/2] sunrpc/cache: centralise handling of size limit on deferred list NeilBrown
1 sibling, 1 reply; 4+ messages in thread
From: NeilBrown @ 2010-10-07 4:29 UTC (permalink / raw
To: J. Bruce Fields; +Cc: linux-nfs
The return value from cache_defer_req is somewhat confusing.
Various different error codes are returned, but the single caller is
only interested in success or failure.
In fact it can measure this success or failure itself by checking
CACHE_PENDING, which makes the point of the code more explicit.
So change cache_defer_req to return 'void' and test CACHE_PENDING
after it completes, to see if the request was actually deferred or
not.
Similarly setup_deferral and cache_wait_req don't need a return value,
so make them void and remove some code.
The call to cache_revisit_request (to guard against a race) is only
needed for the second call to setup_deferral, so move it out of
setup_deferral to after that second call. With the first call the
race is handled differently (by explicitly calling
'wait_for_completion').
Signed-off-by: NeilBrown <neilb@suse.de>
---
net/sunrpc/cache.c | 59 ++++++++++++++++++++--------------------------------
1 files changed, 23 insertions(+), 36 deletions(-)
diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
index 1e72cc9..a438a9c 100644
--- a/net/sunrpc/cache.c
+++ b/net/sunrpc/cache.c
@@ -38,7 +38,7 @@
#define RPCDBG_FACILITY RPCDBG_CACHE
-static int cache_defer_req(struct cache_req *req, struct cache_head *item);
+static void cache_defer_req(struct cache_req *req, struct cache_head *item);
static void cache_revisit_request(struct cache_head *item);
static void cache_init(struct cache_head *h)
@@ -269,7 +269,8 @@ int cache_check(struct cache_detail *detail,
}
if (rv == -EAGAIN) {
- if (cache_defer_req(rqstp, h) < 0) {
+ cache_defer_req(rqstp, h);
+ if (!test_bit(CACHE_PENDING, &h->flags)) {
/* Request is not deferred */
rv = cache_is_valid(detail, h);
if (rv == -EAGAIN)
@@ -525,7 +526,7 @@ static void __hash_deferred_req(struct cache_deferred_req *dreq, struct cache_he
hlist_add_head(&dreq->hash, &cache_defer_hash[hash]);
}
-static int setup_deferral(struct cache_deferred_req *dreq, struct cache_head *item)
+static void setup_deferral(struct cache_deferred_req *dreq, struct cache_head *item)
{
struct cache_deferred_req *discard;
@@ -547,13 +548,6 @@ static int setup_deferral(struct cache_deferred_req *dreq, struct cache_head *it
if (discard)
/* there was one too many */
discard->revisit(discard, 1);
-
- if (!test_bit(CACHE_PENDING, &item->flags)) {
- /* must have just been validated... */
- cache_revisit_request(item);
- return -EAGAIN;
- }
- return 0;
}
struct thread_deferred_req {
@@ -568,18 +562,17 @@ static void cache_restart_thread(struct cache_deferred_req *dreq, int too_many)
complete(&dr->completion);
}
-static int cache_wait_req(struct cache_req *req, struct cache_head *item)
+static void cache_wait_req(struct cache_req *req, struct cache_head *item)
{
struct thread_deferred_req sleeper;
struct cache_deferred_req *dreq = &sleeper.handle;
- int ret;
sleeper.completion = COMPLETION_INITIALIZER_ONSTACK(sleeper.completion);
dreq->revisit = cache_restart_thread;
- ret = setup_deferral(dreq, item);
+ setup_deferral(dreq, item);
- if (ret ||
+ if (!test_bit(CACHE_PENDING, &item->flags) ||
wait_for_completion_interruptible_timeout(
&sleeper.completion, req->thread_wait) <= 0) {
/* The completion wasn't completed, so we need
@@ -599,41 +592,35 @@ static int cache_wait_req(struct cache_req *req, struct cache_head *item)
wait_for_completion(&sleeper.completion);
}
}
- if (test_bit(CACHE_PENDING, &item->flags)) {
- /* item is still pending, try request
- * deferral
- */
- return -ETIMEDOUT;
- }
- /* only return success if we actually deferred the
- * request. In this case we waited until it was
- * answered so no deferral has happened - rather
- * an answer already exists.
- */
- return -EEXIST;
}
-static int cache_defer_req(struct cache_req *req, struct cache_head *item)
+static void cache_defer_req(struct cache_req *req, struct cache_head *item)
{
struct cache_deferred_req *dreq;
- int ret;
+ int timeout;
- if (cache_defer_cnt >= DFR_MAX) {
+ if (cache_defer_cnt >= DFR_MAX)
/* too much in the cache, randomly drop this one,
* or continue and drop the oldest
*/
if (net_random()&1)
- return -ENOMEM;
- }
+ return;
+
+
if (req->thread_wait) {
- ret = cache_wait_req(req, item);
- if (ret != -ETIMEDOUT)
- return ret;
+ cache_wait_req(req, item, timeout);
+ if (!test_bit(CACHE_PENDING, &item->flags))
+ return;
}
dreq = req->defer(req);
if (dreq == NULL)
- return -ENOMEM;
- return setup_deferral(dreq, item);
+ return;
+ setup_deferral(dreq, item);
+ if (!test_bit(CACHE_PENDING, &item->flags))
+ /* Bit could have been cleared before we managed to
+ * set up the deferral, so need to revisit just in case
+ */
+ cache_revisit_request(item);
}
static void cache_revisit_request(struct cache_head *item)
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 0/2] revised sunrpc deferral patches
@ 2010-10-07 4:29 NeilBrown
2010-10-07 4:29 ` [PATCH 1/2] sunrpc: Simplify cache_defer_req and related functions NeilBrown
2010-10-07 4:29 ` [PATCH 2/2] sunrpc/cache: centralise handling of size limit on deferred list NeilBrown
0 siblings, 2 replies; 4+ messages in thread
From: NeilBrown @ 2010-10-07 4:29 UTC (permalink / raw
To: J. Bruce Fields; +Cc: linux-nfs
Hi Bruce,
Here is the incremental clean-up as requested, plus I've fixed the
problem I found with the cache_limit_deferers patch.
I think that'll do for now.
I cannot think of any better way to do what lockd does with
deferrals, so I think we need to keep deferrals after all.
Thanks,
NeilBrown
---
NeilBrown (2):
sunrpc: Simplify cache_defer_req and related functions.
sunrpc/cache: centralise handling of size limit on deferred list.
net/sunrpc/cache.c | 110 +++++++++++++++++++++++++++-------------------------
1 files changed, 58 insertions(+), 52 deletions(-)
--
Signature
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] sunrpc: Simplify cache_defer_req and related functions.
2010-10-07 4:29 ` [PATCH 1/2] sunrpc: Simplify cache_defer_req and related functions NeilBrown
@ 2010-10-12 0:07 ` J. Bruce Fields
0 siblings, 0 replies; 4+ messages in thread
From: J. Bruce Fields @ 2010-10-12 0:07 UTC (permalink / raw
To: NeilBrown; +Cc: linux-nfs
On Thu, Oct 07, 2010 at 03:29:46PM +1100, NeilBrown wrote:
> The return value from cache_defer_req is somewhat confusing.
> Various different error codes are returned, but the single caller is
> only interested in success or failure.
>
> In fact it can measure this success or failure itself by checking
> CACHE_PENDING, which makes the point of the code more explicit.
>
> So change cache_defer_req to return 'void' and test CACHE_PENDING
> after it completes, to see if the request was actually deferred or
> not.
>
> Similarly setup_deferral and cache_wait_req don't need a return value,
> so make them void and remove some code.
>
> The call to cache_revisit_request (to guard against a race) is only
> needed for the second call to setup_deferral, so move it out of
> setup_deferral to after that second call. With the first call the
> race is handled differently (by explicitly calling
> 'wait_for_completion').
Thanks, applied both of these. But, this:
> -static int cache_wait_req(struct cache_req *req, struct cache_head *item)
...
> + cache_wait_req(req, item, timeout);
suggests this version of the patch wasn't really tested! Would you mind
doing some testing on the version I've just pushed out?
--b.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2010-10-12 0:07 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-10-07 4:29 [PATCH 0/2] revised sunrpc deferral patches NeilBrown
2010-10-07 4:29 ` [PATCH 1/2] sunrpc: Simplify cache_defer_req and related functions NeilBrown
2010-10-12 0:07 ` J. Bruce Fields
2010-10-07 4:29 ` [PATCH 2/2] sunrpc/cache: centralise handling of size limit on deferred list NeilBrown
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.