Return-Path: Received: from fieldses.org ([173.255.197.46]:32842 "EHLO fieldses.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752784AbcHSPzc (ORCPT ); Fri, 19 Aug 2016 11:55:32 -0400 Date: Fri, 19 Aug 2016 11:55:32 -0400 From: "J. Bruce Fields" To: Chuck Lever Cc: "J. Bruce Fields" , Linux NFS Mailing List Subject: Re: WARN_ON added to rpc_create() Message-ID: <20160819155531.GG32329@fieldses.org> 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> <20160819154721.GF32329@fieldses.org> <7E7D9CD1-1B68-4C2B-AF29-DFB7ED6CA4EE@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <7E7D9CD1-1B68-4C2B-AF29-DFB7ED6CA4EE@oracle.com> Sender: linux-nfs-owner@vger.kernel.org List-ID: On Fri, Aug 19, 2016 at 11:51:13AM -0400, Chuck Lever wrote: > > > On Aug 19, 2016, at 11:47 AM, J. Bruce Fields wrote: > > > > On Fri, Aug 19, 2016 at 11:06:16AM -0400, 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: > >>>> 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, > > > > I haven't been following that rule. > > > > Once upon a time, I would have put a BUG() there. Then Linus pointed > > out that sometimes a BUG() can bork the machine badly enough that the > > backtrace doesn't even make it to the logs, rendering it useless. (And > > I believe that could be the case here since this is running as a work > > item.) So, I stick a WARN() there instead and don't worry much what > > happens afterwards. > > I still don't understand. If you would have put a BUG here, then > why does this logic continue and create the transport anyway? It seemed worth 1 line of screen real estate to say "this should never happen", but not 3? > Well, it's a nit, so I'll drop it. But, I mean, I don't care that much either. > >> 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. > > > > So, could we just agree that WARN_ON means "there's a programming > > error", regardless of what happens next? Backtraces should never happen > > on a working kernel. > > > > And then ignore the following code path. Unless it's something that's > > obviously going to immediately oops in the warned case, in which case if > > we really want the warning then we should return if that looks safer. > > > > But I don't have really strong feelings about this case, the warning may > > be academic since setup_callback_client() makes this look obviously > > impossible, so if you want to reorganize this somehow, feel free to give > > it a shot. > > Right, it's that obviously impossible part that made me wonder why > there was a warning here in the first place. > > OK, the fix is to do BC_TCP -> BC and put our pencils down. Do you > want me to send you a patch That would be great.--b. > or do you plan to take care of it?