Fix TSAN-report racing access to checkedDiskForMmap_ and forceMmapOff_ #9685

riversand963 · 2022-03-11T16:13:18Z

Summary:
Our TSAN reports a race condition as follows when running test

gtest-parallel -r 100 ./external_sst_file_test --gtest_filter=ExternalSSTFileTest.MultiThreaded

leads to the following

WARNING: ThreadSanitizer: data race (pid=2683148)
  Write of size 1 at 0x556fede63340 by thread T7:
    #0 rocksdb::(anonymous namespace)::PosixFileSystem::OpenWritableFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, rocksdb::FileOptions const&, bool, std::unique_ptr<rocksdb::FSWritableFile, std::default_delete<rocksdb::FSWritableFile> >*, rocksdb::IODebugContext*) internal_repo_rocksdb/repo/env/fs_posix.cc:334 (external_sst_file_test+0xb61ac4)
    #1 rocksdb::(anonymous namespace)::PosixFileSystem::ReopenWritableFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, rocksdb::FileOptions const&, std::unique_ptr<rocksdb::FSWritableFile, std::default_delete<rocksdb::FSWritableFile> >*, rocksdb::IODebugContext*) internal_repo_rocksdb/repo/env/fs_posix.cc:382 (external_sst_file_test+0xb5ba96)
    #2 rocksdb::CompositeEnv::ReopenWritableFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::unique_ptr<rocksdb::WritableFile, std::default_delete<rocksdb::WritableFile> >*, rocksdb::EnvOptions const&) internal_repo_rocksdb/repo/env/composite_env.cc:334 (external_sst_file_test+0xa6ab7f)
    #3 rocksdb::EnvWrapper::ReopenWritableFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::unique_ptr<rocksdb::WritableFile, std::default_delete<rocksdb::WritableFile> >*, rocksdb::EnvOptions const&) internal_repo_rocksdb/repo/include/rocksdb/env.h:1428 (external_sst_file_test+0x561f3e)
Previous read of size 1 at 0x556fede63340 by thread T4:
    #0 rocksdb::(anonymous namespace)::PosixFileSystem::OpenWritableFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, rocksdb::FileOptions const&, bool, std::unique_ptr<rocksdb::FSWritableFile, std::default_delete<rocksdb::FSWritableFile> >*, rocksdb::IODebugContext*) internal_repo_rocksdb/repo/env/fs_posix.cc:328 (external_sst_file_test+0xb61a70)
    #1 rocksdb::(anonymous namespace)::PosixFileSystem::ReopenWritableFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator
...

Fix by making sure the following block gets executed only once:

      if (!checkedDiskForMmap_) {
        // this will be executed once in the program's lifetime.
        // do not use mmapWrite on non ext-3/xfs/tmpfs systems.
        if (!SupportsFastAllocate(fname)) {
          forceMmapOff_ = true;
        }
        checkedDiskForMmap_ = true;
      }

Differential Revision: D34780308

facebook-github-bot · 2022-03-11T16:13:35Z

This pull request was exported from Phabricator. Differential Revision: D34780308

facebook-github-bot · 2022-03-12T01:54:07Z

This pull request was exported from Phabricator. Differential Revision: D34780308

pdillinger · 2022-03-14T17:27:10Z

The existing code here is odd. Why not run this check as part of the PosixFileSystem constructor? Well, it depends on testing the POSIX filesystem of a particular path. But there's no guarantee that later paths are on the same kind of filesystem as this first written file path. Perhaps we should use fstatfs on each file. (I have no idea why statfs was used instead.)

The critical change was in 1ba5abc

riversand963 · 2022-03-14T17:52:10Z

Yeah, it feels a little weird to me.
According to the impl. of SupportsFastAllocate(fname), the result of this function is the same for all files on the same FS. We should be able to call this function only once in initialization.
We can store the path to the mount point of the fs, and if fname is a path under the mount point, we can re-use the result of SupportsFastAllocate(). One question is how to deal with /mount/point/../../other/fs/db, maybe use canonical_file_path() on all file paths before storing them as part of the FS object in RocksDB.

pdillinger · 2022-03-14T18:38:48Z

We can store the path to the mount point of the fs, ...

That's still a hack. (Mounts can change, etc.) IMO we should just do the simple and apparently correct thing (ask fstatfs for each file open for write with use_mmap_writes) until someone says the performance is unacceptable and proposes an optimization.

Though it looks like someone cares about fstatfs calls that could be unnecessary in some contexts: https://groups.google.com/g/rocksdb/c/J2Z656yfOH0 Hmm. Perhaps we should have a force_mmap_writes option that allows bypassing some usual safety/efficiency checks, to allow maximum efficiency if you know what you're doing.

pdillinger · 2022-03-14T20:34:00Z

env/fs_posix.cc

-        // do not use mmapWrite on non ext-3/xfs/tmpfs systems.
-        if (!SupportsFastAllocate(fname)) {
-          forceMmapOff_ = true;
+      std::call_once(check_disk_for_mmap_once_, [&fname, this]() {


I would be ok with this as a "better than before" fix if the two occurrences of this code were consolidated into one function and the checkedDiskForMmap_ field was removed (because of just one call_once)

On second thought, I plan to pursue this direction without changing behavior in a PR dedicated to fixing TSAN-error. We can follow up with potential fixes/optimizations in future PRs @pdillinger @mrambacher

riversand963 · 2022-03-14T20:48:55Z

I will update the PR according to your suggestion, because I don't think calling a fstat once for each SST is going to be very expensive on local file system.

A question:

Mounts can change, etc

Do we support this? Or we just cannot assume users don't do it?

pdillinger · 2022-03-14T21:09:50Z

Mounts can change, etc

Do we support this? Or we just cannot assume users don't do it?

I would expect anything supported by posix should work for RocksDB FileSystem. I would not expect to need to restart RocksDB processes just because mount points have changed. Even if not directly impacted at the moment, https://github.com/facebookexperimental/eden/blob/main/eden/fs/docs/Overview.md comes to mind, as they use RocksDB and dynamic mounts.

mrambacher · 2022-03-14T21:19:51Z

env/fs_posix.cc

+      std::call_once(check_disk_for_mmap_once_, [&fname, this]() {
+        if (!checkedDiskForMmap_) {


I am not sure what the point of this optimization is. I think the code calls another fstat on to determine if range sync is supported, and there might be another one as well. Is it possible to somehow combine these optimizations into one system call?

fstatfs yes. It is tricky because of conditional compilation etc. but should be possible. (I don't recommend attempting in this PR, so that the TSAN fix is low risk.)

facebook-github-bot · 2022-03-16T02:42:22Z

This pull request was exported from Phabricator. Differential Revision: D34780308

pdillinger

An improvement for sure

pdillinger · 2022-03-17T23:16:49Z

env/fs_posix.cc

+    std::call_once(
+        s_check_disk_for_mmap_once,
+        [this](int fdesc) {
+          // this will be executed once in the program's lifetime.


I would feel slightly better if there was assert(this == FileSystem::Default().get()); to ensure singleton.

riversand963 · 2022-03-18T00:42:26Z

Thanks @pdillinger for the review!

…n opening Posix WritableFile (facebook#9685) Summary: Pull Request resolved: facebook#9685 Our TSAN reports a race condition as follows when running test ``` gtest-parallel -r 100 ./external_sst_file_test --gtest_filter=ExternalSSTFileTest.MultiThreaded ``` leads to the following ``` WARNING: ThreadSanitizer: data race (pid=2683148) Write of size 1 at 0x556fede63340 by thread T7: #0 rocksdb::(anonymous namespace)::PosixFileSystem::OpenWritableFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, rocksdb::FileOptions const&, bool, std::unique_ptr<rocksdb::FSWritableFile, std::default_delete<rocksdb::FSWritableFile> >*, rocksdb::IODebugContext*) internal_repo_rocksdb/repo/env/fs_posix.cc:334 (external_sst_file_test+0xb61ac4) #1 rocksdb::(anonymous namespace)::PosixFileSystem::ReopenWritableFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, rocksdb::FileOptions const&, std::unique_ptr<rocksdb::FSWritableFile, std::default_delete<rocksdb::FSWritableFile> >*, rocksdb::IODebugContext*) internal_repo_rocksdb/repo/env/fs_posix.cc:382 (external_sst_file_test+0xb5ba96) #2 rocksdb::CompositeEnv::ReopenWritableFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::unique_ptr<rocksdb::WritableFile, std::default_delete<rocksdb::WritableFile> >*, rocksdb::EnvOptions const&) internal_repo_rocksdb/repo/env/composite_env.cc:334 (external_sst_file_test+0xa6ab7f) #3 rocksdb::EnvWrapper::ReopenWritableFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::unique_ptr<rocksdb::WritableFile, std::default_delete<rocksdb::WritableFile> >*, rocksdb::EnvOptions const&) internal_repo_rocksdb/repo/include/rocksdb/env.h:1428 (external_sst_file_test+0x561f3e) Previous read of size 1 at 0x556fede63340 by thread T4: #0 rocksdb::(anonymous namespace)::PosixFileSystem::OpenWritableFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, rocksdb::FileOptions const&, bool, std::unique_ptr<rocksdb::FSWritableFile, std::default_delete<rocksdb::FSWritableFile> >*, rocksdb::IODebugContext*) internal_repo_rocksdb/repo/env/fs_posix.cc:328 (external_sst_file_test+0xb61a70) #1 rocksdb::(anonymous namespace)::PosixFileSystem::ReopenWritableFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator ... ``` Fix by making sure the following block gets executed only once: ``` if (!checkedDiskForMmap_) { // this will be executed once in the program's lifetime. // do not use mmapWrite on non ext-3/xfs/tmpfs systems. if (!SupportsFastAllocate(fname)) { forceMmapOff_ = true; } checkedDiskForMmap_ = true; } ``` Reviewed By: pdillinger Differential Revision: D34780308 fbshipit-source-id: 9fe0cf5d87e2dfef5547913e1dadaa41f6ae5125

facebook-github-bot · 2022-03-18T00:43:45Z

This pull request was exported from Phabricator. Differential Revision: D34780308

…n opening Posix WritableFile (#9685) Summary: Pull Request resolved: #9685 Our TSAN reports a race condition as follows when running test ``` gtest-parallel -r 100 ./external_sst_file_test --gtest_filter=ExternalSSTFileTest.MultiThreaded ``` leads to the following ``` WARNING: ThreadSanitizer: data race (pid=2683148) Write of size 1 at 0x556fede63340 by thread T7: #0 rocksdb::(anonymous namespace)::PosixFileSystem::OpenWritableFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, rocksdb::FileOptions const&, bool, std::unique_ptr<rocksdb::FSWritableFile, std::default_delete<rocksdb::FSWritableFile> >*, rocksdb::IODebugContext*) internal_repo_rocksdb/repo/env/fs_posix.cc:334 (external_sst_file_test+0xb61ac4) #1 rocksdb::(anonymous namespace)::PosixFileSystem::ReopenWritableFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, rocksdb::FileOptions const&, std::unique_ptr<rocksdb::FSWritableFile, std::default_delete<rocksdb::FSWritableFile> >*, rocksdb::IODebugContext*) internal_repo_rocksdb/repo/env/fs_posix.cc:382 (external_sst_file_test+0xb5ba96) #2 rocksdb::CompositeEnv::ReopenWritableFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::unique_ptr<rocksdb::WritableFile, std::default_delete<rocksdb::WritableFile> >*, rocksdb::EnvOptions const&) internal_repo_rocksdb/repo/env/composite_env.cc:334 (external_sst_file_test+0xa6ab7f) #3 rocksdb::EnvWrapper::ReopenWritableFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::unique_ptr<rocksdb::WritableFile, std::default_delete<rocksdb::WritableFile> >*, rocksdb::EnvOptions const&) internal_repo_rocksdb/repo/include/rocksdb/env.h:1428 (external_sst_file_test+0x561f3e) Previous read of size 1 at 0x556fede63340 by thread T4: #0 rocksdb::(anonymous namespace)::PosixFileSystem::OpenWritableFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, rocksdb::FileOptions const&, bool, std::unique_ptr<rocksdb::FSWritableFile, std::default_delete<rocksdb::FSWritableFile> >*, rocksdb::IODebugContext*) internal_repo_rocksdb/repo/env/fs_posix.cc:328 (external_sst_file_test+0xb61a70) #1 rocksdb::(anonymous namespace)::PosixFileSystem::ReopenWritableFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator ... ``` Fix by making sure the following block gets executed only once: ``` if (!checkedDiskForMmap_) { // this will be executed once in the program's lifetime. // do not use mmapWrite on non ext-3/xfs/tmpfs systems. if (!SupportsFastAllocate(fname)) { forceMmapOff_ = true; } checkedDiskForMmap_ = true; } ``` Reviewed By: pdillinger Differential Revision: D34780308 fbshipit-source-id: b761f66b24c8b5b8389d86ea371c8542b8d869d5

facebook-github-bot added CLA Signed fb-exported labels Mar 11, 2022

riversand963 force-pushed the export-D34780308 branch from 4cf686c to e585145 Compare March 12, 2022 01:54

pdillinger reviewed Mar 14, 2022

View reviewed changes

mrambacher reviewed Mar 14, 2022

View reviewed changes

riversand963 force-pushed the export-D34780308 branch from e585145 to 9f6e825 Compare March 16, 2022 02:42

pdillinger approved these changes Mar 17, 2022

View reviewed changes

riversand963 force-pushed the export-D34780308 branch from 9f6e825 to 3ba22ae Compare March 18, 2022 00:43

facebook-github-bot closed this in 3bdbf67 Mar 18, 2022

riversand963 deleted the export-D34780308 branch March 18, 2022 04:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix TSAN-report racing access to checkedDiskForMmap_ and forceMmapOff_ #9685

Fix TSAN-report racing access to checkedDiskForMmap_ and forceMmapOff_ #9685

riversand963 commented Mar 11, 2022

facebook-github-bot commented Mar 11, 2022

facebook-github-bot commented Mar 12, 2022

pdillinger commented Mar 14, 2022

riversand963 commented Mar 14, 2022

pdillinger commented Mar 14, 2022

pdillinger Mar 14, 2022

riversand963 Mar 16, 2022

riversand963 commented Mar 14, 2022

pdillinger commented Mar 14, 2022

mrambacher Mar 14, 2022

pdillinger Mar 15, 2022

facebook-github-bot commented Mar 16, 2022

pdillinger left a comment

pdillinger Mar 17, 2022

riversand963 commented Mar 18, 2022

facebook-github-bot commented Mar 18, 2022

		std::call_once(check_disk_for_mmap_once_, [&fname, this]() {
		if (!checkedDiskForMmap_) {

Fix TSAN-report racing access to checkedDiskForMmap_ and forceMmapOff_ #9685

Fix TSAN-report racing access to checkedDiskForMmap_ and forceMmapOff_ #9685

Conversation

riversand963 commented Mar 11, 2022

facebook-github-bot commented Mar 11, 2022

facebook-github-bot commented Mar 12, 2022

pdillinger commented Mar 14, 2022

riversand963 commented Mar 14, 2022

pdillinger commented Mar 14, 2022

pdillinger Mar 14, 2022

Choose a reason for hiding this comment

riversand963 Mar 16, 2022

Choose a reason for hiding this comment

riversand963 commented Mar 14, 2022

pdillinger commented Mar 14, 2022

mrambacher Mar 14, 2022

Choose a reason for hiding this comment

pdillinger Mar 15, 2022

Choose a reason for hiding this comment

facebook-github-bot commented Mar 16, 2022

pdillinger left a comment

Choose a reason for hiding this comment

pdillinger Mar 17, 2022

Choose a reason for hiding this comment

riversand963 commented Mar 18, 2022

facebook-github-bot commented Mar 18, 2022