remove mutex for write by Little-Wallace · Pull Request #197 · tikv/rocksdb

Little-Wallace · 2020-10-19T05:56:48Z

Signed-off-by: Little-Wallace bupt2013211450@gmail.com

Summary

Cherry-pick facebook#7516
This PR is to avoid write operation blocking on mutex when compaction or other method hold the mutex too long.

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

This reverts commit b84bd77. Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

tabokie · 2022-01-27T08:06:47Z

Could you pass facebook's CI, or at least figure out why they failed? I see you have several builds timed out. That usually means certain cases are hanging (they don't output PASSED log nor error log).

Little-Wallace · 2022-02-01T05:41:01Z

/test

Little-Wallace · 2022-02-07T03:01:27Z

Could you pass facebook's CI, or at least figure out why they failed? I see you have several builds timed out. That usually means certain cases are hanging (they don't output PASSED log nor error log).

It seems to be a unstable test. These tests run a long time and may fail because of environment.

tabokie · 2022-02-07T03:26:35Z

@Little-Wallace I don't think so. There are some unstable ones, but they won't cause nearly half of the tests to fail (usually at most 2).

Little-Wallace · 2022-02-11T07:07:00Z

@Little-Wallace I don't think so. There are some unstable ones, but they won't cause nearly half of the tests to fail (usually at most 2).

The tests did not fail. But the process was killed because out of time

tabokie · 2022-02-11T07:33:02Z

@Little-Wallace Some tests are hanging, mostly likely on some sync point conditions. It's better to figure out exactly what are those conditions, even though we might not need to fix them.

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

tabokie

Could you explain the bug you just fixed in upstream? So I know where the review process is flawed.

tabokie · 2022-02-15T07:54:17Z

db/error_handler.h

    // A flag indicating whether automatic recovery from errors is enabled
    bool auto_recovery_;
    bool recovery_in_prog_;
+    std::atomic<bool> stop_state_;


Suggested change

std::atomic<bool> stop_state_;

std::atomic<bool> db_stopped_;

tabokie · 2022-02-15T07:58:31Z

db/error_handler.h

-     return !bg_error_.ok() &&
-            bg_error_.severity() >= Status::Severity::kHardError;
-    }
+   bool IsDBStopped() { return stop_state_.load(std::memory_order_acquire); }


Add comment "Do not require DB mutex held."

tabokie · 2022-02-15T07:59:50Z

db/db_impl/db_impl.cc

+                         new_options.max_background_compactions,
+                         new_options.max_background_jobs,
+                         new_options.base_background_compactions,
+                         /* parallelize_compactions */ true);


Suggested change

/* parallelize_compactions */ true);

true /* parallelize_compactions */);

It seems that all code of rocksdb place the comment before param

You're half right. It's a per-file styling and this file happens to use postposition which is rare within the whole codebase...

tabokie · 2022-02-15T08:04:25Z

db/db_impl/db_impl_compaction_flush.cc

    // "number < current_log_number".
    MarkLogsSynced(current_log_number - 1, true, s);
    if (!s.ok()) {
-      error_handler_.SetBGError(s, BackgroundErrorReason::kFlush);


Where do you set this error now?

It will be saved at "db/db_impl/db_impl_compaction_flush.cc" L549 and L210

In this function, we will not hold mutex so that we can not save BG error here.

Little-Wallace · 2022-02-15T12:22:47Z

Could you explain the bug you just fixed in upstream? So I know where the review process is flawed.

The function SyncClosedLogs must not hold mutex_ because this function may wait for other thread until they finish wal fsync. So we must unlock mutex_ before every where calling SyncClosedLogs

Little-Wallace · 2022-02-15T12:25:14Z

I fix the problem of facebook#7516 in this commit: facebook@5fe832a

There are some other bugs which are bout code only in main branch of facebook/rocksdb, so I did not port them here.

tonyxuqqi · 2022-02-17T22:12:05Z

db/db_impl/db_impl.cc

      ROCKS_LOG_INFO(immutable_db_options_.info_log,
                     "Created column family [%s] (ID %u)",
                     column_family_name.c_str(), (unsigned)cfd->GetID());
+      single_column_family_mode_.store(false, std::memory_order_release);


why move this line from 2135?

I forget why I place it here.... Maybe I can try to place it back to origin

tonyxuqqi · 2022-02-17T22:43:09Z

db/db_impl/db_impl_compaction_flush.cc

@@ -175,7 +174,9 @@ Status DBImpl::FlushMemTableToOutputFile(
    // flushed SST may contain data from write batches whose updates to
    // other column families are missing.
    // SyncClosedLogs() may unlock and re-lock the db_mutex.


"db_mutex" -> log_write_mutex_

tonyxuqqi · 2022-02-17T22:58:26Z

db/db_impl/db_impl.cc

-        ret = s;
+  {
+    InstrumentedMutexLock lock(&log_write_mutex_);
+    for (auto l : logs_to_free_) {


What's the benefit to make logs_to_free_ be protected by both log_write_mutex_ and mutex_

It makes no sense. But i think unlock mutex_ during this function may cause other bad case.

tabokie

Looks good overall. But I'd prefer seeing a full test report before merging.

tabokie · 2022-02-21T07:25:45Z

db/db_impl/db_impl.h

    }
  };
-
+  struct LogContext {


Add newlines around this struct definition.

tabokie · 2022-02-21T07:28:08Z

db/db_impl/db_impl.h

  InstrumentedCondVar atomic_flush_install_cv_;

  bool wal_in_db_path_;
+  std::atomic<uint64_t> max_total_wal_size_;


Put it together with max_total_in_memory_state_.

tabokie · 2022-02-21T07:37:36Z

db/db_impl/db_impl_write.cc

+  if (max_total_wal_size > 0) {
+    return max_total_wal_size;
+  }
+  return 4 * max_total_in_memory_state_;


max_total_in_memory_state_ is an atomic now, need to specify memory order as well?

Little-Wallace · 2022-02-21T09:01:20Z

I have finished the benchmark test. See details in https://docs.google.com/document/d/1f72BGmN1kREorkaCzhbsB0oocTP13IKJw4jy5mfm-TE/edit#heading=h.mtjjjfyw1ei
I will run a long time stable test to make sure this PR would not cause other bug or error.

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

Connor1996 · 2022-02-23T03:51:45Z

db/db_impl/db_impl_write.cc

-    mutex_.Lock();
-    bool need_log_sync = !write_options.disableWAL && write_options.sync;
-    bool need_log_dir_sync = need_log_sync && !log_dir_synced_;
+    LogContext log_context;


why not init need_log_dir_sync here?

need_log_dir_sync was protected by mutex_ before. This pr will protected this member variable by log_write_mutex_ so it could only be accessed in the method PreprocessWrite.

you mean log_dir_synced_ is protected by mutex_? Oh, reasonable.

Connor1996

rocksdb/db/db_impl/db_impl_write.cc

Line 1194 in 46d7b19

// It's safe to access logs_ with unlocked mutex_ here because:

change mutex_ to log_write_mutex_

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

Connor1996

should update the comment

Connor1996 · 2022-02-23T11:41:22Z

db/db_impl/db_impl_files.cc

@@ -220,10 +216,7 @@ void DBImpl::FindObsoleteFiles(JobContext* job_context, bool force,
        continue;


the log_sync_cv_ may wait here while holding the mutex

~~I think condvar::Wait will implicitly release mutex (a mutex is paired with it when constructed).~~ Oh, you mean mutex_. That's a good catch.

yes, it will release the pairing mutex log_write_mutex_, but not mutex_

Good catch... I'm thinking about how to solve this problem....

I'm not sure whether it is still correctly to release mutex_ here because if some other thread calling FindObsoleteFiles but this thread is only finish half of its task.

mutex_ is released before, I don't see a problem here. But you can't simply re-acquire the mutex after Wait(). That will break the lock order constraint.

Maybe you can skip those unsync-ed log files, and clean them in the next round?

OK I will unlock mutex_ before FindObsoleteLogFiles method. PTAL

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

Connor1996

rest LGTM. Can we add a test to cover the case of the mutex?

Little-Wallace · 2022-03-01T06:10:44Z

rest LGTM. Can we add a test to cover the case of the mutex?

I thought there are enough tests in RocksDB to cover this condition? I have passed the CI of facebook to make sure that it is no problem

Little-Wallace added 7 commits October 19, 2020 13:54

remove mutex for write

5730f5e

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

Merge branch '6.4.tikv' into mutex

1ca2b0e

Merge branch '6.4.tikv' into mutex

add5369

Merge branch '6.4.tikv' into mutex

cf42e20

Merge branch '6.4.tikv' into mutex

c9493c7

Merge branch '6.4.tikv' of https://github.com/tikv/rocksdb into mutex

9da6f60

test: fix some perf context position

b84bd77

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

Little-Wallace force-pushed the mutex branch from 5339dc2 to b84bd77 Compare January 24, 2022 06:49

Revert "test: fix some perf context position"

b914b6b

This reverts commit b84bd77. Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

Little-Wallace force-pushed the mutex branch from e17d72e to b914b6b Compare January 27, 2022 06:58

Little-Wallace added 2 commits January 27, 2022 14:59

fix alive_log_files_

914ca62

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

fix comment

1f0b313

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

Little-Wallace added 2 commits February 11, 2022 22:17

do not hold mutex when fsync

d17b969

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

fix lock

59ab24b

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

tabokie reviewed Feb 15, 2022

View reviewed changes

tonyxuqqi reviewed Feb 17, 2022

View reviewed changes

tabokie reviewed Feb 21, 2022

View reviewed changes

address comment

46d7b19

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

tabokie approved these changes Feb 21, 2022

View reviewed changes

Connor1996 self-requested a review February 22, 2022 08:37

Connor1996 reviewed Feb 23, 2022

View reviewed changes

addressed

55409a2

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

Connor1996 reviewed Feb 23, 2022

View reviewed changes

Little-Wallace added 2 commits February 24, 2022 13:53

release mutex before find obsolete log files

5a17178

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

address comment

bc172cb

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

Connor1996 reviewed Mar 1, 2022

View reviewed changes

tabokie mentioned this pull request Jun 9, 2022

Do not hold mutex when write keys if not necessary facebook/rocksdb#7516

Closed

Merge branch '6.4.tikv' into mutex

6bacce4

	std::atomic<bool> stop_state_;
	std::atomic<bool> db_stopped_;

	/* parallelize_compactions */ true);
	true /* parallelize_compactions */);

		@@ -220,10 +216,7 @@ void DBImpl::FindObsoleteFiles(JobContext* job_context, bool force,
		continue;

Conversation

Little-Wallace commented Oct 19, 2020

Summary

Uh oh!

tabokie commented Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Little-Wallace commented Feb 1, 2022

Uh oh!

Little-Wallace commented Feb 7, 2022

Uh oh!

tabokie commented Feb 7, 2022

Uh oh!

Little-Wallace commented Feb 11, 2022

Uh oh!

tabokie commented Feb 11, 2022

Uh oh!

tabokie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Little-Wallace commented Feb 15, 2022

Uh oh!

Little-Wallace commented Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tabokie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Little-Wallace commented Feb 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Connor1996 Feb 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

tabokie commented Jan 27, 2022 •

edited

Loading

Little-Wallace commented Feb 15, 2022 •

edited

Loading

Connor1996 Feb 23, 2022 •

edited

Loading

tabokie Feb 23, 2022 •

edited

Loading

tabokie Feb 24, 2022 •

edited

Loading