Return-Path: linux-nfs-owner@vger.kernel.org Received: from mail-ie0-f172.google.com ([209.85.223.172]:46838 "EHLO mail-ie0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932860AbaKRXCq (ORCPT ); Tue, 18 Nov 2014 18:02:46 -0500 Received: by mail-ie0-f172.google.com with SMTP id ar1so10513993iec.3 for ; Tue, 18 Nov 2014 15:02:46 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <9B8AE29D-A51D-4D04-965A-0B3C91DDB8A4@oracle.com> References: <1415833444-48129-1-git-send-email-trond.myklebust@primarydata.com> <20141112214128.4fc43a49@synchrony.poochiereds.net> <20141118201049.GH7419@fieldses.org> <9B8AE29D-A51D-4D04-965A-0B3C91DDB8A4@oracle.com> Date: Tue, 18 Nov 2014 15:02:45 -0800 Message-ID: Subject: Re: [PATCH] SUNRPC: Fix locking around callback channel reply receive From: Trond Myklebust To: Chuck Lever Cc: "J. Bruce Fields" , Jeff Layton , Linux NFS Mailing List Content-Type: text/plain; charset=UTF-8 Sender: linux-nfs-owner@vger.kernel.org List-ID: On Tue, Nov 18, 2014 at 12:14 PM, Chuck Lever wrote: > > On Nov 18, 2014, at 3:10 PM, Bruce Fields wrote: > >> On Wed, Nov 12, 2014 at 09:41:28PM -0500, Jeff Layton wrote: >>> On Wed, 12 Nov 2014 18:04:04 -0500 >>> Trond Myklebust wrote: >>> >>>> Both xprt_lookup_rqst() and xprt_complete_rqst() require that you >>>> take the transport lock in order to avoid races with xprt_transmit(). >> >> Thanks, looks right. >> >> Have you seen this in practice? (I'm just wondering whether it's worth >> a stable cc:.) > > Since the backchannel has a single slot, I wonder if it > would be possible to race here. You would think not, but AFAICS the back channel code uses a soft mount with a timeout of lease_period/10. In case of a timeout, the slot is just released and the next request is queued. IOW: I believe that it is perfectly possible for the client to be a little late responding to the callback, and then to have the reply there race with the timeout. Cheers Trond >> --b. >> >>>> Signed-off-by: Trond Myklebust >>>> Cc: Bruce Fields >>>> --- >>>> net/sunrpc/svcsock.c | 27 ++++++++++++++++----------- >>>> 1 file changed, 16 insertions(+), 11 deletions(-) >>>> >>>> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c >>>> index 3f959c681885..f9c052d508f0 100644 >>>> --- a/net/sunrpc/svcsock.c >>>> +++ b/net/sunrpc/svcsock.c >>>> @@ -1019,17 +1019,12 @@ static int receive_cb_reply(struct svc_sock *svsk, struct svc_rqst *rqstp) >>>> xid = *p++; >>>> calldir = *p; >>>> >>>> - if (bc_xprt) >>>> - req = xprt_lookup_rqst(bc_xprt, xid); >>>> - >>>> - if (!req) { >>>> - printk(KERN_NOTICE >>>> - "%s: Got unrecognized reply: " >>>> - "calldir 0x%x xpt_bc_xprt %p xid %08x\n", >>>> - __func__, ntohl(calldir), >>>> - bc_xprt, ntohl(xid)); >>>> + if (!bc_xprt) >>>> return -EAGAIN; >>>> - } >>>> + spin_lock_bh(&bc_xprt->transport_lock); >>>> + req = xprt_lookup_rqst(bc_xprt, xid); >>>> + if (!req) >>>> + goto unlock_notfound; >>>> >>>> memcpy(&req->rq_private_buf, &req->rq_rcv_buf, sizeof(struct xdr_buf)); >>>> /* >>>> @@ -1040,11 +1035,21 @@ static int receive_cb_reply(struct svc_sock *svsk, struct svc_rqst *rqstp) >>>> dst = &req->rq_private_buf.head[0]; >>>> src = &rqstp->rq_arg.head[0]; >>>> if (dst->iov_len < src->iov_len) >>>> - return -EAGAIN; /* whatever; just giving up. */ >>>> + goto unlock_eagain; /* whatever; just giving up. */ >>>> memcpy(dst->iov_base, src->iov_base, src->iov_len); >>>> xprt_complete_rqst(req->rq_task, rqstp->rq_arg.len); >>>> rqstp->rq_arg.len = 0; >>>> + spin_unlock_bh(&bc_xprt->transport_lock); >>>> return 0; >>>> +unlock_notfound: >>>> + printk(KERN_NOTICE >>>> + "%s: Got unrecognized reply: " >>>> + "calldir 0x%x xpt_bc_xprt %p xid %08x\n", >>>> + __func__, ntohl(calldir), >>>> + bc_xprt, ntohl(xid)); >>>> +unlock_eagain: >>>> + spin_unlock_bh(&bc_xprt->transport_lock); >>>> + return -EAGAIN; >>>> } >>>> >>>> static int copy_pages_to_kvecs(struct kvec *vec, struct page **pages, int len) >>> >>> Nice catch. It would also be good to pair this with a >>> lockdep_assert_held() call in xprt_lookup_rqst, but that could be added >>> in a separate patch. >>> >>> Reviewed-by: Jeff Layton >> -- >> 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 > > -- > Chuck Lever > chuck[dot]lever[at]oracle[dot]com > > > -- Trond Myklebust Linux NFS client maintainer, PrimaryData trond.myklebust@primarydata.com