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

["J. Bruce Fields" <bfields@fieldses.org>]

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