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

Fix memory leak for ColumnFamily drop with live iterator (#7749)

Browse Source 
Summary:
Uncommon bug seen by ASAN with
ColumnFamilyTest.LiveIteratorWithDroppedColumnFamily, if the last two
references to a ColumnFamilyData are both SuperVersions (during
InstallSuperVersion). The fix is to use UnrefAndTryDelete even in
SuperVersion::Cleanup but with a parameter to avoid re-entering Cleanup
on the same SuperVersion being cleaned up.

ColumnFamilyData::Unref is considered unsafe so removed.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/7749

Test Plan: ./column_family_test --gtest_filter=*LiveIter* --gtest_repeat=100

Reviewed By: jay-zhuang

Differential Revision: D25354304

Pulled By: pdillinger

fbshipit-source-id: e78f3a3f67c40013b8432f31d0da8bec55c5321c
This commit is contained in:
Peter Dillinger 2020-12-11 11:17:11 -08:00 committed by Facebook GitHub Bot
parent 07c0fc002a
commit b1ee191405
3 changed files with 10 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
db/column_family.cc
View File

@ -449,9 +449,7 @@ void SuperVersion::Cleanup() {
to_delete.push_back(m);
}
current->Unref();
if (cfd->Unref()) {
delete cfd;
}
cfd->UnrefAndTryDelete(this);
}
void SuperVersion::Init(ColumnFamilyData* new_cfd, MemTable* new_mem,
@ -660,7 +658,7 @@ ColumnFamilyData::~ColumnFamilyData() {
}
}
bool ColumnFamilyData::UnrefAndTryDelete() {
bool ColumnFamilyData::UnrefAndTryDelete(SuperVersion* sv_under_cleanup) {
int old_refs = refs_.fetch_sub(1);
assert(old_refs > 0);
@ -670,7 +668,11 @@ bool ColumnFamilyData::UnrefAndTryDelete() {
return true;
}
if (old_refs == 2 && super_version_ != nullptr) {
// If called under SuperVersion::Cleanup, we should not re-enter Cleanup on
// the same SuperVersion. (But while installing a new SuperVersion, this
// cfd could be referenced only by two SuperVersions.)
if (old_refs == 2 && super_version_ != nullptr &&
super_version_ != sv_under_cleanup) {
// Only the super_version_ holds me
SuperVersion* sv = super_version_;
super_version_ = nullptr;

14
db/column_family.h
View File

@ -279,21 +279,11 @@ class ColumnFamilyData {
// holding a DB mutex, or as the leader in a write batch group).
void Ref() { refs_.fetch_add(1); }
// Unref decreases the reference count, but does not handle deletion
// when the count goes to 0. If this method returns true then the
// caller should delete the instance immediately, or later, by calling
// FreeDeadColumnFamilies(). Unref() can only be called while holding
// a DB mutex, or during single-threaded recovery.
bool Unref() {
int old_refs = refs_.fetch_sub(1);
assert(old_refs > 0);
return old_refs == 1;
}
// UnrefAndTryDelete() decreases the reference count and do free if needed,
// return true if this is freed else false, UnrefAndTryDelete() can only
// be called while holding a DB mutex, or during single-threaded recovery.
bool UnrefAndTryDelete();
// sv_under_cleanup is only provided when called from SuperVersion::Cleanup.
bool UnrefAndTryDelete(SuperVersion* sv_under_cleanup = nullptr);
// SetDropped() can only be called under following conditions:
// 1) Holding a DB mutex,

5
db/version_set_test.cc
View File

@ -2292,10 +2292,7 @@ TEST_P(VersionSetTestDropOneCF, HandleDroppedColumnFamilyInAtomicGroup) {
mutex_.Unlock();
ASSERT_OK(s);
ASSERT_EQ(1, called);
if (cfd_to_drop->Unref()) {
delete cfd_to_drop;
cfd_to_drop = nullptr;
}
cfd_to_drop->UnrefAndTryDelete();
}
INSTANTIATE_TEST_CASE_P(