remove mutex for write #197

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

Signed-off-by: Little-Wallace [email protected]

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 <[email protected]>

This reverts commit b84bd77. Signed-off-by: Little-Wallace <[email protected]>

Signed-off-by: Little-Wallace <[email protected]>

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 <[email protected]>

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

@@ -67,6 +61,7 @@ class ErrorHandler {
    // 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 - 1" is equivalent to
    // "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

@@ -2140,6 +2148,7 @@ Status DBImpl::CreateColumnFamilyImpl(const ColumnFamilyOptions& cf_options,
      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

@@ -1131,7 +1131,13 @@ class DBImpl : public DB {
      }
    }
  };
-
+  struct LogContext {


Add newlines around this struct definition.

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

db/db_impl/db_impl.h

@@ -1934,6 +1939,7 @@ class DBImpl : public DB {
  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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

fix comment

1f0b313

Signed-off-by: Little-Wallace <[email protected]>

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

do not hold mutex when fsync

d17b969

Signed-off-by: Little-Wallace <[email protected]>

fix lock

59ab24b

Signed-off-by: Little-Wallace <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

address comment

bc172cb

Signed-off-by: Little-Wallace <[email protected]>

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;

remove mutex for write #197

Are you sure you want to change the base?

remove mutex for write #197

Conversation

Little-Wallace commented Oct 19, 2020

Summary

tabokie commented Jan 27, 2022 • edited Loading

Little-Wallace commented Feb 1, 2022

Little-Wallace commented Feb 7, 2022

tabokie commented Feb 7, 2022

Little-Wallace commented Feb 11, 2022

tabokie commented Feb 11, 2022

tabokie 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

Little-Wallace commented Feb 15, 2022

Little-Wallace commented Feb 15, 2022 • 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

Choose a reason for hiding this comment

tabokie 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

Little-Wallace commented Feb 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Connor1996 Feb 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Connor1996 left a comment

Choose a reason for hiding this comment

Connor1996 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tabokie Feb 23, 2022 • 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

tabokie Feb 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Connor1996 left a comment

Choose a reason for hiding this comment

Little-Wallace commented Mar 1, 2022

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