Cache deferral improvements - try N+2

The idmap code manages request deferal by waiting for a reply from userspace rather than putting the NFS request on a queue to be retried from the start.
Now that the common deferal code does this there is no need for the special code in idmap.

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

---
 fs/nfsd/nfs4idmap.c |  105 +++++----------------------------------------------
 1 files changed, 11 insertions(+), 94 deletions(-)

fs/nfsd/nfs4idmap.c +11 -94

index c78dbf4..f0695e8 100644
@@ -482,109 +482,26 @@ nfsd_idmap_shutdown(void)
 	cache_unregister(&nametoid_cache);
 }
 
-/*
- * Deferred request handling
- */
-
-struct idmap_defer_req {
-       struct cache_req		req;
-       struct cache_deferred_req deferred_req;
-       wait_queue_head_t	waitq;
-       atomic_t			count;
-};
-
-static inline void
-put_mdr(struct idmap_defer_req *mdr)
-{
-	if (atomic_dec_and_test(&mdr->count))
-		kfree(mdr);
-}
-
-static inline void
-get_mdr(struct idmap_defer_req *mdr)
-{
-	atomic_inc(&mdr->count);
-}
-
-static void
-idmap_revisit(struct cache_deferred_req *dreq, int toomany)
-{
-	struct idmap_defer_req *mdr =
-		container_of(dreq, struct idmap_defer_req, deferred_req);
-
-	wake_up(&mdr->waitq);
-	put_mdr(mdr);
-}
-
-static struct cache_deferred_req *
-idmap_defer(struct cache_req *req)
-{
-	struct idmap_defer_req *mdr =
-		container_of(req, struct idmap_defer_req, req);
-
-	mdr->deferred_req.revisit = idmap_revisit;
-	get_mdr(mdr);
-	return (&mdr->deferred_req);
-}
-
-static inline int
-do_idmap_lookup(struct ent *(*lookup_fn)(struct ent *), struct ent *key,
-		struct cache_detail *detail, struct ent **item,
-		struct idmap_defer_req *mdr)
-{
-	*item = lookup_fn(key);
-	if (!*item)
-		return -ENOMEM;
-	return cache_check(detail, &(*item)->h, &mdr->req);
-}
-
-static inline int
-do_idmap_lookup_nowait(struct ent *(*lookup_fn)(struct ent *),
-			struct ent *key, struct cache_detail *detail,
-			struct ent **item)
-{
-	int ret = -ENOMEM;
-
-	*item = lookup_fn(key);
-	if (!*item)
-		goto out_err;
-	ret = -ETIMEDOUT;
-	if (!test_bit(CACHE_VALID, &(*item)->h.flags)
-			|| (*item)->h.expiry_time < get_seconds()
-			|| detail->flush_time > (*item)->h.last_refresh)
-		goto out_put;
-	ret = -ENOENT;
-	if (test_bit(CACHE_NEGATIVE, &(*item)->h.flags))
-		goto out_put;
-	return 0;
-out_put:
-	cache_put(&(*item)->h, detail);
-out_err:
-	*item = NULL;
-	return ret;
-}
-
 static int
 idmap_lookup(struct svc_rqst *rqstp,
 		struct ent *(*lookup_fn)(struct ent *), struct ent *key,
 		struct cache_detail *detail, struct ent **item)
 {
-	struct idmap_defer_req *mdr;
 	int ret;
 
-	mdr = kzalloc(sizeof(*mdr), GFP_KERNEL);
-	if (!mdr)
+	*item = lookup_fn(key);
+	if (!*item)
 		return -ENOMEM;
-	atomic_set(&mdr->count, 1);
-	init_waitqueue_head(&mdr->waitq);
-	mdr->req.defer = idmap_defer;
-	ret = do_idmap_lookup(lookup_fn, key, detail, item, mdr);
-	if (ret == -EAGAIN) {
-		wait_event_interruptible_timeout(mdr->waitq,
-			test_bit(CACHE_VALID, &(*item)->h.flags), 1 * HZ);
-		ret = do_idmap_lookup_nowait(lookup_fn, key, detail, item);
+ retry:
+	ret = cache_check(detail, &(*item)->h, &rqstp->rq_chandle);
+
+	if (ret == -ETIMEDOUT) {
+		struct ent *prev_item = *item;
+		*item = lookup_fn(key);
+		if (*item != prev_item)
+			goto retry;
+		cache_put(&(*item)->h, detail);
 	}
-	put_mdr(mdr);
 	return ret;
 }
 

If we drop a request in the sunrpc layer, either due kmalloc failure, or due to a cache miss when we could not queue the request for later replay, then close the connection to encourage the client to retry sooner.

Note that if the drop happens in the NFS layer, NFSERR_JUKEBOX (aka NFS4ERR_DELAY) is returned to guide the client concerning replay.

net/sunrpc/svc.c +0 -0

 	case SVC_DROP:
 		goto dropit;
 	case SVC_COMPLETE:

net/sunrpc/svcauth_unix.c +8 -3

index 2073116..e91b550 100644
@@ -674,6 +674,8 @@ static struct group_info *unix_gid_find(uid_t uid, struct svc_rqst *rqstp)
 	switch (ret) {
 	case -ENOENT:
 		return ERR_PTR(-ENOENT);
+	case -ETIMEDOUT:
+		return ERR_PTR(-ESHUTDOWN);
 	case 0:
 		gi = get_group_info(ug->gi);
 		cache_put(&ug->h, &unix_gid_cache);

@@ -720,8 +722,9 @@ svcauth_unix_set_client(struct svc_rqst *rqstp)
 	switch (cache_check(&ip_map_cache, &ipm->h, &rqstp->rq_chandle)) {
 		default:
 			BUG();
-		case -EAGAIN:
 		case -ETIMEDOUT:
+			return SVC_CLOSE;
+		case -EAGAIN:
 			return SVC_DROP;
 		case -ENOENT:
 			return SVC_DENIED;

@@ -736,6 +739,8 @@ svcauth_unix_set_client(struct svc_rqst *rqstp)
 	switch (PTR_ERR(gi)) {
 	case -EAGAIN:
 		return SVC_DROP;
+	case -ESHUTDOWN:
+		return SVC_CLOSE;
 	case -ENOENT:
 		break;
 	default:

@@ -776,7 +781,7 @@ svcauth_null_accept(struct svc_rqst *rqstp, __be32 *authp)
 	cred->cr_gid = (gid_t) -1;
 	cred->cr_group_info = groups_alloc(0);
 	if (cred->cr_group_info == NULL)
-		return SVC_DROP; /* kmalloc failure - client must retry */
+		return SVC_CLOSE; /* kmalloc failure - client must retry */
 
 	/* Put NULL verifier */
 	svc_putnl(resv, RPC_AUTH_NULL);

@@ -840,7 +845,7 @@ svcauth_unix_accept(struct svc_rqst *rqstp, __be32 *authp)
 		goto badcred;
 	cred->cr_group_info = groups_alloc(slen);
 	if (cred->cr_group_info == NULL)
-		return SVC_DROP;
+		return SVC_CLOSE;
 	for (i = 0; i < slen; i++)
 		GROUP_AT(cred->cr_group_info, i) = svc_getnl(argv);
 	if (svc_getu32(argv) != htonl(RPC_AUTH_NULL) || svc_getu32(argv) != 0) {

Apologies for the delay getting to the rest of these.

Quoted 7 lines · NeilBrown

If we drop a request in the sunrpc layer, either due kmalloc failure, or due to a cache miss when we could not queue the request for later replay, then close the connection to encourage the client to retry sooner.

Note that if the drop happens in the NFS layer, NFSERR_JUKEBOX (aka NFS4ERR_DELAY) is returned to guide the client concerning replay.

Looks fine, but:

net/sunrpc/svc.c svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv) · line 1060

Quoted 11 lines

net/sunrpc/svc.c +3 -0

index d9017d6..6359c42 100644
@@ -1055,6 +1055,9 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
 		goto err_bad;
 	case SVC_DENIED:
 		goto err_bad_auth;
+	case SVC_CLOSE:
+		if (test_bit(XPT_TEMP, &rqstp->rq_xprt->xpt_flags))
+			svc_close_xprt(rqstp->rq_xprt);

There are also dropit's later in svc_process_common when xdr encoding fails. I wonder if we should close there?

Well, it's an odd case. Seems like it should almost be declared a programming error and made a WARN().

Applying as is.--b.

Quoted 31 lines · On Tue, 21 Sep 2010 16:53:43 -0400 "J. Bruce Fields" <[email protected]>…

Apologies for the delay getting to the rest of these.

Quoted 7 lines · NeilBrown

If we drop a request in the sunrpc layer, either due kmalloc failure, or due to a cache miss when we could not queue the request for later replay, then close the connection to encourage the client to retry sooner.

Note that if the drop happens in the NFS layer, NFSERR_JUKEBOX (aka NFS4ERR_DELAY) is returned to guide the client concerning replay.

Looks fine, but:

net/sunrpc/svc.c svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv) · line 1060

Quoted 11 lines

net/sunrpc/svc.c +3 -0

index d9017d6..6359c42 100644
@@ -1055,6 +1055,9 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
 		goto err_bad;
 	case SVC_DENIED:
 		goto err_bad_auth;
+	case SVC_CLOSE:
+		if (test_bit(XPT_TEMP, &rqstp->rq_xprt->xpt_flags))
+			svc_close_xprt(rqstp->rq_xprt);

There are also dropit's later in svc_process_common when xdr encoding fails. I wonder if we should close there?

Well, it's an odd case. Seems like it should almost be declared a programming error and made a WARN().

Quoted 1 line

Applying as is.--b.

Thanks, I was wondering what had happened to these...

I cannot find them in your git though - not pushed yet, or am I looking in the wrong branch (for-2.6.37 and nfsd-next seem the obvious choices).

For the other 'dropit' cases I think the svc_authorise failure is most likely to be interesting. That can only fail if svcauth_gss_wrap_resp_* fails. I don't really know what that means that ... maybe we should close the connection in that case.

Now that a slight delay in getting a reply to an upcall doesn't require deferring of requests, request deferral for all NFSv4 requests - the concept doesn't really fit with the v4 model.

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

---
 fs/nfsd/nfs4proc.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

fs/nfsd/nfs4proc.c +2 -2

index 59ec449..fca3621 100644
@@ -1031,8 +1031,8 @@ nfsd4_proc_compound(struct svc_rqst *rqstp,
 	resp->cstate.session = NULL;
 	fh_init(&resp->cstate.current_fh, NFS4_FHSIZE);
 	fh_init(&resp->cstate.save_fh, NFS4_FHSIZE);
-	/* Use the deferral mechanism only for NFSv4.0 compounds */
-	rqstp->rq_usedeferral = (args->minorversion == 0);
+	/* Don't use the deferral mechanism NFSv4. */
+	rqstp->rq_usedeferral = 0;
 
 	/*
 	 * According to RFC3010, this takes precedence over all other errors.

Note that there is a conflict between this patch set and the other one. The other changes someing in nfs4idmap.c, and this one removes all the code that was changed. Very easy to resolve.

If the 'wait_for_completion_interruptible_timeout' completes due to interrupt or timeout, just before cache_revisit request gets around to calling ->revisit and thus the completion, we have a race with bad consequences.

cache_defer_req will see that sleeper.handle.hash is empty (because cache_revisit_request has already done the list_del_init), and that CACHE_PENDING is clear, and so will exit that function leaving any local variables such as 'sleeper' undefined.

Meanwhile cache_revisit_request could delete sleeper.handle.recent from the 'pending' list, and call sleeper.hande.revisit, any of which could have new garbage values and so could trigger a BUG.

Quoted 10 lines · NeilBrown

If the 'wait_for_completion_interruptible_timeout' completes due to interrupt or timeout, just before cache_revisit request gets around to calling ->revisit and thus the completion, we have a race with bad consequences.

cache_defer_req will see that sleeper.handle.hash is empty (because cache_revisit_request has already done the list_del_init), and that CACHE_PENDING is clear, and so will exit that function leaving any local variables such as 'sleeper' undefined.

Quoted 3 lines

Meanwhile cache_revisit_request could delete sleeper.handle.recent from the 'pending' list, and call sleeper.hande.revisit, any of which could have new garbage values and so could trigger a BUG.

Thanks, this looks correct to me!

I wish it were simpler, but don't see how right now.

Some superficial cleanup might help me at least--see below (untested), which attempts to make obvious the first-try-sleeping-then-try-deferral logic, by turning cache_defer_req into basically:

	if (req->thread_wait) {
		ret = cache_wait_req(req, item);
		if (ret != -ETIMEDOUT)
			return ret;
	}
	dreq = req->defer(req);
	if (dreq == NULL)
		return -ENOMEM;

...

I also wonder whether there would be a way to make most of this just another ->defer() method, so that we wouldn't need to switch on req->thread_wait here at all. But maybe there's not a nice way to do that. (Certainly I prefer your approach to what the idmap code was doing.)

--b.

net/sunrpc/cache.c +96 -77

index 2c5297f..da872f9 100644
@@ -509,60 +509,39 @@ static LIST_HEAD(cache_defer_list);
 static struct list_head cache_defer_hash[DFR_HASHSIZE];
 static int cache_defer_cnt;
 
-struct thread_deferred_req {
-	struct cache_deferred_req handle;
-	struct completion completion;
-};
-static void cache_restart_thread(struct cache_deferred_req *dreq, int too_many)
+static void __unhash_deferred_req(struct cache_deferred_req *dreq)
 {
-	struct thread_deferred_req *dr =
-		container_of(dreq, struct thread_deferred_req, handle);
-	complete(&dr->completion);
+	list_del_init(&dreq->recent);
+	list_del_init(&dreq->hash);
+	cache_defer_cnt--;
 }
 
-static int cache_defer_req(struct cache_req *req, struct cache_head *item)
+static void __hash_deferred_req(struct cache_deferred_req *dreq, struct cache_head *item)
 {
-	struct cache_deferred_req *dreq, *discard;
 	int hash = DFR_HASH(item);
-	struct thread_deferred_req sleeper;
 
-	if (cache_defer_cnt >= DFR_MAX) {
-		/* too much in the cache, randomly drop this one,
-		 * or continue and drop the oldest below
-		 */
-		if (net_random()&1)
-			return -ENOMEM;
-	}
-	if (req->thread_wait) {
-		dreq = &sleeper.handle;
-		sleeper.completion =
-			COMPLETION_INITIALIZER_ONSTACK(sleeper.completion);
-		dreq->revisit = cache_restart_thread;
-	} else
-		dreq = req->defer(req);
+	list_add(&dreq->recent, &cache_defer_list);
+	if (cache_defer_hash[hash].next == NULL)
+		INIT_LIST_HEAD(&cache_defer_hash[hash]);
+	list_add(&dreq->hash, &cache_defer_hash[hash]);
+}
 
- retry:
-	if (dreq == NULL)
-		return -ENOMEM;
+static int setup_deferral(struct cache_deferred_req *dreq, struct cache_head *item)
+{
+	struct cache_deferred_req *discard;
 
 	dreq->item = item;
 
 	spin_lock(&cache_defer_lock);
 
-	list_add(&dreq->recent, &cache_defer_list);
-
-	if (cache_defer_hash[hash].next == NULL)
-		INIT_LIST_HEAD(&cache_defer_hash[hash]);
-	list_add(&dreq->hash, &cache_defer_hash[hash]);
+	__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);
-		list_del_init(&discard->recent);
-		list_del_init(&discard->hash);
-		cache_defer_cnt--;
+		__unhash_deferred_req(discard);
 	}
 	spin_unlock(&cache_defer_lock);
 

@@ -575,44 +554,88 @@ static int cache_defer_req(struct cache_req *req, struct cache_head *item)
 		cache_revisit_request(item);
 		return -EAGAIN;
 	}
+	return 0;
+}
 
-	if (dreq == &sleeper.handle) {
-		if (wait_for_completion_interruptible_timeout(
-			    &sleeper.completion, req->thread_wait) <= 0) {
-			/* The completion wasn't completed, so we need
-			 * to clean up
-			 */
-			spin_lock(&cache_defer_lock);
-			if (!list_empty(&sleeper.handle.hash)) {
-				list_del_init(&sleeper.handle.recent);
-				list_del_init(&sleeper.handle.hash);
-				cache_defer_cnt--;
-				spin_unlock(&cache_defer_lock);
-			} else {
-				/* cache_revisit_request already removed
-				 * this from the hash table, but hasn't
-				 * called ->revisit yet.  It will very soon
-				 * and we need to wait for it.
-				 */
-				spin_unlock(&cache_defer_lock);
-				wait_for_completion(&sleeper.completion);
-			}
-		}
-		if (test_bit(CACHE_PENDING, &item->flags)) {
-			/* item is still pending, try request
-			 * deferral
+struct thread_deferred_req {
+	struct cache_deferred_req handle;
+	struct completion completion;
+};
+
+static void cache_restart_thread(struct cache_deferred_req *dreq, int too_many)
+{
+	struct thread_deferred_req *dr =
+		container_of(dreq, struct thread_deferred_req, handle);
+	complete(&dr->completion);
+}
+
+static int 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);
+	if (ret)
+		return ret;
+
+	if (wait_for_completion_interruptible_timeout(
+		    &sleeper.completion, req->thread_wait) <= 0) {
+		/* The completion wasn't completed, so we need
+		 * to clean up
+		 */
+		spin_lock(&cache_defer_lock);
+		if (!list_empty(&sleeper.handle.hash)) {
+			__unhash_deferred_req(&sleeper.handle);
+			spin_unlock(&cache_defer_lock);
+		} else {
+			/* cache_revisit_request already removed
+			 * this from the hash table, but hasn't
+			 * called ->revisit yet.  It will very soon
+			 * and we need to wait for it.
 			 */
-			dreq = req->defer(req);
-			goto retry;
+			spin_unlock(&cache_defer_lock);
+			wait_for_completion(&sleeper.completion);
 		}
-		/* 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.
+	}
+	if (test_bit(CACHE_PENDING, &item->flags)) {
+		/* item is still pending, try request
+		 * deferral
 		 */
-		return -EEXIST;
+		return -ETIMEDOUT;
 	}
-	return 0;
+	/* 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)
+{
+	struct cache_deferred_req *dreq;
+	int ret;
+
+	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;
+	}
+	if (req->thread_wait) {
+		ret = cache_wait_req(req, item);
+		if (ret != -ETIMEDOUT)
+			return ret;
+	}
+	dreq = req->defer(req);
+	if (dreq == NULL)
+		return -ENOMEM;
+	return setup_deferral(dreq, item);
 }
 
 static void cache_revisit_request(struct cache_head *item)

@@ -632,9 +655,8 @@ static void cache_revisit_request(struct cache_head *item)
 			dreq = list_entry(lp, struct cache_deferred_req, hash);
 			lp = lp->next;
 			if (dreq->item == item) {
-				list_del_init(&dreq->hash);
-				list_move(&dreq->recent, &pending);
-				cache_defer_cnt--;
+				__unhash_deferred_req(dreq);
+				list_add(&dreq->recent, &pending);
 			}
 		}
 	}

@@ -657,11 +679,8 @@ void cache_clean_deferred(void *owner)
 	spin_lock(&cache_defer_lock);
 
 	list_for_each_entry_safe(dreq, tmp, &cache_defer_list, recent) {
-		if (dreq->owner == owner) {
-			list_del_init(&dreq->hash);
-			list_move(&dreq->recent, &pending);
-			cache_defer_cnt--;
-		}
+		if (dreq->owner == owner)
+			__unhash_deferred_req(dreq);
 	}
 	spin_unlock(&cache_defer_lock);
 

net/sunrpc/svc_xprt.c +1 -2

index 8ff6840..95fc3e8 100644
@@ -665,7 +665,7 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
 		atomic_add(rqstp->rq_reserved, &xprt->xpt_reserved);
 
 		/* As there is a shortage of threads and this request
-		 * had to be queue, don't allow the thread to wait so
+		 * had to be queued, don't allow the thread to wait so
 		 * long for cache updates.
 		 */
 		rqstp->rq_chandle.thread_wait = 1*HZ;
--

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

Quoted 11 lines · On Thu, 26 Aug 2010 17:08:00 -0400 "J. Bruce Fields" <[email protected]>…

Quoted 10 lines · NeilBrown

If the 'wait_for_completion_interruptible_timeout' completes due to interrupt or timeout, just before cache_revisit request gets around to calling ->revisit and thus the completion, we have a race with bad consequences.

cache_defer_req will see that sleeper.handle.hash is empty (because cache_revisit_request has already done the list_del_init), and that CACHE_PENDING is clear, and so will exit that function leaving any local variables such as 'sleeper' undefined.

Quoted 7 lines

Quoted 3 lines

Meanwhile cache_revisit_request could delete sleeper.handle.recent from the 'pending' list, and call sleeper.hande.revisit, any of which could have new garbage values and so could trigger a BUG.

Thanks, this looks correct to me!

I wish it were simpler, but don't see how right now.

I think the way to make it simpler is to get rid of the deferral altogether. The more I think about it the less I like it :-)

The deferral code effectively gives you DFR_MAX extra virtual threads. They each are processing one request but have (supposedly) much lower over-head than creating real threads. So there are two groups of threads:
- those that can wait a longish time for a reply to an upcall
- those that only wait for IO.

This distinction could well still be useful as we don't really want all threads to be tied up waiting on upcalls so that no really work can be done. However we don't really need the deferral mechanism to achieve this.

Instead we impose a limit on the number of threads that can be in waiting on a completion (in cache_wait_req in your revised code).
If the number of such threads ever reaches - say - half the total, then we start dropping requests and closing TCP connections (or start new threads if we ever work out a good way to dynamically manage the thread pool).

So I would suggest:
- putting in a counter and limiting the number of waiting threads to half the pool size
- removing all the deferral code
- looking at cleaning up what is left to make it more readable.

???

Quoted 15 lines · On Thu, 26 Aug 2010 17:08:00 -0400 "J. Bruce Fields" <[email protected]>…

Quoted 13 lines · NeilBrown

If the 'wait_for_completion_interruptible_timeout' completes due to interrupt or timeout, just before cache_revisit request gets around to calling ->revisit and thus the completion, we have a race with bad consequences.

cache_defer_req will see that sleeper.handle.hash is empty (because cache_revisit_request has already done the list_del_init), and that CACHE_PENDING is clear, and so will exit that function leaving any local variables such as 'sleeper' undefined.

Meanwhile cache_revisit_request could delete sleeper.handle.recent from the 'pending' list, and call sleeper.hande.revisit, any of which could have new garbage values and so could trigger a BUG.

Quoted 3 lines

Thanks, this looks correct to me!

I wish it were simpler, but don't see how right now.

I think the way to make it simpler is to get rid of the deferral altogether. The more I think about it the less I like it :-)

The deferral code effectively gives you DFR_MAX extra virtual threads. They each are processing one request but have (supposedly) much lower over-head than creating real threads. So there are two groups of threads:
- those that can wait a longish time for a reply to an upcall
- those that only wait for IO.

This distinction could well still be useful as we don't really want all threads to be tied up waiting on upcalls so that no really work can be done. However we don't really need the deferral mechanism to achieve this.

Instead we impose a limit on the number of threads that can be in waiting on a completion (in cache_wait_req in your revised code).
If the number of such threads ever reaches - say - half the total, then we start dropping requests and closing TCP connections (or start new threads if we ever work out a good way to dynamically manage the thread pool).

So I would suggest:
- putting in a counter and limiting the number of waiting threads to half the pool size
- removing all the deferral code
- looking at cleaning up what is left to make it more readable.

???

NeilBrown

Quoted 18 lines · On Mon, 30 Aug 2010 09:36:08 +1000 Neil Brown <[email protected]> wrote:

Quoted 15 lines · On Thu, 26 Aug 2010 17:08:00 -0400 "J. Bruce Fields" <[email protected]>…

Quoted 13 lines · NeilBrown

If the 'wait_for_completion_interruptible_timeout' completes due to interrupt or timeout, just before cache_revisit request gets around to calling ->revisit and thus the completion, we have a race with bad consequences.

cache_defer_req will see that sleeper.handle.hash is empty (because cache_revisit_request has already done the list_del_init), and that CACHE_PENDING is clear, and so will exit that function leaving any local variables such as 'sleeper' undefined.

Meanwhile cache_revisit_request could delete sleeper.handle.recent from the 'pending' list, and call sleeper.hande.revisit, any of which could have new garbage values and so could trigger a BUG.

Quoted 6 lines

Quoted 3 lines

Thanks, this looks correct to me!

I wish it were simpler, but don't see how right now.

I think the way to make it simpler is to get rid of the deferral altogether. The more I think about it the less I like it :-)

I was in the process of doing this "getting rid of deferral" when I discovered that lockd now uses deferral too (and has done for 4 years! I am out of touch!!). This is for NFS exporting cluster filesystems where 'lock' might take a while, but returns a 'grant'.

As lockd is single-threads we cannot just let it wait for the grant in-process.

So I guess I cannot rip all that code out after all :-(