From owner-svn-src-head@FreeBSD.ORG Mon May 13 01:56:18 2013 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 14F6E414; Mon, 13 May 2013 01:56:18 +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 DD1C6C19; Mon, 13 May 2013 01:56:17 +0000 (UTC) Received: from ralph.baldwin.cx (pool-173-70-85-244.nwrknj.fios.verizon.net [173.70.85.244]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 34EECB948; Sun, 12 May 2013 21:56:17 -0400 (EDT) From: John Baldwin To: attilio@freebsd.org Subject: Re: svn commit: r250411 - in head/sys: conf kern sys Date: Sun, 12 May 2013 21:49:13 -0400 User-Agent: KMail/1.13.7 (FreeBSD/9.1-STABLE; KDE/4.8.4; amd64; ; ) References: <201305091628.r49GSI33039873@svn.freebsd.org> <201305101533.26992.jhb@freebsd.org> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <201305122149.13566.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Sun, 12 May 2013 21:56:17 -0400 (EDT) Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, Marcel Moolenaar , src-committers@freebsd.org, Marcel Moolenaar X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 13 May 2013 01:56:18 -0000 On Saturday, May 11, 2013 10:34:50 AM Attilio Rao wrote: > On Fri, May 10, 2013 at 9:33 PM, John Baldwin wrote: > > On Friday, May 10, 2013 2:51:20 pm Marcel Moolenaar wrote: > >> On May 10, 2013, at 9:11 AM, John Baldwin wrote: > >> > On Friday, May 10, 2013 11:46:54 am Marcel Moolenaar wrote: > >> >>> 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. > >> >> > >> >> With more file system types in use, this will get mixed up with the > >> >> other file systems and noise you get is rather severe. It is a big > >> >> problem for us at Juniper. > >> > > >> > Note, it is very specific that the second lock is always "devfs". I > >> > think that points to this being isolated to a few specific places, > >> > not a generic ordering problem. > >> > >> Alas, that's not the case. These LORs are reported between ufs and > >> unionfs, or ufs and isofs, etc. It's not just between something and > >> devfs. > > > > Ugh, I have only seen them with devfs so had presumed them to be more > > localized (and thus more easily targeted). In that case your change > > may be as fine-grained as we can get. I would also like to still keep > > WITNESS checking between vnode locks and other lock types, and > > LK_NOWITNESS would remove that, so between your change and Attilio's > > approach I do prefer yours. > > > >> I'm not sure the only options we have are to ignore the problem > >> or implement a general fix. If we set out to silence witness for > >> the known false positives then it's ok to handle them on a case > >> by case basis. We'll see patterns soon enough and then re-code > >> the solutions in terms of those patterns. If we're lucky we see > >> a single general solution, but if not, then it's fine to have a > >> handful of special case. The worse we can do is not address it > >> at all. > > > > I was assuming that the reversals were far more specific, and knowing > > about other false positives like the dirhash one, I want a generic way > > to do more fine-grained marking of false positives. If there were only > > a few places we would need to mark to fix the reversals you see, then I > > would prefer the suspend/resume approach for your case. However, the > > reversals you are masking sound too widespread to support that. > > The solution to this is what I proposed: pass down a LK_NOWITNESS for > instances where you don't want to check for witness. > This is per lock-call. > You localize the fix to the instance you want to shut down and you > skip witness for every false-positive. > When you commit the LK_NOWITNESS you mention explicitely the reason > why the reported LOR is a false positive so it is also documented on > svn. > > I still don't understand what you are objecting. Marcel objections' > were completely no-sense (Don't want to involve Witness) but at least > I expect some decent one by you. I think the problem is you'd have to tag an awful lot of places to catch all the issues Marcel is reporting. If you want to take all the LORs and tag them instead, go for it. Also, while LK_NOWITESS works fine for lockmgr's API (it already takes a flag argument), having that sort of thing in our other lock APIs is messier (they generally do not have flags). This is why a per-thread flag seemed simpler to me for this as you don't have to change umpteen locking APIs to add a new flags field. -- John Baldwin