Some improvements to request deferral and related code

'loose' was a mis-spelling of 'lose', and even that wasn't a good word choice.
So give this function a more useful name.

Signed-off-by NeilBrown <[email protected]>

---

 net/sunrpc/cache.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

net/sunrpc/cache.c +4 -4

index ff0c230..d19c075 100644
@@ -101,7 +101,7 @@ struct cache_head *sunrpc_cache_lookup(struct cache_detail *detail,
 EXPORT_SYMBOL_GPL(sunrpc_cache_lookup);
 
 
-static void queue_loose(struct cache_detail *detail, struct cache_head *ch);
+static void cache_dequeue(struct cache_detail *detail, struct cache_head *ch);
 
 static int cache_fresh_locked(struct cache_head *head, time_t expiry)
 {

@@ -117,7 +117,7 @@ static void cache_fresh_unlocked(struct cache_head *head,
 		cache_revisit_request(head);
 	if (test_and_clear_bit(CACHE_PENDING, &head->flags)) {
 		cache_revisit_request(head);
-		queue_loose(detail, head);
+		cache_dequeue(detail, head);
 	}
 }
 

@@ -457,7 +457,7 @@ static int cache_clean(void)
 				)
 				continue;
 			if (test_and_clear_bit(CACHE_PENDING, &ch->flags))
-				queue_loose(current_detail, ch);
+				cache_dequeue(current_detail, ch);
 
 			if (atomic_read(&ch->ref.refcount) == 1)
 				break;

@@ -920,7 +920,7 @@ static const struct file_operations cache_file_operations = {
 };
 
 
-static void queue_loose(struct cache_detail *detail, struct cache_head *ch)
+static void cache_dequeue(struct cache_detail *detail, struct cache_head *ch)
 {
 	struct cache_queue *cq;
 	spin_lock(&queue_lock);

While deferred requests normally get revisited quite quickly, it is possible for a request to remain in the deferral queue when the cache item is discarded. We can easily make sure that doesn't happen by calling cache_revisit_request just before the final 'put'.

Also there is a small chance that a race would cause one thread to defer a request against a cache item while another thread is failing to queue and upcall for that item. So when the upcall fails, make sure to revisit all deferred requests.

Signed-off-by NeilBrown <[email protected]>

---

 net/sunrpc/cache.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

net/sunrpc/cache.c +1 -0

index d19c075..44f4516 100644
@@ -221,6 +221,7 @@ int cache_check(struct cache_detail *detail,
 			switch (cache_make_upcall(detail, h)) {
 			case -EINVAL:
 				clear_bit(CACHE_PENDING, &h->flags);
+				cache_revisit_request(h);
 				if (rv == -EAGAIN) {
 					set_bit(CACHE_NEGATIVE, &h->flags);
 					cache_fresh_unlocked(h, detail,

net/sunrpc/cache.c +1 -0

 			cache_put(ch, d);
+		}
 	} else
 		spin_unlock(&cache_list_lock);
 

The extra call to cache_revisit_request in cache_fresh_unlocked is not needed, as should have been fairly clear at the time of
commit 4013edea9a0b6cdcb1fdf5d4011e47e068fd6efb

If there are requests to be revisited, then we can be sure that CACHE_PENDING is set, so the second call is sufficient.

So remove the first call.
Then remove the 'new' parameter,
then remove the return value for cache_fresh_locked which is only used to provide the value for 'new'.

Signed-off-by NeilBrown <[email protected]>

---

 net/sunrpc/cache.c |   23 ++++++++++-------------
 1 files changed, 10 insertions(+), 13 deletions(-)

net/sunrpc/cache.c +10 -13

index 44f4516..c1f897c 100644
@@ -103,18 +103,16 @@ EXPORT_SYMBOL_GPL(sunrpc_cache_lookup);
 
 static void cache_dequeue(struct cache_detail *detail, struct cache_head *ch);
 
-static int cache_fresh_locked(struct cache_head *head, time_t expiry)
+static void cache_fresh_locked(struct cache_head *head, time_t expiry)
 {
 	head->expiry_time = expiry;
 	head->last_refresh = get_seconds();
-	return !test_and_set_bit(CACHE_VALID, &head->flags);
+	set_bit(CACHE_VALID, &head->flags);
 }
 
 static void cache_fresh_unlocked(struct cache_head *head,
-			struct cache_detail *detail, int new)
+				 struct cache_detail *detail)
 {
-	if (new)
-		cache_revisit_request(head);
 	if (test_and_clear_bit(CACHE_PENDING, &head->flags)) {
 		cache_revisit_request(head);
 		cache_dequeue(detail, head);

@@ -130,7 +128,6 @@ struct cache_head *sunrpc_cache_update(struct cache_detail *detail,
 	 */
 	struct cache_head **head;
 	struct cache_head *tmp;
-	int is_new;
 
 	if (!test_bit(CACHE_VALID, &old->flags)) {
 		write_lock(&detail->hash_lock);

@@ -139,9 +136,9 @@ struct cache_head *sunrpc_cache_update(struct cache_detail *detail,
 				set_bit(CACHE_NEGATIVE, &old->flags);
 			else
 				detail->update(old, new);
-			is_new = cache_fresh_locked(old, new->expiry_time);
+			cache_fresh_locked(old, new->expiry_time);
 			write_unlock(&detail->hash_lock);
-			cache_fresh_unlocked(old, detail, is_new);
+			cache_fresh_unlocked(old, detail);
 			return old;
 		}
 		write_unlock(&detail->hash_lock);

@@ -165,11 +162,11 @@ struct cache_head *sunrpc_cache_update(struct cache_detail *detail,
 	*head = tmp;
 	detail->entries++;
 	cache_get(tmp);
-	is_new = cache_fresh_locked(tmp, new->expiry_time);
+	cache_fresh_locked(tmp, new->expiry_time);
 	cache_fresh_locked(old, 0);
 	write_unlock(&detail->hash_lock);
-	cache_fresh_unlocked(tmp, detail, is_new);
-	cache_fresh_unlocked(old, detail, 0);
+	cache_fresh_unlocked(tmp, detail);
+	cache_fresh_unlocked(old, detail);
 	cache_put(old, detail);
 	return tmp;
 }

@@ -224,8 +221,8 @@ int cache_check(struct cache_detail *detail,
 				cache_revisit_request(h);
 				if (rv == -EAGAIN) {
 					set_bit(CACHE_NEGATIVE, &h->flags);
-					cache_fresh_unlocked(h, detail,
-					     cache_fresh_locked(h, get_seconds()+CACHE_NEW_EXPIRY));
+					cache_fresh_locked(h, get_seconds()+CACHE_NEW_EXPIRY);
+					cache_fresh_unlocked(h, detail);
 					rv = -ENOENT;
 				}
 				break;

If cache_defer_req did not leave the request on a queue, then it could possibly have waited long enough that the cache became valid. So check the status after the call.

Signed-off-by NeilBrown <[email protected]>

---

 net/sunrpc/cache.c |   53 ++++++++++++++++++++++++++++++++--------------------
 1 files changed, 33 insertions(+), 20 deletions(-)

net/sunrpc/cache.c +16 -0

index c1f897c..bff4baa 100644
@@ -173,6 +173,22 @@ struct cache_head *sunrpc_cache_update(struct cache_detail *detail,
 EXPORT_SYMBOL_GPL(sunrpc_cache_update);
 
 static int cache_make_upcall(struct cache_detail *detail, 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)
+		return -EAGAIN;
+	else {
+		/* entry is valid */
+		if (test_bit(CACHE_NEGATIVE, &h->flags))
+			return -ENOENT;
+		else
+			return 0;
+	}
+}
 /*
  * This is the generic cache management routine for all
  * the authentication caches.

net/sunrpc/cache.c +3 -11

+ *           upcall completed but item is still invalid (implying that
+ *           the cache item has been replaced with a newer one).
  * -ENOENT if cache entry was negative
  */
 int cache_check(struct cache_detail *detail,

@@ -192,17 +210,7 @@ int cache_check(struct cache_detail *detail,
 	long refresh_age, age;
 
 	/* First decide return status as best we can */
-	if (!test_bit(CACHE_VALID, &h->flags) ||
-	    h->expiry_time < get_seconds())
-		rv = -EAGAIN;
-	else if (detail->flush_time > h->last_refresh)
-		rv = -EAGAIN;
-	else {
-		/* entry is valid */
-		if (test_bit(CACHE_NEGATIVE, &h->flags))
-			rv = -ENOENT;
-		else rv = 0;
-	}
+	rv = cache_is_valid(detail, h);
 
 	/* now see if we want to start an upcall */
 	refresh_age = (h->expiry_time - h->last_refresh);

net/sunrpc/cache.c +9 -3

+			rv = cache_is_valid(detail, h);
+			if (rv == -EAGAIN)
+				rv = -ETIMEDOUT;
+		}
+	}
 	if (rv)
 		cache_put(h, detail);
 	return rv;

@@ -557,11 +569,11 @@ static int cache_defer_req(struct cache_req *req, struct cache_head *item)
 		 * or continue and drop the oldest below
 		 */
 		if (net_random()&1)
-			return -ETIMEDOUT;
+			return 0;
 	}
 	dreq = req->defer(req);
 	if (dreq == NULL)
-		return -ETIMEDOUT;
+		return 0;
 
 	dreq->item = item;
 

@@ -591,8 +603,9 @@ static int cache_defer_req(struct cache_req *req, struct cache_head *item)
 	if (!test_bit(CACHE_PENDING, &item->flags)) {
 		/* must have just been validated... */
 		cache_revisit_request(item);
+		return 0;
 	}
-	return 0;
+	return 1;
 }
 
 static void cache_revisit_request(struct cache_head *item)

Quoted 4 lines · NeilBrown

If cache_defer_req did not leave the request on a queue, then it could possibly have waited long enough that the cache became valid. So check the status after the call.

Quoted 6 lines

Signed-off-by NeilBrown <[email protected]>

* Returns 0 if the cache_head can be used, or cache_puts it and returns
- * -EAGAIN if upcall is pending,
- * -ETIMEDOUT if upcall failed and should be retried,
+ * -EAGAIN if upcall is pending and request has been queued
+ * -ETIMEDOUT if upcall failed or request could not be queue or

s/queue/queued/

Review int cache_check(struct cache_detail *detail, · line 248

Quoted 11 lines

(quoted diff) +3 -4

@@ -235,10 +243,14 @@ int cache_check(struct cache_detail *detail,
 		}
 	}
 
-	if (rv == -EAGAIN)
-		if (cache_defer_req(rqstp, h) != 0)
-			rv = -ETIMEDOUT;
-
+	if (rv == -EAGAIN) {
+		if (cache_defer_req(rqstp, h) == 0) {
+			/* Request is not deferred */

The code might be more self-explanatory if we wrote:

		if (cache_defer_req(rqstp, h) == -ETIMEDOUT) {

Well, at least it would be obvious we're handling the "failure" case? (Even if admittedly it's a "failure" that we may be able to handle).

It always takes me a little thought whenever I encounter a boolean-returning function whose name doesn't have an obvious truth value (list_empty, cache_is_valid).

From c970b6abce98044de573336b3a867b7ed39642e4 Mon Sep 17 00:00:00 2001

Quoted 5 lines · On Tuesday August 4, [email protected] wrote:

Quoted 4 lines · NeilBrown

If cache_defer_req did not leave the request on a queue, then it could possibly have waited long enough that the cache became valid. So check the status after the call.

Quoted 9 lines

Quoted 6 lines

Signed-off-by NeilBrown <[email protected]>

* Returns 0 if the cache_head can be used, or cache_puts it and returns
- * -EAGAIN if upcall is pending,
- * -ETIMEDOUT if upcall failed and should be retried,
+ * -EAGAIN if upcall is pending and request has been queued
+ * -ETIMEDOUT if upcall failed or request could not be queue or

s/queue/queued/

:-)

Quoted 14 lines

net/sunrpc/cache.c int cache_check(struct cache_detail *detail, · line 248

Quoted 11 lines

net/sunrpc/cache.c +3 -4

@@ -235,10 +243,14 @@ int cache_check(struct cache_detail *detail,
 		}
 	}
 
-	if (rv == -EAGAIN)
-		if (cache_defer_req(rqstp, h) != 0)
-			rv = -ETIMEDOUT;
-
+	if (rv == -EAGAIN) {
+		if (cache_defer_req(rqstp, h) == 0) {
+			/* Request is not deferred */

The code might be more self-explanatory if we wrote:

Quoted 8 lines

		if (cache_defer_req(rqstp, h) == -ETIMEDOUT) {

Well, at least it would be obvious we're handling the "failure" case? (Even if admittedly it's a "failure" that we may be able to handle).

It always takes me a little thought whenever I encounter a boolean-returning function whose name doesn't have an obvious truth value (list_empty, cache_is_valid).

I certainly see you point. For consistency in the kernel, if the function name doesn't sound like a boolean it should return 0 or positive on success and negative for error.

But despite that I changed cache_defer_req to return 0 or 1 rather than -ETIMEDOUT or 0...

There are three possibly results of cache_defer_req:
a/ the request has been stored for later processing b/ there was a failure while trying to store the request c/ there was no need to store the request because the cache
item is no longer waiting for a reply.

While 'a' is success and 'b' is an error, 'c' doesn't exactly fit in to either. However 'b' and 'c' are treated the same way by cache_check.
So returning '-ETIMEDOUT' for both 'b' and 'c' seemed wrong.

The current return value is a true/false value for the assertion "the request was successfully deferred". But choosing a name for cache_defer_req which makes that meaning obvious seems clumsy.

Thinks.....

Maybe
a -> 0 (success, we deferred the request)
b -> -ENOMEM (failed to find somewhere to store the request) c -> -EAGAIN (something happened .. check again).

and in cache_check we write

   if (cache_defer_req(rqstp, h) < 0) {
         /* Request is not deferred */

which maybe a bit more self explanatory??

NeilBrown

Quoted 1 line

From c970b6abce98044de573336b3a867b7ed39642e4 Mon Sep 17 00:00:00 2001

From: NeilBrown <[email protected]>
Date: Thu, 6 Aug 2009 14:56:13 +1000
Subject: [PATCH] sunrpc/cache: recheck cache validity after cache_defer_req

If cache_defer_req did not leave the request on a queue, then it could possibly have waited long enough that the cache became valid. So check the status after the call.

Signed-off-by NeilBrown <[email protected]>

---
 net/sunrpc/cache.c |   51 ++++++++++++++++++++++++++++++++-------------------
 1 files changed, 32 insertions(+), 19 deletions(-)

net/sunrpc/cache.c +32 -19

index c1f897c..cec2574 100644
@@ -173,6 +173,22 @@ struct cache_head *sunrpc_cache_update(struct cache_detail *detail,
 EXPORT_SYMBOL_GPL(sunrpc_cache_update);
 
 static int cache_make_upcall(struct cache_detail *detail, 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)
+		return -EAGAIN;
+	else {
+		/* entry is valid */
+		if (test_bit(CACHE_NEGATIVE, &h->flags))
+			return -ENOENT;
+		else
+			return 0;
+	}
+}
 /*
  * This is the generic cache management routine for all
  * the authentication caches.

@@ -181,8 +197,10 @@ static int cache_make_upcall(struct cache_detail *detail, struct cache_head *h);
  *
  *
  * Returns 0 if the cache_head can be used, or cache_puts it and returns
- * -EAGAIN if upcall is pending,
- * -ETIMEDOUT if upcall failed and should be retried,
+ * -EAGAIN if upcall is pending and request has been queued
+ * -ETIMEDOUT if upcall failed or request could not be queued or
+ *           upcall completed but item is still invalid (implying that
+ *           the cache item has been replaced with a newer one).
  * -ENOENT if cache entry was negative
  */
 int cache_check(struct cache_detail *detail,

@@ -192,17 +210,7 @@ int cache_check(struct cache_detail *detail,
 	long refresh_age, age;
 
 	/* First decide return status as best we can */
-	if (!test_bit(CACHE_VALID, &h->flags) ||
-	    h->expiry_time < get_seconds())
-		rv = -EAGAIN;
-	else if (detail->flush_time > h->last_refresh)
-		rv = -EAGAIN;
-	else {
-		/* entry is valid */
-		if (test_bit(CACHE_NEGATIVE, &h->flags))
-			rv = -ENOENT;
-		else rv = 0;
-	}
+	rv = cache_is_valid(detail, h);
 
 	/* now see if we want to start an upcall */
 	refresh_age = (h->expiry_time - h->last_refresh);

@@ -235,10 +243,14 @@ int cache_check(struct cache_detail *detail,
 		}
 	}
 
-	if (rv == -EAGAIN)
-		if (cache_defer_req(rqstp, h) != 0)
-			rv = -ETIMEDOUT;
-
+	if (rv == -EAGAIN) {
+		if (cache_defer_req(rqstp, h) < 0) {
+			/* Request is not deferred */
+			rv = cache_is_valid(detail, h);
+			if (rv == -EAGAIN)
+				rv = -ETIMEDOUT;
+		}
+	}
 	if (rv)
 		cache_put(h, detail);
 	return rv;

@@ -557,11 +569,11 @@ static int cache_defer_req(struct cache_req *req, struct cache_head *item)
 		 * or continue and drop the oldest below
 		 */
 		if (net_random()&1)
-			return -ETIMEDOUT;
+			return -ENOMEM;
 	}
 	dreq = req->defer(req);
 	if (dreq == NULL)
-		return -ETIMEDOUT;
+		return -ENOMEM;
 
 	dreq->item = item;
 

@@ -591,6 +603,7 @@ static int cache_defer_req(struct cache_req *req, struct cache_head *item)
 	if (!test_bit(CACHE_PENDING, &item->flags)) {
 		/* must have just been validated... */
 		cache_revisit_request(item);
+		return -EAGAIN;
 	}
 	return 0;
 }

Signature · 1 line

1.6.3.3

Yes, OK, seems reasonable enough.

So, apologies, I've lost track of the rest of your patches, but I'm still very much interested. Will you have a chance to rebase and resend? My for-2.6.32 branch at

	git://linux-nfs.org/~bfields/linux.git for-2.6.32

has what I've applied so far, plus a merge of Trond's queued patches (which includes some changes to the cache code).

In cache_defer_req, 'dreq' is used for two significantly different values that happen to be of the same type.

This is both confusing, and make it hard to extend the range of one of the values as we will in the next patch.
So introduce 'discard' to take one of the values.

Signed-off-by NeilBrown <[email protected]>

---

 net/sunrpc/cache.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

net/sunrpc/cache.c +1 -1

index 2f19463..4892c5c 100644
@@ -561,7 +561,7 @@ static int cache_defer_cnt;
 
 static int cache_defer_req(struct cache_req *req, struct cache_head *item)
 {
-	struct cache_deferred_req *dreq;
+	struct cache_deferred_req *dreq, *discard;
 	int hash = DFR_HASH(item);
 
 	if (cache_defer_cnt >= DFR_MAX) {

@@ -586,20 +586,20 @@ static int cache_defer_req(struct cache_req *req, struct cache_head *item)
 	list_add(&dreq->hash, &cache_defer_hash[hash]);
 

net/sunrpc/cache.c +3 -3

 		cache_defer_cnt--;
 	}
 	spin_unlock(&cache_defer_lock);
 
-	if (dreq) {
+	if (discard)
 		/* there was one too many */
-		dreq->revisit(dreq, 1);
-	}
+		discard->revisit(discard, 1);
+
 	if (!test_bit(CACHE_PENDING, &item->flags)) {
 		/* must have just been validated... */
 		cache_revisit_request(item);

Quoted 8 lines · NeilBrown

In cache_defer_req, 'dreq' is used for two significantly different values that happen to be of the same type.

This is both confusing, and make it hard to extend the range of one of the values as we will in the next patch.
So introduce 'discard' to take one of the values.

Signed-off-by NeilBrown <[email protected]>

net/sunrpc/cache.c static int cache_defer_req(struct cache_req *req, struct cache_head *item) · line 594

Quoted 13 lines

net/sunrpc/cache.c +3 -3

@@ -586,20 +586,20 @@ static int cache_defer_req(struct cache_req *req, struct cache_head *item)
 	list_add(&dreq->hash, &cache_defer_hash[hash]);
 
 	/* it is in, now maybe clean up */
-	dreq = NULL;
+	discard = NULL;
 	if (++cache_defer_cnt > DFR_MAX) {
-		dreq = list_entry(cache_defer_list.prev,
-				  struct cache_deferred_req, recent);
+		discard = list_entry(cache_defer_list.prev,
+				     struct cache_deferred_req, recent);
 		list_del_init(&dreq->recent);
 		list_del_init(&dreq->hash);

Shouldn't these two "dreq"'s be "discard"'s as well?

When we look up an entry in the uid->gidlist cache, we take a reference to the content but don't drop the reference to the cache entry. So it never gets freed.

Signed-off-by NeilBrown <[email protected]>

---

 net/sunrpc/svcauth_unix.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

net/sunrpc/svcauth_unix.c +1 -0

index f2de152..0afeb30 100644
@@ -660,6 +660,7 @@ static int unix_gid_find(uid_t uid, struct group_info **gip,
 	case 0:
 		*gip = ug->gi;
 		get_group_info(*gip);
+		cache_put(&ug->h, &unix_gid_cache);
 		return 0;
 	case -ETIMEDOUT:
 		prevug = ug;

Hi Bruce and all.

When I wrote the authentication cache all those years ago I handled cache misses by arranging to simply retry the whole request, either by saving a copy for be replayed later, or by waiting for the client to resend.

As we know this isn't ideal. Large requests (e.g. writes) don't get saved, TCP connections don't resend in a suitable time, and NFSv4 request are sufficiently complex that restarting from the beginning isn't really a good idea.

So I have finally implemented the "wait a while" approach which has probably been suggested several times and is even implemented in a somewhat hackish way for nfsidmap.

Yes, I know I have probably argued against this in the past. I was wrong.

This series fixes a few little bugs and tidies up some code but does two main important things.

1/ 'allow thread to block....' will wait a little while if there is a cache miss. If an answer is available in that time, it continues on it's merry way. If no answer arrives, the old deferral approach is used. It waits 5 seconds if there are spare nfsd threads, and 1 second if there all threads are busy. I have almost nothing with which to justify these numbers.

From the protocol point of view I don't know if there's any rule of

2/ 'close connection when...' provides a better fall back for TCP. If the sunrpc layer decides to drop the request without queueing it at all, it will close the connection. This encourages the client to retry sooner.

Please review and comment.

If you like them all, they can be pulled from

      git://neil.brown.name/linux-2.6/ nfsd

Thanks,
NeilBrown

---

NeilBrown (12):
sunrpc: close connection when a request is irretrievably lost. sunrpc/cache: change deferred-request hash table to use hlist.
sunrpc: fix memory leak in unix_gid cache.
nfsd/idmap: drop special request deferal in favour of improved default. sunrpc/cache: retry cache lookups that return -ETIMEDOUT sunrpc/cache: allow thread to block while waiting for cache update. sunrpc/cache: avoid variable over-loading in cache_defer_req sunrpc/cache: use list_del_init for the list_head entries in cache_deferred_req sunrpc/cache: recheck cache validity after cache_defer_req sunrpc/cache: simplify cache_fresh_locked and cache_fresh_unlocked. sunrpc/cache: make sure deferred requests eventually get revisited. sunrpc/cache: rename queue_loose to cache_dequeue

 fs/nfsd/export.c               |   18 ++++
 fs/nfsd/nfs4idmap.c            |  105 +++---------------------
 include/linux/sunrpc/cache.h   |    5 +
 include/linux/sunrpc/svcauth.h |   10 ++
 net/sunrpc/cache.c             |  173 ++++++++++++++++++++++++++--------------
 net/sunrpc/svc.c               |    3 +
 net/sunrpc/svc_xprt.c          |   11 +++
 net/sunrpc/svcauth_unix.c      |   31 ++++++-
 8 files changed, 192 insertions(+), 164 deletions(-)

Quoted 9 lines · NeilBrown

This series fixes a few little bugs and tidies up some code but does two main important things.

1/ 'allow thread to block....' will wait a little while if there is a cache miss. If an answer is available in that time, it continues on it's merry way. If no answer arrives, the old deferral approach is used. It waits 5 seconds if there are spare nfsd threads, and 1 second if there all threads are busy. I have almost nothing with which to justify these numbers.

I think the v4 server at least should return NFS4ERR_DELAY in this case instead of doing the internal replay. That avoids possible problems with non-idempotent compound ops.

Quoted 2 lines

From the protocol point of view I don't know if there's any rule of

thumb about when it'd be best to return DELAY. Perhaps it's best to avoid it whenever possible, but when the delay is on the order of seconds it sounds reasonable to me.

Quoted 11 lines · On Tuesday August 4, [email protected] wrote:

Quoted 9 lines · NeilBrown

This series fixes a few little bugs and tidies up some code but does two main important things.

1/ 'allow thread to block....' will wait a little while if there is a cache miss. If an answer is available in that time, it continues on it's merry way. If no answer arrives, the old deferral approach is used. It waits 5 seconds if there are spare nfsd threads, and 1 second if there all threads are busy. I have almost nothing with which to justify these numbers.

Quoted 3 lines

I think the v4 server at least should return NFS4ERR_DELAY in this case instead of doing the internal replay. That avoids possible problems with non-idempotent compound ops.

If the request has been handed to nfsd, then yes I agree. We probably want some way for nfsd to mark the request as "don't replay" so that an error will propagate out. Currently we map that error to EJUKEBOX for v3 or v4, but you are right, we want ERR_DELAY for v4.

If the request is still in the RPC code (trying to identify the origin or to decode the crypto) then we cannot return ERR_DELAY, but as none of the request will have been processed yet, there is no room for a problem with non-idempotent ops.

It has occurred to me that we could throw away the current request deferral completely: if we don't feel comfortable delaying the thread for as long as it takes, we just return an error or drop the request (closing any connection).
I'm not sure I'd be comfortable doing that if there were only a few (8?) threads though.
Maybe if we got dynamic nfsd threads so that new ones could be created on demand I would feel quite happy to discard the deferral stuff and just use a delay.

Quoted 6 lines

Quoted 3 lines

From the protocol point of view I don't know if there's any rule of

thumb about when it'd be best to return DELAY. Perhaps it's best to avoid it whenever possible, but when the delay is on the order of seconds it sounds reasonable to me.

Of course you don't know how long the delay will be until it happens:-)

But I agree. Delay internally if possible, but as soon as that seems to be awkward (e.g. run out of threads), return DELAY

Thanks,
NeilBrown
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html