Return-Path: Received: from userp1040.oracle.com ([156.151.31.81]:30777 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754047AbcHSPGX (ORCPT ); Fri, 19 Aug 2016 11:06:23 -0400 Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 9.3 \(3124\)) Subject: Re: WARN_ON added to rpc_create() From: Chuck Lever In-Reply-To: <20160819145055.GA2956@parsley.fieldses.org> Date: Fri, 19 Aug 2016 11:06:16 -0400 Cc: "J. Bruce Fields" , Linux NFS Mailing List Message-Id: <77E4C34B-23DD-4F3B-9D6B-670855E3DC0D@oracle.com> References: <42D0C152-58F9-4467-B86D-2A7A25544CE4@oracle.com> <20160803174724.GA5993@fieldses.org> <5E7D6A55-B7F3-411D-A74B-E8BCE04BCF02@oracle.com> <20160818215611.GA25052@fieldses.org> <20160819145055.GA2956@parsley.fieldses.org> To: "J. Bruce Fields" Sender: linux-nfs-owner@vger.kernel.org List-ID: > On Aug 19, 2016, at 10:50 AM, J. Bruce Fields wrote: > > On Thu, Aug 18, 2016 at 06:11:43PM -0400, Chuck Lever wrote: >> >>> On Aug 18, 2016, at 5:56 PM, J. Bruce Fields wrote: >>> >>> On Wed, Aug 03, 2016 at 03:40:11PM -0400, Chuck Lever wrote: >>>> >>>>> On Aug 3, 2016, at 1:47 PM, bfields@fieldses.org wrote: >>>>> >>>>> On Wed, Aug 03, 2016 at 11:27:47AM -0400, Chuck Lever wrote: >>>>>> Hi Bruce- >>>>>> >>>>>> I see that commit 39a9beab5acb83176e8b9a4f0778749a09341f1f >>>>>> Author: J. Bruce Fields >>>>>> AuthorDate: Tue May 17 12:38:21 2016 -0400 >>>>>> >>>>>> rpc: share one xps between all backchannels >>>>>> >>>>>> has added this piece of code: >>>>>> >>>>>> @@ -452,10 +452,20 @@ static struct rpc_clnt *rpc_create_xprt(struct rpc_create_args *args, >>>>>> struct rpc_clnt *clnt = NULL; >>>>>> struct rpc_xprt_switch *xps; >>>>>> >>>>>> - xps = xprt_switch_alloc(xprt, GFP_KERNEL); >>>>>> - if (xps == NULL) { >>>>>> - xprt_put(xprt); >>>>>> - return ERR_PTR(-ENOMEM); >>>>>> + if (args->bc_xprt && args->bc_xprt->xpt_bc_xps) { >>>>>> + WARN_ON(args->protocol != XPRT_TRANSPORT_BC_TCP); >>>>>> + xps = args->bc_xprt->xpt_bc_xps; >>>>>> + xprt_switch_get(xps); >>>>>> + } else { >>>>>> >>>>>> >>>>>> the WARN_ON here fires on the server whenever I use NFSv4.1 on RDMA. >>>>>> >>>>>> Can you say why it was added? Is there something RPC/RDMA needs to >>>>>> do to make the code safe? >>>>> >>>>> What is args->protocol in this case? >>>>> >>>>> Digging around... OK, I missed that BC_TCP and BC_RDMA were defined as >>>>> OR's of an XPRT_TRANSPORT_BC bit with the identifier of the underlying >>>>> transport. That makes sense. >>>>> >>>>> So, I should have just used XPRT_TRANSPORT_BC there--I think all I meant >>>>> was "is this a backchannel". >>>>> >>>>> Does that fix the problem? >>>> >>>> This simple fix eliminates the log noise: >>>> >>>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c >>>> index 2808d55..f94caf7 100644 >>>> --- a/net/sunrpc/clnt.c >>>> +++ b/net/sunrpc/clnt.c >>>> @@ -520,7 +520,7 @@ struct rpc_clnt *rpc_create(struct rpc_create_args *args) >>>> char servername[48]; >>>> >>>> if (args->bc_xprt) { >>>> - WARN_ON(args->protocol != XPRT_TRANSPORT_BC_TCP); >>>> + WARN_ON(!(args->protocol & XPRT_TRANSPORT_BC)); >>>> xprt = args->bc_xprt->xpt_bc_xprt; >>>> if (xprt) { >>>> xprt_get(xprt); >>>> >>>> >>>> This code seems to come from: >>>> >>>> commit d50039ea5ee63c589b0434baa5ecf6e5075bb6f9 >>>> Author: J. Bruce Fields >>>> AuthorDate: Mon May 16 17:03:42 2016 -0400 >>>> >>>> nfsd4/rpc: move backchannel create logic into rpc code >>>> >>>> >>>> Where it may have been copied from: >>>> >>>> -static struct rpc_clnt *create_backchannel_client(struct rpc_create_args *args) >>>> -{ >>>> - struct rpc_xprt *xprt; >>>> - >>>> - if (args->protocol != XPRT_TRANSPORT_BC_TCP) >>>> - return rpc_create(args); >>>> - >>>> - xprt = args->bc_xprt->xpt_bc_xprt; >>>> - if (xprt) { >>>> - xprt_get(xprt); >>>> - return rpc_create_xprt(args, xprt); >>>> - } >>>> - >>>> - return rpc_create(args); >>>> -} >>>> >>>> There's no warning here. In fact, protocol != BC_TCP seems to >>>> be expected. >>> >>> The protocol should be BC_TCP (OK, actually just BC) if and only if >>> bc_xprt is set. >>> >>> (The BC_TCP case is the 4.1+ case, the other is the 4.0 case. In the >>> 4.1+ case, the new client uses an existing (client-initiated) >>> connection, in the 4.0 case, the new client must also have a new >>> connection. >>> >>> In the 4.0 case we'll always create a new xprt, in the 4.1 case we might >>> or might not--depends on whether that particular connection has been >>> used for a backchannel previously.) >> >> OK, but why is a WARN_ON needed here? Why not return -EINVAL, >> for example (once you've corrected BC_TCP -> BC) ? > > Well, it would be a programming bug, so I'd want a WARN_ON or similar > somewhere, I don't care particularly where it is if you see a better way > to organize things. The way it works now, the WARN_ON fires, but the logic goes ahead and creates the transport anyway. If this is a programming bug, it should fail and return an error, no transport should be created. I can see a WARN_ON being useful because it displays a backtrace which identifies the broken caller. If it is not a programming bug (which is implied by the fact that a transport is created anyway) then no WARN_ON is needed. If you think it is correct that a WARN_ON fires _and_ a transport is created, could a comment be added explaining that? The new logic seems less straightforward to me than what it replaces. -- Chuck Lever