Fix TSAN-report racing access to checkedDiskForMmap_ and forceMmapOff_

[riversand963]

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]

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

[facebook-github-bot]

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

[pdillinger]

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]

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]

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]

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)

[riversand963]

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]

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]

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]

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?

[pdillinger]

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]

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

[pdillinger]

An improvement for sure

[pdillinger]

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

[riversand963]

Thanks @pdillinger for the review!

[facebook-github-bot]

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