svn commit: r250411 - in head/sys: conf kern sys

[Attilio Rao]

On Sat, May 11, 2013 at 4:34 PM, Attilio Rao <attilio at freebsd.org> wrote:
> On Fri, May 10, 2013 at 9:33 PM, John Baldwin <jhb at freebsd.org> wrote:
>> On Friday, May 10, 2013 2:51:20 pm Marcel Moolenaar wrote:
>>>
>>> On May 10, 2013, at 9:11 AM, John Baldwin <jhb at freebsd.org> 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.

And finally, all the logic to do this seems already implemented. I
didn't recall that.
See this:
http://svnweb.freebsd.org/base?view=revision&revision=179554

What is missing is similar logic for sx(9) and rwlock(9). We can
simply add _flags() variant of such functions if we need it.
But Marcel's approach is not helping on that side as well, if not for
a simple red-herring effect.

Attilio


--
Peace can only be achieved by understanding - A. Einstein