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