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

[John Baldwin]

On Saturday, May 11, 2013 10:34:50 AM Attilio Rao 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.

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
_______________________________________________
svn-src-all at freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe at freebsd.org"