I didn't see an obvious reason for the use of these marker nodes. Were they intended to keep the lower FS from being unmounted during an in-progress notification by virtue of mnt_uppers being non-empty, or is there some other reason they're needed? If we no longer need them, I should be able to clean this code up quite a bit.

It seems like we should be able to clean the FFS error handling code up quite a bit by just directly issuing dounmount with MNT_FORCE | MNT_RECURSE | MNT_TASKQUEUE from the failing context. That would allow us to get rid of the fsfail_task construct and would also avoid blocking the global taskqueue_thread with a potentially-expensive recursive unmount.

Suppose that this delayed unmount becomes popular enough, and one of our upper mounts also has upper mounts. Wouldn't the system deadlock?

They are used in the current code because iteration is protected by mount interlock, but we obviously have to drop the interlock for the notification call. This allows modification of the tailq, and we need to regain the iteration position after the call.

This is the case where the "base" filesystem is being unmounted, and the calling thread has requested for it to be done synchronously, i.e. MNT_TASKQUEUE is not present in flags. The uppers, and the uppers of uppers etc., will all be unmounted from the taskqueue context with MNT_TASKQUEUE and won't block here. I've tested recursively unmounting ufs->unionfs->nullfs and it does work.

Given a stacked filesystem hierarchy A->B->C, there could also be a case in which both 'A' and 'B' could be requested to synchronously (and recursively) unmount, but those requests would need to happen in different threads, neither of which could be the taskqueue.

Is there a specific scenario you're concerned about?

I think that's the part I don't understand; uppers can't be removed from the queue during this window, they can only be added at the tail. That means they'll be added somewhere after the marker and picked up by the iteration anyway. Is there some other problem I've missed?

I mean, you delegated one unmount to the taskqueue, and now taskqueue executes it. If this delegated unmount needs to delegate another unmount, it schedules a task and sleeps waiting for the task to finish. But it sleeps in the context of the taskqueue which should execute that another unmount, so the new task basically never picked up for execution.

What prevents a removal from the list?

The delegated unmount doesn't sleep though. Since it was issued by the taskqueue, it will have MNT_TASKQUEUE in 'flags' and instead will just requeue itself at the end of recursive unmount queue and return EINPROGRESS.

vfs_drain_upper_locked(), which is called by both vfs_unregister_upper() and vfs_unregister_for_notification().
Even before this change, nullfs waited for MNTK_VGONE_UPPER to drain, although it doesn't seem like that would've worked with more than one in-progress notification.

I concur with your suggestion.

@kib can the code for marker nodes safely be removed?

Is there a good way to test the fsfail case? I've shied away from doing the cleanup mentioned here without a good way to test it.

Is there a good way to test the fsfail case? I've shied away from doing the cleanup mentioned here without a good way to test it.

Peter Holm has some good tests that start up activity on a UFS filesystem and then kill the underlying disk device.
See tools/test/stress2/misc/gnop8.sh as a prototype for how to run the test.

Yes it can be removed now, I agree.

All the other struct mount fields have a "mnt_" prefix, these new ones ought to have that prefix as well.

You should update this comment with the names of any new flags that are now valid to use with unmount(). It looks like MNT_BYFSID can already be used with unmount, so that flag should be listed here too.

The MNT_* flags are part of the syscall interface, and the name of an interface flag ought to describe what it does rather than how it does it. This is really about making the unmount asynchronous, and the fact that a taskqueue is the kernel abstraction that is used to make the operation asynchronous is just an internal detail. I would suggest "MNT_ASYNC" as the name, but I see there's already a flag with that name. However, MNT_ASYNC is currently only used for mount and not for unmount, so there would be no ambiguity to use the existing MNT_ASYNC flag in this new context.

Or are these new flags not intended to be part of the syscall interface? I don't see any change here to add support in the umount command for using these new flags.

Done.

Good point, I've renamed MNT_TASKQUEUE to MNT_DEFERRED. That said, these new flags aren't intended to be exposed through the sysctl interface. I wouldn't object to exposing MNT_RECURSE through something like a '-r' option to umount(8), but I see no reason to ever expose MNT_DEFERRED to userspace.

This reminds me of something that occurred to me while I was writing the code though: these flags are effectively prevented from being exposed to userspace because they're outside the 32-bit range of unmount_args->flags.
It seems a bit fragile to rely on that though...should we have something like an MNT_KERNEL mask for flags that are only allowed to be passed from kernel-initiated operations, and return EINVAL for any userspace request that includes any of them?