Skip to content
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
12 changes: 10 additions & 2 deletions src/cargo/core/compiler/build_runner/mod.rs
Copy link
Member

Choose a reason for hiding this comment

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

The motivation is to move logic to determine if we should disable locking outside of try_acquire()

While I kinda can guess why this is useful, would you mind saying it a bit clearer how this is going to be used and evolve to help the fine grained locking implementation?

(Or any comments/code I should read first in the other PR?)

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason is to eventually add a LockingMode enum that can be controlled by the cargo operation.
The vision I have is the have 3 variants Disabled, Coarse, Fine which can be determined by the operation & environment.

For example, cargo clean would use either Disabled or Coarse as it does not have access to the dep graph.
Another motivation is to fallback to Coarse grain locking if we detect that Fine grain locking may fail. (e.g. too many file descriptors)

See determine_locking_mode() in the original PR for proposed heuristics.

Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use std::sync::{Arc, Mutex};

use crate::core::PackageId;
use crate::core::compiler::compilation::{self, UnitOutput};
use crate::core::compiler::locking::LockingStrategy;
use crate::core::compiler::{self, Unit, artifact};
use crate::util::cache_lock::CacheLockMode;
use crate::util::errors::CargoResult;
Expand Down Expand Up @@ -89,6 +90,10 @@ pub struct BuildRunner<'a, 'gctx> {
/// because the target has a type error. This is in an Arc<Mutex<..>>
/// because it is continuously updated as the job progresses.
pub failed_scrape_units: Arc<Mutex<HashSet<UnitHash>>>,

/// The locking mode to use for this build.
/// By default we use coarse grain locking, but disable locking on some filesystems like NFS
pub locking_strategy: LockingStrategy,
}

impl<'a, 'gctx> BuildRunner<'a, 'gctx> {
Expand All @@ -111,6 +116,8 @@ impl<'a, 'gctx> BuildRunner<'a, 'gctx> {
}
};

let locking_strategy = LockingStrategy::determine_locking_strategy(&bcx.ws)?;

Ok(Self {
bcx,
compilation: Compilation::new(bcx)?,
Expand All @@ -128,6 +135,7 @@ impl<'a, 'gctx> BuildRunner<'a, 'gctx> {
lto: HashMap::new(),
metadata_for_doc_units: HashMap::new(),
failed_scrape_units: Arc::new(Mutex::new(HashSet::new())),
locking_strategy,
})
}

Expand Down Expand Up @@ -360,11 +368,11 @@ impl<'a, 'gctx> BuildRunner<'a, 'gctx> {
#[tracing::instrument(skip_all)]
pub fn prepare_units(&mut self) -> CargoResult<()> {
let dest = self.bcx.profiles.get_dir_name();
let host_layout = Layout::new(self.bcx.ws, None, &dest)?;
let host_layout = Layout::new(self.bcx.ws, None, &dest, &self.locking_strategy)?;
let mut targets = HashMap::new();
for kind in self.bcx.all_kinds.iter() {
if let CompileKind::Target(target) = *kind {
let layout = Layout::new(self.bcx.ws, Some(target), &dest)?;
let layout = Layout::new(self.bcx.ws, Some(target), &dest, &self.locking_strategy)?;
targets.insert(target, layout);
}
}
Expand Down
29 changes: 21 additions & 8 deletions src/cargo/core/compiler/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@

use crate::core::Workspace;
use crate::core::compiler::CompileTarget;
use crate::core::compiler::locking::{LockingMode, LockingStrategy};
use crate::util::{CargoResult, FileLock};
use cargo_util::paths;
use std::path::{Path, PathBuf};
Expand All @@ -126,6 +127,7 @@ impl Layout {
ws: &Workspace<'_>,
target: Option<CompileTarget>,
dest: &str,
locking_strategy: &LockingStrategy,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming the decision is made outside of Layout for future changes. However, I can't easily evaluate that and so I would prefer we instead keep the decision inside of Layout

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the reason for this is I plan add build_runner argument to LockingStrategy::determine_locking_strategy() as its needed to determine if we want to enable fine grain locking. (number of build units)

I could move this into Layout but I am not in love with the prospect of adding build_runner as an arg to Layout::new()

Copy link
Member

Choose a reason for hiding this comment

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

That sounds like we should have a number_of_units arg for Layout::new?

Copy link
Member Author

@ranger-ross ranger-ross Oct 31, 2025

Choose a reason for hiding this comment

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

That sounds like we should have a number_of_units arg for Layout::new?

We could, though it would be an Option since cargo clean does not have a build_runner.
So it does not know the number of units.
See https://github.com/rust-lang/cargo/pull/16177/files#diff-5a67831086004bef79bcf2ccf2d5d804dc0fafe90e4fc116fe4cdc59c78f5049R120

My thought was to have a struct with some semantic meaning be passed into Layout::new() instead of adding primitive types that grow over time.

Copy link
Member

Choose a reason for hiding this comment

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

My thought was to have a struct with some semantic meaning be passed into Layout::new() instead of adding primitive types that grow over time.

This sounds like a good plan!

) -> CargoResult<Layout> {
let is_new_layout = ws.gctx().cli_unstable().build_dir_new_layout;
let mut root = ws.target_dir();
Expand All @@ -152,18 +154,29 @@ impl Layout {
// For now we don't do any more finer-grained locking on the artifact
// directory, so just lock the entire thing for the duration of this
// compile.
let artifact_dir_lock =
dest.open_rw_exclusive_create(".cargo-lock", ws.gctx(), "build directory")?;

let build_dir_lock = if root != build_root {
Some(build_dest.open_rw_exclusive_create(
let artifact_dir_lock = match locking_strategy.artifact_dir() {
LockingMode::Disabled => None,
LockingMode::Coarse => Some(dest.open_rw_exclusive_create(
".cargo-lock",
ws.gctx(),
"build directory",
)?)
"artifact directory",
)?),
};

assert_eq!(locking_strategy.is_unified_output_dir(), root == build_root);
let build_dir_lock = if !locking_strategy.is_unified_output_dir() {
match locking_strategy.build_dir() {
LockingMode::Disabled => None,
LockingMode::Coarse => Some(build_dest.open_rw_exclusive_create(
".cargo-lock",
ws.gctx(),
"build directory",
)?),
}
} else {
None
};

let root = root.into_path_unlocked();
let build_root = build_root.into_path_unlocked();
let dest = dest.into_path_unlocked();
Expand Down Expand Up @@ -222,7 +235,7 @@ pub struct ArtifactDirLayout {
timings: PathBuf,
/// The lockfile for a build (`.cargo-lock`). Will be unlocked when this
/// struct is `drop`ped.
_lock: FileLock,
_lock: Option<FileLock>,
}

impl ArtifactDirLayout {
Expand Down
66 changes: 66 additions & 0 deletions src/cargo/core/compiler/locking.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
use tracing::debug;

use crate::{CargoResult, core::Workspace, util::flock::is_on_nfs_mount};

/// The strategy to use for locking during a build.
///
/// A strategy is made up of multiple locking modes for different directories.
#[derive(Debug)]
pub struct LockingStrategy {
/// The locking mode for the artifact-dir
artifact_dir: LockingMode,
/// The locking mode for the build-dir
///
/// Will be `None` when artifact_dir and build_dir are the same directory.
build_dir: Option<LockingMode>,
}

impl LockingStrategy {
/// Determines the locking strategy the current environment can support.
Comment on lines +8 to +19
Copy link
Contributor

Choose a reason for hiding this comment

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

While I know what change this is prep for, could we simplify it down for what is needed for this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

In what way would you like to see this simplified?
I believe this logic is the minimum to keep the behavior unchanged.

pub fn determine_locking_strategy(ws: &Workspace<'_>) -> CargoResult<Self> {
let artifact_dir_locking_mode = match is_on_nfs_mount(ws.target_dir().as_path_unlocked()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we matching on bool?

Copy link
Member Author

Choose a reason for hiding this comment

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

For me find matching on bools easier to read when its a simple transformation

let foo = match my_bool {
   true => MyEnum::Variant1,
   false => MyEnum::Variant2,
};

// vs

let foo = if my_bool {
   MyEnum::Variant1
} else {
   MyEnum::Variant2
};

Though I later added the debug! statements which makes it less clean.
Happy to change to if statements if you'd prefer that

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, in OCaml world matching bools is quite common.

true => {
debug!("NFS detected. Disabling file system locking for artifact-dir");
LockingMode::Disabled
}
false => LockingMode::Coarse,
};
let build_dir_locking_mode = if ws.target_dir() == ws.build_dir() {
None
} else {
Some(match is_on_nfs_mount(ws.build_dir().as_path_unlocked()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: To me, I prefer returning wrapped values like this to be limited to values and not expressions. It just always reads in a funny way to me and adds extra nesting on wrapping to complex expressions that could instead use them being simplified.

true => {
debug!("NFS detected. Disabling file system locking for build-dir");
LockingMode::Disabled
}
false => LockingMode::Coarse,
})
};
Ok(Self {
artifact_dir: artifact_dir_locking_mode,
build_dir: build_dir_locking_mode,
})
}

pub fn artifact_dir(&self) -> &LockingMode {
&self.artifact_dir
}

pub fn build_dir(&self) -> &LockingMode {
self.build_dir.as_ref().unwrap_or(&self.artifact_dir)
}

/// If the artifact_dir and build_dir are the same directory.
pub fn is_unified_output_dir(&self) -> bool {
self.build_dir.is_none()
}
}

/// The locking mode that will be used for output directories.
#[derive(Debug)]
pub enum LockingMode {
/// Completely disables locking (used for filesystems that do not support locking)
Disabled,
/// Coarse grain locking (Profile level)
Coarse,
}
1 change: 1 addition & 0 deletions src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ pub mod future_incompat;
pub(crate) mod job_queue;
pub(crate) mod layout;
mod links;
pub mod locking;
mod lto;
mod output_depinfo;
mod output_sbom;
Expand Down
14 changes: 9 additions & 5 deletions src/cargo/ops/cargo_clean.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use crate::core::compiler::locking::LockingStrategy;
use crate::core::compiler::{CompileKind, CompileMode, Layout, RustcTargetData};
use crate::core::profiles::Profiles;
use crate::core::{PackageIdSpec, PackageIdSpecQuery, TargetKind, Workspace};
Expand Down Expand Up @@ -116,15 +117,18 @@ fn clean_specs(
let target_data = RustcTargetData::new(ws, &requested_kinds)?;
let (pkg_set, resolve) = ops::resolve_ws(ws, dry_run)?;
let prof_dir_name = profiles.get_dir_name();
let host_layout = Layout::new(ws, None, &prof_dir_name)?;
let locking_strategy = LockingStrategy::determine_locking_strategy(&ws)?;
let host_layout = Layout::new(ws, None, &prof_dir_name, &locking_strategy)?;
// Convert requested kinds to a Vec of layouts.
let target_layouts: Vec<(CompileKind, Layout)> = requested_kinds
.into_iter()
.filter_map(|kind| match kind {
CompileKind::Target(target) => match Layout::new(ws, Some(target), &prof_dir_name) {
Ok(layout) => Some(Ok((kind, layout))),
Err(e) => Some(Err(e)),
},
CompileKind::Target(target) => {
match Layout::new(ws, Some(target), &prof_dir_name, &locking_strategy) {
Ok(layout) => Some(Ok((kind, layout))),
Err(e) => Some(Err(e)),
}
}
CompileKind::Host => None,
})
.collect::<CargoResult<_>>()?;
Expand Down
4 changes: 2 additions & 2 deletions src/cargo/util/flock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,7 @@ fn acquire(
}

#[cfg(all(target_os = "linux", not(target_env = "musl")))]
fn is_on_nfs_mount(path: &Path) -> bool {
pub fn is_on_nfs_mount(path: &Path) -> bool {
use std::ffi::CString;
use std::mem;
use std::os::unix::prelude::*;
Expand All @@ -445,7 +445,7 @@ fn is_on_nfs_mount(path: &Path) -> bool {
}

#[cfg(any(not(target_os = "linux"), target_env = "musl"))]
fn is_on_nfs_mount(_path: &Path) -> bool {
pub fn is_on_nfs_mount(_path: &Path) -> bool {
false
}

Expand Down
2 changes: 1 addition & 1 deletion src/cargo/util/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ mod dependency_queue;
pub mod diagnostic_server;
pub mod edit_distance;
pub mod errors;
mod flock;
pub mod flock;
pub mod frontmatter;
pub mod graph;
mod hasher;
Expand Down