dounmount(9): temporarily enable recursion for the covered vnode lock

[kib]

Summary

For some complex nullfs mount configurations, it is possible to get the
covered vnode lock for the mount shared with some inside-mount vnode
lock.  Then at unmount time, vflush() would recurse on the covered vnode
lock when reclaiming the vnode.

Work around it, but temprorarily allowing recursion on the covered vnode
lock.  Disable recursion after the unmount if it was not enabled before.

PR:     297174


lockmgr(9): add lockcanrecurse(9)

Also add vnode locking wrappers for lockcanrecurse(9) and
lockdisablerecurse(9).

[jah]

sys/kern/vfs_mount.c

1884

disablerec isn't used here, did you mean to make the VN_LOCK_DREC() call conditional on it?

2326

Recommend adding a comment on why we need to do this.

[kib]

Add comment.
Do not unconditionally disable recursion for coveredvnode on error.

[jah]

sys/kern/vfs_mount.c

2326

Just to double-check: instead of forcing the covered vnode lock to recurse in all cases, would it be possible to handle this in a more surgical fashion in nullfs, specifically by adding LK_CANRECURSE when needed in null_lock()?

[kib]

sys/kern/vfs_mount.c

2326

Yes, this is the right question, and I tried to do that way somehow initially. But I did not see how to detect the case in null_lock(). So I decided that impact would be relatively minuscule if I just enable it for covered vnode in unmount.

Also, could other stacked filesystems be affected?

[jah]

sys/kern/vfs_mount.c

2326

unionfs deals with a somewhat similar issue by forcing LK_CANRECURSE if locking the root vnode (VV_ROOT), see the comment around use of LK_CANRECURSE in unionfs_lock(). That works because the semantics of mount_unionfs mean that the covered vnode will always be either the upper or lower base vnode of the unionfs root vnode. Here the situation seems to be different because it looks as though you have a nullfs alias onto a subdirectory of the aliased directory, so the covered vnode lock would be shared by something other than the nullfs root. Would it be possible to simply have null_lock() check the lower vnode against the mount's covered vnode to see if it needs to force LK_CANRECURSE? Another option might be to have null_nodeget() detect the situation and set a flag on the vnode for later use by null_lock().

[kib]

sys/kern/vfs_mount.c

2326

First, I am not sure that it is easy to see where the vnode is covered: it might be the covered vnode for different mount, I believe.
Then, null_lock() is very inconvenient place to set the LK_CANRECURSE: we need exclusive lock to set the flag, but this code region does not own it yet.

For null_nodeget() option, the canrecurse would be global for whole lifetime of the lower vnode. The proposed workaround makes the recursion enabled only for unmount dynamic scope.

In other words, I understand some hackishness of my patch, but IMO it is good enough and have advantages over other possible solutions.

[jah]

sys/kern/vfs_mount.c

2326

I'm not sure if any of the above suggestions would actually be better than what you already have here. One nice thing about the change you have here is that it would allow us to get rid of that check in unionfs_lock(). I haven't thought through the possible impact of globally enabling recursion on this path (e.g. whether it could mask real bugs), but I could believe it would be unlikely to be a real problem.

[jah]

sys/kern/vfs_mount.c

2326

First, I am not sure that it is easy to see where the vnode is covered: it might be the covered vnode for different mount, I believe.

My assumption was that you'd need to explicitly query the null_mount from the nullfs vnode to determine the covered vnode, assuming you could safely access the mount object. You wouldn't want to just query VIRF_MOUNTPOINT on the lower vnode, as you say that could be for an unrelated mount.

Then, null_lock() is very inconvenient place to set the LK_CANRECURSE: we need exclusive lock to set the flag, but this code region does not own it yet.

You don't need to already hold an exclusive lock to pass LK_CANRECURSE to VOP_LOCK(). That's what I had in mind, not calling lock_allowrecurse() which would require the lock.

For null_nodeget() option, the canrecurse would be global for whole lifetime of the lower vnode. The proposed workaround makes the recursion enabled only for unmount dynamic scope.

Yep, that's a definite tradeoff. Any solution that uses null_lock() will apply for all uses of that vnode. It's not clear to me whether it's better to have one vnode that allows recursion even when it doesn't need to vs. a common unmount path that allows recursion even if the filesystem doesn't need it. I could believe that neither one would pose a real problem in practice.