Skip to content

Commit

Permalink
Prevent deadlock in db_stress with DbStressCompactionFilter (facebook…
Browse files Browse the repository at this point in the history
…#8956)

Summary:
The cyclic dependency was:

- `StressTest::OperateDb()` locks the mutex for key 'k'
- `StressTest::OperateDb()` calls a function like `PauseBackgroundWork()`, which waits for pending compaction to complete.
- The pending compaction reaches key `k` and `DbStressCompactionFilter::FilterV2()` calls `Lock()` on that key's mutex, which hangs forever.

The cycle can be broken by using a new function, `port::Mutex::TryLock()`, which returns immediately upon failure to acquire a lock. In that case `DbStressCompactionFilter::FilterV2()` can just decide to keep the key.

Pull Request resolved: facebook#8956

Reviewed By: riversand963

Differential Revision: D31183718

Pulled By: ajkr

fbshipit-source-id: 329e4a31ce43085af174cf367ef560b5a04399c5
(cherry picked from commit 791bff5)

Conflicts:
	port/port_posix.cc -- `errnoStr` is not used in the version
			      we're cherry-picking on. Let's stay
			      with `strerror`.
  • Loading branch information
ajkr authored and rzarzynski committed May 31, 2024
1 parent c540de6 commit 1418e86
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 3 deletions.
13 changes: 11 additions & 2 deletions db_stress_tool/db_stress_compaction_filter.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,17 @@ class DbStressCompactionFilter : public CompactionFilter {
bool ok = GetIntVal(key.ToString(), &key_num);
assert(ok);
(void)ok;
MutexLock key_lock(state_->GetMutexForKey(cf_id_, key_num));
if (!state_->Exists(cf_id_, key_num)) {
port::Mutex* key_mutex = state_->GetMutexForKey(cf_id_, key_num);
if (!key_mutex->TryLock()) {
return Decision::kKeep;
}
// Reaching here means we acquired the lock.

bool key_exists = state_->Exists(cf_id_, key_num);

key_mutex->Unlock();

if (!key_exists) {
return Decision::kRemove;
}
return Decision::kKeep;
Expand Down
12 changes: 11 additions & 1 deletion port/port_posix.cc
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ extern const bool kDefaultToAdaptiveMutex = false;
namespace port {

static int PthreadCall(const char* label, int result) {
if (result != 0 && result != ETIMEDOUT) {
if (result != 0 && result != ETIMEDOUT && result != EBUSY) {
fprintf(stderr, "pthread %s: %s\n", label, strerror(result));
abort();
}
Expand Down Expand Up @@ -87,6 +87,16 @@ void Mutex::Unlock() {
PthreadCall("unlock", pthread_mutex_unlock(&mu_));
}

bool Mutex::TryLock() {
bool ret = PthreadCall("trylock", pthread_mutex_trylock(&mu_)) == 0;
#ifndef NDEBUG
if (ret) {
locked_ = true;
}
#endif
return ret;
}

void Mutex::AssertHeld() {
#ifndef NDEBUG
assert(locked_);
Expand Down
3 changes: 3 additions & 0 deletions port/port_posix.h
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,9 @@ class Mutex {

void Lock();
void Unlock();

bool TryLock();

// this will assert if the mutex is not locked
// it does NOT verify that mutex is held by a calling thread
void AssertHeld();
Expand Down
10 changes: 10 additions & 0 deletions port/win/port_win.h
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,16 @@ class Mutex {
mutex_.unlock();
}

bool TryLock() {
bool ret = mutex_.try_lock();
#ifndef NDEBUG
if (ret) {
locked_ = true;
}
#endif
return ret;
}

// this will assert if the mutex is not locked
// it does NOT verify that mutex is held by a calling thread
void AssertHeld() {
Expand Down

0 comments on commit 1418e86

Please sign in to comment.