Return-Path: Received: from aserp1040.oracle.com ([141.146.126.69]:50155 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754040AbcHSPTT (ORCPT ); Fri, 19 Aug 2016 11:19:19 -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: <77E4C34B-23DD-4F3B-9D6B-670855E3DC0D@oracle.com> Date: Fri, 19 Aug 2016 11:19:13 -0400 Cc: "J. Bruce Fields" , Linux NFS Mailing List Message-Id: <9A57EC90-3D43-4802-B912-736718311DAA@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> <77E4C34B-23DD-4F3B-9D6B-670855E3DC0D@oracle.com> To: "J. Bruce Fields" Sender: linux-nfs-owner@vger.kernel.org List-ID: > On Aug 19, 2016, at 11:06 AM, Chuck Lever wrote: > >> >> 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. Also, WARN_ONCE might be preferable. -- Chuck Lever