Skip to content

Commit

Permalink
Prevent segfault because SizeUnderCompaction was called without any l…
Browse files Browse the repository at this point in the history
…ocks.

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
facebook#4  leveldb::VersionSet::SizeBeingCompacted (this=this@entry=0x7f0b490931c0, level=level@entry=4) at db/version_set.cc:1827
facebook#5  0x000000000043a3c8 in leveldb::VersionSet::Finalize (this=this@entry=0x7f0b490931c0, v=v@entry=0x7f0b3b86b480) at db/version_set.cc:1420
facebook#6  0x00000000004418d1 in leveldb::VersionSet::LogAndApply (this=0x7f0b490931c0, edit=0x7f0b3dc8c200, mu=0x7f0b490835b0, new_descriptor_log=<optimized out>) at db/version_set.cc:1016
facebook#7  0x00000000004222b2 in leveldb::DBImpl::InstallCompactionResults (this=this@entry=0x7f0b49083400, compact=compact@entry=0x7f0b2b8330f0) at db/db_impl.cc:1473
facebook#8  0x0000000000426027 in leveldb::DBImpl::DoCompactionWork (this=this@entry=0x7f0b49083400, compact=compact@entry=0x7f0b2b8330f0) at db/db_impl.cc:1757
facebook#9  0x0000000000426690 in leveldb::DBImpl::BackgroundCompaction (this=this@entry=0x7f0b49083400, madeProgress=madeProgress@entry=0x7f0b41bf2d1e, deletion_state=...) at db/db_impl.cc:1268
facebook#10 0x0000000000428f42 in leveldb::DBImpl::BackgroundCall (this=0x7f0b49083400) at db/db_impl.cc:1170
facebook#11 0x000000000045348e in BGThread (this=0x7f0b49023100) at util/env_posix.cc:941
facebook#12 leveldb::(anonymous namespace)::PosixEnv::BGThreadWrapper (arg=0x7f0b49023100) at util/env_posix.cc:874
facebook#13 0x00007f0b4a7cf10d in start_thread (arg=0x7f0b41bf3700) at pthread_create.c:301
facebook#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
  • Loading branch information
dhruba committed Mar 11, 2013
1 parent c04c956 commit ebf16f5
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 21 deletions.
54 changes: 36 additions & 18 deletions db/version_set.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1009,11 +1009,15 @@ Status VersionSet::LogAndApply(VersionEdit* edit, port::Mutex* mu,
// Unlock during expensive MANIFEST log write. New writes cannot get here
// 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();

// 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.
Finalize(v);
Finalize(v, size_being_compacted);
UpdateFilesBySize(v);

// Write new record to MANIFEST log
Expand Down Expand Up @@ -1226,8 +1230,12 @@ Status VersionSet::Recover() {
if (s.ok()) {
Version* v = new Version(this, current_version_number_++);
builder.SaveTo(v);

// 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;
AppendVersion(v);
manifest_file_number_ = next_file;
Expand Down Expand Up @@ -1350,8 +1358,12 @@ Status VersionSet::DumpManifest(Options& options, std::string& dscname,
if (s.ok()) {
Version* v = new Version(this, 0);
builder.SaveTo(v);

// 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);
manifest_file_number_ = next_file;
next_file_number_ = next_file + 1;
Expand All @@ -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;
int max_score_level = 0;
Expand Down Expand Up @@ -1417,7 +1430,7 @@ void VersionSet::Finalize(Version* v) {
} else {
// Compute the ratio of current size to size limit.
const uint64_t level_bytes = TotalFileSize(v->files_[level]) -
SizeBeingCompacted(level);
size_being_compacted[level];
score = static_cast<double>(level_bytes) / MaxBytesForLevel(level);
if (score > 1) {
// Log(options_->info_log, "XXX score l%d = %d ", level, (int)score);
Expand Down Expand Up @@ -1822,19 +1835,22 @@ void VersionSet::ReleaseCompactionFiles(Compaction* c, Status status) {
}

// The total size of files that are currently being compacted
uint64_t VersionSet::SizeBeingCompacted(int level) {
uint64_t total = 0;
for (std::set<Compaction*>::iterator it =
compactions_in_progress_[level].begin();
it != compactions_in_progress_[level].end();
++it) {
Compaction* c = (*it);
assert(c->level() == level);
for (int i = 0; i < c->num_input_files(0); i++) {
total += c->input(0,i)->file_size;
// at at every level upto the penultimate level.
void VersionSet::SizeBeingCompacted(std::vector<uint64_t>& sizes) {
for (int level = 0; level < NumberLevels()-1; level++) {
uint64_t total = 0;
for (std::set<Compaction*>::iterator it =
compactions_in_progress_[level].begin();
it != compactions_in_progress_[level].end();
++it) {
Compaction* c = (*it);
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) {
Expand Down Expand Up @@ -1916,7 +1932,9 @@ Compaction* VersionSet::PickCompaction() {

// compute the compactions needed. It is better to do it here
// 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
// the compactions triggered by seeks.
Expand Down
6 changes: 3 additions & 3 deletions db/version_set.h
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,7 @@ class VersionSet {

void Init(int num_levels);

void Finalize(Version* v);
void Finalize(Version* v, std::vector<uint64_t>&);

void GetRange(const std::vector<FileMetaData*>& inputs,
InternalKey* smallest,
Expand Down Expand Up @@ -459,8 +459,8 @@ class VersionSet {
void operator=(const VersionSet&);

// Return the total amount of data that is undergoing
// compactions at this level
uint64_t SizeBeingCompacted(int level);
// compactions per level
void SizeBeingCompacted(std::vector<uint64_t>&);

// Returns true if any one of the parent files are being compacted
bool ParentRangeInCompaction(const InternalKey* smallest,
Expand Down

0 comments on commit ebf16f5

Please sign in to comment.