Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

env: detect the lowerdir overlay mount options at runtime #1192

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions crates/spfs/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,6 @@ default = []
# If enabled, will create the "local" repository in a subdirectory
# of the standard storage root, named "ci/pipeline_${CI_PIPELINE_ID}".
gitlab-ci-local-repo-isolation = []
# Use "mount" commands that are compatible with older centos7-era kernels that
# do not support the "lowerdir+=" overlayfs options. "legacy-mount-options"
# can run into path length limits when mounting many layers.
# https://github.com/spkenv/spk/issues/968
legacy-mount-options = []
sentry = ["dep:sentry"]
server = ["hyper/server", "tokio-util/codec", "tokio-util/io-util"]
"protobuf-src" = ["dep:protobuf-src"]
Expand Down
28 changes: 21 additions & 7 deletions crates/spfs/src/env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1036,13 +1036,16 @@ const OVERLAY_ARGS_INDEX: &str = "index";
const OVERLAY_ARGS_INDEX_ON: &str = "index=on";
const OVERLAY_ARGS_METACOPY: &str = "metacopy";
const OVERLAY_ARGS_METACOPY_ON: &str = "metacopy=on";
pub(crate) const OVERLAY_ARGS_LOWERDIR_APPEND: &str = "lowerdir+";

/// A struct for holding the options that will be included
/// in the overlayfs mount command when mounting an environment.
#[derive(Default)]
pub(crate) struct OverlayMountOptions {
/// Specifies that the overlay file system is mounted as read-only
pub read_only: bool,
/// The lowerdir+ mount option will be used to append layers when true.
pub lowerdir_append: bool,
/// When true, inodes are indexed in the mount so that
/// files which share the same inode (hardlinks) are broken
/// in the final mount and changes to one file don't affect
Expand Down Expand Up @@ -1073,11 +1076,22 @@ impl OverlayMountOptions {
fn new(rt: &runtime::Runtime) -> Self {
Self {
read_only: !rt.status.editable,
lowerdir_append: true,
break_hardlinks: true,
metadata_copy_up: true,
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slightly related -- one observation is that break_hardlinks is always true yet to_options() has behavior that checks whether it's false (which can never happen).

In an earlier iteration I was looking at whether we can toggle all of these bools, not just lowerdir_append, inside query() so that to_options() can be simplified. The rationale was to avoid having to query the params hashset in both query() and to_options() so that we can do it just once inside query().

This is the check inside of to_options() that I'm referring to:

        if !self.break_hardlinks && params.contains(OVERLAY_ARGS_INDEX) {
            opts.push(OVERLAY_ARGS_INDEX_ON);
        }

This seems like dead code because break_hardlinks is never false. I figured that maybe this was left this way for possible future changes where some other component might be toggling break_hardlinks, but I just wanted to point this out in case this is something that was an oversight that needs some cleanup to remove the break_hardlinks bool.

In any case, let me know if we think that adjusting more of these bools inside of query() with the goal of simplifying to_options() and removing the call to let params = runtime::overlayfs::overlayfs_available_options(); is worth doing.

This PR is currently a minimal but slightly less efficient approach. If we did want to toggle these other bools inside of query(), and/or cleanup the break_hardlinks bools, then that could be a separate follow-up PR.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC the break_hardlinks was something that I had working and either it didn't work the way that I thought or there was some adjustment that we made to the logic which has orphaned it as you note. Definitely worth looking into as a separate effort

}

/// Update state variables to match the features supported by the current overlay version.
fn query(mut self) -> Self {
let params = runtime::overlayfs::overlayfs_available_options();
if self.lowerdir_append && !params.contains(OVERLAY_ARGS_LOWERDIR_APPEND) {
self.lowerdir_append = false;
}

self
}

/// Return the options that should be included in the mount request.
pub fn to_options(&self) -> Vec<&'static str> {
let params = runtime::overlayfs::overlayfs_available_options();
Expand Down Expand Up @@ -1114,7 +1128,7 @@ pub(crate) fn get_overlay_args<P: AsRef<Path>>(
// Allocate a large buffer up front to avoid resizing/copying.
let mut args = String::with_capacity(4096);

let mount_options = OverlayMountOptions::new(rt);
let mount_options = OverlayMountOptions::new(rt).query();
for option in mount_options.to_options() {
args.push_str(option);
args.push(',');
Expand All @@ -1126,19 +1140,19 @@ pub(crate) fn get_overlay_args<P: AsRef<Path>>(
// the rightmost on the command line is the bottom layer, and the
// leftmost is on the top). For more details see:
// https://docs.kernel.org/filesystems/overlayfs.html#multiple-lower-layers
if cfg!(feature = "legacy-mount-options") {
args.push_str("lowerdir=");
if mount_options.lowerdir_append {
for path in layer_dirs.iter().rev() {
args.push_str("lowerdir+=");
args.push_str(&path.as_ref().to_string_lossy());
args.push(':');
args.push(',');
}
args.push_str("lowerdir+=");
} else {
args.push_str("lowerdir=");
for path in layer_dirs.iter().rev() {
args.push_str("lowerdir+=");
args.push_str(&path.as_ref().to_string_lossy());
args.push(',');
args.push(':');
}
args.push_str("lowerdir+=");
}
args.push_str(&rt.config.lower_dir.to_string_lossy());

Expand Down
27 changes: 27 additions & 0 deletions crates/spfs/src/runtime/overlayfs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use std::collections::HashSet;
use std::io::{BufRead, BufReader};
use std::os::unix::fs::MetadataExt;

use crate::env::OVERLAY_ARGS_LOWERDIR_APPEND;
use crate::{Error, Result};

#[cfg(test)]
Expand Down Expand Up @@ -57,10 +58,36 @@ fn query_overlayfs_available_options() -> Result<HashSet<String>> {
#[cfg(target_os = "linux")]
fn parse_modinfo_params<R: BufRead>(reader: &mut R) -> Result<HashSet<String>> {
let mut params = HashSet::new();
let mut vermagic_seen = false;
for line in reader.lines() {
let line = line.map_err(|err| {
Error::String(format!("Failed to read kernel module information: {err}"))
})?;

// The output from "modinfo overlay" looks like this:
// ...
// vermagic: 6.12.9-amd64 SMP preempt mod_unload modversions
// ...
// parm: metacopy:Default to on or off for the metadata only copy up feature (bool)
//
// The "vermagic:" line appears before the "parm:" lines.

if !vermagic_seen {
let version_string = match line.strip_prefix("vermagic:") {
Some(remainder) => remainder.trim(),
None => continue,
};
vermagic_seen = true;
let mut parts = version_string.splitn(3, '.'); // ("6", "12", "9-...")
let major_version = parts.next().unwrap_or("0").parse().unwrap_or(0);
let minor_version = parts.next().unwrap_or("0").parse().unwrap_or(0);
// The "lowerdir+" option was added in Linux v6.8.
// https://docs.kernel.org/6.8/filesystems/overlayfs.html#multiple-lower-layers
if major_version >= 7 || (major_version == 6 && minor_version >= 8) {
params.insert(OVERLAY_ARGS_LOWERDIR_APPEND.to_string());
}
continue;
}
let param = match line.strip_prefix("parm:") {
Some(remainder) => remainder.trim(),
None => continue,
Expand Down
Loading