autofs: implement VOP_RMDIR()

[rew]

Summary

The autofs kernel module uses an in-memory data structure that
represents a directory layout where filesystems can be automounted to.

When a directory is removed from an autofs filesystem (i.e. an autofs
node is removed from the in-memory data structure), immediately
reclaim the vnode from autofs using vgone() from vop_rmdir().

• not for the commit message but a note to reviewers --

it seems a bit unorthodox calling vgone() from vop_rmdir() so im curious
about this approach

the reason I did this was that it is more straight-forward to alloc/free
autofs nodes and vnodes from scratch as opposed to re-using the nodes
and having a mechanism that synchronizes the in-memory autofs directory
layout with active/deleted autofs nodes

autofs doesn't use nearly as many vnodes as a traditional filesystem

[kib]

Your change consists of two parts, each of which causes IMO quite serious questions:

• you removed the protection of an_vnode_lock from reclaim. It seems that the lock is needed to synchronize with autofs_node_vn().
• you changed the basic model of the autofs node lifecycle where nodes were only destroyed on unmount, to destroy them at runtime.

Neither of the changes are even mentioned in the summary, nor it is explained why these changes are needed and why they are correct.

[rew]

In D57159#1310300, @kib wrote:

• you removed the protection of an_vnode_lock from reclaim. It seems that the lock is needed to synchronize with autofs_node_vn().

with the lifecycle change, the autofs node is deleted in autofs_rmdir() and when reclaim() happens the an_vnode_lock is already destroyed.

am_lock is taken for autofs_node_new() and autofs_node_find() but then the lock is dropped before autofs_node_vn() - I'm thinking the locking could be refined to be less racy given the autofs node lifecycle change

• you changed the basic model of the autofs node lifecycle where nodes were only destroyed on unmount, to destroy them at runtime.

yes, an autofs filesystem isn't stored on-disk so the (in-memory) autofs nodes represent the current directory layout where filesystems can be automounted to.

when an autofs node is created, it becomes a possible mountpoint for a filesystem...there's times when these autofs nodes become stale and need to be removed

Neither of the changes are even mentioned in the summary, nor it is explained why these changes are needed and why they are correct.

these changes are needed to remove stale autofs entries when removable media is removed from the system, see https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=241531

per correctness, it made sense (to me) create/delete autofs nodes as they become active/stale so that the autofs node tree is the source of truth for the current directory layout of an autofs filesystem

I want to hook up autofs to devctl events or nlsysevents so that removable media can be automounted on attach - and when removable media is detached, the stale autofs node will be removed

open to suggestions or figuring out other questions you have - thanks

[kib]

I spent some time reading autofs, and I see that it is currently quite broken. Your change breaks it even more.

Right now, there are two invariants:

• nodes are never destroyed until unmount
• node lock is before vnode lock, for vnode attached to the node

The 'never destroyed' part is what makes the autofs_node_vn() usually not crash, since node is guaranteed to be alive until unmount. I think that a parallel unmount would break it, if the node is removed before the node lock is taken. Your patch removes this safety.

The current code is broken WRT the lock order, the vnode reclaim takes the node lock after the vnode lock. It is somewhat puzzled me why witness is silent, but apparently node lock is initialized to suppress witness checks. Reclaim must take the node lock, otherwise it races with autofs_node_vn(). Your patch removes this precaution as well.

I did not read all details about autofs, but it is enough to write the review above.

[rew]

In D57159#1310749, @kib wrote:

I spent some time reading autofs, and I see that it is currently quite broken. Your change breaks it even more.

I'm willing to spend time to sort out locking issues and draft a separate review before pursuing this one

do you have time/interest in reviewing if I post a review for autofs locking fixes?

and if you are able to give a high-level overview of how the locks should interact..it'd be much appreciated

thanks for your time thus far