The current diff seems a bit incomplete - previous one seemed ok, something between Phab and Git, perhaps?

Anyway: first, I think you really should use FSIDs. In particular, you might have a direct mount (an autofs mounted on eg /mnt, and then automounted other filesystem on top of that same directory, instead of a subdirectory, like with indirect mounts).

Also, the "const struct statfs *sfp" could be spelled "const struct statfs *sb", for consistency (even though I've failed at this myself, see find_statfs() just below).

While this patch works and was tested by the reporter; it does spam the log file.

autounmountd(8) will attempt to unmount a filesystem that has been automounted every 600 seconds (the default). If the filesystem is busy, the unmount fails; which is expected behavior.

The log file is getting spammed because the function I introduced calls umount(8) using auto_popen() and auto_pclose().

auto_pclose() logs all warnings when the exiting process (umount in this case) returns an error (device busy).

So, every 600 seconds an umount error is logged when the filesystem is still in use.

A few thoughts:

1. use log_debugx() instead of log_warnx() within auto_plcose().
2. don't let autounmountd attempt to umount filesystems that are still in use.
3. dont worry about it
4. step away from the keyboard
5. (3) and (4).

I'm not sure this is the correct approach. First, well, as you've mentioned it spams the logs, and I'd rather not complicate the logging mechanism just to work around that. Second - it probably doesn't matter on your typical desktop, but I still feel that the overhead might be a bit too much, esp with autounmountd configured to be really aggressive (think autounmountd -t 2 -r 2).

Given that all this is just a workaround for NFS: perhaps instead of executing umount(8), just add code to autounmountd(8) to run rpc.umntall(8) after unmounting an nfs share? In other words: do unmount_by_fsid() like before, but if the filesystem type is "nfs", also execute rpc.umntall(8) for that specific host/path using auto_popen().

In D27801#636318, @trasz wrote:

I'm not sure this is the correct approach. First, well, as you've mentioned it spams the logs, and I'd rather not complicate the logging mechanism just to work around that.

No need to complicate the logging mechanism. In auto_pclose(), it's literally changing log_warnx() to log_debugx().

Look at the consumers of auto_pclose(). When auto_plcose() returns with an error code, every consumer then calls log_errx(). In short, it's getting double-logged anyway. Switching to log_debugx() within auto_pclose() will also fix that. An alternative would be to not use the autofs popen/pclose functions, which also solves the double-logging issue.

Second - it probably doesn't matter on your typical desktop, but I still feel that the overhead might be a bit too much, esp with autounmountd configured to be really aggressive (think autounmountd -t 2 -r 2).

Hmm...the real wtf here is trying to unmount all the automounted filesystems every two seconds - it'd be better if autounmountd(8) was notified as soon as the filesystem can be unmounted. autounmountd(8) should support instant unmounting and not in the form of while (1) { unmount; sleep 2 } (which is basically what your provided command snippet does).

Given that all this is just a workaround for NFS: perhaps instead of executing umount(8), just add code to autounmountd(8) to run rpc.umntall(8) after unmounting an nfs share?

automount(8) and autounmountd(8) can both unmount automounted filesystems, they'd benefit from sharing a common unmount routine. That's why I introduced unmount_automount() in the review.

Even using rpc.umntall(8), a common unmount routine (shared between automount(8) and autounmountd(8)) is still beneficial. If you don't have a common unmount routine, you'd have to sprinkle rpc.umntall's wherever unmount(3) is called.

In other words: do unmount_by_fsid() like before, but if the filesystem type is "nfs", also execute rpc.umntall(8) for that specific host/path using auto_popen().

Why? umount(8) works without having to filter by the filesystem type.

A few take-aways:

1. The filesystems are mounted with mount(8), it should be perfectly acceptable to unmount them with umount(8).
2. The aggressive timeout scenario shouldn't be a blocker for using umount(8).
3. If the logging pitfalls of umount(8) with auto_pclose() can't be reconciled by modifying auto_pclose(); then I suggest not using the auto_popen()/auto_pclose() functions when making the umount(8) call.

In D27801#636992, @rew wrote:

In D27801#636318, @trasz wrote:

I'm not sure this is the correct approach. First, well, as you've mentioned it spams the logs, and I'd rather not complicate the logging mechanism just to work around that.

No need to complicate the logging mechanism. In auto_pclose(), it's literally changing log_warnx() to log_debugx().

Look at the consumers of auto_pclose(). When auto_plcose() returns with an error code, every consumer then calls log_errx(). In short, it's getting double-logged anyway. Switching to log_debugx() within auto_pclose() will also fix that. An alternative would be to not use the autofs popen/pclose functions, which also solves the double-logging issue.

It's not double-logged, because both log different things. The calls to log_warnx() from auto_popen() are informative, the calls to log_errx() afterwards are just to exit.

Second - it probably doesn't matter on your typical desktop, but I still feel that the overhead might be a bit too much, esp with autounmountd configured to be really aggressive (think autounmountd -t 2 -r 2).

Hmm...the real wtf here is trying to unmount all the automounted filesystems every two seconds - it'd be better if autounmountd(8) was notified as soon as the filesystem can be unmounted. autounmountd(8) should support instant unmounting and not in the form of while (1) { unmount; sleep 2 } (which is basically what your provided command snippet does).

It would be nice for the kernel to be able to notify userspace that a filesystem is no longer active, that's true. But currently there's no such a way I know of.

Given that all this is just a workaround for NFS: perhaps instead of executing umount(8), just add code to autounmountd(8) to run rpc.umntall(8) after unmounting an nfs share?

automount(8) and autounmountd(8) can both unmount automounted filesystems, they'd benefit from sharing a common unmount routine. That's why I introduced unmount_automount() in the review.

Even using rpc.umntall(8), a common unmount routine (shared between automount(8) and autounmountd(8)) is still beneficial. If you don't have a common unmount routine, you'd have to sprinkle rpc.umntall's wherever unmount(3) is called.

Agreed, removing useless code duplication is a good thing.

In other words: do unmount_by_fsid() like before, but if the filesystem type is "nfs", also execute rpc.umntall(8) for that specific host/path using auto_popen().

Why? umount(8) works without having to filter by the filesystem type.

For two reasons. First, executing umount(8) is somewhat costly; there's a whole lot going on there, as opposed to a unmount(2), which, in the common case, will just dereference a few pointers, notice there's at least one active vnode, and then return.

Second, and this is the actually important one: because this complicates one system component to work around a problem in another. There's no reason to use umount(8) instead of unmount(2) other than working around a hack in our current NFS (not even all of NFS, this is only applicable to NFS3) implementation. When you paper over bugs all over the place instead of fixing them the right way, you end up with technological debt.

A few take-aways:

1. The filesystems are mounted with mount(8), it should be perfectly acceptable to unmount them with umount(8).

Filesystems are mounted with mount(8) for a reason, which applies to almost all of them, and which is reasonable (ie not a random bug workaround). That's why we can have separate mount_whatever(8) binaries. Stuff like FUSE requires running filesystem-specific code in userspace.

Unmounting is different. There are no separate umount programs. And the only reason for making autofs use umount(8) is to work around a leftover from the eighties applicable to a single specific filesystem.

2. The aggressive timeout scenario shouldn't be a blocker for using umount(8).

It isn't. It's a... "secondary argument", for a lack of better word - if #1 wasn't the case, the aggresive timeout wouldn't matter; I don't think the overhead would be noticeable.

3. If the logging pitfalls of umount(8) with auto_pclose() can't be reconciled by modifying auto_pclose(); then I suggest not using the auto_popen()/auto_pclose() functions when making the umount(8) call.

So... what happens if umount(8) fails for a reason? As in, actual reason, not the one we expect. If we were to go that route, I'd suggest adding an option to umount(8) to silently ignore that specific error.

Basically: if we can't fix the problem properly, for example by making the kernel notify the NFS3 server it's already talking to, let's at least make the workaround simple and non-intrusive. Thus my suggestion of using rpc.umntall(8). (Which, as you've suggested, would also benefit from refactoring to have a common unmount code.)

At the end of the day, /var/db/mounttab and /var/db/mountdtab are purely advisory (as I understand it). If you don't care about autofs maintaining the consistency of these files, then I don't care. I'm starting to spend too much of my time talking about files that don't even matter.

It's not double-logged, because both log different things. The calls to log_warnx() from auto_popen() are informative, the calls to log_errx() afterwards are just to exit.

my mistake, thanks for the clarification.

executing umount(8) is somewhat costly; there's a whole lot going on there, as opposed to a unmount(2)

yes, it does cost more - it also fixes all the issues we're discussing.

Second, and this is the actually important one: because this complicates one system component to work around a problem in another. There's no reason to use umount(8) instead of unmount(2) other than working around a hack in our current NFS (not even all of NFS, this is only applicable to NFS3) implementation.

Well, that means duplicating the work umount(8) does - autofs needs to play nicely NFS3, last I checked, NFS3 was still in use.

Unmounting is different. There are no separate umount programs. And the only reason for making autofs use umount(8) is to work around a leftover from the eighties applicable to a single specific filesystem.

yea, exactly - and umount(8) already targets that specific filesystem.

So... what happens if umount(8) fails for a reason? As in, actual reason, not the one we expect.

same thing when unmount(2) fails, look at the error and decide what to do - maybe just get rid of unmount(2) and use umount(8). When the eighties call and want NFS3 back, we can go back to unmount(2).

If we were to go that route, I'd suggest adding an option to umount(8) to silently ignore that specific error.

uhhh..strongly disagree with that.

let's at least make the workaround simple and non-intrusive. Thus my suggestion of using rpc.umntall(8).

agreed. I only know what the man page says about rpc.umntall(8), but it looks like it needs a hostname/path. This would require to do a backward lookup using the FSID:

if (unmount is succesful && IS_NFS)
     hostname = get_name_from_fsid(FSID)  /* probably by a statfs lookup? */
     auto_popen(rpc.umntall, hostname);

At any rate, really appreciate you taking the time to look at my review - you're knowledge and insight has been helpful, thanks.

Thanks for not giving up on this.

So, first of all: I think we can just call "rpc.umntall -k" after unmounting, without passing it the host and path. And I think we can skip checking the filesystem type and just call it every time we successfully unmount anything (as opposed to doing it on each retried unmount attempt); the "-k" option will make it iterate /var/db/mounttab and not do anything if there's no NFS server to notify.

I do see a potential problem with it, though, and it's kind of inherent to the way NFS3 works. Imagine a situation where we're unmounting a filesystem; we're past the point where we called unmount(2), rpc.umntall(8) has scanned the filesystem list and determined it needs to notify the remote mountd(8). In that very moment some process wants to access the filesystem again; automountd(8) does its job... And then the rpc.umntall(8), running in parallel, sends the MOUNTPROC_UMNT RPC. We end up with a mounted filesystem without a mounttab entry. Very similar scenario would happen if we were using umount(8) instead. I'm not sure if it would break anything - I'm not sure what purpose mountd(8) serves after a filesystem is mounted - but it's something to keep in mind.

Given we don't have much time to get it into 13.0-RELEASE, and refactoring can be somewhat tricky (for example, the whole autofs machinery tries to do very precise logging, only skipping stuff that it's absolutely sure is fine, eg only ignoring EBUSY when autounmounting, but not when called by hand as "automount -u"), I'd lean towards not doing the refactoring at this time, and instead simply slapping the call to rpc.umntall in those two places where we call unmount(2).

This sounds good to me - I’ll get to this in a couple days/this week.

ping

if we're on the same page here, I'll pass this patch off to the bugzilla reporter for testing (in addition to the testing I've done)...