I'm not sure there is any need for a fix in unionfs_ins_cached_vnode(). The current thread cannot have the containing directory's lower vnode locked (else, there is some violation of the cross-filesystem lock rule, basically not to establish any order, higher up in the call stack) because of systematic creation of shadow directories at lookup.

(I also have to go back to my project to globally simplify locking of multiple vnodes in the VFS and double-check whether it can really work.)

Removing the cache is primarily for semantics and lifecycle reasons, but indeed will have a huge simplification effect on several parts of unionfs. However, I don't think it makes much sense to do so without at the same time revising the semantics. I'll do both in the first phase of the plan.

Ah yes, dooming of the lower vnode would happen only on recursive unmount. This is also a paradigm that I will change. But that's true for the time being.

I'd like to check what could happen nonetheless after the lower vnode is doomed, because as you say, even if vflush() is eventually called, we don't really know what "shortly" means, and I'd rather not rely on that.

A small comment explaining why lvp needs to be locked would be great. What about the VV_ROOT case?

Yes, that's a problem, and likely the cause of @pho 's deadlock.

I wouldn't say that VOP_OPEN() is not truly needed. The current state in the VFS is sadly completely ad-hoc, some filesystem expecting it and some others not, with various degrees of consequences. I'm not sure at this point if we will require VOP_OPEN()/VOP_CLOSE() around VOP_READDIR() or not at all, only that we'll have to make a consistent choice.

unionfs_check_rmdir() should return 0 if the directory is (determined) empty or ENOTEMPTY if it is not, but only if the determination didn't get any error. Any error in each of these calls should lead to bailing out immediately with that error, and callers adapted to cope.

This is not a new bug. However, the old code was quite close, but would in the end convert any error to ENOTEMPTY, while the new code steps farther away from that.

lvp must be unlocked here.

Same as above for uvp.

Please keep this warning, as I don't think this case can occur (and should be fixed if it does).

(Too bad C is not Lispy enough...)

VOP_RENAME() seems to be a typo here. Did you mean VOP_READDIR()?

VV_ROOT case? (see unionfs_noderem())

Technically, we don't proactively create shadow directories during lookup if the unionfs instance is mounted read-only (however unlikely it may be for a user to do that). But I'd also like to go back to the question I asked at the top of this review: how likely is it for this specific udvp->lvp locking sequence to present a practical risk of deadlock? In other words, how likely is it that some other agent will want to lock those two specific vnodes on different filesystems in the opposite order?

Actually I don't think this is a case we should encounter today. At this point, if the insmntque1() call hasn't already failed then the unionfs unmount operation's vflush() call should block on the lock we currently hold for the newly-allocated vnode. This should then prevent dooming of the underlying vnodes.

Perhaps these VN_IS_DOOMED() checks should instead be converted to VNASSERTs?

This is really to simplify unionfs_lock() (per the comment there) by synchronizing against any pending lock operation that may still be using the lower vnode lock. That should never be the case for the root vnode, as the root vnode should always have both an upper and lower vnode for its entire lifecycle, so unionfs_lock() should never attempt to use the lower root vnode. Moreover, if we are unmounting a non-"below" unionfs mount, this vn_lock_pair() call will assert because lvp will already be locked (as it is the covered vnode).

I'll add a comment to this effect.

These are error handling cases, so I wouldn't be very concerned about the extra locking cycles. But the other issue is that dvp may be reclaimed due to relocking of udvp in various places (unionfs_relookup, VOP_MKDIR, VOP_SETATTR). At that point, in locking terms udvp will no longer be an alias for dvp, so dvp must be explicitly relocked anyway.

I do prefer your label naming and placement of vn_finished_write() though, so I'll change those.

oops, good catch

good catch, should also branch to unionfs_lookup_cleanup.

This warning actually does occur, and in fact it's extremely spammy during unionfs stress2 testing. IIRC this is because vgonel() calls VOP_CLOSE() on any active vnode, regardless of whether anything had it open.

Indeed, that's what I meant.

Looking back over unionfs_get_node_status(), perhaps a better change would be to keep the warning but allow the close() path to use a NULL status object if one doesn't already exist, rather than allocating a new one through unionfs_get_node_status(). This would certainly be more efficient.

That's right, I forgot the read-only case (which I don't think is that unlikely), mostly because assuming that the containing directory has an upper vnode is so ingrained in the current code (and OK as an assumption as long as it is made as part of a write operation). This also is something that I'll likely change (time of creation for shadow directories should become configurable).

Concerning the different FS deadlock risk, I have a hard time trying to find any useful scenario where two FSes would be combined in opposite ways. That said, foot shooting is easy, and an administrator may try to mount unionfs with layers swapped (I'm pretty sure there is even a PR about such a scenario), which could lead to deadlocks. And that is an unacceptable system behavior, however infrequent it is. However, this doesn't mean this has to be prevented at the level of vnodes. A deadlock avoidance scheme that would directly prevent such erroneous mounts seems a much better solution to me, and will allow to simplify a lot of code.

Since dooming of the underlying vnodes cannot happen before unmount of the union above, if insmntque1() doesn't return failure after having locked the target underlying vnode, the latter simply can't be doomed before it is unlocked. That means that the two VN_IS_DOOMED() calls can't return true, and the blocks below the ifs are effectively dead code.

Given the unionfs vnode is itself locked (through the appropriate underlying vnode), I agree that lowervp can't be doomed regardless it was locked or not because the unionfs vnode can't be doomed yet, given that inmsntque1() checks for MNTK_UNMOUNT, and dounmount() sets this flag first thing, so before dooming the unionfs vnode, which requires its lock. I think we should add at least an assertion about lowervp.

To conclude, I'd just remove the ifs, whose blocks are dead code, and would add assertions that both underlying vnodes are not doomed. And your changes here indeed do not disturb what callers see.

And, for a below mount, calling vn_lock_pair() on the root vnode would assert as well since the upper vnode's lock is locked recursively.

Yes, a comment is warranted. Thanks.

Trailing whitespace.

Trailing whitespace.

Superfluous whitespace.

Looking back over unionfs_get_node_status(), perhaps a better change would be to keep the warning but allow the close() path to use a NULL status object if one doesn't already exist, rather than allocating a new one through unionfs_get_node_status(). This would certainly be more efficient.

Yes, I spotted that more than a year ago and still remember it. Would be fine.

There is indeed the VOP_CLOSE() pairing problem in the VFS. I had some plan at some point, but mostly forgot it and will have to think about it again.

You have to reload unp here for the ENOENT case, else this may cause a crash or an access to the wrong structure from the code at unionfs_open_abort.

Trailing whitespace.

Trailing whitespace.

Same trailing whitespaces as in unionfs_open().

Trailing whitespace.

Trailing whitespace.

All this block seems superfluous. There's no need to tweak flags on LK_UPGRADE or LK_DOWNGRADE. These don't make sense if the calling thread hasn't already locked the vnode, and in this case the underlying lock object can only be changed by this very thread (by an operation that requires holding the lock exclusively and gives back the lock as it is), which means the current thread can always know the state of the lock correctly. Therefore, we should just obey it.

Please add a comment explaining why this is needed, along the lines of: "In-between unlock of the interlock and lock of the lower vnode, another process has a window where it could also acquire the lower vnode lock and then doom the unionfs vnode (changing its lock to v_lock) or perform an operation leading to the creation of an upper vnode, which then would be the one to lock. So we have to check for this once the lower vnode is locked, and if so restart the locking operation."

VV_ROOT case? (see unionfs_noderem())

Answered in unionfs_noderem(), thanks.

Please add a small comment saying that 'lvp_locked' is necessary false on VV_ROOT.

Mentioning unionfs_node_update() as well here and the additional guarantee we need below.

(Proposed text has been justified by hand.)

unp must be reloaded and could be NULL here (as you handled it in unionfs_setextattr). Alternatively, test for error being ENOENT.

You might be thinking of bug 172334, which landed on me fairly recently. If there is a general way to prevent such conflicting mounts at the time the mount is requested (and if that is in fact that only case that could lead to LOR in places like this), then I agree that would definitely be preferable to a vnode-centric solution.

'flags' should be passed here instead of ap->a_flags, otherwise we may miss LK_CANRECURSE set above. More generally, I'll clean up the flags handling in this function so that all updates are made against the 'flags' local, with ap->a_flags only modified in the one case in which we need to pass 'ap' along.

I think you're right that the LK_DOWNGRADE case is superfluous: that should instead be an assert, as already holding lvp's lock exclusive should guarantee that we don't first issue the downgrade request to lvp and then find that the unionfs vnode has been doomed or copied up.

The LK_UPGRADE case is needed though: LK_UPGRADE may cause the lock to be temporarily dropped (e.g. consider the case in which the lock has multiple shared holders), creating a window in which dooming or copy-up is possible.

Yes, exactly. I had listed that PR in the proposal document. I don't think we need to design/implement the mount dependencies deadlock avoidance scheme right now, as it isn't a prerequisite for the changes here. I'll address that point in the first phase as well.

But the other issue is that dvp may be reclaimed due to relocking of udvp in various places (unionfs_relookup, VOP_MKDIR, VOP_SETATTR).

Right, I have been too optimistic with this. Given that, I agree it's not worth the complexity of trying to distinguish the case where dvp and udvp locks are still aliased to try to optimize locking. Optimizing locking is not just for performance, it also gives hints of locking expectations at some points, so can help understanding (or, at least, help ask the right questions).

Avoid an unlock/relock for uvp in the common case.

Yes, the LK_DOWNGRADE case is superfluous for the reason you stated, so an assert would be better.

I forgot about a possible lock drop in LK_UPGRADE. You're right then, the LK_UPGRADE case where LK_EXCLUSIVE is explicitly set is absolutely necessary (and the flag has to be explicit because we are changing the lock on which to operate, and we don't have this one locked).

I think for this we'll want VOP_VPUT_PAIR(udvp, &uvp, false), as we'll be unlocking udvp while a lock is still held on its child.

You're right, this is what, e.g., kern_mkdirat() does.

The need to call VOP_VPUT_PAIR() (a misnomer) complicates the VFS interface in non-obvious ways (and is not documented at all). For a while, I've been having the impression that introduction of that operation may have been ill-advised, but to be fair have not spent enough time to see if a better alternative is possible yet.

I find it very odd that unionfs_lookup() went to the trouble of explicitly creating a negative cache entry in the ENOENT case for dot-dot lookup but does not create a positive entry for successful dot-dot lookup. I don't see any apparent reason for this behavior.