backup: Reduce the number of GetBlobs. - #503
Open
mathieu-plak wants to merge 1 commit into
Open
Conversation
* When getting an object out of the cache it's not needed to recheck that it exists in the repo. This used to be true because the vfs cache was local and so it could be lying (repo changed due to another user). Now that we base the vfs cache on a previous snapshot we know _by definition_ that the object exists and so BlobExists will always be true. * BlobExists is a "slow" function and is the hottest path of them all, reducing its usage is always a win. * On a non changing backup this divides by 4 the number of calls we emit to the state cache. * This is a scary diff, I've been sitting on it for a while, so this needs to get some extra considerations from reviewers.
There was a problem hiding this comment.
Pull request overview
This PR reduces repository BlobExists lookups on the backup hot path by trusting the VFS cache (seeded from a previous snapshot) and reusing cached object/entry MACs without re-validating their presence in the repository.
Changes:
- Removed
BlobExists(RT_OBJECT, ...)checks when reusing cached content objects. - Removed
BlobExists(RT_VFS_ENTRY, ...)checks when reusing cached VFS directory/file entry MACs. - Keeps the rest of the backup pipeline (scanlog -> indexes -> persist) unchanged, relying on cached blobs already being present.
Comments suppressed due to low confidence (2)
snapshot/backup.go:861
- writeDirectoryEntry now reuses cachedPath.MAC without verifying that the referenced RT_VFS_ENTRY blob exists in the destination repository. This removes the only safety net when the VFS cache is accidentally sourced from a different repo (or otherwise not guaranteed to be present), which can persist a VFS btree pointing at missing entry blobs.
if cachedPath != nil {
dirEntryMAC = cachedPath.MAC
serialized, err := dirEntry.ToBytes()
if err != nil {
return err
snapshot/backup.go:902
- writeFileEntry now trusts cachedPath.MAC (RT_VFS_ENTRY) on cache hits without confirming it exists in the destination repository. If the VFS cache comes from another repository (possible because WithVFSCache does not validate) this will record file entries whose VFS_ENTRY blobs are missing, yielding a corrupted snapshot.
if cachedPath != nil {
fileEntryMAC = cachedPath.MAC
if fileEntry.Object == (objects.MAC{}) && cachedPath.ObjectMAC != (objects.MAC{}) {
fileEntry.Object = cachedPath.ObjectMAC
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
826
to
+830
| if cachedPath != nil && cachedPath.ObjectMAC != (objects.MAC{}) { | ||
| if snap.repository.BlobExists(resources.RT_OBJECT, cachedPath.ObjectMAC) { | ||
| return &contentMeta{ | ||
| ObjectMAC: cachedPath.ObjectMAC, | ||
| Size: cachedPath.FileInfo.Size(), | ||
| Chunks: cachedPath.Chunks, | ||
| Entropy: cachedPath.Entropy, | ||
| ContentType: cachedPath.ContentType, | ||
| }, nil | ||
| } | ||
| return &contentMeta{ | ||
| ObjectMAC: cachedPath.ObjectMAC, | ||
| Size: cachedPath.FileInfo.Size(), | ||
| Chunks: cachedPath.Chunks, |
Contributor
Author
There was a problem hiding this comment.
Yeah that would require a mac colision -_-. Though as kloset is a library maybe we want to add a small assertion in WithVFSCache that the repos UUID match. But to be 100% clear this can't happen without a hash collision.
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
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.
When getting an object out of the cache it's not needed to recheck that it exists in the repo. This used to be true because the vfs cache was local and so it could be lying (repo changed due to another user). Now that we base the vfs cache on a previous snapshot we know by definition that the object exists and so BlobExists will always be true.
BlobExists is a "slow" function and is the hottest path of them all, reducing its usage is always a win.
On a non changing backup this divides by 4 the number of calls we emit to the state cache.
This is a scary diff, I've been sitting on it for a while, so this needs to get some extra considerations from reviewers.