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@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> > Cc: Neil Brown <neilb@xxxxxxx>
>
> Acked-by: NeilBrown <neilb@xxxxxxx>
>
> 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
