Attempt to use reflinks by default on Linux#17753
Attempt to use reflinks by default on Linux#17753zanieb wants to merge 4 commits intoastral-sh:rf-hl-fbfrom
Conversation
9b89193 to
e899e09
Compare
No special reason, it just didn't feel worth it given how prevalent ext4 is. |
|
Apparently |
|
The reflink-copy crate might be worth looking at; it's cross-OS and has a normal-copy fallback. |
Consolidates our file copying and linking logic in `uv-fs` instead of having a duplicate implementation in `uv-install-wheel`. This required introducing some new abstractions in `uv-fs` to support the features wheel installation needs. I think this is a blocker to improving our reflink handling in #17753 and also generally desirable since the copy fallback logic is complicated.
e899e09 to
535336f
Compare
535336f to
058fd3c
Compare
On Linux, default to `LinkMode::Clone` (reflink) instead of `LinkMode::Hardlink`. The clone fallback chain is now: Clone -> Hardlink -> Copy, so on filesystems that don't support reflinks (e.g., ext4), we try one reflink, fail, then fall back to hardlinks — which is what we were doing before. The overhead of the single failed FICLONE ioctl is negligible (~80ms in benchmarks). On filesystems that support reflinks (btrfs, XFS, bcachefs), users get copy-on-write benefits automatically without needing to opt in. https://claude.ai/code/session_015S1a39fWo8vqMHhjUe8uoG
058fd3c to
d72f1ca
Compare
c6772dc to
081827e
Compare
Replace UV_INTERNAL__TEST_EXPECT_REFLINK and UV_INTERNAL__TEST_DIR usage in link tests with two new env vars: - UV_INTERNAL__TEST_REFLINK_FS: path on a reflink-capable filesystem - UV_INTERNAL__TEST_TMP_FS: path on a non-reflink filesystem This enables comprehensive test coverage: - Same-device reflink (both dirs on reflink fs) - Same-device fallback (both dirs on non-reflink fs) - Cross-device clone fallback (reflink fs -> tmpfs and vice versa) - Cross-device hardlink fallback (must fall back to copy) - Cross-device symlink (should work across devices) - Cross-device copy (sanity check) The Linux CI job now creates both a btrfs loopback and a tmpfs mount. The macOS CI job sets TEST_REFLINK_FS to the runner temp (APFS).
081827e to
3da3164
Compare
crates/uv-test/src/lib.rs
Outdated
| /// Returns `None` if `UV_INTERNAL__TEST_REFLINK_FS` is not set. The virtual environment | ||
| /// is **not** created; use `context.venv().assert().success()` after this. |
There was a problem hiding this comment.
Maybe something more like..
Why do we say "and venv" above?
| /// Returns `None` if `UV_INTERNAL__TEST_REFLINK_FS` is not set. The virtual environment | |
| /// is **not** created; use `context.venv().assert().success()` after this. | |
| /// Returns `None` if `UV_INTERNAL__TEST_REFLINK_FS` is not set. | |
| /// | |
| /// Note a virtual environment is not created automatically. |
crates/uv-test/src/lib.rs
Outdated
| /// Move the working directory (and venv) to a non-reflink filesystem. | ||
| /// | ||
| /// Returns `None` if `UV_INTERNAL__TEST_TMP_FS` is not set. The virtual environment | ||
| /// is **not** created; use `context.venv().assert().success()` after this. |
There was a problem hiding this comment.
See similar comments about reflink fs
crates/uv-test/src/lib.rs
Outdated
| Some(self.with_cache_on_fs(&dir)) | ||
| } | ||
|
|
||
| /// Move the cache directory to a non-reflink filesystem. |
There was a problem hiding this comment.
I think we should say
Use a cache directory on a the file system specified by [
...].See the environment variable for details about the expectations.
crates/uv-static/src/env_vars.rs
Outdated
| /// Path to a directory on a filesystem that supports reflink / copy-on-write | ||
| /// (e.g., btrfs, XFS with reflink, APFS). Used by link tests to verify that | ||
| /// reflink operations succeed. |
There was a problem hiding this comment.
| /// Path to a directory on a filesystem that supports reflink / copy-on-write | |
| /// (e.g., btrfs, XFS with reflink, APFS). Used by link tests to verify that | |
| /// reflink operations succeed. | |
| /// Path to a directory on a filesystem that supports copy-on-write, e.g., btrfs or APFS. | |
| /// | |
| /// When populated, uv will run additional tests that require this functionality. |
crates/uv-static/src/env_vars.rs
Outdated
| /// Path to a directory on a filesystem that does **not** support reflink | ||
| /// (e.g., tmpfs, ext4, HFS+). Used by link tests to verify fallback behavior | ||
| /// when reflink is unavailable, and for cross-device tests when combined with | ||
| /// `UV_INTERNAL__TEST_REFLINK_FS`. |
There was a problem hiding this comment.
| /// Path to a directory on a filesystem that does **not** support reflink | |
| /// (e.g., tmpfs, ext4, HFS+). Used by link tests to verify fallback behavior | |
| /// when reflink is unavailable, and for cross-device tests when combined with | |
| /// `UV_INTERNAL__TEST_REFLINK_FS`. | |
| /// Path to a directory on an alternative filesystem for testing. | |
| /// | |
| /// This filesystem must not support copy-on-write and must be a different filesystem | |
| /// then the default for the test suite. | |
| /// | |
| /// When populated, uv will run additional tests that cover cross-filesystem linking. |
There was a problem hiding this comment.
This actually makes me think we should have UV_INTERNAL__TEST_NOCOW_FS and UV_INTERNAL__TEST_ALT_FS instead of overloading the requirements for this variable. We can just set both in CI?
crates/uv-static/src/env_vars.rs
Outdated
| /// reflink operations succeed. | ||
| #[attr_hidden] | ||
| #[attr_added_in("next release")] | ||
| pub const UV_INTERNAL__TEST_REFLINK_FS: &'static str = "UV_INTERNAL__TEST_REFLINK_FS"; |
There was a problem hiding this comment.
Let's call it COW_FS instead of REFLINK_FS
| let canonical = tmp.path().canonicalize().expect("Failed to canonicalize"); | ||
| self.temp_dir = ChildPath::new(tmp.path()).child("temp"); | ||
| fs_err::create_dir_all(&self.temp_dir).expect("Failed to create working directory"); | ||
| self.venv = ChildPath::new(canonical.join(".venv")); |
There was a problem hiding this comment.
Do we need to do this? Does it have any value?
There was a problem hiding this comment.
I guess it's helpful for the tests? In which case it does make sense to document and set?
523f9d3 to
458c5de
Compare
458c5de to
44dd020
Compare
In preparation for #17753 though this technically could affect macOS too if you have a HFS+ drive which supports hardlinks but not reflinks. I manually tested this on a Linux with cross-filesystem links and varied levels of hardlink and reflink support. #17753 will include infrastructure for testing in CI.
Copy of #17753 which GitHub auto-closed. This adds test infrastructure for cross-device links and file systems with reflink support. In short, we create a few extra file systems on the CI runners then provide their paths to the test suite using environment variables to ensure we have coverage. If the variables are not set, the tests are skipped. --------- Co-authored-by: Claude <noreply@anthropic.com>
This was briefly discussed internally https://discord.com/channels/1039017663004942429/1343693513073623205/1451276005380718714
I'm not sure why we don't do this. I agree with the sentiment at #17722 that non-ext4 file systems are prevalent and we should at least try one reflink before falling back to hardlinks.