Revise LockWAL API and WAL collection #10953

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

Closed

riversand963 wants to merge 8 commits into facebook:main from riversand963:fix-deadlock-1

Contributor

riversand963 commented Nov 15, 2022 •

edited

Loading

RocksDB has two public APIs:

DB::LockWAL()/DB::UnlockWAL() which acquire/release DBImpl::log_write_mutex,
DB::GetSortedWalFiles() which internally calls DBImpl::FindObsoleteFiles() which acquires/releases DBImpl::log_write_mutex.

According to the comment on DBImpl::log_write_mutex_: https://github.com/facebook/rocksdb/blob/7.8.fb/db/db_impl/db_impl.h#L2287:L2288

Note: to avoid dealock, if needed to acquire both log_write_mutex_ and mutex_, the order should be first mutex_ and then log_write_mutex_.

This puts limitations on how applications can use the LockWAL() API. After LockWAL() returns ok, then application
should not perform any operation that acquires mutex_. Currently, the use case of LockWAL() is MyRocks implementing
the MySQL storage engine handlerton lock_hton_log interface. The operation that MyRocks performs after LockWAL()
is GetSortedWalFiless() which not only acquires mutex_, but also log_write_mutex_.

There are two issues:

Applications using these two APIs may hang if one thread calls GetSortedWalFiles() after
calling LockWAL() because log_write_mutex is not recursive.
Two threads may dead lock due to lock order inversion.

Fix for 1

To fix 1, we can figure out whether the calling thread of FindObsoleteFiles() is holding log_write_mutex.
Therefore, we need to track the thread currently holding a mutex. The tracking logic is put in InstrumentedMutex.

Note that std::thread::id can be reused in the same process if the prior thread finishes. We make sure a thread
holding the mutex will reset owner to a default-constructed std::thread::id before calling Unlock(). Also note
that CondVar::Wait() releases the mutex without resetting owner. But keep in mind that when a thread is not sleeping,
and the thread is holding the mutex, then owner is set to the result of calling std::this_thread::get_id().

Fix for 2

To fix 2, we can update LockWAL() API interface and implementation so that

LockWAL() returns information about WALs in the MANIFEST for future verification with actual WAL files
LockWAL() acquires mutex before log_write_mutex, moving part of the work from GetSortedWalFiles() to LockWAL()

Note that application should make sure UnLockWAL() is called if and only if LockWAL() returns success.

We also need to add a new API DB::GetSortedWalFilesWithFileDeletionDisabled() which assumes that file deletions
has been disabled and no need to acquire any mutex_.

Test plan:
make check

facebook-github-bot added the CLA Signed label

riversand963 changed the title ~~Conditionally tracking ID of thread holding mutex~~ Conditionally track ID of thread holding mutex

riversand963 force-pushed the fix-deadlock-1 branch from c735776 to 50bcf44 Compare

November 15, 2022 05:16

Contributor Author

riversand963 commented Nov 15, 2022

Contributor Author

riversand963 commented Nov 15, 2022 •

edited

Loading

Need to address the lock-order-inversion

UPDATE: done

laurynas-biveinis mentioned this pull request

pfs.log_status query hangs if rocksdb_file_deletions were not disabled beforehand facebook/mysql-5.6#1253

Closed

Contributor Author

riversand963 commented Nov 15, 2022

According to comment: https://github.com/facebook/rocksdb/blob/main/db/db_impl/db_impl.h#L2290:L2291, application should always lock DBImpl::mutex_ before DBImpl::log_write_mutex_. Therefore, current DB::LockWAL() implementation is also questionable.

riversand963 force-pushed the fix-deadlock-1 branch from 4a32763 to f358248 Compare

November 17, 2022 21:22

Contributor

facebook-github-bot commented Nov 17, 2022

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

riversand963 marked this pull request as ready for review

November 17, 2022 21:38

riversand963 changed the title ~~Conditionally track ID of thread holding mutex~~ Revise LockWAL API and WAL collection

Contributor

facebook-github-bot commented Nov 17, 2022

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

Contributor

facebook-github-bot commented Nov 17, 2022

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

riversand963 requested a review from siying

November 17, 2022 23:50

riversand963 force-pushed the fix-deadlock-1 branch from 473d439 to 6bafc70 Compare

November 18, 2022 17:47

Contributor

facebook-github-bot commented Nov 18, 2022

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

Contributor

facebook-github-bot commented Nov 18, 2022

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

Contributor

facebook-github-bot commented Nov 21, 2022

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

Contributor

facebook-github-bot commented Nov 21, 2022

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

riversand963 force-pushed the fix-deadlock-1 branch from a396691 to d8754f4 Compare

November 21, 2022 19:37

Contributor

facebook-github-bot commented Nov 21, 2022

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

Contributor

facebook-github-bot commented Nov 21, 2022

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

riversand963 added 7 commits

November 21, 2022 12:32


          unit test

6a66441


          track owner


          Update LockWAL() and UnlockWAL()

667e52d


          fix lite


          Update LockWAL and GetSortedWalFiles


          Fix linter

80c3e20


          Update comments

bde268a

riversand963 force-pushed the fix-deadlock-1 branch from d8754f4 to 50cae81 Compare

November 21, 2022 20:33

Contributor

facebook-github-bot commented Nov 21, 2022

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

riversand963 force-pushed the fix-deadlock-1 branch from 50cae81 to ee51e95 Compare

November 21, 2022 20:45

Contributor

facebook-github-bot commented Nov 21, 2022

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

Contributor

facebook-github-bot commented Nov 21, 2022

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


          For windows build that disables _POSIX_THREADS

63e257d

riversand963 force-pushed the fix-deadlock-1 branch from ee51e95 to 63e257d Compare

November 21, 2022 21:26

Contributor

facebook-github-bot commented Nov 21, 2022

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

Contributor

facebook-github-bot commented Nov 21, 2022

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

ajkr reviewed

View reviewed changes

db/db_impl/db_impl.cc

+                    }
+                  }
+                }
                 log_write_mutex_.Lock();

Contributor

ajkr Nov 28, 2022

This doesn't look like it'd block writes like the original PR (#5146) claimed. Does it? I think explicitly blocking writes (like a write stop) may work better and be simpler.

I also fail to see how the backup worked before without calling DisableFileDeletions(). Did it work?

laurynas-biveinis Nov 28, 2022

I also fail to see how the backup worked before without calling DisableFileDeletions(). Did it work?

MyRocks backup tools do disable file deletions first.

Contributor Author

riversand963 Nov 28, 2022

The #5146 PR claimed that: "Walk through the storage engines and lock writes on each engine. For
InnoDB, redo log is locked. For MyRocks, WAL should be locked."
My original impression is that WAL is locked, thus any writes with WAL will be blocked after this call.

Contributor Author

riversand963 Nov 28, 2022 •

edited

Loading

Furthermore, I thought introducing write stop is undesirable for users, but maybe that's over-optimizing for this use case.

Contributor

ajkr Nov 28, 2022

My original impression is that WAL is locked, thus any writes with WAL will be blocked after this call.

OK, I didn't have that impression by looking at how log_write_mutex_ is used. I will test it then since my concern isn't shared.

Contributor Author

riversand963 Nov 28, 2022

Taking another look at https://github.com/facebook/rocksdb/blob/7.9.fb/db/db_impl/db_impl_write.cc#L1213. In fact, all threads calling PreprocessWrite() may be blocked.

Contributor

ajkr Nov 28, 2022

It did not work originally but #7516 fixed it, probably without noticing. I still think we should re-think the approach as it feels unstable and unintuitive.

Contributor Author

riversand963 Nov 28, 2022

I am also hesitant about exposing log_write_mutex_ to application.
Maybe similar to what innodb does, we don't need keep the log_write_mutex locked as long as we disable file deletion.

riversand963 commented

View reviewed changes

monitoring/instrumented_mutex.h

+                const bool track_owner_ = false;
+              #if defined(OS_WIN) && !defined(_POSIX_THREADS)
+                std::atomic<port::ThreadId> owner_{kDummyOwner};

Contributor Author

riversand963 Nov 30, 2022

Also need to account for cond var wait, etc.

Contributor Author

riversand963 commented Dec 6, 2022

Pursue #11020

riversand963 closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels