Skip to content

add locking around calls to RecalculateWriteStallConditions in column_family_test#4474

Closed
miasantreble wants to merge 3 commits intofacebook:masterfrom
miasantreble:fix-data-race-stats-dump
Closed

add locking around calls to RecalculateWriteStallConditions in column_family_test#4474
miasantreble wants to merge 3 commits intofacebook:masterfrom
miasantreble:fix-data-race-stats-dump

Conversation

@miasantreble
Copy link
Copy Markdown
Contributor

@miasantreble miasantreble commented Oct 9, 2018

this should fix the current failing TSAN jobs:
The callstack for TSAN:

WARNING: ThreadSanitizer: data race (pid=87440)
Read of size 8 at 0x7d580000fce0 by thread T22 (mutexes: write M548703):
#0 rocksdb::InternalStats::DumpCFStatsNoFileHistogram(std::__cxx11::basic_string<char, std::char_traits, std::allocator >) db/internal_stats.cc:1204 (column_family_test+0x00000080eca7)
#1 rocksdb::InternalStats::DumpCFStats(std::__cxx11::basic_string<char, std::char_traits, std::allocator >) db/internal_stats.cc:1169 (column_family_test+0x0000008106d0)
#2 rocksdb::InternalStats::HandleCFStats(std::__cxx11::basic_string<char, std::char_traits, std::allocator >, rocksdb::Slice) db/internal_stats.cc:578 (column_family_test+0x000000810720)
#3 rocksdb::InternalStats::GetStringProperty(rocksdb::DBPropertyInfo const&, rocksdb::Slice const&, std::__cxx11::basic_string<char, std::char_traits, std::allocator >) db/internal_stats.cc:488 (column_family_test+0x00000080670c)
#4 rocksdb::DBImpl::DumpStats() db/db_impl.cc:625 (column_family_test+0x00000070ce9a)

Previous write of size 8 at 0x7d580000fce0 by main thread:
#0 rocksdb::InternalStats::AddCFStats(rocksdb::InternalStats::InternalCFStatsType, unsigned long) db/internal_stats.h:324 (column_family_test+0x000000693bbf)
#1 rocksdb::ColumnFamilyData::RecalculateWriteStallConditions(rocksdb::MutableCFOptions const&) db/column_family.cc:818 (column_family_test+0x000000693bbf)
#2 rocksdb::ColumnFamilyTest_WriteStallSingleColumnFamily_Test::TestBody() db/column_family_test.cc:2563 (column_family_test+0x0000005e5a49)

@miasantreble miasantreble changed the title add locking around calls to RecalculateWriteStallConditions in column_family_test add locking around calls to RecalculateWriteStallConditions in column_family_test Oct 9, 2018
riversand963
riversand963 approved these changes Oct 9, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@riversand963 riversand963 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM.
It looks to me that in unit tests, we always need to hold the mutex when updating the statistics after cac87fc. I'm thinking maybe we should add a new function to ColumnFamilyTest that looks similar to the following:

void RecalculateWriteStallConditions(cfd, mutable_cf_options)  {
  dbfull()->TEST_LockMutex();
  cfd->RecalculateWriteStallConditions(mutable_cf_options);
  dbfull()-> TEST_UnlockMutex();
}

What do you think?

@miasantreble
Copy link
Copy Markdown
Contributor Author

miasantreble commented Oct 9, 2018

@riversand963 sounds like a good plan

facebook-github-bot
facebook-github-bot reviewed Oct 9, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

miasantreble has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

riversand963
riversand963 approved these changes Oct 9, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@riversand963 riversand963 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM again with a minor comment.

db/column_family_test.cc

void RecalculateWriteStallConditions(ColumnFamilyData* cfd,
const MutableCFOptions& mutable_cf_options) {
dbfull()->TEST_LockMutex();
Copy link
Copy Markdown
Contributor

@riversand963 riversand963 Oct 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should have mentioned this in the previous comment. Maybe it's helpful to add a brief explanation about why we need the lock here? Just mention which thread is also accessing the stats data structure.

facebook-github-bot
facebook-github-bot reviewed Oct 9, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

miasantreble has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@miasantreble miasantreble deleted the fix-data-race-stats-dump branch October 9, 2018 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anand1976 anand1976 Awaiting requested review from anand1976

2 more reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@riversand963 riversand963 riversand963 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CLA Signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@miasantreble @facebook-github-bot @riversand963