Clean up unionfs thread arguments and implement VOP_VPUT_PAIR

[jah]

Summary

This review contains 2 commits:

1. Remove unnecessary thread argument from unionfs_nodeget() and _noderem()

No functional change intended.

2. unionfs: implement VOP_VPUT_PAIR

unionfs must pass VOP_VPUT_PAIR directly to the underlying FS so that
it can have a chance to manage any special locking considerations that
may be necessary.

The unionfs implementation is based heavily on the corresponding nullfs
implementation.

[kib]

The thread arg removal patch looks fine.

sys/fs/unionfs/union_vnops.c

2612

I suspect it is not safe to do VOP_VPUT_PAIR() on one pair while another one is locked. This note is true for other call to VOP_VPUT_PAIR() in this function as well.

Is it really needed to call both VOP_VPUT_PAIR()? If yes, there is a fundamental brokeness in unionfs design. It should be so that you need to do either upper, or lower call, not both.

[jah]

sys/fs/unionfs/union_vnops.c

2612

The behavior here is based on the fact that currently unionfs_lock()/unionfs_unlock() operate on both lower and upper vnodes, if present.

I already have plenty of concerns about that behavior, and at some point I'd like to sit down and see if I can rethink the whole locking scheme. I haven't yet seen a failure that I can attribute to unionfs_lock()'s behavior, but it seems dangerous to lock two vnodes without any assistance from vn_lock_pair() or something like it. I'm also skeptical that the lower vnode lock is needed at all once the upper vnode is present, although propagating the lower vnode lock state to the upper vnode during e.g. unionfs_copyfile() would still be problematic.

Is your specific concern here about the possibility for an underlying FS implementation to relock the vnodes, leading to LOR? Does that concern still hold even though unlock_vp is set in the calls to the lower FS?

[kib]

sys/fs/unionfs/union_vnops.c

2612

If you own a vnode lock on some mp, you generally cannot safely lock any vnode on other mp.

At very least, lookup() establishes the order between locks by following the path: containing directory lock is before any vnode lock having a name in this directory. Through vp_crossmp, this order also crosses mount points.

If you own one vnode lock, you cannot know where is another vnode' lock in this order.

But there are more isolated and corner cases. One big can of worms is the UFS quota support. First, quota file locks are essentially after any other vnode locks due to the code organization, but they can be reached by normal lookups. But, quota files can be put on other filesystem (not the one which they serve). This adds even more LOR violations.

[jah]

sys/fs/unionfs/union_vnops.c

2612

Then that's really the fundamental brokenness, that unionfs_lock() always tries to independently lock both vnodes, right?
As long as that remains true, then I don't think unionfs_vput_pair() has any choice but to call VOP_VPUT_PAIR() on both vnodes.

I could be missing something, but given that I'm setting unlock_vp on both vnodes, it doesn't seem that I'm making the problem worse with this change. This change also fixes a panic that's easy to reproduce during stress testing, so what I'd like to do is commit this change along with a TODO comment that it should be revisited once the unionfs locking scheme has been reworked.

Then, I'll see if I can fix the fundamental issue with unionfs_lock(). Ultimately, most unionfs operations other than lookup() don't care about the lower vnode once the upper vnode is present. I wonder if I could take the approach of changing unionfs_lock() to only lock one vnode or the other, while using vn_lock_pair() for the small number of cases in which both vnode locks need to overlap?

[kib]

sys/fs/unionfs/union_vnops.c

2612

I do not intend to block this change in any way.

Yes, my belief is that reasonable unionfs implementation should be fine with only locking upper or lower vnode (or dvp/vp pair), almost never it needs to lock both upper and lower symultaneously. Open question is how to detect whether we have or not the upper vnode, in safe manner. Another open question is how to lock all four safely.

Both UFS and msdosfs rename handle safe locking of three or four vnodes, fdvp/fvp and tdvp/(tvp or NULL). But since locks are dropped, VOPs have to do sleep-less relookup to ensure that names did not change.

[jah]

Remove some write-only variables, add locking commentary

[pho]

All of the unions tests in stress2 runs without any issues when D33008.98931.diff is added.

[jah]

ping

[kib]

In D33008#752188, @jah wrote:

ping

You should take responsibility for unionfs commits on yourself. I already stated my opinion, including the note that I do not object against the change in any way. Commit it with 'Discussed with: kib' tagline.