From fdecf19d9152fe30b06b4ee59f5ecd326dc8805b Mon Sep 17 00:00:00 2001 From: Adam Simpkins Date: Thu, 15 Mar 2018 13:33:54 -0700 Subject: [PATCH] update FuseChannel to signal the session complete future immediately Summary: This changes FuseChannel to fulfill the session complete future immediately in the thread where it finishes. This may occur inside a FUSE worker thread. It is now up to the caller to use `via()` if they want their callback to run in a separate thread. This simplifies the FuseChannel code and also addresses a crash that occurs in the unit tests sometimes: If the FuseChannel destructor is invoked without explicitly invoking the FuseChannel, it would schedule the session complete promise to be fulfilled in a separate EventBase thread. However, the promise may then have already been destroyed by the time it can run in the EventBase thread. There are still additional synchronization issues to fix in the FuseChannel tests, but this resolves one problem. Reviewed By: chadaustin Differential Revision: D7282001 fbshipit-source-id: be64d3ef6a0e664ed7a2cf93a549acc140184095 --- eden/fs/fuse/FuseChannel.cpp | 8 +------- eden/fs/fuse/FuseChannel.h | 16 +++++++++------- eden/fs/fuse/test/FuseChannelTest.cpp | 8 -------- eden/fs/inodes/EdenMount.cpp | 7 ++----- 4 files changed, 12 insertions(+), 27 deletions(-) diff --git a/eden/fs/fuse/FuseChannel.cpp b/eden/fs/fuse/FuseChannel.cpp index 1324ced056..cd38567791 100644 --- a/eden/fs/fuse/FuseChannel.cpp +++ b/eden/fs/fuse/FuseChannel.cpp @@ -11,7 +11,6 @@ #include #include -#include #include #include #include @@ -313,13 +312,11 @@ void FuseChannel::sendRawReply(const iovec iov[], size_t count) const { FuseChannel::FuseChannel( folly::File&& fuseDevice, AbsolutePathPiece mountPath, - folly::EventBase* eventBase, size_t numThreads, Dispatcher* const dispatcher) : bufferSize_(std::max(size_t(getpagesize()) + 0x1000, MIN_BUFSIZE)), numThreads_(numThreads), dispatcher_(dispatcher), - eventBase_(eventBase), mountPath_(mountPath), fuseDevice_(std::move(fuseDevice)) { CHECK_GE(numThreads_, 1); @@ -570,10 +567,7 @@ void FuseChannel::fuseWorkerThread() noexcept { } if (complete) { - eventBase_->runInEventBaseThread([this]() { - sessionCompletePromise_.setValue( - runState_.load(std::memory_order_relaxed)); - }); + sessionCompletePromise_.setValue(runState_.load(std::memory_order_relaxed)); } } diff --git a/eden/fs/fuse/FuseChannel.h b/eden/fs/fuse/FuseChannel.h index 7cfe242bb4..4038760153 100644 --- a/eden/fs/fuse/FuseChannel.h +++ b/eden/fs/fuse/FuseChannel.h @@ -25,7 +25,6 @@ namespace folly { class RequestContext; -class EventBase; } // namespace folly namespace facebook { @@ -62,7 +61,6 @@ class FuseChannel { FuseChannel( folly::File&& fuseDevice, AbsolutePathPiece mountPath, - folly::EventBase* eventBase, size_t numThreads, Dispatcher* const dispatcher); @@ -221,15 +219,20 @@ class FuseChannel { void finishRequest(const fuse_in_header& header); /** - * Returns a Future that will complete when all of the - * fuse threads have been joined and when all pending - * fuse requests initiated by the kernel have been - * responded to. + * Returns a SemiFuture that will complete when all of the fuse threads have + * been joined and when all pending fuse requests initiated by the kernel + * have been responded to. * * Will throw if called more than once. * * The session completion future will only be signaled if initialization * (via initialize() or takeoverInitialize()) has completed successfully. + * + * This future may be fulfilled inside one of the FUSE worker threads that is + * in the process of shutting down. Therefore is not safe to call + * FuseChannel::destroy() directly in the thread where this future is + * fulfilled. Callers should generally use Future::via() to move execution + * to another executor thread. */ folly::Future getSessionCompleteFuture(); @@ -383,7 +386,6 @@ class FuseChannel { const size_t bufferSize_{0}; const size_t numThreads_; Dispatcher* const dispatcher_{nullptr}; - folly::EventBase* const eventBase_; const AbsolutePath mountPath_; /* diff --git a/eden/fs/fuse/test/FuseChannelTest.cpp b/eden/fs/fuse/test/FuseChannelTest.cpp index ce62e22af1..16aa26fd51 100644 --- a/eden/fs/fuse/test/FuseChannelTest.cpp +++ b/eden/fs/fuse/test/FuseChannelTest.cpp @@ -10,7 +10,6 @@ #include "eden/fs/fuse/FuseChannel.h" #include -#include #include #include #include "eden/fs/fuse/Dispatcher.h" @@ -22,7 +21,6 @@ using namespace facebook::eden::fusell; using namespace std::literals::chrono_literals; using folly::ByteRange; using folly::Future; -using folly::ScopedEventBaseThread; using folly::Unit; using std::make_unique; @@ -33,15 +31,10 @@ class TestDispatcher : public Dispatcher { class FuseChannelTest : public ::testing::Test { protected: - void SetUp() override { - eventBaseThread_ = make_unique(); - } - std::unique_ptr createChannel(size_t numThreads = 2) { return make_unique( fuse_.start(), mountPath_, - eventBaseThread_->getEventBase(), numThreads, &dispatcher_); } @@ -71,7 +64,6 @@ class FuseChannelTest : public ::testing::Test { ThreadLocalEdenStats stats_; TestDispatcher dispatcher_{&stats_}; AbsolutePath mountPath_{"/fake/mount/path"}; - std::unique_ptr eventBaseThread_; }; } // namespace diff --git a/eden/fs/inodes/EdenMount.cpp b/eden/fs/inodes/EdenMount.cpp index dc55aec614..e547d29046 100644 --- a/eden/fs/inodes/EdenMount.cpp +++ b/eden/fs/inodes/EdenMount.cpp @@ -714,13 +714,10 @@ void EdenMount::createFuseChannel( threadPool_ = std::move(threadPool); channel_ = std::make_unique( - std::move(fuseDevice), - path_, - eventBase_, - FLAGS_fuseNumThreads, - dispatcher_.get()); + std::move(fuseDevice), path_, FLAGS_fuseNumThreads, dispatcher_.get()); channel_->getSessionCompleteFuture() + .via(eventBase_) .then([this] { // In case we are performing a graceful restart, // extract the fuse device now.