Skip to content

Commit

Permalink
eden: fix data race on shutdown
Browse files Browse the repository at this point in the history
Summary:
We can't allow ~EdenServer to delete the memory until we're sure that
the other threads are done.  To ensure that, we need to notify the condition
variable while the aux thread still holds the lock.  This makes sure that the
thread destroying the EdenServer waits for the aux thread to release the lock
before we check the predicate and proceed to deleting the memory.

```
SUMMARY  ThreadSanitizer: data race /
/common/concurrency/Event.cpp:107 in facebook::common::concurrency::Event::set() const
==================
I0909 14:51:18.543072 4147554 main.cpp:173] edenfs performing orderly shutdown
I0909 14:51:18.555794 4148654 Channel.cpp:177] session completed
I0909 14:51:18.556011 4148654 EdenServer.cpp:192] mount point "/tmp/eden_test.0ostuc90/mounts/main" stopped
==================
WARNING: ThreadSanitizer: data race (pid=4147554)
  Write of size 8 at 0x7fff9e182d90 by main thread:
    #0 pthread_cond_destroy <null> (edenfs+0x00000007671a)
    #1 facebook::eden::EdenServer::~EdenServer() /
/eden/fs/service/EdenServer.cpp:93 (libeden_fs_service_server.so+0x0000000b96cd)
    #2 main /
/eden/fs/service/main.cpp:176 (edenfs+0x000000018515)

  Previous read of size 8 at 0x7fff9e182d90 by thread T73:
    #0 pthread_cond_broadcast <null> (edenfs+0x0000000765b7)
    #1 __gthread_cond_broadcast /home/engshare/third-party2/libgcc/4.9.x/src/gcc-4_9/x86_64-facebook-linux/libstdc++-v3/include/x86_64-facebook-linux/bits/gthr-default.h:852 (libstdc++.so.6+0x0000000e14f8)
    #2 std::condition_variable::notify_all() /home/engshare/third-party2/libgcc/4.9.x/src/gcc-4_9/x86_64-facebook-linux/libstdc++-v3/src/c++11/../../../.././libstdc++-v3/src/c++11/condition_variable.cc:72 (libstdc
++.so.6+0x0000000e14f8)
    #3 facebook::eden::EdenServer::mount(std::shared_ptr<facebook::eden::EdenMount>, std::unique_ptr<facebook::eden::ClientConfig, std::default_delete<facebook::eden::ClientConfig> >)::$_0::operator()() const /
/
/eden/fs/service/EdenServer.cpp:145 (libeden_fs_service_server.so+0x0000000bcdb5)
    #4 std::_Function_handler<void (), facebook::eden::EdenServer::mount(std::shared_ptr<facebook::eden::EdenMount>, std::unique_ptr<facebook::eden::ClientConfig, std::default_delete<facebook::eden::ClientConfig>
>)::$_0>::_M_invoke(std::_Any_data const&) /
/third-party-buck/gcc-4.9-glibc-2.20-fb/build/libgcc/include/c++/trunk/functional:2039 (libeden_fs_service_server.so+0x0000000bcab0)
    #5 std::function<void ()>::operator()() const /
/third-party-buck/gcc-4.9-glibc-2.20-fb/build/libgcc/include/c++/trunk/functional:2439 (libeden_fuse_fusell.so+0x00000020fbb9)
    #6 facebook::eden::fusell::MountPoint::start(bool, std::function<void ()> const&)::$_0::operator()() const /
/eden/fuse/MountPoint.cpp:69 (libeden_fuse_fusell.so+0x000000237447
)
    #7 void std::_Bind_simple<facebook::eden::fusell::MountPoint::start(bool, std::function<void ()> const&)::$_0 ()>::_M_invoke<>(std::_Index_tuple<>) /
/third-party-buck/gcc-4.9-
glibc-2.20-fb/build/libgcc/include/c++/trunk/functional:1699 (libeden_fuse_fusell.so+0x000000237048)
    #8 std::_Bind_simple<facebook::eden::fusell::MountPoint::start(bool, std::function<void ()> const&)::$_0 ()>::operator()() /
/third-party-buck/gcc-4.9-glibc-2.20-fb/build/libgc
c/include/c++/trunk/functional:1688 (libeden_fuse_fusell.so+0x000000236ff8)
    #9 std::thread::_Impl<std::_Bind_simple<facebook::eden::fusell::MountPoint::start(bool, std::function<void ()> const&)::$_0 ()> >::_M_run() /
/third-party-buck/gcc-4.9-glibc-2.
20-fb/build/libgcc/include/c++/trunk/thread:115 (libeden_fuse_fusell.so+0x000000236d8c)
    #10 execute_native_thread_routine /home/engshare/third-party2/libgcc/4.9.x/src/gcc-4_9/x86_64-facebook-linux/libstdc++-v3/src/c++11/../../../.././libstdc++-v3/src/c++11/thread.cc:84 (libstdc++.so.6+0x0000000e6
ec0)
```

Reviewed By: simpkins

Differential Revision: D3844846

fbshipit-source-id: 545474bc1aff8621dbeb487dcd6b54c82828ff3b
  • Loading branch information
wez authored and Facebook Github Bot 4 committed Sep 9, 2016
1 parent c750977 commit 96d6684
Showing 1 changed file with 8 additions and 1 deletion.
9 changes: 8 additions & 1 deletion eden/fs/service/EdenServer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -194,8 +194,15 @@ void EdenServer::mountFinished(EdenMount* edenMount) {
std::lock_guard<std::mutex> guard(mountPointsMutex_);
auto numErased = mountPoints_.erase(mountPath);
CHECK_EQ(numErased, 1);
// This notify and the erase above MUST happen while holding
// mountPointsMutex_ otherwise we can have a race in the case that there
// are two threads in mountFinished(). If the erasure and the notify are
// not done under the lock, the predicate check in ~EdenServer will see
// that mountPoints_ is empty and proceed to delete mountPointsCV_ out
// from under the other thread(s) as they attempt to call notify_all() on
// it.
mountPointsCV_.notify_all();
}
mountPointsCV_.notify_all();
}

EdenServer::MountList EdenServer::getMountPoints() const {
Expand Down

0 comments on commit 96d6684

Please sign in to comment.