[no-release-notes] go/store/nbs: During a GC process, take dependencies on chunks that are read through the ChunkStore. #8760
Conversation
This file contains 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
…equests so that a GC does not end while they are in progress.
…pendencies on chunks during reads.
…chunks which were in the memtable but get filtered out because they are already in the store.
…h for chunks that are written but which are already present in the store.
…g, make it so that reading chunks as part of the GC process does not take further dependencies on them and never blocks on waitForGC.
max-hoffman
approved these changes
Jan 24, 2025
| defer gcs.newGen.mu.RUnlock() | ||
| return gcs.hasMany(toHasRecords(hashes)) | ||
| func (gcs *GenerationalNBS) HasMany(ctx context.Context, hashes hash.HashSet) (hash.HashSet, error) { | ||
| absent, err := gcs.newGen.HasMany(ctx, hashes) |
There was a problem hiding this comment.
is the diff just from collapsing gcs.getMany? or is there more to this?
Comment on lines
+234
to
+235
| err := nbs.waitForGC(ctx) | ||
| nbs.mu.Unlock() |
There was a problem hiding this comment.
i'm sure this is intentional, but is it better to block here than let other sessions try to do partial work before blocking on the same wait? ex: marked chunks cache hits under this miss
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.
Previously GC was constructed to walk the transitive closure of reachable chunks from when it started, and to take additional dependencies on any chunks that were written to the ChunkStore during the GC process. This change makes it so that we can additionally take dependencies on any chunks that are read from the ChunkStore during the GC process.
For the
nbslayer to signal a new dependency during a GC, it makes use of akeeperFunc, which is passed into theBeginGCcall. The contract is that, ifkeeperFuncis non-niland it returnsfalse, the chunk address which was given to the function will make its way back to a call toSaveHasheson someMarkAndSweeperwhose contents will end up in the final store once the GC is completed.keeperFunc, however, is allowed to returntrue, which signals to the block store that the in-progress GC has passed a certain point in time and can no longer make that guarantee. As a result, anytimekeeperFuncreturnstrue, the block store implementation needs to block until the GC process is over, and then it should resume the operation from the beginning.That machinery already exists for the
Put()andCommit()use case. This PR adds machinery to do all of that on various read paths that explicitly touch chunks, such asHas,HasMany,Get,GetManyandGetManyCompressed. In order to add dependency tracking on the read path, it's important for the GC process itself to not cause the chunks it has read to form additional dependencies. This PR makes a slight change tomarkAndSweeperso that it can fetch chunks from the source store without recording unnecessary duplicate dependencies and potentially becoming mired in deadlocks.