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

[Marcel Moolenaar]

On May 10, 2013, at 6:52 AM, John Baldwin <jhb at freebsd.org> wrote:
>>> 
>>> 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.

I do not disagree that it isn't a fix. I wasn't aiming to fix it.

>  However, I think LK_NOWITNESS is also too large
> of a sledgehammer for this as well.

I agree. I think it's worse, by virtue of it being presented as both
better as well as a fix.

> 
> 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.


> 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.

This is a similar approach to what Attilio suggested (IMO) and I'm
very concerned on 2 grounds:
1.  Disable/suspending witness and making it easy to do so has known
    side-effects: people will use it simple to get rid of witness
    warnings and not even bother to understand the root cause of the
    witness complaints and thus fix the root cause problem. Fixing
    the root cause could be improving witness, or fixing the locks.
    If we add means and ways to have locks not checked by witness
    then witness stops being the useful tool it mostly is now.
2.  This, to me, also doesn't come close to fixing the root cause
    problem. It's a just a different bandaid. What I would like to
    see is a statement as to why witness reports LORs (in this case)
    and why they are not, so that we can improve witness.
    For example, a statement could go like this: witness operates on
    lock names. All vnode structures have a lock and they bare the
    name of the file system. Consequently, we have many instances of
    a particular lock type that share the same name. This causes,
    ... (please complete this statement appropriately).
    If our root cause problem is fundamentally that witness cannot
    distinguish 1 UFS vnode lock from another UFS vnode lock then
    we should fix that. Either:
    1.  Construct unique lock names when witness is enabled, or
    2.  Create the lock with a flag to tell witness that there are
	multiple locks of the same name and as such does not have
	enough the knowledge to report LORs adequately. How we
	change witness in the face of such a flag is another story
	and a good subject for ongoing discussion.

I'm all for fixing this. My change should be backed out if we have
a real fix. But I must say that I'm disappointed by the responses
of both you and Attilio. I do not get a sense that either of you
knows what the problem is. I definitely do not have had time to
understand the problem, let alone work up a real fix, so I claim
ignorance without prejudice and I'm the first to admit that this is
not a fix. But the best suggestion I get is to exclude the locks
from any consideration by witness and that's worse than what I did.
I merely added a kernel option to supressing the printing. Not even
the checking.

And all I did is to allow someone (= Juniper) to not print the LOR
for this well-known and mostly ignored case that is impacting our
ability to keep witness enabled. And the reason I had to do that is
that this is a long-standing LOR that isn't being addressed. The
FreeBSD community apparently has settled on just ignoring it.

Let's do better. Let's understand the root cause and work up a real
fix that doesn't involve disabling witness or excluding them from
witness. I have no problem doing the grunt work on that in due time
and rip out my hack at the same time.

-- 
Marcel Moolenaar
marcel at xcllnt.net