Date: Fri, 10 May 2013 09:52:44 -0400 From: John Baldwin <jhb@freebsd.org> To: Marcel Moolenaar <marcel@xcllnt.net> Cc: attilio@freebsd.org, svn-src-head@freebsd.org, svn-src-all@freebsd.org, Marcel Moolenaar <marcel@freebsd.org>, src-committers@freebsd.org Subject: Re: svn commit: r250411 - in head/sys: conf kern sys Message-ID: <201305100952.45101.jhb@freebsd.org> In-Reply-To: <CC06FD75-868C-40B3-9C10-D66B56327803@xcllnt.net> References: <201305091628.r49GSI33039873@svn.freebsd.org> <CAJ-FndBY%2ByuUdvO4zP3kf2W4gDvB-uih19bqdmkFW3E4NcrHtw@mail.gmail.com> <CC06FD75-868C-40B3-9C10-D66B56327803@xcllnt.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thursday, May 09, 2013 4:56:33 pm Marcel Moolenaar wrote: > > On May 9, 2013, at 9:46 AM, Attilio Rao <attilio@freebsd.org> wrote: > > > On Thu, May 9, 2013 at 6:28 PM, Marcel Moolenaar <marcel@freebsd.org> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201305100952.45101.jhb>