Conversation
Deduplication metrics counted every matched range as deduped and added it to total_bytes before the defrag-prevention gate. When the gate rejects the range, its chunks fall through to the new-data path, which does its own total/new accounting, so every defrag-prevented byte was double-counted in total_bytes and misreported as deduped. Any debug build cleaning a file that trips defrag prevention panics on the file_size == total_bytes debug_assert in SingleFileCleaner::finish. Move the deduped/total increments after the gate so each chunk is counted exactly once, either as deduped or as new. The regression test reproduces the scenario organically: content uploaded as small shuffled pieces (fragmented CAS layout), then cleanly re-uploaded from a client with no local shard cache. Dedup only matches in piece-sized runs, the CPR tracker starts rejecting ranges, and the old accounting inflated total_bytes by exactly defrag_prevented_dedup_bytes.
This was referenced Jul 3, 2026
XciD
added a commit
to huggingface/hf-mount
that referenced
this pull request
Jul 6, 2026
## Problem `link()` returns ENOTSUP, so hardlink-first import pipelines fall back to a full copy through the mount. Copying a file whose bytes already live in CAS re-chunks and re-uploads everything, and dedup against the existing xorbs runs into xet-core's defrag prevention, which re-stores a large share of the bytes in short fragments. Measured on production buckets (multi-GB media files imported this way): ~2x stored bytes, +90-160% xorb overhead, reconstructions of 15-17k segments alternating between two xorb sets, CPR pinned at the defrag-prevention hysteresis equilibrium (~8 chunks/segment). Related: the metrics side of that investigation is huggingface/xet-core#886. ## Change Implement `link()` as a **server-side copy**: one batch `AddFile` pointing at the source's committed xet hash. No bytes move through CAS, the import is instant, and the original clean layout is preserved. The alias gets its own inode, so this intentionally diverges from POSIX same-inode semantics: writes to one path never affect the other, and `st_ino` differs between the two paths. Callers that hardlink for instant-copy semantics (the *arr import case) get exactly what they need. The previous ENOTSUP rationale (in-memory links never persisted to the Hub) no longer applies since the alias is a real Hub entry. Guards, mirroring the rename/create paths: - dirty source or no committed hash: ENOTSUP (callers keep their copy fallback, and we never alias a stale hash) - overlay mode: ENOTSUP (must not mutate the remote) - directory source: EPERM, missing parent: ENOENT, non-directory parent: ENOTDIR, existing target: EEXIST (checked against remote children too, and re-checked under the write lock) - read-only mount: EROFS, OS junk names: EACCES Structured like `rename()`: validate under read lock, commit the batch op, insert the alias under write lock (with negative-cache removal and queued-delete cancellation for the destination path). NFS backend unchanged: nfsserve 0.11 does not dispatch `NFSPROC3_LINK` (proc_unavail), so hardlinks over NFS fail at the protocol level before reaching us. Supporting it would need an upstream nfsserve change. ## Tests Four new tests in `virtual_fs/tests.rs`: happy path (exactly one AddFile with the source hash, no delete, alias resolvable with its own inode, source untouched), dirty source (ENOTSUP, no remote op), existing target (EEXIST, no remote op), directory source (EPERM). Full lib suite: 340 passed.
Collaborator
|
Close in favor of #931 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
In
FileDeduper::process_chunks, a matched dedup range is counted intodeduped_chunks,deduped_bytes,total_chunksandtotal_bytesbefore the defrag-prevention gate (allow_dedup_on_next_range). When the gate rejects the range, its chunks fall through to the new-data path below, which does its owntotal_*/new_*accounting. As a result:total_bytes(measured:total_bytes = file_size + defrag_prevented_dedup_bytes, exactly),deduped_byteseven though they were re-stored,file_size == total_bytesdebug_assertinSingleFileCleaner::finish.File reconstruction info is unaffected (the registered file size and segments are correct); this is a metrics/telemetry bug plus a debug-build crash.
Fix
Move the four increments after the defrag gate so each chunk is counted exactly once, either as deduped (gate passed) or as new (gate rejected, new-data path).
defrag_prevented_*counters keep their existing semantics.Regression test
test_dedup_metrics_when_defrag_prevention_triggersreproduces the scenario organically rather than by mocking the tracker: upload content as small shuffled pieces (fragmented CAS layout, as produced by out-of-order writers), then cleanly re-upload the assembled file from a client with an empty shard cache, so dedup discovery goes through the sampled global dedup queries and only matches in piece-sized runs. The CPR tracker then rejects ranges (defrag_prevented_dedup_bytes > 0is asserted so the test cannot pass vacuously). Before the fix the test fails on theSingleFileCleanerdebug_assert withtotal_bytesinflated by exactlydefrag_prevented_dedup_bytes; after the fixdeduped + new == total == file_sizeholds.Side note from the same investigation, out of scope here: on such fragmented references, defrag prevention re-stores a significant share of already-stored content (~17% in the test above, ~50% observed on production bucket files, i.e. ~2x stored bytes with heavily fragmented layouts). That looks like a design discussion worth having separately.