nfsd cache_check fix and change to nfserr_dropit handling

nfsd cache_check fix and change to nfserr_dropit handling

[J. Bruce Fields]

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

--b.

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

[J. Bruce Fields]

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 <bfields@redhat.com>
---
 fs/nfsd/nfsproc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
index 08e1726..dc9c2e3 100644
--- a/fs/nfsd/nfsproc.c
+++ b/fs/nfsd/nfsproc.c
@@ -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 },
-- 
1.7.1

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

[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 <bfields@redhat.com>
Cc: Neil Brown <neilb@suse.de>
---
 net/sunrpc/cache.c |   18 +++++++++++-------
 1 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
index e433e75..0d6002f 100644
--- a/net/sunrpc/cache.c
+++ b/net/sunrpc/cache.c
@@ -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)
-- 
1.7.1

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

[NeilBrown]

On Mon,  3 Jan 2011 13:30:51 -0500 "J. Bruce Fields" <bfields@redhat.com>
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.
> 
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> Cc: Neil Brown <neilb@suse.de>

Acked-by: NeilBrown <neilb@suse.de>

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

Thanks,
NeilBrown


> ---
>  net/sunrpc/cache.c |   18 +++++++++++-------
>  1 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
> index e433e75..0d6002f 100644
> --- a/net/sunrpc/cache.c
> +++ b/net/sunrpc/cache.c
> @@ -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)

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

[J. Bruce Fields]

On Tue, Jan 04, 2011 at 04:13:05PM +1100, NeilBrown wrote:
> On Mon,  3 Jan 2011 13:30:51 -0500 "J. Bruce Fields" <bfields@redhat.com>
> 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.
> > 
> > Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > Cc: Neil Brown <neilb@suse.de>
> 
> Acked-by: NeilBrown <neilb@suse.de>
> 
> I was over-simplifying a bit there, wasn't I?
> And the rest of the series looks good too.

Good, thanks!--b.

> 
> Thanks,
> NeilBrown
> 
> 
> > ---
> >  net/sunrpc/cache.c |   18 +++++++++++-------
> >  1 files changed, 11 insertions(+), 7 deletions(-)
> > 
> > diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
> > index e433e75..0d6002f 100644
> > --- a/net/sunrpc/cache.c
> > +++ b/net/sunrpc/cache.c
> > @@ -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)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/5] nfsd4: simpler request dropping

[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 <bfields@redhat.com>
---
 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(-)

diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 2bae1d8..18743c4 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -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;
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 5a3085b..d45c482 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -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 */
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 6359c42..df1931f 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -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;
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index 5eae53b..173f3b9 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -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;
-- 
1.7.1

[PATCH 4/5] nfsd: stop translating EAGAIN to nfserr_dropit

[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 <bfields@redhat.com>
---
 fs/nfsd/nfsproc.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
index dc9c2e3..fd608a2 100644
--- a/fs/nfsd/nfsproc.c
+++ b/fs/nfsd/nfsproc.c
@@ -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 },
-- 
1.7.1

[PATCH 5/5] nfsd4: remove some unnecessary dropit handling

[J. Bruce Fields]

We no longer need a few of these special cases.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 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(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index f80c399..fd6694b 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -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);
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 73adcfb..b82e368 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -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;
 	}
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 437b462..364aae7 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -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:
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 184938f..6a3af2f 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -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;
 
-- 
1.7.1

Re: nfsd cache_check fix and change to nfserr_dropit handling

[J. Bruce Fields]

On Mon, Jan 03, 2011 at 01:30:49PM -0500, J. Bruce Fields wrote:
> 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.

> 
> --b.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html