nfsd cache_check fix and change to nfserr_dropit handling

PATCH In review J. Bruce Fields 3 participants 9 messages Jan 3, 2011 – Jan 4, 2011 1 message not in the archive

Subsystems (3)

Networking NFS client NFS server

lore

Share & tools

lore

J. Bruce Fields

author

2011-01-03 18:30 UTC #0 lore raw

A little more bugfixing and cleanup to be queued up for 2.6.38 if nobody sees a problem with it.

--b.

J. Bruce Fields

author

2011-01-03 18:30 UTC #1 lore raw

PATCH 1/5 nfsd4: don't drop requests on -ENOMEM

We never want to drop a request if we could return a JUKEBOX/DELAY error instead; so, convert to nfserr_jukebox and let nfsd_dispatch() convert that to a dropit error as a last resort if JUKEBOX/DELAY is unavailable (as in the NFSv2 case).

Signed-off-by J. Bruce Fields <[email protected]>

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

fs/nfsd/nfsproc.c +1 -1

index 08e1726..dc9c2e3 100644
@@ -736,7 +736,7 @@ nfserrno (int errno)
 		{ nfserr_jukebox, -ETIMEDOUT },
 		{ nfserr_jukebox, -ERESTARTSYS },
 		{ nfserr_dropit, -EAGAIN },
-		{ nfserr_dropit, -ENOMEM },
+		{ nfserr_jukebox, -ENOMEM },
 		{ nfserr_badname, -ESRCH },
 		{ nfserr_io, -ETXTBSY },
 		{ nfserr_notsupp, -EOPNOTSUPP },

Signature · 1 line

1.7.1

J. Bruce Fields

author

2011-01-03 18:31 UTC #2 lore raw

PATCH 2/5 nfsd4: avoid double reply caused by deferral race

In reply to J. Bruce Fields

Commit d29068c431599fa "sunrpc: Simplify cache_defer_req and related functions." asserted that cache_check() could determine success or failure of cache_defer_req() by checking the CACHE_PENDING bit.

This isn't quite right.

We need to know whether cache_defer_req() created a deferred request, in which case sending an rpc reply has become the responsibility of the deferred request, and it is important that we not send our own reply, resulting in two different replies to the same request.

And the CACHE_PENDING bit doesn't tell us that; we could have succesfully created a deferred request at the same time as another thread cleared the CACHE_PENDING bit.

So, partially revert that commit, to ensure that cache_check() returns -EAGAIN if and only if a deferred request has been created.

Signed-off-by J. Bruce Fields <[email protected]>

Cc Neil Brown <[email protected]>

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

net/sunrpc/cache.c +11 -7

index e433e75..0d6002f 100644
@@ -37,7 +37,7 @@
 
 #define	 RPCDBG_FACILITY RPCDBG_CACHE
 
-static void cache_defer_req(struct cache_req *req, struct cache_head *item);
+static bool 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)

@@ -268,9 +268,11 @@ int cache_check(struct cache_detail *detail,
 	}
 
 	if (rv == -EAGAIN) {
-		cache_defer_req(rqstp, h);
-		if (!test_bit(CACHE_PENDING, &h->flags)) {
-			/* Request is not deferred */
+		if (!cache_defer_req(rqstp, h)) {
+			/*
+			 * Request was not deferred; handle it as best
+			 * we can ourselves:
+			 */
 			rv = cache_is_valid(detail, h);
 			if (rv == -EAGAIN)
 				rv = -ETIMEDOUT;

@@ -618,18 +620,19 @@ static void cache_limit_defers(void)
 		discard->revisit(discard, 1);
 }
 
-static void cache_defer_req(struct cache_req *req, struct cache_head *item)
+/* Return true if and only if a deferred request is queued. */
+static bool cache_defer_req(struct cache_req *req, struct cache_head *item)
 {
 	struct cache_deferred_req *dreq;
 
 	if (req->thread_wait) {
 		cache_wait_req(req, item);
 		if (!test_bit(CACHE_PENDING, &item->flags))
-			return;
+			return false;
 	}
 	dreq = req->defer(req);
 	if (dreq == NULL)
-		return;
+		return false;
 	setup_deferral(dreq, item, 1);
 	if (!test_bit(CACHE_PENDING, &item->flags))
 		/* Bit could have been cleared before we managed to

@@ -638,6 +641,7 @@ static void cache_defer_req(struct cache_req *req, struct cache_head *item)
 		cache_revisit_request(item);
 
 	cache_limit_defers();
+	return true;
 }
 
 static void cache_revisit_request(struct cache_head *item)

Signature · 1 line

1.7.1

NeilBrown

Acked-by on v1 2/5

2011-01-04 05:13 UTC #3 lore raw

Re: [PATCH 2/5] nfsd4: avoid double reply caused by deferral race

Quoted 15 lines · On Mon, 3 Jan 2011 13:30:51 -0500 "J. Bruce Fields" <[email protected]> wrote:

Commit d29068c431599fa "sunrpc: Simplify cache_defer_req and related functions." asserted that cache_check() could determine success or failure of cache_defer_req() by checking the CACHE_PENDING bit.
This isn't quite right.
We need to know whether cache_defer_req() created a deferred request, in which case sending an rpc reply has become the responsibility of the deferred request, and it is important that we not send our own reply, resulting in two different replies to the same request.
And the CACHE_PENDING bit doesn't tell us that; we could have succesfully created a deferred request at the same time as another thread cleared the CACHE_PENDING bit.

Quoted 5 lines

So, partially revert that commit, to ensure that cache_check() returns -EAGAIN if and only if a deferred request has been created.
Signed-off-by J. Bruce Fields <[email protected]>
Cc Neil Brown <[email protected]>

Acked-by NeilBrown <[email protected]>

I was over-simplifying a bit there, wasn't I?
And the rest of the series looks good too.

Thanks,
NeilBrown

Quoted 63 lines

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

net/sunrpc/cache.c +11 -7

index e433e75..0d6002f 100644
@@ -37,7 +37,7 @@
 
 #define	 RPCDBG_FACILITY RPCDBG_CACHE
 
-static void cache_defer_req(struct cache_req *req, struct cache_head *item);
+static bool 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)

@@ -268,9 +268,11 @@ int cache_check(struct cache_detail *detail,
 	}
 
 	if (rv == -EAGAIN) {
-		cache_defer_req(rqstp, h);
-		if (!test_bit(CACHE_PENDING, &h->flags)) {
-			/* Request is not deferred */
+		if (!cache_defer_req(rqstp, h)) {
+			/*
+			 * Request was not deferred; handle it as best
+			 * we can ourselves:
+			 */
 			rv = cache_is_valid(detail, h);
 			if (rv == -EAGAIN)
 				rv = -ETIMEDOUT;

@@ -618,18 +620,19 @@ static void cache_limit_defers(void)
 		discard->revisit(discard, 1);
 }
 
-static void cache_defer_req(struct cache_req *req, struct cache_head *item)
+/* Return true if and only if a deferred request is queued. */
+static bool cache_defer_req(struct cache_req *req, struct cache_head *item)
 {
 	struct cache_deferred_req *dreq;
 
 	if (req->thread_wait) {
 		cache_wait_req(req, item);
 		if (!test_bit(CACHE_PENDING, &item->flags))
-			return;
+			return false;
 	}
 	dreq = req->defer(req);
 	if (dreq == NULL)
-		return;
+		return false;
 	setup_deferral(dreq, item, 1);
 	if (!test_bit(CACHE_PENDING, &item->flags))
 		/* Bit could have been cleared before we managed to

@@ -638,6 +641,7 @@ static void cache_defer_req(struct cache_req *req, struct cache_head *item)
 		cache_revisit_request(item);
 
 	cache_limit_defers();
+	return true;
 }
 
 static void cache_revisit_request(struct cache_head *item)

A message that is not in the archive (sent off-list or not yet received): [email protected]

J. Bruce Fields

2011-01-04 18:19 UTC #5 lore raw

Re: [PATCH 2/5] nfsd4: avoid double reply caused by deferral race

Quoted 21 lines · NeilBrown

Quoted 18 lines · On Mon, 3 Jan 2011 13:30:51 -0500 "J. Bruce Fields" <[email protected]> wrote:
Commit d29068c431599fa "sunrpc: Simplify cache_defer_req and related functions." asserted that cache_check() could determine success or failure of cache_defer_req() by checking the CACHE_PENDING bit.
This isn't quite right.
We need to know whether cache_defer_req() created a deferred request, in which case sending an rpc reply has become the responsibility of the deferred request, and it is important that we not send our own reply, resulting in two different replies to the same request.
And the CACHE_PENDING bit doesn't tell us that; we could have succesfully created a deferred request at the same time as another thread cleared the CACHE_PENDING bit.
So, partially revert that commit, to ensure that cache_check() returns -EAGAIN if and only if a deferred request has been created.

Quoted 7 lines

Quoted 2 lines
Signed-off-by J. Bruce Fields <[email protected]>
Cc Neil Brown <[email protected]>
Acked-by NeilBrown <[email protected]>
I was over-simplifying a bit there, wasn't I?
And the rest of the series looks good too.

Good, thanks!--b.

Quoted 73 lines

Thanks,
NeilBrown

Quoted 56 lines

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

net/sunrpc/cache.c +10 -7

index e433e75..0d6002f 100644
@@ -37,7 +37,7 @@
 
 #define	 RPCDBG_FACILITY RPCDBG_CACHE
 
-static void cache_defer_req(struct cache_req *req, struct cache_head *item);
+static bool 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)

@@ -268,9 +268,11 @@ int cache_check(struct cache_detail *detail,
 	}
 
 	if (rv == -EAGAIN) {
-		cache_defer_req(rqstp, h);
-		if (!test_bit(CACHE_PENDING, &h->flags)) {
-			/* Request is not deferred */
+		if (!cache_defer_req(rqstp, h)) {
+			/*
+			 * Request was not deferred; handle it as best
+			 * we can ourselves:
+			 */
 			rv = cache_is_valid(detail, h);
 			if (rv == -EAGAIN)
 				rv = -ETIMEDOUT;

@@ -618,18 +620,19 @@ static void cache_limit_defers(void)
 		discard->revisit(discard, 1);
 }
 
-static void cache_defer_req(struct cache_req *req, struct cache_head *item)
+/* Return true if and only if a deferred request is queued. */
+static bool cache_defer_req(struct cache_req *req, struct cache_head *item)
 {
 	struct cache_deferred_req *dreq;
 
 	if (req->thread_wait) {
 		cache_wait_req(req, item);
 		if (!test_bit(CACHE_PENDING, &item->flags))
-			return;
+			return false;
 	}
 	dreq = req->defer(req);
 	if (dreq == NULL)
-		return;
+		return false;
 	setup_deferral(dreq, item, 1);
 	if (!test_bit(CACHE_PENDING, &item->flags))
 		/* Bit could have been cleared before we managed to

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

Quoted 7 lines

net/sunrpc/cache.c +1 -0

@@ -638,6 +641,7 @@ static void cache_defer_req(struct cache_req *req, struct cache_head *item)
 		cache_revisit_request(item);
 
 	cache_limit_defers();
+	return true;
 }
 
 static void cache_revisit_request(struct cache_head *item)

--
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

J. Bruce Fields

author

2011-01-03 18:31 UTC #6 lore raw

PATCH 3/5 nfsd4: simpler request dropping

In reply to J. Bruce Fields

Currently we use -EAGAIN returns to determine when to drop a deferred request. On its own, that is error-prone, as it makes us treat -EAGAIN returns from other functions specially to prevent inadvertent dropping.

So, use a flag on the request instead.

Returning an error on request deferral is still required, to prevent further processing, but we no longer need worry that an error return on its own could result in a drop.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/nfssvc.c           |    2 +-
 include/linux/sunrpc/svc.h |    1 +
 net/sunrpc/svc.c           |    3 ++-
 net/sunrpc/svc_xprt.c      |    1 +
 4 files changed, 5 insertions(+), 2 deletions(-)

fs/nfsd/nfssvc.c +1 -1

index 2bae1d8..18743c4 100644
@@ -608,7 +608,7 @@ nfsd_dispatch(struct svc_rqst *rqstp, __be32 *statp)
 	/* Now call the procedure handler, and encode NFS status. */
 	nfserr = proc->pc_func(rqstp, rqstp->rq_argp, rqstp->rq_resp);
 	nfserr = map_new_errors(rqstp->rq_vers, nfserr);
-	if (nfserr == nfserr_dropit) {
+	if (nfserr == nfserr_dropit || rqstp->rq_dropme) {
 		dprintk("nfsd: Dropping request; may be revisited later\n");
 		nfsd_cache_update(rqstp, RC_NOCACHE, NULL);
 		return 0;

include/linux/sunrpc/svc.h +1 -0

index 5a3085b..d45c482 100644
@@ -269,6 +269,7 @@ struct svc_rqst {
 	struct cache_req	rq_chandle;	/* handle passed to caches for 
 						 * request delaying 
 						 */
+	bool			rq_dropme;
 	/* Catering to nfsd */
 	struct auth_domain *	rq_client;	/* RPC peer info */
 	struct auth_domain *	rq_gssclient;	/* "gss/"-style peer info */

net/sunrpc/svc.c +2 -1

index 6359c42..df1931f 100644
@@ -1005,6 +1005,7 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
 	rqstp->rq_splice_ok = 1;
 	/* Will be turned off only when NFSv4 Sessions are used */
 	rqstp->rq_usedeferral = 1;
+	rqstp->rq_dropme = false;
 
 	/* Setup reply header */
 	rqstp->rq_xprt->xpt_ops->xpo_prep_reply_hdr(rqstp);

@@ -1106,7 +1107,7 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
 		*statp = procp->pc_func(rqstp, rqstp->rq_argp, rqstp->rq_resp);
 
 		/* Encode reply */
-		if (*statp == rpc_drop_reply) {
+		if (rqstp->rq_dropme) {
 			if (procp->pc_release)
 				procp->pc_release(rqstp, NULL, rqstp->rq_resp);
 			goto dropit;

net/sunrpc/svc_xprt.c +1 -0

index 5eae53b..173f3b9 100644
@@ -1019,6 +1019,7 @@ static struct cache_deferred_req *svc_defer(struct cache_req *req)
 	}
 	svc_xprt_get(rqstp->rq_xprt);
 	dr->xprt = rqstp->rq_xprt;
+	rqstp->rq_dropme = true;
 
 	dr->handle.revisit = svc_revisit;
 	return &dr->handle;

Signature · 1 line

1.7.1

J. Bruce Fields

author

2011-01-03 18:31 UTC #7 lore raw

PATCH 4/5 nfsd: stop translating EAGAIN to nfserr_dropit

In reply to J. Bruce Fields

We no longer need this.

Also, EWOULDBLOCK is generally a synonym for EAGAIN, but that may not be true on all architectures, so map it as well.

Signed-off-by J. Bruce Fields <[email protected]>

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

fs/nfsd/nfsproc.c +2 -1

index dc9c2e3..fd608a2 100644
@@ -735,7 +735,8 @@ nfserrno (int errno)
 		{ nfserr_stale, -ESTALE },
 		{ nfserr_jukebox, -ETIMEDOUT },
 		{ nfserr_jukebox, -ERESTARTSYS },
-		{ nfserr_dropit, -EAGAIN },
+		{ nfserr_jukebox, -EAGAIN },
+		{ nfserr_jukebox, -EWOULDBLOCK },
 		{ nfserr_jukebox, -ENOMEM },
 		{ nfserr_badname, -ESRCH },
 		{ nfserr_io, -ETXTBSY },

Signature · 1 line

1.7.1

J. Bruce Fields

author

2011-01-03 18:31 UTC #8 lore raw

PATCH 5/5 nfsd4: remove some unnecessary dropit handling

In reply to J. Bruce Fields

We no longer need a few of these special cases.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/nfs4proc.c  |    4 ----
 fs/nfsd/nfs4state.c |    2 --
 fs/nfsd/nfs4xdr.c   |    2 --
 fs/nfsd/vfs.c       |    4 ----
 4 files changed, 0 insertions(+), 12 deletions(-)

fs/nfsd/nfs4proc.c +0 -4

index f80c399..fd6694b 100644
@@ -1156,10 +1156,6 @@ encode_op:
 
 		nfsd4_increment_op_stats(op->opnum);
 	}
-	if (!rqstp->rq_usedeferral && status == nfserr_dropit) {
-		dprintk("%s Dropit - send NFS4ERR_DELAY\n", __func__);
-		status = nfserr_jukebox;
-	}
 
 	resp->cstate.status = status;
 	fh_put(&resp->cstate.current_fh);

fs/nfsd/nfs4state.c +0 -2

index 73adcfb..b82e368 100644
@@ -2509,8 +2509,6 @@ static __be32 nfs4_get_vfs_file(struct svc_rqst *rqstp, struct nfs4_file
 	if (!fp->fi_fds[oflag]) {
 		status = nfsd_open(rqstp, cur_fh, S_IFREG, access,
 			&fp->fi_fds[oflag]);
-		if (status == nfserr_dropit)
-			status = nfserr_jukebox;
 		if (status)
 			return status;
 	}

fs/nfsd/nfs4xdr.c +0 -2

index 437b462..364aae7 100644
@@ -2328,8 +2328,6 @@ nfsd4_encode_dirent(void *ccdv, const char *name, int namlen,
 	case nfserr_resource:
 		nfserr = nfserr_toosmall;
 		goto fail;
-	case nfserr_dropit:
-		goto fail;
 	case nfserr_noent:
 		goto skip_entry;
 	default:

fs/nfsd/vfs.c +0 -4

index 184938f..6a3af2f 100644
@@ -380,8 +380,6 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
 		 * we need to break all leases.
 		 */
 		host_err = break_lease(inode, O_WRONLY | O_NONBLOCK);
-		if (host_err == -EWOULDBLOCK)
-			host_err = -ETIMEDOUT;
 		if (host_err) /* ENOMEM or EWOULDBLOCK */
 			goto out_nfserr;

@@ -752,8 +750,6 @@ nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
 	 */
 	if (!(access & NFSD_MAY_NOT_BREAK_LEASE))
 		host_err = break_lease(inode, O_NONBLOCK | ((access & NFSD_MAY_WRITE) ? O_WRONLY : 0));
-	if (host_err == -EWOULDBLOCK)
-		host_err = -ETIMEDOUT;
 	if (host_err) /* NOMEM or WOULDBLOCK */
 		goto out_nfserr;

Signature · 1 line

1.7.1

J. Bruce Fields

2011-01-03 20:12 UTC #9 lore raw

In reply to J. Bruce Fields

Quoted 2 lines · J. Bruce Fields

A little more bugfixing and cleanup to be queued up for 2.6.38 if nobody sees a problem with it.

My fingers seem to have decided that no matter what I'm working on, it must start with "nfsd4:". Sorry about that; fixing up the subject lines....

--b.

Quoted 6 lines

--b.
--
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