unionfs: various locking fixes

[jah]

Summary

--Clearing cached subdirectories in unionfs_noderem() should be done

under the vnode interlock

--When preparing to switch the vnode lock in both unionfs_node_update()

and unionfs_noderem(), the incoming lock should be acquired before
updating the v_vnlock field to point to it.  Otherwise we effectively
break the locking contract for a brief window.

unionfs: assorted style fixes

No functional change intended, beyond slightly different panic strings

[jah]

phabricator doesn't make it very clear, but the locking and style changes are 2 different commits.

[kib]

sys/fs/unionfs/union_subr.c

215

There should be spaces around binary ops, like uppervp == NULLVP

442

I do not think that () are needed there.

I am not sure what to do about possible lock recursion there. It would be enough to assert that either of lower/upper vnodes are not recursively locked, if exist. unionfs_node_update() does handle the recursion.

You might also add the LK_NOWAIT flag, since the lock must not be owned by anybody.

1255

This is a strange block. Either we cannot handle such situation (empty read) at all, or we ignore it, under DIAGNOSTIC or not.

Most likely, this check should be moved somewhere into vop_readdir_post under INVARIANTS.

1329

Again this is strange, panic will re-enter the debugger. Perhaps it should be KASSERT, and then remove notyet?

[jah]

sys/fs/unionfs/union_subr.c

1255

Agreed. For now I'll make this a KASSERT in the same place as the current check.

1329

I'm not sure why the KDB part was written like this; I suspect the vnops check under notyet was pasted from the equivalent nullfs code, But in any case, I'd rather have a KASSERT here to catch some strange corner case where we're not using a unionfs node, before trying to access fields of that node.

[jah]

Further style fixes, improve locking assertions and vnode checking

[kib]

sys/fs/unionfs/union.h

119 ↗

(On Diff #97729)

121 ↗

(On Diff #97729)

122 ↗

(On Diff #97729)

Generally, VOP_LOCK() implementations _must_ be able to handle reclaimed vnodes. Because we might wait for the lock while VOP_RECLAIM() is running.

I.e. I think the case is there to stay.

sys/fs/unionfs/union_subr.c

189

Are uvp/lvp supposed to be locked there (I assume yes)?

445

[jah]

sys/fs/unionfs/union_subr.c

189

Good point, actually no they won't be locked for unionfs_get_cached_vnode(). unionfs_nodeget() first tries to lookup any matching vnode from the cache and returns if it finds one. Only after that does it lock lvp/uvp, at which point lvp/uvp are also checked for VI_DOOMED. lvp/uvp will therefore be locked at the time unionfs_ins_cached_vnode() is called, and they will not be passed to unionfs_ins_cached_vnode() if they are doomed, so the VDIR assert there is valid.

Here they will be referenced but not locked, so v_type may still transition to VBAD.

[jah]

Remove asserts from unionfs_get_cached_vnode(), add locking asserts to unionfs_ins_cached_vnode()