Disable manual compaction during `ReFitLevel()` #7250

ajkr · 2020-08-12T22:42:52Z

Manual compaction with CompactRangeOptions::change_levels set could
refit to a level targeted by another manual compaction. If
force_consistency_checks were disabled, it could be possible for
overlapping files to be written at that target level.

This PR prevents the possibility by calling DisableManualCompaction()
prior to ReFitLevel(). It also improves the manual compaction disabling
mechanism to wait for pending manual compactions to complete before
returning, and support disabling from multiple threads.

Fixes #6432.

Test Plan: crash test command that repro'd the bug reliably:

$ TEST_TMPDIR=/dev/shm python tools/db_crashtest.py blackbox --simple -target_file_size_base=524288 -write_buffer_size=1048576 -clear_column_family_one_in=0 -reopen=0 -max_key=10000000 -column_families=1 -max_background_compactions=8 -compact_range_one_in=100000 -compression_type=none -compaction_style=1 -num_levels=5 -universal_min_merge_width=4 -universal_max_merge_width=8 -level0_file_num_compaction_trigger=12 -rate_limiter_bytes_per_sec=1048576000 -universal_max_size_amplification_percent=100 --duration=3600 --interval=60 --use_direct_io_for_flush_and_compaction=0 --use_direct_reads=0 --enable_compaction_filter=0

ajkr · 2020-08-12T22:46:16Z

Will let the crash test run for a long time. While this diff was still undergoing changes, I saw an unexplained assertion failure on AssertHeld(), and another unexplained failure saying "Compaction input files inconsistent". I am not sure those are fixed in the current version, or if they are related to this problem at all.

facebook-github-bot

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

siying · 2020-08-13T00:27:01Z

db/db_impl/db_impl_compaction_flush.cc

-  manual_compaction_paused_.store(false, std::memory_order_release);
+  InstrumentedMutexLock l(&mutex_);
+  assert(manual_compaction_paused_ > 0);
+  manual_compaction_paused_.fetch_add(-1, std::memory_order_release);


Do we need a signal here?

Any pending manual compactions were already finished (typically by failing with Status::Incomplete) during DisableManualCompaction(), so it's my understanding that nothing is waiting on this condition. manual_compaction_paused_ is quite a confusing name as nothing seems to be paused..

Tangential - another name that confused me a lot in this PR is "incomplete", as a manual compaction returning Status::Incomplete, and a manual compaction setting ManualCompactionState::incomplete, mean completely different things :p.

siying · 2020-08-13T00:29:04Z

db/db_impl/db_impl_compaction_flush.cc

@@ -848,11 +848,17 @@ Status DBImpl::CompactRange(const CompactRangeOptions& options,
  if (options.change_level) {
    ROCKS_LOG_INFO(immutable_db_options_.info_log,
                   "[RefitLevel] waiting for background threads to stop");
+    DisableManualCompaction();


I can see this would prevent some issues, but is only disabling manual compaction in refitting phase enough? If it is not, should we disable it in the whole CompactRange() phase if change_level is true?

This PR ensures any concurrent manual compaction commits its files before we do ReFitLevel(). We know ReFitLevel() has a bug in identifying which levels are empty -- but that bug appears independent of whether those levels became nonempty due to a manual compaction or automatic compaction. I don't know of any bug at this point that is specific to manual compaction running concurrently with a change_level CompactRange() and committing prior to ReFitLevel().

That said, I wouldn't mind if we disabled all flushes, compactions, and ingestions for the entire duration of a change_level CompactRange(). Then we can simplify things a lot. But it's a significant change in terms of code and behavior.

It makes sense. Disabling manual compaction here and check level emptiness might be good enough.

Side note: DBImpl::refitting_level_ (see https://github.com/facebook/rocksdb/blob/master/db/db_impl/db_impl_compaction_flush.cc#L1300-L1306) can probably be removed now?

Could it be possible two manual compactions both finish all their RunManualCompaction() steps before either one calls DisableManualCompaction()? In that case the calls to DisableManualCompaction() do not appear effective since manual compactions are only registered during RunManualCompaction() steps. Then, the two ReFitLevel()s can execute in parallel. I do hope we can consolidate these eventually, just am not sure we're there yet.

facebook-github-bot · 2020-08-13T06:27:10Z

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

ajkr · 2020-08-13T06:28:31Z

Will let the crash test run for a long time. While this diff was still undergoing changes, I saw an unexplained assertion failure on AssertHeld(), and another unexplained failure saying "Compaction input files inconsistent". I am not sure those are fixed in the current version, or if they are related to this problem at all.

The crash test succeeds for several hours.
Also, added a unit test, which is verified to repro the overlapping invariant violation.

facebook-github-bot · 2020-08-13T06:42:37Z

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

facebook-github-bot

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

facebook-github-bot · 2020-08-13T06:48:51Z

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

facebook-github-bot

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

siying

I'm OK with the change. Maybe @ltamasi can give it another pass, as it is tricky.

ltamasi

Thanks for the fix @ajkr ! LGTM, just a couple of minor comments.

ltamasi · 2020-08-13T23:04:54Z

db/db_impl/db_impl_compaction_flush.cc

    s = PauseBackgroundWork();
+    bool bg_paused = false;


Minor but we could achieve the same effect by moving ContinueBackgroundWork inside the if block, right?

ltamasi · 2020-08-13T23:05:38Z

db/db_impl/db_impl_compaction_flush.cc

-  manual_compaction_paused_.store(false, std::memory_order_release);
+  InstrumentedMutexLock l(&mutex_);
+  assert(manual_compaction_paused_ > 0);
+  manual_compaction_paused_.fetch_add(-1, std::memory_order_release);


We could use fetch_sub here.

ltamasi · 2020-08-13T23:06:29Z

db/db_test2.cc

-        auto paused = reinterpret_cast<std::atomic<bool>*>(arg);
-        ASSERT_FALSE(paused->load(std::memory_order_acquire));
-        paused->store(true, std::memory_order_release);
+        auto paused = reinterpret_cast<std::atomic<int>*>(arg);


Minor: static_cast would suffice here and below.

ltamasi · 2020-08-13T23:13:12Z

db/db_impl/db_impl_compaction_flush.cc

@@ -848,11 +848,17 @@ Status DBImpl::CompactRange(const CompactRangeOptions& options,
  if (options.change_level) {
    ROCKS_LOG_INFO(immutable_db_options_.info_log,
                   "[RefitLevel] waiting for background threads to stop");
+    DisableManualCompaction();


Side note: DBImpl::refitting_level_ (see https://github.com/facebook/rocksdb/blob/master/db/db_impl/db_impl_compaction_flush.cc#L1300-L1306) can probably be removed now?

Manual compaction with `CompactRangeOptions::change_levels` set could refit to a level targeted by another manual compaction. If force_consistency_checks were disabled, it could be possible for overlapping files to be written at that target level. This PR prevents the possibility by calling `DisableManualCompaction()` prior to `ReFitLevel()`. It also improves the manual compaction disabling mechanism to wait for pending manual compactions to complete before returning, and support disabling from multiple threads. Fixes facebook#6432. Test Plan: crash test command that repro'd the bug reliably: ``` $ TEST_TMPDIR=/dev/shm python tools/db_crashtest.py blackbox --simple -target_file_size_base=524288 -write_buffer_size=1048576 -clear_column_family_one_in=0 -reopen=0 -max_key=10000000 -column_families=1 -max_background_compactions=8 -compact_range_one_in=100000 -compression_type=none -compaction_style=1 -num_levels=5 -universal_min_merge_width=4 -universal_max_merge_width=8 -level0_file_num_compaction_trigger=12 -rate_limiter_bytes_per_sec=1048576000 -universal_max_size_amplification_percent=100 --duration=3600 --interval=60 --use_direct_io_for_flush_and_compaction=0 --use_direct_reads=0 --enable_compaction_filter=0 ```

ajkr

Thanks very much for the fast and detailed reviews!

ajkr · 2020-08-14T17:15:12Z

db/db_impl/db_impl_compaction_flush.cc

@@ -848,11 +848,17 @@ Status DBImpl::CompactRange(const CompactRangeOptions& options,
  if (options.change_level) {
    ROCKS_LOG_INFO(immutable_db_options_.info_log,
                   "[RefitLevel] waiting for background threads to stop");
+    DisableManualCompaction();


Could it be possible two manual compactions both finish all their RunManualCompaction() steps before either one calls DisableManualCompaction()? In that case the calls to DisableManualCompaction() do not appear effective since manual compactions are only registered during RunManualCompaction() steps. Then, the two ReFitLevel()s can execute in parallel. I do hope we can consolidate these eventually, just am not sure we're there yet.

ajkr · 2020-08-14T17:16:19Z

db/db_impl/db_impl_compaction_flush.cc

    s = PauseBackgroundWork();
+    bool bg_paused = false;


ajkr · 2020-08-14T17:17:04Z

db/db_impl/db_impl_compaction_flush.cc

-  manual_compaction_paused_.store(false, std::memory_order_release);
+  InstrumentedMutexLock l(&mutex_);
+  assert(manual_compaction_paused_ > 0);
+  manual_compaction_paused_.fetch_add(-1, std::memory_order_release);


ajkr · 2020-08-14T17:19:01Z

db/db_test2.cc

-        auto paused = reinterpret_cast<std::atomic<bool>*>(arg);
-        ASSERT_FALSE(paused->load(std::memory_order_acquire));
-        paused->store(true, std::memory_order_release);
+        auto paused = reinterpret_cast<std::atomic<int>*>(arg);


facebook-github-bot · 2020-08-14T17:27:29Z

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

facebook-github-bot

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

facebook-github-bot · 2020-08-14T18:42:26Z

@ajkr merged this pull request in a1aa3f8.

Summary: Manual compaction with `CompactRangeOptions::change_levels` set could refit to a level targeted by another manual compaction. If force_consistency_checks were disabled, it could be possible for overlapping files to be written at that target level. This PR prevents the possibility by calling `DisableManualCompaction()` prior to `ReFitLevel()`. It also improves the manual compaction disabling mechanism to wait for pending manual compactions to complete before returning, and support disabling from multiple threads. Fixes facebook#6432. Pull Request resolved: facebook#7250 Test Plan: crash test command that repro'd the bug reliably: ``` $ TEST_TMPDIR=/dev/shm python tools/db_crashtest.py blackbox --simple -target_file_size_base=524288 -write_buffer_size=1048576 -clear_column_family_one_in=0 -reopen=0 -max_key=10000000 -column_families=1 -max_background_compactions=8 -compact_range_one_in=100000 -compression_type=none -compaction_style=1 -num_levels=5 -universal_min_merge_width=4 -universal_max_merge_width=8 -level0_file_num_compaction_trigger=12 -rate_limiter_bytes_per_sec=1048576000 -universal_max_size_amplification_percent=100 --duration=3600 --interval=60 --use_direct_io_for_flush_and_compaction=0 --use_direct_reads=0 --enable_compaction_filter=0 ``` Reviewed By: ltamasi Differential Revision: D23090800 Pulled By: ajkr fbshipit-source-id: afcbcd51b42ce76789fdb907d8b9ada790709c13

facebook-github-bot added the CLA Signed label Aug 12, 2020

ajkr force-pushed the fix-manual-change-levels branch from afd19a2 to 61afb8f Compare August 12, 2020 22:58

ajkr requested a review from siying August 12, 2020 23:10

facebook-github-bot reviewed Aug 12, 2020

View reviewed changes

siying reviewed Aug 13, 2020

View reviewed changes

ajkr force-pushed the fix-manual-change-levels branch from 30765c2 to 02cfa16 Compare August 13, 2020 06:26

facebook-github-bot reviewed Aug 13, 2020

View reviewed changes

ajkr requested a review from ltamasi August 13, 2020 17:47

siying reviewed Aug 13, 2020

View reviewed changes

ltamasi approved these changes Aug 13, 2020

View reviewed changes

ajkr added 7 commits August 14, 2020 10:18

update HISTORY.md

86171e5

make format

96e477b

add unit test

e7ad120

ensure unit test fails before this PR

dc2cd0b

fix lite

55b331c

address comments

46cf49c

ajkr force-pushed the fix-manual-change-levels branch from 09eb9d5 to 46cf49c Compare August 14, 2020 17:26

ajkr commented Aug 14, 2020

View reviewed changes

facebook-github-bot reviewed Aug 14, 2020

View reviewed changes

facebook-github-bot closed this in a1aa3f8 Aug 14, 2020

facebook-github-bot added the Merged label Aug 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable manual compaction during `ReFitLevel()` #7250

Disable manual compaction during `ReFitLevel()` #7250

ajkr commented Aug 12, 2020 •

edited

Loading

ajkr commented Aug 12, 2020

facebook-github-bot left a comment

siying Aug 13, 2020

ajkr Aug 13, 2020

siying Aug 13, 2020

ajkr Aug 13, 2020

siying Aug 13, 2020

ltamasi Aug 13, 2020

ajkr Aug 14, 2020

facebook-github-bot commented Aug 13, 2020

ajkr commented Aug 13, 2020 •

edited

Loading

facebook-github-bot commented Aug 13, 2020

facebook-github-bot left a comment

facebook-github-bot commented Aug 13, 2020

facebook-github-bot left a comment

siying left a comment

ltamasi left a comment

ltamasi Aug 13, 2020

ajkr Aug 14, 2020

ltamasi Aug 13, 2020

ajkr Aug 14, 2020

ltamasi Aug 13, 2020

ajkr Aug 14, 2020

ltamasi Aug 13, 2020

ajkr left a comment

ajkr Aug 14, 2020

ajkr Aug 14, 2020

ajkr Aug 14, 2020

ajkr Aug 14, 2020

facebook-github-bot commented Aug 14, 2020

facebook-github-bot left a comment

facebook-github-bot commented Aug 14, 2020

Disable manual compaction during ReFitLevel() #7250

Disable manual compaction during ReFitLevel() #7250

Conversation

ajkr commented Aug 12, 2020 • edited Loading

ajkr commented Aug 12, 2020

facebook-github-bot 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

facebook-github-bot commented Aug 13, 2020

ajkr commented Aug 13, 2020 • edited Loading

facebook-github-bot commented Aug 13, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Aug 13, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

siying left a comment

Choose a reason for hiding this comment

ltamasi 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

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

facebook-github-bot commented Aug 14, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Aug 14, 2020

Disable manual compaction during `ReFitLevel()` #7250

Disable manual compaction during `ReFitLevel()` #7250

ajkr commented Aug 12, 2020 •

edited

Loading

ajkr commented Aug 13, 2020 •

edited

Loading