spfs: add an option for mounting overlayfs layers using syscalls #1181
Conversation
davvid
force-pushed
the
mount-syscalls
branch
3 times, most recently
from
cc65962 to
5b5ba1a
Compare
davvid
force-pushed
the
mount-syscalls
branch
2 times, most recently
from
d46ae20 to
d122c81
Compare
Teach spfs to mount /spfs using direct overlayfs syscalls. This is provided as an opt-in feature so that users can test this alongside the current "mount"-based implementation. If no issues are encountered then this can be made the default behavior in the future. This approach avoids the command-line length limits that are present when using the "mount" command to create the /spfs mount. Related-to: spkenv#968 Related-to: spkenv#1164
davvid
force-pushed
the
mount-syscalls
branch
from
d122c81 to
7f63180
Compare
davvid
force-pushed
the
mount-syscalls
branch
from
7f63180 to
cb0ada7
Compare
| pub const SPFS_DIR: &str = "/spfs"; | ||
| pub const SPFS_DIR_PREFIX: &str = "/spfs/"; | ||
| const SPFS_DIR_CSTR: &CStr = c"/spfs"; |
There was a problem hiding this comment.
Maybe redefine SPFS_DIR as SPFS_DIR_CSTR.to_str().expect("valid utf-8")?
| .expect("allocate CString for work_dir"); | ||
|
|
||
| // fd = fsopen("overlay", FSOPEN_CLOEXEC) | ||
| let rc = unsafe { syscall!(SYS_fsopen, c"overlay".as_ptr(), FSOPEN_CLOEXEC) }; |
There was a problem hiding this comment.
Please add // Safety: ... comments that describe what safety invariants are being upheld to all uses of unsafe.
| .expect("allocate CString for lower_dir"); | ||
| let upper_dir = CString::new(rt.config.upper_dir.to_string_lossy().as_ref()) | ||
| .expect("allocate CString for upper_dir"); | ||
| let work_dir = CString::new(rt.config.work_dir.to_string_lossy().as_ref()) | ||
| .expect("allocate CString for work_dir"); |
There was a problem hiding this comment.
These expect strings imply an allocation error but CString::new returns an error if the value contains nulls. Since this is a function that returns a Result it is preferred to return an Err instead of panic here.
| ) | ||
| .into()); | ||
| } | ||
| nix::unistd::close(mount_fd as std::ffi::c_int)?; |
There was a problem hiding this comment.
I found this for documentation on fsopen: https://github.com/brauner/man-pages-md/blob/main/fsopen.md
Do you have any other documentation you could link to? Documentation doesn't appear to be readily available yet.
This doc doesn't mention closing the fd after move_mount. It makes me wonder, though, if we need to close any successfully opened fds before returning an error if any of the other steps fails. Since this code would run in the context of a short-lived process I imagine not? I also note the use of CLOEXEC.
I've read this far and tbh this is all unfamiliar territory to me so I can say it looks reasonable but my opinion isn't worth much.
| return Err(format!( | ||
| "mount_overlayfs_syscalls::SYS_fsopen(overlay, FSOPEN_CLOEXEC) error: {}", | ||
| fd | ||
| ) |
There was a problem hiding this comment.
| return Err(format!( | |
| "mount_overlayfs_syscalls::SYS_fsopen(overlay, FSOPEN_CLOEXEC) error: {}", | |
| fd | |
| ) | |
| return Err(format!("mount_overlayfs_syscalls::SYS_fsopen(overlay, FSOPEN_CLOEXEC) error: {fd}") |
We prefer to inline when possible but haven't turned on the clippy lint for that (yet?).
| // Setup the lowerdir directories in reverse order so that the overlayfs will apply them in | ||
| // the order we want. The first path specified is the top layer and the last is the bottom layer. | ||
| // For more details see: https://docs.kernel.org/filesystems/overlayfs.html#multiple-lower-layers | ||
| for layer in layer_dirs.iter().rev() { |
There was a problem hiding this comment.
Thoughts on integrating with the feature flag from #1183 to support the :-separated style too?
The link above mentions lowerdir+ support was added in kernel v6.8, but fsopen is supported since v5.2. Meaning we'd have access to fsopen in Rocky (kernel v5.14) but can't use this style of specifying lowerdirs.
Teach spfs to mount /spfs using direct overlayfs syscalls.
This is provided as an opt-in feature so that users can test this
alongside the current "mount"-based implementation. If no issues are
encountered then this can be made the default behavior in the future.
This approach avoids the command-line length limits that are present
when using the "mount" command to create the /spfs mount.
Related-to: #1164
Related-to: #968