Skip to content

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

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

Closed
riversand963 wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions HISTORY.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
* Fixed a bug that `Iterator::Refresh()` reads stale keys after DeleteRange() performed.
* Fixed a race condition when disable and re-enable manual compaction.
* Fixed automatic error recovery failure in atomic flush.
* Fixed a race condition when mmaping a WritableFile on POSIX.

### Public API changes
* Remove BlockBasedTableOptions.hash_index_allow_collision which already takes no effect.
Expand Down
45 changes: 22 additions & 23 deletions env/fs_posix.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,14 +325,7 @@ class PosixFileSystem : public FileSystem {
SetFD_CLOEXEC(fd, &options);

if (options.use_mmap_writes) {
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;
}
MaybeForceDisableMmap(fd);
}
if (options.use_mmap_writes && !forceMmapOff_) {
result->reset(new PosixMmapFile(fname, fd, page_size_, options));
Expand Down Expand Up @@ -431,14 +424,7 @@ class PosixFileSystem : public FileSystem {
}

if (options.use_mmap_writes) {
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;
}
MaybeForceDisableMmap(fd);
}
if (options.use_mmap_writes && !forceMmapOff_) {
result->reset(new PosixMmapFile(fname, fd, page_size_, options));
Expand Down Expand Up @@ -999,8 +985,7 @@ class PosixFileSystem : public FileSystem {
}
#endif
private:
bool checkedDiskForMmap_;
bool forceMmapOff_; // do we override Env options?
bool forceMmapOff_ = false; // do we override Env options?

// Returns true iff the named directory exists and is a directory.
virtual bool DirExists(const std::string& dname) {
Expand All @@ -1011,10 +996,10 @@ class PosixFileSystem : public FileSystem {
return false; // stat() failed return false
}

bool SupportsFastAllocate(const std::string& path) {
bool SupportsFastAllocate(int fd) {
#ifdef ROCKSDB_FALLOCATE_PRESENT
struct statfs s;
if (statfs(path.c_str(), &s)) {
if (fstatfs(fd, &s)) {
return false;
}
switch (s.f_type) {
Expand All @@ -1028,11 +1013,26 @@ class PosixFileSystem : public FileSystem {
return false;
}
#else
(void)path;
(void)fd;
return false;
#endif
}

void MaybeForceDisableMmap(int fd) {
static std::once_flag s_check_disk_for_mmap_once;
assert(this == FileSystem::Default().get());
std::call_once(
s_check_disk_for_mmap_once,
[this](int fdesc) {
// this will be executed once in the program's lifetime.
Copy link
Contributor

@pdillinger pdillinger Mar 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// do not use mmapWrite on non ext-3/xfs/tmpfs systems.
if (!SupportsFastAllocate(fdesc)) {
forceMmapOff_ = true;
}
},
fd);
}

#ifdef ROCKSDB_IOURING_PRESENT
bool IsIOUringEnabled() {
if (RocksDbIOUringEnable && RocksDbIOUringEnable()) {
Expand Down Expand Up @@ -1166,8 +1166,7 @@ size_t PosixFileSystem::GetLogicalBlockSizeForWriteIfNeeded(
}

PosixFileSystem::PosixFileSystem()
: checkedDiskForMmap_(false),
forceMmapOff_(false),
: forceMmapOff_(false),
page_size_(getpagesize()),
allow_non_owner_access_(true) {
#if defined(ROCKSDB_IOURING_PRESENT)
Expand Down