automount(8): try umount(8) before unmount(3)

[rew]

Summary

When called as automount -u, try using umount(8) before unmount(3). When an
NFS mount is being unmounted, umount(8) notifies the mountd server and if
successful, will also remove the associated entry from /var/db/mounttab.

This uses auto_popen() to call umount(8) in the same fashion that
automountd(8) uses auto_popen() to call mount(8) when it mounts the filesystem.

PR:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=251395
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=251906

Test Plan

mount NFS using automountd(8)
ensure entry is in /var/db/mounttab
automount -u
ensure entry is removed from /var/db/mounttab

[rew]

The fsid wasn't be constructed correctly in all cases.

Use f_mntfromname member from the statfs struct for umount(8) instead.

[trasz]

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).

[rew]

It is incomplete, my mistake.

I'm updating this review with my original (broken) patch.

The patch for computing FSID's worked for me locally, but failed when tested by
Martin as reported at https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=251906#c5.
Looks like the string buffer didn't get populated correctly..unfortunately
I wasn't been able to reproduce that issue locally. Hence, the switch to
using f_mntfromname.

Even with a direct map, I thought f_mntfromname will be different and f_mnttoname
would be the same. Even so, I hear what you're saying; FSID's are guaranteed to
be unique.

I'll see if I can track down where my programming error was in computing the FSID.
It'd be nice if umount(8) would accept the FSID in the format of "FSID:val0:val1"
instead of hexadecimal, but eh. unmount(3) accepts the "FSID:val0:val1" format.

I'll make the suggested changes and update when I figure the FSID error.

Thanks for the feedback.

[rew]

Create a common unmount routine, unmount_automount(), to be used by
unmount_by_statfs() and unmount_by_fsid(). unmount_automount() defaults to
using umount(8) and will fallback to unmount(3).

Fix bug while building up hexadecimal FSID string. The issue was using
strlen() on an uninitialized character array.

The error returned from trying to unmount FSID:0:0 is EACCES, not
EPERM.

The reason I centralized umount_by_statfs() and umount_by_fsid() is so that umount(8) will consistently be given priority over unmount(3).

The motivation for giving priority to umount(8) is that it does some additional work
when unmounting NFS filesystems. Specifically, writing/cleaning up
/var/db/mounttab and /var/db/mountdtab files on the client and server.

[rew]

fix missed free() call.

[rew]

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).

[trasz]

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().

[rew]

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.

[trasz]

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.)

[rew]

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.

[rew]

I’m resurrecting this - I do care about fixing this. The reporter, whose been an invaluable resource for testing and bug reports w.r.t. autofs would like this fixed.

trasz@, let me know if you’re satisfied with the approach below and I’ll update this review again:

In the common unmount routine:

if (unmount is successful)
     mount_info = lookup_by_fsid(fsid) /* using getmntinfo */
    if (mount_info filesystem type is NFS)
         auto_popen/pclose(rpc.umntall)

lookup_by_fsid would just loop through getmntinfo and filter out the correct fsid.

[trasz]

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).

[rew]

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

[rew]

Call rpc.umntall(8) when an automounted filesystem is successfully unmounted.

• introduce a function called rpc_umntall() that sets up the call to rpc.umntall(8).

rpc_umntall() is used by unmount_by_fsid and unmount_by_statfs.

[rew]

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)...

[trasz]

Looks good to me. Thanks for fixing it :-)