refactor(locking): Created locking strategy to control locking mode

[ranger-ross]

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:

1. The nfs check was not removed from try_acquire() as the registry cache still relies on that check.
2. 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]

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

[epage]

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

[ranger-ross]

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

[weihanglo]

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

[ranger-ross]

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.

[epage]

Why are we matching on bool?

[ranger-ross]

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

[epage]

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.

[epage]

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

[ranger-ross]

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

[weihanglo]

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

[ranger-ross]

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.