refactor(locking): Make disabling locking on NFS mounts explicit #16177

ranger-ross · 2025-10-30T06:32:55Z

What does this PR try to resolve?

~~This PR introduces a LockingStrategy struct and LockingMode enum to allow more control over the locking strategy used during compilation.~~
This work is done in preparation to enable fine grain locking.

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

See: Implement fine grain locking for the target directory #16155
cc: More granular locking in cargo_rustc #4282
For reference this behavior of disabling locking on NFS was originally added in cargo update hangs forever #2615

Notes:

The nfs check was not removed from try_acquire() as the registry cache still relies on that check.
In Implement fine grain locking for the target directory #16155 there was an oversight which did not consider if artifact-dir was on an nfs mount but not build-dir. This is now taken into consideration by LockingStrategy in this PR.

How to test and review this PR?

I created a local nfs mount over localhost and mounted it at /mnt/nfs_test, then set CARGO_BUILD_BUILD_DIR to a directory in that mount.

CARGO_BUILD_BUILD_DIR=/mnt/nfs_test/dummy-build CARGO_LOG=cargo::core::compiler::locking=debug lc build
   0.022885439s DEBUG cargo::core::compiler::locking: NFS detected. Disabling file system locking for build-dir
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.02s

after build I verified no .cargo-lock exists in the build-dir

tree /mnt/nfs_test/dummy-build
/mnt/nfs_test/dummy-build
└── debug
    ├── build
    ├── deps
    │   ├── foo-ad75d8ca32b29d47
    │   └── foo-ad75d8ca32b29d47.d
    ├── examples
    └── incremental
        └── foo-2vk25yx3lch9b
            ├── s-hciaevvrup-08c34gu-5appljltgguri514nuemj1ibu
            │   ├── 1gwd5tj9uf8jpg5ioumt8go3d.o
            │   ├── 6eaoidoi64c9ig6k0953bogii.o
            │   ├── 7e23o2bk07r4ts7z2xhkfxxam.o
            │   ├── 7gxp0h0prbuhv3t5u8dhcyjyk.o
            │   ├── 9rdgyadsm9zgjys7rfpm7d5at.o
            │   ├── c8w9viecm6mfutn314xg83y53.o
            │   ├── dep-graph.bin
            │   ├── query-cache.bin
            │   └── work-products.bin
            └── s-hciaevvrup-08c34gu.lock

Not sure if there is an easy way to test this in CI and I am unsure if building in network filesystem mounts are a common enough usecase to warrant adding that complexity to the test CI.

Other observation during testing

I noticed that is_on_nfs_mount() will return false if the directory does not exist but will be nested with in an NFS mount. After the directory is created by a previous cargo invocation it will begin returning true. I believe this is a bug, though its probably low priority given that this is a probably niche usecase. I did not attempt to fix this in this PR.

rustbot · 2025-10-30T06:33:00Z

r? @epage

rustbot has assigned @epage.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

src/cargo/core/compiler/layout.rs

src/cargo/core/compiler/locking.rs

epage · 2025-10-30T18:28:40Z

src/cargo/core/compiler/locking.rs

+        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()) {


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.

src/cargo/core/compiler/locking.rs

weihanglo · 2025-10-31T05:09:13Z

src/cargo/core/compiler/build_runner/mod.rs

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

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.

weihanglo

Looks good from my end

View changes since this review

Update cargo submodule 22 commits in 6368002885a04cbeae39a82cf5118f941559a40a..445fe4a68f469bf936b2fd81de2c503b233a7f4f 2025-10-31 14:31:52 +0000 to 2025-11-07 18:08:19 +0000 - fix(depinfo): prevent invalid trailing backslash on Windows (rust-lang/cargo#16223) - refactor: Remove lazycell (rust-lang/cargo#16224) - refactor: extract ConfigValue to its own module (rust-lang/cargo#16222) - fix(config): non-mergeable list from cli should take priority (rust-lang/cargo#16220) - fix(compile): build.warnings=deny shouldn't block hard warnings (rust-lang/cargo#16213) - fix: display absolute path in the `missing in PATH` warning (rust-lang/cargo#16125) - fix: non-mergeable list from config cli merge the same way (rust-lang/cargo#16219) - docs(contrib): Link out to rustc diagnostic style guide (rust-lang/cargo#16216) - fix: Remove build-plan (rust-lang/cargo#16212) - Add native completions for `--package` on various commands (rust-lang/cargo#16210) - fix(completions): don't wrap completion item help in parenthesis (rust-lang/cargo#16215) - refactor(locking): Make disabling locking on NFS mounts explicit (rust-lang/cargo#16177) - docs(unstable): Move compile-time-deps out of Stabilized section (rust-lang/cargo#16211) - docs(ref): Rename DEP_NAME_KEY to DEP_LINKS_KEY (rust-lang/cargo#16205) - feat(build-analysis): emit rebuild reason log entry (rust-lang/cargo#16203) - chore: Update dependencies (rust-lang/cargo#16200) - chore(deps): update cargo-semver-checks to v0.45.0 (rust-lang/cargo#16190) - chore(deps): update msrv (rust-lang/cargo#16178) - refactor: embed deserialize validation logic in ProgressConfig (rust-lang/cargo#16194) - refactor(gctx): extract config schema to a module (rust-lang/cargo#16195) - chore: bump to 0.94.0; update changelog (rust-lang/cargo#16191) - chore(deps): update rust crate gix to 0.74.0 (rust-lang/cargo#16186) r? ghost

rustbot assigned epage Oct 30, 2025

ranger-ross mentioned this pull request Oct 30, 2025

Implement fine grain locking for the target directory #16155

Open

5 tasks

epage reviewed Oct 30, 2025

View reviewed changes

src/cargo/core/compiler/layout.rs Outdated Show resolved Hide resolved

epage reviewed Oct 30, 2025

View reviewed changes

src/cargo/core/compiler/locking.rs Outdated Show resolved Hide resolved

epage reviewed Oct 30, 2025

View reviewed changes

src/cargo/core/compiler/locking.rs Outdated Show resolved Hide resolved

weihanglo reviewed Oct 31, 2025

View reviewed changes

weihanglo approved these changes Oct 31, 2025

View reviewed changes

refactor(locking): Make disabling locking on NFS mounts explict

231505c

ranger-ross force-pushed the refactor-locking-mode branch from 4a6d2b9 to 231505c Compare November 6, 2025 09:18

ranger-ross changed the title ~~refactor(locking): Created locking strategy to control locking mode~~ refactor(locking): Make disabling locking on NFS mounts explicit Nov 6, 2025

epage approved these changes Nov 6, 2025

View reviewed changes

epage added this pull request to the merge queue Nov 6, 2025

Merged via the queue into rust-lang:master with commit 1e4db68 Nov 6, 2025
25 checks passed

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 6, 2025

weihanglo mentioned this pull request Nov 7, 2025

Update cargo submodule rust-lang/rust#148674

Merged

rustbot added this to the 1.93.0 milestone Nov 8, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(locking): Make disabling locking on NFS mounts explicit #16177

refactor(locking): Make disabling locking on NFS mounts explicit #16177

Uh oh!

ranger-ross commented Oct 30, 2025 •

edited

Loading

Uh oh!

rustbot commented Oct 30, 2025

Uh oh!

Uh oh!

Uh oh!

epage Oct 30, 2025

Uh oh!

Uh oh!

weihanglo Oct 31, 2025

Uh oh!

ranger-ross Oct 31, 2025

Uh oh!

weihanglo left a comment •

edited by rustbot

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

refactor(locking): Make disabling locking on NFS mounts explicit #16177

refactor(locking): Make disabling locking on NFS mounts explicit #16177

Uh oh!

Conversation

ranger-ross commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR try to resolve?

How to test and review this PR?

Other observation during testing

Uh oh!

rustbot commented Oct 30, 2025

Uh oh!

Uh oh!

Uh oh!

epage Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

weihanglo Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

ranger-ross Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

weihanglo left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ranger-ross commented Oct 30, 2025 •

edited

Loading

weihanglo left a comment •

edited by rustbot

Loading