Re: [PATCH 6/9] sunrpc/cache: retry cache lookups that return -ETIMEDOUT

[Neil Brown <neilb@suse.de>]

On Thu, 3 Dec 2009 11:57:29 -0500
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Wed, Dec 02, 2009 at 05:11:27PM -0500, J. Bruce Fields wrote:
> > On Wed, Sep 09, 2009 at 04:32:54PM +1000, NeilBrown wrote:
> > > @@ -793,9 +793,18 @@ exp_find_key(svc_client *clp, int fsid_type, u32 *fsidv, struct cache_req *reqp)
> > >  	memcpy(key.ek_fsid, fsidv, key_len(fsid_type));
> > >  
> > >  	ek = svc_expkey_lookup(&key);
> > > + again:
> > >  	if (ek == NULL)
> > >  		return ERR_PTR(-ENOMEM);
> > >  	err = cache_check(&svc_expkey_cache, &ek->h, reqp);
> > > +	if (err == -ETIMEDOUT) {
> > > +		struct svc_expkey *prev_ek = ek;
> > > +		ek = svc_expkey_lookup(&key);
> > > +		if (ek != prev_ek)
> > > +			goto again;
> > > +		if (ek)
> > > +			cache_put(&ek->h, &svc_expkey_cache);
> > > +	}
> > 
> > This is very subtle.  (We're comparing to a pointer which may not
> > actually point to anything any more.)  And it's repeated in every
> > caller.  Is there any simpler way to handle this?
> 
> Actually, is it even right?  After the cache_check:
> 
> > >  	err = cache_check(&svc_expkey_cache, &ek->h, reqp);
> 
> we no longer hold a reference on ek, so it could be freed.  There's no
> reason that address couldn't then be reused for something else.  It's
> even possible that a new entry could be created at the same address
> here.  So:
> 
> > > +	if (err == -ETIMEDOUT) {
> > > +		struct svc_expkey *prev_ek = ek;
> > > +		ek = svc_expkey_lookup(&key);
> 
> the "ek" that's returned might well equal prev_ek,
> 
> > > +		if (ek != prev_ek)
> > > +			goto again;
> 
> but that doesn't necessarily imply that this is the same object that
> used to exist at that address.  So we could still return an ek which
> isn't actually a positive cache entry.
> 
> Am I missing something?

No, I don't think so.

How about this as an alternate.  I have only compile tested it, nothing more.
But if it looks good to you I'll make sure it really works.

I don't much like the way that the ipmap lookups came out, but they are a bit
awkward because the previous value is cached for improved performance.

Thanks for the review.
NeilBrown

>From 6eb6129ee478951ba1f77cdef0f13cf5a7e3f2cd Mon Sep 17 00:00:00 2001
From: NeilBrown <neilb@suse.de>
Date: Wed, 9 Sep 2009 16:22:53 +1000
Subject: [PATCH] sunrpc/cache: retry cache lookups that return -ETIMEDOUT

If cache_check returns -ETIMEDOUT, then the cache item is not
up-to-date, but there is no pending upcall.
This could mean the data is not available, or it could mean that the
good data has been stored in a new cache item.

So re-do the lookup and if that returns a new item, proceed using that
item.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/export.c                  |   74 ++++++++++++++++++++----------------
 include/linux/sunrpc/cache.h      |    5 ++
 net/sunrpc/auth_gss/svcauth_gss.c |   50 +++++++++++++------------
 net/sunrpc/cache.c                |   30 +++++++++++++++
 net/sunrpc/svcauth_unix.c         |   47 +++++++++++++++++++----
 5 files changed, 140 insertions(+), 66 deletions(-)

diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
index 984a5eb..99144d5 100644
--- a/fs/nfsd/export.c
+++ b/fs/nfsd/export.c
@@ -273,10 +273,9 @@ static struct cache_detail svc_expkey_cache = {
 	.alloc		= expkey_alloc,
 };
 
-static struct svc_expkey *
-svc_expkey_lookup(struct svc_expkey *item)
+static int
+svc_expkey_hash(struct svc_expkey *item)
 {
-	struct cache_head *ch;
 	int hash = item->ek_fsidtype;
 	char * cp = (char*)item->ek_fsid;
 	int len = key_len(item->ek_fsidtype);
@@ -284,6 +283,14 @@ svc_expkey_lookup(struct svc_expkey *item)
 	hash ^= hash_mem(cp, len, EXPKEY_HASHBITS);
 	hash ^= hash_ptr(item->ek_client, EXPKEY_HASHBITS);
 	hash &= EXPKEY_HASHMASK;
+	return hash;
+}
+
+static struct svc_expkey *
+svc_expkey_lookup(struct svc_expkey *item)
+{
+	struct cache_head *ch;
+	int hash = svc_expkey_hash(item);
 
 	ch = sunrpc_cache_lookup(&svc_expkey_cache, &item->h,
 				 hash);
@@ -297,13 +304,7 @@ static struct svc_expkey *
 svc_expkey_update(struct svc_expkey *new, struct svc_expkey *old)
 {
 	struct cache_head *ch;
-	int hash = new->ek_fsidtype;
-	char * cp = (char*)new->ek_fsid;
-	int len = key_len(new->ek_fsidtype);
-
-	hash ^= hash_mem(cp, len, EXPKEY_HASHBITS);
-	hash ^= hash_ptr(new->ek_client, EXPKEY_HASHBITS);
-	hash &= EXPKEY_HASHMASK;
+	int hash = svc_expkey_hash(new);
 
 	ch = sunrpc_cache_update(&svc_expkey_cache, &new->h,
 				 &old->h, hash);
@@ -743,14 +744,22 @@ struct cache_detail svc_export_cache = {
 	.alloc		= svc_export_alloc,
 };
 
-static struct svc_export *
-svc_export_lookup(struct svc_export *exp)
+static int
+svc_export_hash(struct svc_export *exp)
 {
-	struct cache_head *ch;
 	int hash;
+
 	hash = hash_ptr(exp->ex_client, EXPORT_HASHBITS);
 	hash ^= hash_ptr(exp->ex_path.dentry, EXPORT_HASHBITS);
 	hash ^= hash_ptr(exp->ex_path.mnt, EXPORT_HASHBITS);
+	return hash;
+}
+
+static struct svc_export *
+svc_export_lookup(struct svc_export *exp)
+{
+	struct cache_head *ch;
+	int hash = svc_export_hash(exp);
 
 	ch = sunrpc_cache_lookup(&svc_export_cache, &exp->h,
 				 hash);
@@ -764,10 +773,7 @@ static struct svc_export *
 svc_export_update(struct svc_export *new, struct svc_export *old)
 {
 	struct cache_head *ch;
-	int hash;
-	hash = hash_ptr(old->ex_client, EXPORT_HASHBITS);
-	hash ^= hash_ptr(old->ex_path.dentry, EXPORT_HASHBITS);
-	hash ^= hash_ptr(old->ex_path.mnt, EXPORT_HASHBITS);
+	int hash = svc_export_hash(old);
 
 	ch = sunrpc_cache_update(&svc_export_cache, &new->h,
 				 &old->h,
@@ -782,8 +788,8 @@ svc_export_update(struct svc_export *new, struct svc_export *old)
 static struct svc_expkey *
 exp_find_key(svc_client *clp, int fsid_type, u32 *fsidv, struct cache_req *reqp)
 {
-	struct svc_expkey key, *ek;
-	int err;
+	struct svc_expkey key;
+	struct cache_head *h;
 	
 	if (!clp)
 		return ERR_PTR(-ENOENT);
@@ -792,13 +798,14 @@ exp_find_key(svc_client *clp, int fsid_type, u32 *fsidv, struct cache_req *reqp)
 	key.ek_fsidtype = fsid_type;
 	memcpy(key.ek_fsid, fsidv, key_len(fsid_type));
 
-	ek = svc_expkey_lookup(&key);
-	if (ek == NULL)
+	h = sunrpc_lookup_check(&svc_expkey_cache, &key.h,
+				reqp, svc_expkey_hash(&key));
+
+	if (h == NULL)
 		return ERR_PTR(-ENOMEM);
-	err = cache_check(&svc_expkey_cache, &ek->h, reqp);
-	if (err)
-		return ERR_PTR(err);
-	return ek;
+	if (IS_ERR(h))
+		return ERR_PTR(PTR_ERR(h));
+	return container_of(h, struct svc_expkey, h);
 }
 
 static int exp_set_key(svc_client *clp, int fsid_type, u32 *fsidv,
@@ -855,8 +862,8 @@ exp_get_fsid_key(svc_client *clp, int fsid)
 static svc_export *exp_get_by_name(svc_client *clp, const struct path *path,
 				     struct cache_req *reqp)
 {
-	struct svc_export *exp, key;
-	int err;
+	struct svc_export key;
+	struct cache_head *h;
 
 	if (!clp)
 		return ERR_PTR(-ENOENT);
@@ -864,13 +871,14 @@ static svc_export *exp_get_by_name(svc_client *clp, const struct path *path,
 	key.ex_client = clp;
 	key.ex_path = *path;
 
-	exp = svc_export_lookup(&key);
-	if (exp == NULL)
+	h = sunrpc_lookup_check(&svc_export_cache, &key.h,
+				reqp, svc_export_hash(&key));
+	if (h == NULL)
 		return ERR_PTR(-ENOMEM);
-	err = cache_check(&svc_export_cache, &exp->h, reqp);
-	if (err)
-		return ERR_PTR(err);
-	return exp;
+	if (IS_ERR(h))
+		return ERR_PTR(PTR_ERR(h));
+
+	return container_of(h, struct svc_export, h);
 }
 
 /*
diff --git a/include/linux/sunrpc/cache.h b/include/linux/sunrpc/cache.h
index ef3db11..31d1687 100644
--- a/include/linux/sunrpc/cache.h
+++ b/include/linux/sunrpc/cache.h
@@ -154,6 +154,11 @@ extern struct cache_head *
 sunrpc_cache_update(struct cache_detail *detail,
 		    struct cache_head *new, struct cache_head *old, int hash);
 
+extern struct cache_head *
+sunrpc_lookup_check(struct cache_detail *detail,
+		    struct cache_head *item,
+		    struct cache_req *rqstp,
+		    int hash);
 extern int
 sunrpc_cache_pipe_upcall(struct cache_detail *detail, struct cache_head *h,
 		void (*cache_request)(struct cache_detail *,
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index f6c51e5..ab05580 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1000,6 +1000,7 @@ static int svcauth_gss_handle_init(struct svc_rqst *rqstp,
 	struct kvec *resv = &rqstp->rq_res.head[0];
 	struct xdr_netobj tmpobj;
 	struct rsi *rsip, rsikey;
+	struct cache_head *h;
 	int ret;
 
 	/* Read the verifier; should be NULL: */
@@ -1029,34 +1030,35 @@ static int svcauth_gss_handle_init(struct svc_rqst *rqstp,
 	}
 
 	/* Perform upcall, or find upcall result: */
-	rsip = rsi_lookup(&rsikey);
+	h = sunrpc_lookup_check(&rsi_cache, &rsikey.h, &rqstp->rq_chandle,
+				rsi_hash(&rsikey));
 	rsi_free(&rsikey);
-	if (!rsip)
+
+	if (!h)
 		return SVC_DROP;
-	switch (cache_check(&rsi_cache, &rsip->h, &rqstp->rq_chandle)) {
-	case -EAGAIN:
-	case -ETIMEDOUT:
-	case -ENOENT:
+
+	if (IS_ERR(h))
 		/* No upcall result: */
 		return SVC_DROP;
-	case 0:
-		ret = SVC_DROP;
-		/* Got an answer to the upcall; use it: */
-		if (gss_write_init_verf(rqstp, rsip))
-			goto out;
-		if (resv->iov_len + 4 > PAGE_SIZE)
-			goto out;
-		svc_putnl(resv, RPC_SUCCESS);
-		if (svc_safe_putnetobj(resv, &rsip->out_handle))
-			goto out;
-		if (resv->iov_len + 3 * 4 > PAGE_SIZE)
-			goto out;
-		svc_putnl(resv, rsip->major_status);
-		svc_putnl(resv, rsip->minor_status);
-		svc_putnl(resv, GSS_SEQ_WIN);
-		if (svc_safe_putnetobj(resv, &rsip->out_token))
-			goto out;
-	}
+
+	rsip = container_of(h, struct rsi, h);
+	ret = SVC_DROP;
+	/* Got an answer to the upcall; use it: */
+	if (gss_write_init_verf(rqstp, rsip))
+		goto out;
+	if (resv->iov_len + 4 > PAGE_SIZE)
+		goto out;
+	svc_putnl(resv, RPC_SUCCESS);
+	if (svc_safe_putnetobj(resv, &rsip->out_handle))
+		goto out;
+	if (resv->iov_len + 3 * 4 > PAGE_SIZE)
+		goto out;
+	svc_putnl(resv, rsip->major_status);
+	svc_putnl(resv, rsip->minor_status);
+	svc_putnl(resv, GSS_SEQ_WIN);
+	if (svc_safe_putnetobj(resv, &rsip->out_token))
+		goto out;
+
 	ret = SVC_COMPLETE;
 out:
 	cache_put(&rsip->h, &rsi_cache);
diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
index 46e9e2b..fbd38d4 100644
--- a/net/sunrpc/cache.c
+++ b/net/sunrpc/cache.c
@@ -265,6 +265,36 @@ int cache_check(struct cache_detail *detail,
 }
 EXPORT_SYMBOL_GPL(cache_check);
 
+struct cache_head *sunrpc_lookup_check(struct cache_detail *detail,
+				       struct cache_head *item,
+				       struct cache_req *rqstp,
+				       int hash)
+{
+	struct cache_head *rv;
+	struct cache_head *prev = NULL;
+	int err = -ETIMEDOUT;
+
+	while (err == -ETIMEDOUT) {
+		rv = sunrpc_cache_lookup(detail, item, hash);
+		if (rv && rv == prev)
+			break;
+		if (prev)
+			cache_put(prev, detail);
+		if (rv == NULL)
+			return NULL;
+
+		prev = cache_get(rv);
+		err = cache_check(detail, rv, rqstp);
+	}
+
+
+	if (prev)
+		cache_put(prev, detail);
+	if (err)
+		return PTR_ERR(err);
+	return rv;
+}
+EXPORT_SYMBOL_GPL(sunrpc_lookup_check);
 /*
  * caches need to be periodically cleaned.
  * For this we maintain a list of cache_detail and
diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
index 117f68a..f4e5908 100644
--- a/net/sunrpc/svcauth_unix.c
+++ b/net/sunrpc/svcauth_unix.c
@@ -658,18 +658,27 @@ static struct unix_gid *unix_gid_lookup(uid_t uid)
 static int unix_gid_find(uid_t uid, struct group_info **gip,
 			 struct svc_rqst *rqstp)
 {
-	struct unix_gid *ug = unix_gid_lookup(uid);
-	if (!ug)
+	struct unix_gid ugid;
+	struct cache_head *h;
+
+	ugid.uid = uid;
+	h = sunrpc_lookup_check(&unix_gid_cache, &ugid.h, 
+				&rqstp->rq_chandle, hash_long(uid, GID_HASHBITS));
+
+	if (!h)
 		return -EAGAIN;
-	switch (cache_check(&unix_gid_cache, &ug->h, &rqstp->rq_chandle)) {
-	case -ENOENT:
-		*gip = NULL;
-		return 0;
-	case 0:
+	if (!IS_ERR(h)) {
+		struct unix_gid *ug = container_of(h, struct unix_gid, h);
 		*gip = ug->gi;
 		get_group_info(*gip);
 		cache_put(&ug->h, &unix_gid_cache);
 		return 0;
+	}
+
+	switch (PTR_ERR(h)) {
+	case -ENOENT:
+		*gip = NULL;
+		return 0;
 	default:
 		return -EAGAIN;
 	}
@@ -681,6 +690,7 @@ svcauth_unix_set_client(struct svc_rqst *rqstp)
 	struct sockaddr_in *sin;
 	struct sockaddr_in6 *sin6, sin6_storage;
 	struct ip_map *ipm;
+	int err;
 
 	switch (rqstp->rq_addr.ss_family) {
 	case AF_INET:
@@ -708,11 +718,30 @@ svcauth_unix_set_client(struct svc_rqst *rqstp)
 	if (ipm == NULL)
 		return SVC_DENIED;
 
-	switch (cache_check(&ip_map_cache, &ipm->h, &rqstp->rq_chandle)) {
+	err = cache_check(&ip_map_cache, &ipm->h, &rqstp->rq_chandle);
+
+	if (err == -ETIMEDOUT) {
+		struct ip_map ip;
+		struct cache_head *h;
+
+		strcpy(ip.m_class, rqstp->rq_server->sv_program->pg_class);
+		ipv6_addr_copy(&ip.m_addr, &sin6->sin6_addr);
+
+		h = sunrpc_lookup_check(&ip_map_cache, &ip.h,
+					&rqstp->rq_chandle, hash_ip6(sin6->sin6_addr));
+		if (h == NULL)
+			return SVC_DENIED;
+		if (IS_ERR(h))
+			err = PTR_ERR(h);
+		else {
+			err = 0;
+			ipm = container_of(h, struct ip_map, h);
+		}
+	}
+	switch(err) {
 		default:
 			BUG();
 		case -EAGAIN:
-		case -ETIMEDOUT:
 			return SVC_DROP;
 		case -ENOENT:
 			return SVC_DENIED;
-- 
1.6.5.3