Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix data race warning of BackupableDBTest.TableFileWithDbChecksumCorruptedDuringBackup #7177

Closed
wants to merge 1 commit into from
Closed

Fix data race warning of BackupableDBTest.TableFileWithDbChecksumCorruptedDuringBackup #7177

wants to merge 1 commit into from

Conversation

gg814
Copy link
Contributor

@gg814 gg814 commented Jul 27, 2020 •
edited
Loading

Summary:
Fix the data race warning by removing an unnecessary variable that causes the warning.

Test plan:
COMPILE_WITH_TSAN=1 make backupable_db_test
./backupable_db_test --gtest_filter=*TableFileWithDbChecksumCorruptedDuringBackup*

facebook-github-bot
facebook-github-bot reviewed Jul 27, 2020
View reviewed changes
Copy link
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.

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

@gg814
Copy link
Contributor Author

gg814 commented Jul 27, 2020

Although there is no data race warning on my vm, circleci still reports there is for BackupableDBTestWithParam.TableFileWithDbChecksumCorruptedDuringBackup. Will also break it down and see what happens...

riversand963
riversand963 reviewed Jul 27, 2020
View reviewed changes
utilities/backupable/backupable_db_test.cc Outdated
kNoShare);

FillDB(db_.get(), 0, keys_iteration);
bool corrupted = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Will multiple threads race for corrupted?

WARNING: ThreadSanitizer: data race (pid=446635)
  Write of size 1 at 0x7fff6df250d7 by thread T24:
    #0 std::_Function_handler<void (void*), rocksdb::(anonymous namespace)::BackupableDBTest_TableFileWithDbChecksumCorruptedDuringBackup_Test::TestBody()::{lambda(void*)#1}>::_M_invoke(std::_Any_data const&, void*&&) <null> (backupable_db_test+0x000000494b92)
    #1 std::function<void (void*)>::operator()(void*) const /mnt/gvfs/third-party2/libgcc/6ace84e956873d53638c738b6f65f3f469cca74c/7.x/platform007/5620abc/include/c++/7.3.0/bits/std_function.h:706 (backupable_db_test+0x00000091e1af)
    #2 rocksdb::SyncPoint::Data::Process(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, void*) test_util/sync_point_impl.cc:121 (backupable_db_test+0x00000091e1af)
    #3 rocksdb::SyncPoint::Process(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, void*) test_util/sync_point.cc:66 (backupable_db_test+0x00000091b304)
    #4 rocksdb::BackupEngineImpl::CopyOrCreateFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, rocksdb::Env*, rocksdb::Env*, rocksdb::EnvOptions const&, bool, rocksdb::RateLimiter*, unsigned long*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, unsigned long, std::function<void ()>) utilities/backupable/backupable_db.cc:1517 (backupable_db_test+0x00000095037c)
    #5 operator() utilities/backupable/backupable_db.cc:858 (backupable_db_test+0x0000009542b8)
    #6 __invoke_impl<void, rocksdb::BackupEngineImpl::Initialize()::<lambda()> > /mnt/gvfs/third-party2/libgcc/6ace84e956873d53638c738b6f65f3f469cca74c/7.x/platform007/5620abc/include/c++/7.3.0/bits/invoke.h:60 (backupable_db_test+0x0000009551ad)
    #7 __invoke<rocksdb::BackupEngineImpl::Initialize()::<lambda()> > /mnt/gvfs/third-party2/libgcc/6ace84e956873d53638c738b6f65f3f469cca74c/7.x/platform007/5620abc/include/c++/7.3.0/bits/invoke.h:95 (backupable_db_test+0x0000009551ad)
    #8 _M_invoke<0> /mnt/gvfs/third-party2/libgcc/6ace84e956873d53638c738b6f65f3f469cca74c/7.x/platform007/5620abc/include/c++/7.3.0/thread:234 (backupable_db_test+0x0000009551ad)
    #9 operator() /mnt/gvfs/third-party2/libgcc/6ace84e956873d53638c738b6f65f3f469cca74c/7.x/platform007/5620abc/include/c++/7.3.0/thread:243 (backupable_db_test+0x0000009551ad)
    #10 _M_run /mnt/gvfs/third-party2/libgcc/6ace84e956873d53638c738b6f65f3f469cca74c/7.x/platform007/5620abc/include/c++/7.3.0/thread:186 (backupable_db_test+0x0000009551ad)
    #11 execute_native_thread_routine <null> (libstdc++.so.6+0x0000000c86df)
  Previous write of size 1 at 0x7fff6df250d7 by thread T33:
    #0 std::_Function_handler<void (void*), rocksdb::(anonymous namespace)::BackupableDBTest_TableFileWithDbChecksumCorruptedDuringBackup_Test::TestBody()::{lambda(void*)#1}>::_M_invoke(std::_Any_data const&, void*&&) <null> (backupable_db_test+0x000000494b92)
    #1 std::function<void (void*)>::operator()(void*) const /mnt/gvfs/third-party2/libgcc/6ace84e956873d53638c738b6f65f3f469cca74c/7.x/platform007/5620abc/include/c++/7.3.0/bits/std_function.h:706 (backupable_db_test+0x00000091e1af)
    #2 rocksdb::SyncPoint::Data::Process(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, void*) test_util/sync_point_impl.cc:121 (backupable_db_test+0x00000091e1af)
    #3 rocksdb::SyncPoint::Process(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, void*) test_util/sync_point.cc:66 (backupable_db_test+0x00000091b304)
    #4 rocksdb::BackupEngineImpl::CopyOrCreateFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, rocksdb::Env*, rocksdb::Env*, rocksdb::EnvOptions const&, bool, rocksdb::RateLimiter*, unsigned long*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, unsigned long, std::function<void ()>) utilities/backupable/backupable_db.cc:1517 (backupable_db_test+0x00000095037c)
    #5 operator() utilities/backupable/backupable_db.cc:858 (backupable_db_test+0x0000009542b8)
    #6 __invoke_impl<void, rocksdb::BackupEngineImpl::Initialize()::<lambda()> > /mnt/gvfs/third-party2/libgcc/6ace84e956873d53638c738b6f65f3f469cca74c/7.x/platform007/5620abc/include/c++/7.3.0/bits/invoke.h:60 (backupable_db_test+0x0000009551ad)
    #7 __invoke<rocksdb::BackupEngineImpl::Initialize()::<lambda()> > /mnt/gvfs/third-party2/libgcc/6ace84e956873d53638c738b6f65f3f469cca74c/7.x/platform007/5620abc/include/c++/7.3.0/bits/invoke.h:95 (backupable_db_test+0x0000009551ad)
    #8 _M_invoke<0> /mnt/gvfs/third-party2/libgcc/6ace84e956873d53638c738b6f65f3f469cca74c/7.x/platform007/5620abc/include/c++/7.3.0/thread:234 (backupable_db_test+0x0000009551ad)
    #9 operator() /mnt/gvfs/third-party2/libgcc/6ace84e956873d53638c738b6f65f3f469cca74c/7.x/platform007/5620abc/include/c++/7.3.0/thread:243 (backupable_db_test+0x0000009551ad)
    #10 _M_run /mnt/gvfs/third-party2/libgcc/6ace84e956873d53638c738b6f65f3f469cca74c/7.x/platform007/5620abc/include/c++/7.3.0/thread:186 (backupable_db_test+0x0000009551ad)
    #11 execute_native_thread_routine <null> (libstdc++.so.6+0x0000000c86df)

@gg814 gg814 force-pushed the fix_backup_corruption_during_copy_test_data_race branch from 41e25c5 to a7eca13 Compare July 27, 2020 21:27
@facebook-github-bot
Copy link
Contributor

@gg814 has updated the pull request. You must reimport the pull request before landing.

@gg814 gg814 force-pushed the fix_backup_corruption_during_copy_test_data_race branch from a7eca13 to 5e4c066 Compare July 27, 2020 22:11
@gg814
Fix data race warning of BackupableDBTest.TableFileWithDbChecksumCorr…
6701fcc 
…uptedDuringBackup

Summary:
Fix the data race warning by removing an unnecessary variable that causes the warning.

Test plan:
`COMPILE_WITH_TSAN=1 make backupable_db_test`
`./backupable_db_test --gtest_filter=*TableFileWithDbChecksumCorruptedDuringBackup*`
@gg814 gg814 force-pushed the fix_backup_corruption_during_copy_test_data_race branch from 5e4c066 to 6701fcc Compare July 27, 2020 22:13
riversand963
riversand963 reviewed Jul 27, 2020
View reviewed changes
utilities/backupable/backupable_db_test.cc
@@ -1303,7 +1303,7 @@ TEST_F(BackupableDBTest, TableFileWithDbChecksumCorruptedDuringBackup) {
OpenDBAndBackupEngine(true /* destroy_old_data */, false /* dummy */, sopt);

FillDB(db_.get(), 0, keys_iteration);
bool corrupted = false;

Copy link
Contributor

Choose a reason for hiding this comment

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

Will make corrupted an atomic variable work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! I tried using an atomic variable but it seems it doesn't work here.

Copy link
Contributor

@riversand963 riversand963 Jul 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

qq: will corrupted always be true afterwards? In which case it can remain false?

I tried with/without the following. Running the test 100 times seems to show that the issue can be fixed.

diff --git a/utilities/backupable/backupable_db_test.cc b/utilities/backupable/backupable_db_test.cc
index 81e4fd456..77addbd55 100644
--- a/utilities/backupable/backupable_db_test.cc
+++ b/utilities/backupable/backupable_db_test.cc
@@ -1303,7 +1303,7 @@ TEST_F(BackupableDBTest, TableFileWithDbChecksumCorruptedDuringBackup) {
     OpenDBAndBackupEngine(true /* destroy_old_data */, false /* dummy */, sopt);

     FillDB(db_.get(), 0, keys_iteration);
-    bool corrupted = false;
+    std::atomic<bool> corrupted{false};
     // corrupt files when copying to the backup directory
     SyncPoint::GetInstance()->SetCallBack(
         "BackupEngineImpl::CopyOrCreateFile:CorruptionDuringBackup",
@@ -1312,13 +1312,13 @@ TEST_F(BackupableDBTest, TableFileWithDbChecksumCorruptedDuringBackup) {
             Slice* d = reinterpret_cast<Slice*>(data);
             if (!d->empty()) {
               d->remove_suffix(1);
-              corrupted = true;
+              corrupted.store(true, std::memory_order_relaxed);
             }
           }
         });
     SyncPoint::GetInstance()->EnableProcessing();
     Status s = backup_engine_->CreateNewBackup(db_.get());
-    if (corrupted) {
+    if (corrupted.load(std::memory_order_relaxed)) {
       ASSERT_NOK(s);
     } else {
       // should not in this path in normal cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I realized I made a mistake when I modified the code this morning... :

    if (corrupted.load(std::memory_order_relaxed)) {
      ASSERT_NOK(backup_engine_->CreateNewBackup(db_.get()));
    } else {
      ASSERT_OK(backup_engine_->CreateNewBackup(db_.get()));
    }

which makes the atomic variable pointless >_<

corrupted is always true except when there are no table files in the backup to be created or all the table files are empty.

Copy link
Contributor

Choose a reason for hiding this comment

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

SG.

@facebook-github-bot
Copy link
Contributor

@gg814 has updated the pull request. You must reimport the pull request before landing.

4 similar comments
@facebook-github-bot
Copy link
Contributor

@gg814 has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@gg814 has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@gg814 has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@gg814 has updated the pull request. You must reimport the pull request before landing.

riversand963
riversand963 approved these changes Jul 28, 2020
View reviewed changes
Copy link
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.

Thanks @gg814 for the fix!

utilities/backupable/backupable_db_test.cc
@@ -1303,7 +1303,7 @@ TEST_F(BackupableDBTest, TableFileWithDbChecksumCorruptedDuringBackup) {
OpenDBAndBackupEngine(true /* destroy_old_data */, false /* dummy */, sopt);

FillDB(db_.get(), 0, keys_iteration);
bool corrupted = false;

Copy link
Contributor

Choose a reason for hiding this comment

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

SG.

facebook-github-bot
facebook-github-bot reviewed Jul 28, 2020
View reviewed changes
Copy link
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.

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

@facebook-github-bot
Copy link
Contributor

@gg814 merged this pull request in 4496719.

codingrhythm pushed a commit to SafetyCulture/rocksdb that referenced this pull request Mar 5, 2021
@gg814 @codingrhythm
Fix data race warning of BackupableDBTest.TableFileWithDbChecksumCorr…
92ba4b6 
…uptedDuringBackup (facebook#7177)

Summary:
Fix the data race warning by removing an unnecessary variable that causes the warning.

Pull Request resolved: facebook#7177

Test Plan:
`COMPILE_WITH_TSAN=1 make backupable_db_test`
`./backupable_db_test --gtest_filter=*TableFileWithDbChecksumCorruptedDuringBackup*`

Reviewed By: riversand963

Differential Revision: D22774430

Pulled By: gg814

fbshipit-source-id: 3b0b1ac344d0375c64da564cc97f98745c289959
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@riversand963 riversand963 riversand963 approved these changes

@akankshamahajan15 akankshamahajan15 Awaiting requested review from akankshamahajan15

Assignees
No one assigned
Labels
CLA Signed Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@gg814 @facebook-github-bot @riversand963