Standardize unmount implementations across backends#687
Standardize unmount implementations across backends#687khanhtranngoccva wants to merge 14 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2aa2af070
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cbd4be116
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| fn unmount_flags() -> nix::mount::MntFlags { | ||
| if cfg!(target_os = "linux") { | ||
| // FIXME: Only supported on Linux 2.4.11+, procfs may be needed to determine version | ||
| nix::mount::MntFlags::MNT_DETACH |
There was a problem hiding this comment.
On BSD/macOS builds this helper is still type-checked as one function because cfg! is just a boolean, so the Linux-only MNT_DETACH associated const must exist even though that branch will not run. The nix BSD MntFlags set does not define MNT_DETACH, so this breaks the non-Linux targets that libc_umount explicitly supports; split the branches with #[cfg] instead of cfg!.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| fn parse_fusermount_unmount_stderr(output: &OsStr) -> Option<OsString> { | ||
| let parse_regex = Regex::new(r"([^:]+): failed to unmount ([^:]+): (.+)") |
There was a problem hiding this comment.
When fusermount fails for a mountpoint whose path contains a colon, this regex stops the mountpoint capture at the first colon in the path, so stderr like fusermount3: failed to unmount /tmp/a:b: Device or resource busy yields b: Device or resource busy as the error text. That prevents the errno lookup from recognizing EBUSY, so retry_on_unmount_errors returns immediately instead of retrying for valid Unix mountpoint paths containing :.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f355082ff8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| unsafe { | ||
| fuse_session_destroy(fuse_session); | ||
| } | ||
| return Err(ensure_last_os_error()); |
There was a problem hiding this comment.
In the fuse_session_mount failure path, the cleanup call runs before reading the failing call's errno. errno is only reliable until the next libc/libfuse call, so fuse_session_destroy can overwrite it and cause callers to see an unrelated or unspecified mount error; capture ensure_last_os_error() before cleanup, and apply the same pattern to the fd < 0 cleanup below.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afce92ce55
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| use crate::Errno; | ||
|
|
||
| unsafe extern "C" { | ||
| #[cfg(not(any(target_os = "dragonfly", target_os = "vxworks", target_os = "rtems")))] |
There was a problem hiding this comment.
For target_os = "dragonfly", build.rs still selects the pure-rust mount backend, so this new module is compiled. This cfg removes the only errno_location declaration on that target, but set_errno calls errno_location() unconditionally, leaving the supported DragonFly build with an unresolved function before the unmount code can compile; add the DragonFly errno symbol or gate set_errno instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 777b0fcdcd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use procfs::process::Process; |
There was a problem hiding this comment.
On the FreeBSD CI path, build.rs selects the pure-rust backend for FreeBSD without libfuse, and .github/workflows/ci.yml runs cargo test --all --all-targets while only skipping mnt::test::mount_unmount. This new test imports Linux procfs and reads /proc/self/mountinfo, which FreeBSD does not provide, so the BSD job will fail unless this test/dependency is gated to Linux or rewritten with a portable mount check.
Useful? React with 👍 / 👎.
| // TODO: Some systems do not support lazy unmounting and may return errors | ||
| .arg("-z") | ||
| .arg("--") | ||
| .env("LC_MESSAGES", "C") |
There was a problem hiding this comment.
When the parent process has LC_ALL set, the child still uses that locale because LC_ALL takes precedence over LC_MESSAGES. In that environment, fusermount can emit localized strerror text, but the lookup below always uses the C-locale mapping, so helper failures such as EBUSY turn into parse/lookup errors and bypass the intended retry; set LC_ALL=C or remove/override it for this child as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9db9bbe4dd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| fuse_session_unmount(self.fuse_session); | ||
| fuse_session_destroy(self.fuse_session); | ||
| }; | ||
| Ok(()) |
There was a problem hiding this comment.
In the libfuse3 backend this path no longer performs the direct libc_umount(&mountpoint) call, and fuse_session_unmount is a void FFI call, so Mount::umount() now returns Ok(()) even when libfuse could not or did not unmount the filesystem. In scenarios such as a kernel-aborted FUSE connection where libfuse's unmount helper does almost nothing, callers will be told cleanup succeeded while the mountpoint remains stale; keep a status-bearing unmount path before destroying the session.
Useful? React with 👍 / 👎.
| &[MountOption::AutoUnmount], | ||
| SessionACL::All, |
There was a problem hiding this comment.
The normal Linux unit-test workflow I checked in .github/workflows/ci.yml installs libfuse packages but does not enable user_allow_other in /etc/fuse.conf. This new test passes SessionACL::All, so fuse_mount_fusermount adds allow_other together with auto_unmount; on the default fusermount configuration that is rejected and the .unwrap() below panics before the unmount behavior is exercised. Gate/ignore this integration-style test or set up the required fuse.conf option.
Useful? React with 👍 / 👎.
|
There is an issue with auto_unmount that I encountered during production: libfuse/libfuse#1529 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1e1eaa02a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| std::thread::sleep(interval); | ||
| continue; |
There was a problem hiding this comment.
When a mount stays busy because another process keeps a cwd or open file under it, the unmount operation can keep returning EBUSY until that process exits. This helper now sleeps and retries forever, and it is called from Drop, so dropping a session in that scenario can hang the application indefinitely instead of returning or logging the busy error as the previous unmount path did; add a timeout/max attempts or propagate persistent EBUSY.
Useful? React with 👍 / 👎.
| MountOption::FSName("fuser".to_string()), | ||
| ]); | ||
| config.n_threads = Some(2); | ||
| config.acl = SessionACL::All; |
There was a problem hiding this comment.
I checked .github/workflows/ci.yml lines 78-84: the Linux job runs cargo test --all after installing libfuse but never enables user_allow_other in /etc/fuse.conf. This test always maps SessionACL::All to allow_other, so default fusermount rejects spawn_mount(...).unwrap() before the auto-unmount behavior is exercised; gate/ignore it unless that fuse.conf setup is present.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a90beccf59
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| &mut iov, | ||
| Some(&mut cmsg_buffer), | ||
| MsgFlags::empty(), | ||
| MsgFlags::MSG_CMSG_CLOEXEC, |
There was a problem hiding this comment.
When building the pure-rust backend for target_os = "openbsd" (selected in build.rs), this line is compiled, but nix 0.30 exposes MSG_CMSG_CLOEXEC only for linux/android, freebsdlike, and netbsdlike targets. That leaves the supported OpenBSD build failing before the mount code compiles; use MsgFlags::empty() with a separate FD_CLOEXEC fallback or cfg this flag to the platforms that define it.
Useful? React with 👍 / 👎.
| MountOption::AutoUnmount, | ||
| MountOption::FSName("fuser".to_string()), | ||
| ]); | ||
| config.n_threads = Some(2); |
There was a problem hiding this comment.
In the FreeBSD CI workflow, this new test is run by cargo test --all --all-targets, but Session::run returns n_threads != 1 is only supported on Linux whenever n_threads is set above one on non-Linux targets. As a result spawn_mount can return a background session whose umount_and_join() fails on FreeBSD for this setting rather than testing unmount behavior; leave the default thread count or gate the test to Linux.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6006d4cf62
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if mapping.contains_left(&errno) { | ||
| continue; | ||
| } | ||
| let error_str = match strerror_r_dynamic(errno.code()) { |
There was a problem hiding this comment.
In musl builds, this table is populated from the fuser process's libc, but the stderr being parsed comes from the host fusermount binary. For example, a static x86_64-unknown-linux-musl binary running on a glibc system will look up musl's EBUSY text (Resource busy, per musl's errno table at https://git.musl-libc.org/cgit/musl/tree/src/errno/__strerror.h), while glibc fusermount reports Device or resource busy; then get_errno_by_message returns None, so busy unmounts become Other and bypass the retry path.
Useful? React with 👍 / 👎.
1 participant
This PR fixes #686.
Fixes implemented:
Limitations: