andreacavalli/rocksdb
0
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Prevent segfault because SizeUnderCompaction was called without any locks.

Browse Source 
Summary:
SizeBeingCompacted was called without any lock protection. This causes
crashes, especially when running db_bench with value_size=128K.
The fix is to compute SizeUnderCompaction while holding the mutex and
passing in these values into the call to Finalize.

(gdb) where
#4  leveldb::VersionSet::SizeBeingCompacted (this=this@entry=0x7f0b490931c0, level=level@entry=4) at db/version_set.cc:1827
#5  0x000000000043a3c8 in leveldb::VersionSet::Finalize (this=this@entry=0x7f0b490931c0, v=v@entry=0x7f0b3b86b480) at db/version_set.cc:1420
#6  0x00000000004418d1 in leveldb::VersionSet::LogAndApply (this=0x7f0b490931c0, edit=0x7f0b3dc8c200, mu=0x7f0b490835b0, new_descriptor_log=<optimized out>) at db/version_set.cc:1016
#7  0x00000000004222b2 in leveldb::DBImpl::InstallCompactionResults (this=this@entry=0x7f0b49083400, compact=compact@entry=0x7f0b2b8330f0) at db/db_impl.cc:1473
#8  0x0000000000426027 in leveldb::DBImpl::DoCompactionWork (this=this@entry=0x7f0b49083400, compact=compact@entry=0x7f0b2b8330f0) at db/db_impl.cc:1757
#9  0x0000000000426690 in leveldb::DBImpl::BackgroundCompaction (this=this@entry=0x7f0b49083400, madeProgress=madeProgress@entry=0x7f0b41bf2d1e, deletion_state=...) at db/db_impl.cc:1268
#10 0x0000000000428f42 in leveldb::DBImpl::BackgroundCall (this=0x7f0b49083400) at db/db_impl.cc:1170
#11 0x000000000045348e in BGThread (this=0x7f0b49023100) at util/env_posix.cc:941
#12 leveldb::(anonymous namespace)::PosixEnv::BGThreadWrapper (arg=0x7f0b49023100) at util/env_posix.cc:874
#13 0x00007f0b4a7cf10d in start_thread (arg=0x7f0b41bf3700) at pthread_create.c:301
#14 0x00007f0b49b4b11d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:115

Test Plan:
make check

I am running db_bench with a value size of 128K to see if the segfault is fixed.

Reviewers: MarkCallaghan, sheki, emayanke

Reviewed By: sheki

CC: leveldb

Differential Revision: https://reviews.facebook.net/D9279
This commit is contained in:
Dhruba Borthakur 2013-03-11 09:47:48 -07:00
parent c04c956b66
commit ebf16f57c9
2 changed files with 39 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

54
db/version_set.cc
View File

@ -1009,11 +1009,15 @@ Status VersionSet::LogAndApply(VersionEdit* edit, port::Mutex* mu,
// Unlock during expensive MANIFEST log write. New writes cannot get here // Unlock during expensive MANIFEST log write. New writes cannot get here
// because &w is ensuring that all new writes get queued. // because &w is ensuring that all new writes get queued.
{ {
// calculate the amount of data being compacted at every level
std::vector<uint64_t> size_being_compacted(NumberLevels()-1);
SizeBeingCompacted(size_being_compacted);
mu->Unlock(); mu->Unlock();
// The calles to Finalize and UpdateFilesBySize are cpu-heavy // The calls to Finalize and UpdateFilesBySize are cpu-heavy
// and is best called outside the mutex. // and is best called outside the mutex.
Finalize(v); Finalize(v, size_being_compacted);
UpdateFilesBySize(v); UpdateFilesBySize(v);
// Write new record to MANIFEST log // Write new record to MANIFEST log
@ -1226,8 +1230,12 @@ Status VersionSet::Recover() {
if (s.ok()) { if (s.ok()) {
Version* v = new Version(this, current_version_number_++); Version* v = new Version(this, current_version_number_++);
builder.SaveTo(v); builder.SaveTo(v);
// Install recovered version // Install recovered version
Finalize(v); std::vector<uint64_t> size_being_compacted(NumberLevels()-1);
SizeBeingCompacted(size_being_compacted);
Finalize(v, size_being_compacted);
v->offset_manifest_file_ = manifest_file_size; v->offset_manifest_file_ = manifest_file_size;
AppendVersion(v); AppendVersion(v);
manifest_file_number_ = next_file; manifest_file_number_ = next_file;
@ -1350,8 +1358,12 @@ Status VersionSet::DumpManifest(Options& options, std::string& dscname,
if (s.ok()) { if (s.ok()) {
Version* v = new Version(this, 0); Version* v = new Version(this, 0);
builder.SaveTo(v); builder.SaveTo(v);
// Install recovered version // Install recovered version
Finalize(v); std::vector<uint64_t> size_being_compacted(NumberLevels()-1);
SizeBeingCompacted(size_being_compacted);
Finalize(v, size_being_compacted);
AppendVersion(v); AppendVersion(v);
manifest_file_number_ = next_file; manifest_file_number_ = next_file;
next_file_number_ = next_file + 1; next_file_number_ = next_file + 1;
@ -1374,7 +1386,8 @@ void VersionSet::MarkFileNumberUsed(uint64_t number) {
} }
} }
void VersionSet::Finalize(Version* v) { void VersionSet::Finalize(Version* v,
std::vector<uint64_t>& size_being_compacted) {
double max_score = 0; double max_score = 0;
int max_score_level = 0; int max_score_level = 0;
@ -1417,7 +1430,7 @@ void VersionSet::Finalize(Version* v) {
} else { } else {
// Compute the ratio of current size to size limit. // Compute the ratio of current size to size limit.
const uint64_t level_bytes = TotalFileSize(v->files_[level]) - const uint64_t level_bytes = TotalFileSize(v->files_[level]) -
SizeBeingCompacted(level); size_being_compacted[level];
score = static_cast<double>(level_bytes) / MaxBytesForLevel(level); score = static_cast<double>(level_bytes) / MaxBytesForLevel(level);
if (score > 1) { if (score > 1) {
// Log(options_->info_log, "XXX score l%d = %d ", level, (int)score); // Log(options_->info_log, "XXX score l%d = %d ", level, (int)score);
@ -1822,19 +1835,22 @@ void VersionSet::ReleaseCompactionFiles(Compaction* c, Status status) {
} }
// The total size of files that are currently being compacted // The total size of files that are currently being compacted
uint64_t VersionSet::SizeBeingCompacted(int level) { // at at every level upto the penultimate level.
uint64_t total = 0; void VersionSet::SizeBeingCompacted(std::vector<uint64_t>& sizes) {
for (std::set<Compaction*>::iterator it = for (int level = 0; level < NumberLevels()-1; level++) {
compactions_in_progress_[level].begin(); uint64_t total = 0;
it != compactions_in_progress_[level].end(); for (std::set<Compaction*>::iterator it =
++it) { compactions_in_progress_[level].begin();
Compaction* c = (*it); it != compactions_in_progress_[level].end();
assert(c->level() == level); ++it) {
for (int i = 0; i < c->num_input_files(0); i++) { Compaction* c = (*it);
total += c->input(0,i)->file_size; assert(c->level() == level);
for (int i = 0; i < c->num_input_files(0); i++) {
total += c->input(0,i)->file_size;
}
} }
sizes[level] = total;
} }
return total;
} }
Compaction* VersionSet::PickCompactionBySize(int level, double score) { Compaction* VersionSet::PickCompactionBySize(int level, double score) {
@ -1916,7 +1932,9 @@ Compaction* VersionSet::PickCompaction() {
// compute the compactions needed. It is better to do it here // compute the compactions needed. It is better to do it here
// and also in LogAndApply(), otherwise the values could be stale. // and also in LogAndApply(), otherwise the values could be stale.
Finalize(current_); std::vector<uint64_t> size_being_compacted(NumberLevels()-1);
current_->vset_->SizeBeingCompacted(size_being_compacted);
Finalize(current_, size_being_compacted);
// We prefer compactions triggered by too much data in a level over // We prefer compactions triggered by too much data in a level over
// the compactions triggered by seeks. // the compactions triggered by seeks.

6
db/version_set.h
View File

@ -387,7 +387,7 @@ class VersionSet {
void Init(int num_levels); void Init(int num_levels);
void Finalize(Version* v); void Finalize(Version* v, std::vector<uint64_t>&);
void GetRange(const std::vector<FileMetaData*>& inputs, void GetRange(const std::vector<FileMetaData*>& inputs,
InternalKey* smallest, InternalKey* smallest,
@ -459,8 +459,8 @@ class VersionSet {
void operator=(const VersionSet&); void operator=(const VersionSet&);
// Return the total amount of data that is undergoing // Return the total amount of data that is undergoing
// compactions at this level // compactions per level
uint64_t SizeBeingCompacted(int level); void SizeBeingCompacted(std::vector<uint64_t>&);
// Returns true if any one of the parent files are being compacted // Returns true if any one of the parent files are being compacted
bool ParentRangeInCompaction(const InternalKey* smallest, bool ParentRangeInCompaction(const InternalKey* smallest,