Fix race condition causing double deletion of ssts #3638

riversand963 · 2018-03-21T18:02:45Z

Possible interleaved execution of background compaction thread calling FindObsoleteFiles (no full scan) / PurgeObsoleteFiles and user thread calling FindObsoleteFiles (full scan) / PurgeObsoleteFiles can lead to race condition on which RocksDB attempts to delete a file twice. The second attempt will fail and return IO error. This may occur to other files, but this PR targets sst.
Also add a unit test to verify that this PR fixes the issue.

The newly added unit test obsolete_files_test has a test case for this scenario, implemented in ObsoleteFilesTest#RaceForObsoleteFileDeletion. TestSyncPoints are used to coordinate the interleaving the user_thread and background compaction thread. They execute as follows

timeline              user_thread                background_compaction thread
t1   |                                          FindObsoleteFiles(full_scan=false)
t2   |     FindObsoleteFiles(full_scan=true)
t3   |                                          PurgeObsoleteFiles
t4   |     PurgeObsoleteFiles
     V

When user_thread invokes FindObsoleteFiles with full scan, it collects ALL files in RocksDB directory, including the ones that background compaction thread have collected in its job context. Then user_thread will see an IO error when trying to delete these files in PurgeObsoleteFiles because background compaction thread has already deleted the file in PurgeObsoleteFiles.
To fix this, we make RocksDB remember which (SST) files have been found by threads after calling FindObsoleteFiles (see DBImpl#files_grabbed_for_purge_). Therefore, when another thread calls FindObsoleteFiles with full scan, it will not collect such files.

This has also been reported in ISSUE 3573.

@ajkr could you take a look and comment? Thanks!

Summary: Two threads running DBImpl::FindObsoleteFiles and DBImpl::PurgeObsoleteFiles concurrently can cause double deletion of a file. Test Plan: Write a unit test called double_deletefile_test to reproduce the following race condition. - Thread A runs `DBImpl::FindObsoleteFiles` which returns the deleted file1. - Some external entity installs a new Version, perhaps due to compaction/flush finishing. - Thread B runs `DBImpl::FindObsoleteFiles` with `doing_the_full_scan=true`. It also finds file1 is eligible for deletion because it's not in the current Version and is present in the DB directory. - Thread A runs `DBImpl::PurgeObsoleteFiles` which deletes file1. - Thread B runs `DBImpl::PurgeObsoleteFiles` which also tries to delete file1, but fails. Reviewers: andrewkr Subscribers: Tasks: T26799498 Tags:

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

…ble-del

facebook-github-bot · 2018-03-22T21:44:31Z

@riversand963 has updated the pull request. View: changes

facebook-github-bot · 2018-03-22T22:45:00Z

@riversand963 has updated the pull request. View: changes

ajkr

looking good, a few comments.

ajkr · 2018-03-23T17:48:29Z

db/double_deletefile_test.cc

+  }
+};
+
+TEST_F(DoubleDeleteFileTest, RaceForObsoleteFileDeletion) {


can we either (1) make this file named more generally (e.g., it could be for all obsolete file tests); or (2) move this test into "db/db_test2.cc"? It's because ideally we don't have a separate file dedicated to each test. If we go with (1), we can follow-up with moving other obsolete file tests into this file (there are more spread across various places).

ajkr · 2018-03-23T18:11:37Z

db/double_deletefile_test.cc

+  CheckFileTypeCounts(options_.wal_dir, 1, 0, 0);
+
+  SyncPoint::GetInstance()->LoadDependency({
+      {"DBImpl::BackgroundCallCompaction:2",


a lot of our tests do follow this naming convention. But I find tests more readable if we use self-descriptive names, like "DBImpl::BackgroundCallCompaction:FoundObsoleteFiles" for this one, which would indicate it happens right after the call to FindObsoleteFiles

ajkr · 2018-03-23T18:22:47Z

db/version_set.h

@@ -938,6 +942,8 @@ class VersionSet {
  std::vector<FileMetaData*> obsolete_files_;
  std::vector<std::string> obsolete_manifests_;

+  std::unordered_map<uint64_t, bool> ssts_grabbed_for_purge_;


what's the bool for? can this be an unordered_set?

According to the comment in DBImpl#PurgeObsoleteFiles above the declaration of sst_live_map local variable:

// Now, convert live list to an unordered map, WITHOUT mutex held; // set is slow.

The bool has no use here, at least for now.

I think it's a typo from 2459f7e#diff-71a34e2e4e097b44064da7d3a2b68bd8R638.

2459f7e#diff-71a34e2e4e097b44064da7d3a2b68bd8R638 explicitly changes unordered_set to unordered_map for performance reason. You mean this change is a typo?

Oh, got it, I assumed the value of the map is used but it isn't. Siying might be able to clarify the motivation of that change.

Either way, once we have vector or autovector in JobContext and VersionStorageInfo, it won't matter as much which representation is chosen.

ajkr · 2018-03-23T18:30:44Z

db/version_set.h

@@ -858,6 +858,10 @@ class VersionSet {
                        std::vector<std::string>* manifest_filenames,
                        uint64_t min_pending_output);

+  bool IsGrabbedForPurge(uint64_t file_number) const;
+  void MarkAsGrabbedForPurge(uint64_t file_number);
+  void MarkAsPurged(uint64_t file_number);


I was thinking about how we can verify the map is properly cleaned up. One idea is assert the map is empty in DBImpl::CloseHelper after pending_purge_obsolete_files_ reaches zero. Would it work?

ajkr · 2018-03-23T18:35:45Z

db/db_impl_compaction_flush.cc

        PurgeObsoleteFiles(job_context);
+        TEST_SYNC_POINT("DBImpl::BackgroundCallCompaction:3");


the sync point with the same name above is probably unneeded.

ajkr · 2018-03-23T18:37:28Z

BTW, you can use the "Import to Phabricator" button, which will create an internal phabricator diff and run other tests on it.

riversand963 · 2018-03-23T18:39:41Z

Cool, I'll do it now. In the meantime, will look into the comments. Thanks!

facebook-github-bot

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

facebook-github-bot · 2018-03-26T16:34:21Z

@riversand963 has updated the pull request. View: changes, changes since last import

facebook-github-bot

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

facebook-github-bot · 2018-03-27T01:05:33Z

@riversand963 has updated the pull request. View: changes, changes since last import

facebook-github-bot

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

ajkr

Looks good! Only minor comments.

ajkr · 2018-03-27T16:58:09Z

db/db_impl.h

@@ -1384,6 +1388,9 @@ class DBImpl : public DB {
  bool HaveManualCompaction(ColumnFamilyData* cfd);
  bool MCOverlap(ManualCompactionState* m, ManualCompactionState* m1);

+  bool ShouldIPurge(uint64_t file_number) const;


What is the "I" for? would ShouldPurge be sufficient?

"I" stands for this thread, but I think I can remove it.

ajkr · 2018-03-27T17:26:31Z

db/obsolete_files_test.cc

+  SyncPoint::GetInstance()->EnableProcessing();
+
+  DBImpl* dbi = reinterpret_cast<DBImpl*>(db_);
+  port::Thread userThread([&]() {


let's use lowercase underscore-separated variable names per google style guide

Which variables should follow google style guide, and which should follow Java style? For example, SyncPoint::GetInstance()->EnableProcessing() vs. user_thread?

ajkr · 2018-03-27T17:28:37Z

db/db_impl_compaction_flush.cc

+// and db mutex (mutex_) should already be held. This function performs a
+// linear scan of an vector (files_grabbed_for_purge_) in search of a
+// certain element. We expect FindObsoleteFiles with full scan to occur once
+// every 10 hours, and the size of the vector is small. Therefore, the cost


maybe mention the 10 hours is the default value to avoid confusion, since it won't be true for everyone

facebook-github-bot · 2018-03-27T19:16:04Z

@riversand963 has updated the pull request. View: changes, changes since last import

facebook-github-bot

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

facebook-github-bot

@riversand963 is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Yanqin Jin added 4 commits March 16, 2018 15:35

empty

b9875ec

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

Merge branch 'double-del' of github.com:riversand963/rocksdb into dou…

dbd3a42

…ble-del

facebook-github-bot added the CLA Signed label Mar 21, 2018

sagar0 requested a review from ajkr March 21, 2018 22:02

sagar0 assigned ajkr Mar 21, 2018

Remove a warning caused by type conversion from uint64_t to int

0449668

Remove a check that can fail due to background compaction.

00b185a

ajkr reviewed Mar 23, 2018

View reviewed changes

facebook-github-bot reviewed Mar 23, 2018

View reviewed changes

Address review comments.

de46b8a

facebook-github-bot reviewed Mar 26, 2018

View reviewed changes

Address issues raised during offline discussion with Andrew.

0d4c978

facebook-github-bot reviewed Mar 27, 2018

View reviewed changes

ajkr approved these changes Mar 27, 2018

View reviewed changes

Address review comments.

f86f051

facebook-github-bot reviewed Mar 27, 2018

View reviewed changes

facebook-github-bot reviewed Mar 28, 2018

View reviewed changes

facebook-github-bot closed this in 1f5def1 Mar 28, 2018

riversand963 deleted the double-del branch March 28, 2018 20:54

ajkr mentioned this pull request Apr 11, 2018

IO error: while unlink() file No such file or directory #3573

Closed

riversand963 mentioned this pull request May 8, 2019

fast look up purge_queue #5290

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix race condition causing double deletion of ssts #3638

Fix race condition causing double deletion of ssts #3638

riversand963 commented Mar 21, 2018 •

edited

Loading

facebook-github-bot commented Mar 22, 2018

facebook-github-bot commented Mar 22, 2018

ajkr left a comment

ajkr Mar 23, 2018

riversand963 Mar 26, 2018

ajkr Mar 23, 2018

riversand963 Mar 26, 2018

ajkr Mar 23, 2018

riversand963 Mar 26, 2018

ajkr Mar 26, 2018

riversand963 Mar 26, 2018

ajkr Mar 26, 2018

ajkr Mar 23, 2018

riversand963 Mar 26, 2018

ajkr Mar 23, 2018

riversand963 Mar 26, 2018

ajkr commented Mar 23, 2018

riversand963 commented Mar 23, 2018

facebook-github-bot left a comment

facebook-github-bot commented Mar 26, 2018

facebook-github-bot left a comment

facebook-github-bot commented Mar 27, 2018

facebook-github-bot left a comment

ajkr left a comment

ajkr Mar 27, 2018

riversand963 Mar 27, 2018 •

edited

Loading

ajkr Mar 27, 2018

riversand963 Mar 27, 2018

ajkr Mar 27, 2018

riversand963 Mar 27, 2018

facebook-github-bot commented Mar 27, 2018

facebook-github-bot left a comment

facebook-github-bot left a comment

		PurgeObsoleteFiles(job_context);
		TEST_SYNC_POINT("DBImpl::BackgroundCallCompaction:3");

Fix race condition causing double deletion of ssts #3638

Fix race condition causing double deletion of ssts #3638

Conversation

riversand963 commented Mar 21, 2018 • edited Loading

facebook-github-bot commented Mar 22, 2018

facebook-github-bot commented Mar 22, 2018

ajkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajkr commented Mar 23, 2018

riversand963 commented Mar 23, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 26, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 27, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

ajkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riversand963 Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 27, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

riversand963 commented Mar 21, 2018 •

edited

Loading

riversand963 Mar 27, 2018 •

edited

Loading