From mboxrd@z Thu Jan 1 00:00:00 1970 From: "J. Bruce Fields" Subject: Re: [PATCH 2/5] nfsd4: avoid double reply caused by deferral race Date: Tue, 4 Jan 2011 13:19:49 -0500 Message-ID: <20110104181949.GC2308@fieldses.org> References: <1294079454-17891-1-git-send-email-bfields@redhat.com> <1294079454-17891-3-git-send-email-bfields@redhat.com> <20110104161305.10fe07fa@notabene.brown> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: "J. Bruce Fields" , linux-nfs@vger.kernel.org To: NeilBrown Return-path: Received: from fieldses.org ([174.143.236.118]:33074 "EHLO fieldses.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751283Ab1ADSTz (ORCPT ); Tue, 4 Jan 2011 13:19:55 -0500 In-Reply-To: <20110104161305.10fe07fa-wvvUuzkyo1EYVZTmpyfIwg@public.gmane.org> Sender: linux-nfs-owner@vger.kernel.org List-ID: On Tue, Jan 04, 2011 at 04:13:05PM +1100, NeilBrown wrote: > On Mon, 3 Jan 2011 13:30:51 -0500 "J. Bruce Fields" > 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 > > Cc: Neil Brown > > Acked-by: NeilBrown > > 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