Serialize rename operations on same object in FilesystemStore [1.6-patch-3] - #2644
Open
cormacrelf wants to merge 4 commits into
Open
Serialize rename operations on same object in FilesystemStore [1.6-patch-3]#2644cormacrelf wants to merge 4 commits into
cormacrelf wants to merge 4 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Fix "File was likely evicted" warning
A map entry whose content file is gone must not poison its key: before the self-heal, every re-upload of that key failed inside check_duplicate_files with NotFound when it opened the missing file, so the key could never be repaired. upload_self_heals_map_disk_divergence deletes a content file out from under a live map entry, uploads the same key again, and asserts the upload drops the stale entry, re-emplaces the file, and succeeds.
If the existing entry's content file is already gone (NotFound), drop the stale map entry and let this upload re-emplace, instead of failing every future write of the key with NotFound. This is the same self-heal get_part already does on a missing file. Divergence can happen when a stale unref() retires a content path it no longer owns (now prevented by the inode guard) or when something outside nativelink deletes the file.
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
1 participant
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.
Description
Background
FilesystemStorekeeps each cache entry in one file on disk. The name of this file comes from the key of the entry. This location is the "content path" of the key. An upload first writes its data to a temp file in the temp directory.emplace_file()then publishes the upload: it renames the temp file to its content path.An LRU map (the "evicting map") tracks all entries. When the map evicts an entry, it calls
unref()on that entry.unref()retires the file: it renames the content path back into the temp directory, under a new unique name. This rename keeps the file readable for tasks that still hold a reference to the entry. When the last reference drops, the store deletes the temp file.Problem and fix
This change corrects a race in
FilesystemStorebetweenemplace_file()andunref(). The race deleted live cache files and caused "Could not make hardlink ... file was likely evicted" errors.The evicting map calls
unref()outside its state lock. As a result, a delayedunref()can run after the store re-inserted and re-emplaced the same key. The content path then has the same name but a different inode. The old code renamed the content path away in all cases. This rename deleted the live file of the newer entry. The map still reported the entry as present.Changes:
EncodedFilePathnow records the inode of the file that it owns at its content path. Before the rename,unref()reads the inode of the content path. If this inode is not the recorded inode,unref()does not rename the file. The file then stays with its newer owner.rename_locks) serializes the two renames that can touch the content path of one key. These renames are the temp-to-content move inemplace_file()and the content-to-temp move inunref(). The hash of the key selects the mutex. This lock closes the window between the inode read inunref()and a concurrentemplace_file().check_duplicate_filesnow repairs a stale map entry whose content file does not exist (NotFound). It removes the stale entry, and the upload emplaces the file again. Before this change, every future write of that key failed.Review questions
Type of change
How Has This Been Tested?
Two new regression tests are in
nativelink-store/tests/filesystem_store_test.rs:stale_unref_does_not_steal_reemplaced_content_file— This test simulates a re-emplace that lands before the deferredunref()of a stale entry. It makes sure that the new content file survives the staleunref(). It also makes sure that the store logs the inode mismatch.upload_self_heals_map_disk_divergence— This test deletes a content file while its map entry stays in the map. Then the test uploads the same key again. It makes sure that this upload removes the stale entry, emplaces the file again, and succeeds.Checklist
bazel test //...passes locallygit amendsee some docsThis change is