vn_lock_pair(): handle the case of vp1->v_vnlock == vp2->v_vnlock

[kib]

Summary

It is not enough to check vp1 == vp2 to detect lock recursion, since
vnodes might share the locks.  This might happen for e.g. stacked
filesystems (nullfs and other), and for FFS snapshots.

Switch from checking vnode equiality to check v_vnlock equiality, and
recheck the condition after vnode relock since reclamation or otner
parallel operation might change the vnode locks under us.

Return a value (not really an error) indicating the case that vnodes
share the lock, to simplify the unlock in caller.

[kib]

Handle NULL parameters.

[markj]

sys/kern/vfs_vnops.c

4357 ↗

(On Diff #177955)

What does "most exclusive" mean here?

4379 ↗

(On Diff #177955)

4408 ↗

(On Diff #177955)

If vp2 is locked here, then isn't it unsafe to lock vp1 on line 4404?

[kib]

sys/kern/vfs_vnops.c

4357 ↗

(On Diff #177955)

If one request is exclusive, and another is shared, then the call is treated as 'both exclusive'. See the same comment on the line 4382, where the code explains the formulation.

4408 ↗

(On Diff #177955)

I think this is a question of semantic. If vp1->v_vnlock == vp2->v_vnlock, should we assume that we must take a recursive lock on v_vnlock, or single lock, and accept the recursive lock on entry? So far I changed the code in assumption that a single lock should be taken:

• the v_vnlock lock might not have enabled recursion for exclusive
• if we take two locks, and there is an TDP_DEADLKTREAT flag set on curthread, then second shared lock would be blocking

[markj]

sys/kern/vfs_vnops.c

4408 ↗

(On Diff #177955)

Sorry, I don't understand. Callers of vn_lock_pair() tend to use the pattern

VOP_UNLOCK(vp1);
if (vp1 != vp2)
    VOP_UNLOCK(vp2);

to unlock the vnodes. So, if vp1 != vp2 && vp1->v_vnlock == vp2->v_vnlock they will release the lock twice, so we must recursively acquire the lock in order to maintain this pattern.

But then, as you pointed out, maybe recursive locking is disallowed.

[kib]

sys/kern/vfs_vnops.c

4408 ↗

(On Diff #177955)

I agree, but this is not avoidable IMO. Callers must be aware either that vnodes can never share lock, like it is in FFS where this interface was used first, or must be aware of possible aliasing of the lock. The recent nullfs self-mounting case is an example of the situation where recursion has to be disabled.

So far Peter found only one weird case with some unionfs test where the pattern of double-locking take place.

[kib]

Make vfs_mount() handle a possibility that covered vp and new mount fs rootvp share the locks.

[jah]

sys/kern/vfs_vnops.c

4408 ↗

(On Diff #177955)

I agree that we shouldn't recurse on the lock behind the caller's back, but to me that seems to beg for a change to the vn_lock_pair() prototype to return the exact lock state (which of the 2 vnodes are locked and whether the lock is aliased) so that the caller can do the right unlock sequence without needing to know about internal vnode state.

[markj]

sys/kern/vfs_vnops.c

4408 ↗

(On Diff #177955)

+1

Or add a new function which handles this uncommon case, and make vn_lock_pair() panic if the two vnodes are different but have the same lock.

[jah]

sys/kern/vfs_vnops.c

4408 ↗

(On Diff #177955)

It might also be nice to have a vn_unlock_pair() function that takes in that return value from vn_lock_pair() and "does the right thing", although that also depends on how common the above pattern is. unionfs for example tends to do weirder things that don't quite fit there.

Speaking of which, what unionfs case was the one that tried to do the double lock? That's probably just a bug, given that I'd expect such a scenario to blow up if attempted with a filesystem like tmpfs that doesn't enable vnode lock recursion.

[kib]

Return status from vn_lock_pair().

[kib]

sys/kern/vfs_vnops.c

4408 ↗

(On Diff #177955)

https://people.freebsd.org/~pho/stress/log/log0665.txt

Seems to be unionfs16.sh from stress2

[jah]

sys/kern/vfs_vnops.c

4408 ↗

(On Diff #177955)

That's a bit different than what I expected to see:
in that case the vn_lock_pair() is coming directly from vfs_domount_first(), and the double lock is probably expected because the test in question mounts unionfs with "-o below" which makes the upper base layer vnode the same as the covered vnode.

But if I'm reading it correctly, it seems your changes here would fix that?

[kib]

sys/kern/vfs_vnops.c

4408 ↗

(On Diff #177955)

According to Peter' report, yes it is.