From owner-svn-src-all@FreeBSD.ORG Fri May 10 13:58:51 2013 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id A35EBAC3; Fri, 10 May 2013 13:58:51 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) by mx1.freebsd.org (Postfix) with ESMTP id 7A3D2AF6; Fri, 10 May 2013 13:58:51 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id CDFB7B960; Fri, 10 May 2013 09:58:50 -0400 (EDT) From: John Baldwin To: Marcel Moolenaar Subject: Re: svn commit: r250411 - in head/sys: conf kern sys Date: Fri, 10 May 2013 09:52:44 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p25; KDE/4.5.5; amd64; ; ) References: <201305091628.r49GSI33039873@svn.freebsd.org> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201305100952.45101.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Fri, 10 May 2013 09:58:50 -0400 (EDT) Cc: attilio@freebsd.org, svn-src-head@freebsd.org, svn-src-all@freebsd.org, Marcel Moolenaar , src-committers@freebsd.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 10 May 2013 13:58:51 -0000 On Thursday, May 09, 2013 4:56:33 pm Marcel Moolenaar wrote: > > On May 9, 2013, at 9:46 AM, Attilio Rao wrote: > > > On Thu, May 9, 2013 at 6:28 PM, Marcel Moolenaar wrote: > >> Author: marcel > >> Date: Thu May 9 16:28:18 2013 > >> New Revision: 250411 > >> URL: http://svnweb.freebsd.org/changeset/base/250411 > >> > >> Log: > >> Add option WITNESS_NO_VNODE to suppress printing LORs between VNODE > >> locks. To support this, VNODE locks are created with the LK_IS_VNODE > >> flag. This flag is propagated down using the LO_IS_VNODE flag. > >> > >> Note that WITNESS still records the LOR. Only the printing and the > >> optional entering into the kernel debugger is bypassed with the > >> WITNESS_NO_VNODE option. > > > > This is the wrong way to deal with such problem and I avoided to do > > something like that on purpose. > > I disagree. We have known LOR messages between VNODE locks that > pollute the console and so far we haven't fixed the root cause > in some form or shape. Silencing this known case is good to > maximize the attention LORs need to be given while still have > witness involved to catch locking problems with vnodes that are > of a different nature. > > > > > The way to fix this is to implement LK_NOWITNESS on a per-lock basis > > into lockmgr, propagate the same concept to the vn_lock() (which > > should be basically done automatically) and finally identify the > > false-positive case and commit for them explicitely LK_NOWITNESS on a > > per-call basis, explaining in detail why the single case reported is a > > false-positive. > > This is worse. You want witness involved. Well, I disagree with both of you a bit. I mostly agree with Attilio in that the committed change is a really large sledgehammer. If we want to ignore all LORs for a large number of locks in the system we might as well remove WITNESS altogether. However, I think LK_NOWITNESS is also too large of a sledgehammer for this as well. AFAIK there are two vnode-related LORs that I can think of: 1) between bufwait vs dirhash (so not even vnode locks) which is well documented in ufs_dirhash.c. I would much prefer to add a flag (maybe have somethign set a flag in the thread) to disable witness checking for the few known safe reversals and let WITNESS still check all other operations for the relevant locks. 2) vnode locks from a local filesystem that report a LOR with a "devfs" vnode. Typical reports are either "ufs" -> "devfs" or in some cases "ufs" -> "devfs" -> "ufs". As with 1), I would much rather tag the offending location than to disable all WITNESS checking on vnode locks. I had originally thought of doing this by passing a flag in the relevant lock call, but that requires having a "flags" variant of all lock calls and passing the flag down, etc. While writing this I thought of an alternative which would be to have a WITNESS_SUSPEND/WITNESS_RESUME pair of macros that either set a per-thread flag that we can invoke around known cases where a reversal is ok. When witness is suspended it would still remember that the lock was acquired, but it would not report a LOR due to that acquisition (we may have to set a flag in the lock instance so that future LORs related to that acquisition are also ignored) and it would not learn any new orders from that acquisition. So in code it would look like: WITNESS_SUSPEND(); vn_lock(...); WITNESS_RESTORE(); Does this sound agreeable? If so, I definitely think Marcel's change should be reverted in favor of this (more general yet more fine-grained) approach instead. -- John Baldwin