dolysis/rocksdb
3
0
Fork 0
mirror of https://github.com/facebook/rocksdb.git synced 2024-11-29 09:36:17 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix a data race in DBImpl::RenameTempFileToOptionsFile (#12347)

Browse source 
Summary:
Pull Request resolved: https://github.com/facebook/rocksdb/pull/12347

`DBImpl::disable_delete_obsolete_files_` should only be accessed while holding the DB mutex to prevent data races. There's a piece of logic in `DBImpl::RenameTempFileToOptionsFile` where this synchronization was previously missing. The patch fixes this issue similarly to how it's handled in `DisableFileDeletions` and `EnableFileDeletions`, that is, by saving the counter value while holding the mutex and then performing the actual file deletion outside the critical section. Note: this PR only fixes the race itself; as a followup, we can also look into cleaning up and optimizing the file deletion logic (which is currently inefficient on multiple different levels).

Reviewed By: jowlyzhang

Differential Revision: D53675153

fbshipit-source-id: 5358e894ee6829d3edfadac50a93d97f8819e481
This commit is contained in:
Levi Tamasi 2024-02-12 13:26:09 -08:00 committed by Facebook GitHub Bot
parent 395d24f0fa
commit de1e3ff6ea
2 changed files with 15 additions and 7 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

9
db/db_impl/db_impl.cc
View file

@ -5551,16 +5551,23 @@ Status DBImpl::RenameTempFileToOptionsFile(const std::string& file_name) {
} }
} }
} }
if (s.ok()) { if (s.ok()) {
int my_disable_delete_obsolete_files;
{
InstrumentedMutexLock l(&mutex_); InstrumentedMutexLock l(&mutex_);
versions_->options_file_number_ = options_file_number; versions_->options_file_number_ = options_file_number;
versions_->options_file_size_ = options_file_size; versions_->options_file_size_ = options_file_size;
my_disable_delete_obsolete_files = disable_delete_obsolete_files_;
} }
if (0 == disable_delete_obsolete_files_) { if (!my_disable_delete_obsolete_files) {
// TODO: Should we check for errors here? // TODO: Should we check for errors here?
DeleteObsoleteOptionsFiles().PermitUncheckedError(); DeleteObsoleteOptionsFiles().PermitUncheckedError();
} }
}
return s; return s;
} }

1
unreleased_history/bug_fixes/fix_rename_temp_file_to_options_file_race.md Normal file
View file

@ -0,0 +1 @@
Fixed a data race in `DBImpl::RenameTempFileToOptionsFile`.