Files will now close if held open too long #239
Merged
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
allada
force-pushed
the
fs-changes
branch
2 times, most recently
from
54f7195
to
909d24d
Compare
allada
commented
Aug 26, 2023
There was a problem hiding this comment.
FYI, I'll be on vacation for the next week so no followups this next week, so no rush.
Reviewable status: 0 of 20 files reviewed, all discussions resolved (waiting on @chrisstaite-menlo)
In testing Buck2, it turns out they hammer the scheduler with jobs and assume it can keep up (which is good). On local testing, where all services are on the same machine it was deadlocking because it was performing the following operations: 1. Open file1 for reading 2. Open file2 for writing 3. Streaming file1 -> file2 Since we allow users to limit the number of open files at any given time, this was deadlocking because file1 was held open waiting for file2 to open, which was waiting for a file to be closed. Since buck2 goes crazy, it was causing a deadlock. In most production systems this is not an issue because the CAS is separated from the workers, but rarely might happen on the workers if the `max_open_files` was set too low. To get around this issue `ResumeableFileSlot` is introduced. It allows callers to use a timeout and call `.close_file()` on it and the next time the struct is used it will re-open the file. related #222 closes #238
chrisstaite-menlo
approved these changes
Aug 30, 2023
There was a problem hiding this comment.
Reviewed 20 of 20 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @allada)
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.
In testing Buck2, it turns out they hammer the scheduler with jobs and assume it can keep up (which is good). On local testing, where all services are on the same machine it was deadlocking because it was performing the following operations:
Since we allow users to limit the number of open files at any given time, this was deadlocking because file1 was held open waiting for file2 to open, which was waiting for a file to be closed. Since buck2 goes crazy, it was causing a deadlock.
In most production systems this is not an issue because the CAS is separated from the workers, but rarely might happen on the workers if the
max_open_fileswas set too low.To get around this issue
ResumeableFileSlotis introduced. It allows callers to use a timeout and call.close_file()on it and the next time the struct is used it will re-open the file.related #222
This change is