move unmount into FsChannel

Summary:
Unify EdenMount's unmount() logic across all FsChannel
implementations.

Reviewed By: kmancini

Differential Revision: D45300782

fbshipit-source-id: a65c06dd1fc3dc1f605804a28f6c8f45078b3135

eden/fs/fuse/FuseChannel.cpp

@@ -886,7 +886,7 @@ FuseChannel::StopFuture FuseChannel::initializeFromTakeover(
   return sessionCompletePromise_.getFuture();
 }
 
-folly::Future<folly::Unit> FuseChannel::unmount() {
+folly::SemiFuture<folly::Unit> FuseChannel::unmount() {
   // TODO: This does not handle the situation where the mount has been moved by,
   // for example, renaming a parent directory, or `mount --move`.
   return privHelper_->fuseUnmount(mountPath_.view());

eden/fs/fuse/FuseChannel.h

@@ -328,7 +328,7 @@ class FuseChannel final : public FsChannel {
    * FuseChannel. The future returned by initialize() will be fulfilled with a
    * non-takeover StopData.
    */
-  FOLLY_NODISCARD folly::Future<folly::Unit> unmount();
+  FOLLY_NODISCARD folly::SemiFuture<folly::Unit> unmount() override;
 
   /**
    * Request that the FuseChannel stop processing new requests, and prepare

eden/fs/inodes/EdenMount.cpp

@@ -986,58 +986,47 @@ folly::SemiFuture<SerializedInodeMap> EdenMount::shutdownImpl(bool doTakeover) {
 }
 
 folly::SemiFuture<folly::Unit> EdenMount::unmount() {
-  return folly::makeFutureWith([this] {
-    auto mountingUnmountingState = mountingUnmountingState_.wlock();
-    if (mountingUnmountingState->fsChannelUnmountStarted()) {
-      return mountingUnmountingState->fsChannelUnmountPromise->getFuture();
-    }
-    mountingUnmountingState->fsChannelUnmountPromise.emplace();
-    if (!mountingUnmountingState->fsChannelMountStarted()) {
-      return folly::makeFuture();
-    }
-    auto mountFuture =
-        mountingUnmountingState->fsChannelMountPromise->getFuture();
-    mountingUnmountingState.unlock();
+  auto mountingUnmountingState = mountingUnmountingState_.wlock();
+  if (mountingUnmountingState->fsChannelUnmountStarted()) {
+    return mountingUnmountingState->fsChannelUnmountPromise->getFuture();
+  }
+  mountingUnmountingState->fsChannelUnmountPromise.emplace();
+  if (!mountingUnmountingState->fsChannelMountStarted()) {
+    return folly::makeFuture();
+  }
+  auto mountFuture =
+      mountingUnmountingState->fsChannelMountPromise->getFuture();
+  mountingUnmountingState.unlock();
 
-    return std::move(mountFuture)
-        .thenTry([this](Try<Unit>&& mountResult) {
-          if (mountResult.hasException()) {
-            return folly::makeFuture();
-          }
-#ifdef _WIN32
-          if (auto* channel = getPrjfsChannel()) {
-            return channel->stop()
-                .via(getServerThreadPool().get())
-                .ensure([this] { channel_.reset(); });
-          } else {
-            return folly::makeFutureWith([]() { NOT_IMPLEMENTED(); });
-          }
-#else
-          // TODO: teach windows to unmount NFS
-          if (auto* nfsChannel = getNfsdChannel()) {
-            return nfsChannel->unmount();
-          } else if (auto* fuseChannel = getFuseChannel()) {
-            // TODO: Is it safe to call FuseChannel::unmount if the FuseChannel
-            // is in the process of starting? Or can we assume that
-            // mountResult.hasException() above covers that case?
-            return fuseChannel->unmount();
-          } else {
-            throw std::runtime_error(
-                "attempting to unmount() an EdenMount without an FsChannel");
-          }
-#endif
-        })
-        .thenTry([this](Try<Unit>&& result) noexcept -> folly::Future<Unit> {
-          auto mountingUnmountingState = mountingUnmountingState_.wlock();
-          XDCHECK(mountingUnmountingState->fsChannelUnmountPromise.has_value());
-          folly::SharedPromise<folly::Unit>* unsafeUnmountPromise =
-              &*mountingUnmountingState->fsChannelUnmountPromise;
-          mountingUnmountingState.unlock();
+  return std::move(mountFuture)
+      .thenTry([this](Try<Unit>&& mountResult) {
+        if (mountResult.hasException()) {
+          return folly::makeSemiFuture();
+        }
+        if (!channel_) {
+          throw std::runtime_error(
+              "attempting to unmount() an EdenMount without an FsChannel");
+        }
+        // If a Future then callback returns a SemiFuture, that SemiFuture is
+        // attached to the implied InlineExecutor.
+        // Therefore, the the following callback will be guaranteed to be fixup
+        // the mountingUnmountingState, even if the returned SemiFuture is
+        // dropped.
+        // TODO: Is it safe to call FsChannel::unmount if the FuseChannel
+        // is in the process of starting? Or can we assume that
+        // mountResult.hasException() above covers that case?
+        return channel_->unmount();
+      })
+      .thenTry([this](Try<Unit>&& result) noexcept -> folly::Future<Unit> {
+        auto mountingUnmountingState = mountingUnmountingState_.wlock();
+        XDCHECK(mountingUnmountingState->fsChannelUnmountPromise.has_value());
+        folly::SharedPromise<folly::Unit>* unsafeUnmountPromise =
+            &*mountingUnmountingState->fsChannelUnmountPromise;
+        mountingUnmountingState.unlock();
 
-          unsafeUnmountPromise->setTry(Try<Unit>{result});
-          return folly::makeFuture<folly::Unit>(std::move(result));
-        });
-  });
+        unsafeUnmountPromise->setTry(Try<Unit>{result});
+        return folly::makeFuture<folly::Unit>(std::move(result));
+      });
 }
 
 const shared_ptr<UnboundedQueueExecutor>& EdenMount::getServerThreadPool()

eden/fs/inodes/FsChannel.h

@@ -43,21 +43,6 @@ class FsChannel {
   virtual ~FsChannel() = default;
 
  public:
-  /**
-   * Returns a short, human-readable (or at least loggable) name for this
-   * FsChannel type.
-   *
-   * e.g. "fuse", "nfs3", "prjfs"
-   */
-  virtual const char* getName() const = 0;
-
-  /**
-   * Ask this FsChannel to stop for a takeover request.
-   *
-   * Returns true if takeover is supported and a takeover attempt has begun.
-   */
-  virtual bool takeoverStop() = 0;
-
   /**
    * Neither FuseChannel and Nfsd3 can be deleted from arbitrary threads.
    *
@@ -68,6 +53,14 @@ class FsChannel {
    */
   virtual void destroy() = 0;
 
+  /**
+   * Returns a short, human-readable (or at least loggable) name for this
+   * FsChannel type.
+   *
+   * e.g. "fuse", "nfs3", "prjfs"
+   */
+  virtual const char* getName() const = 0;
+
   /**
    * An FsChannel must be initialized after construction. This process begins
    * the handshake with the filesystem driver.
@@ -79,6 +72,18 @@ class FsChannel {
    */
   FOLLY_NODISCARD virtual folly::Future<StopFuture> initialize() = 0;
 
+  /**
+   * Ask this FsChannel to remove itself from the filesystem.
+   */
+  FOLLY_NODISCARD virtual folly::SemiFuture<folly::Unit> unmount() = 0;
+
+  /**
+   * Ask this FsChannel to stop for a takeover request.
+   *
+   * Returns true if takeover is supported and a takeover attempt has begun.
+   */
+  virtual bool takeoverStop() = 0;
+
   /**
    * Returns the ProcessAccessLog used to track this channel's filesystem
    * accesses.

eden/fs/inodes/test/CheckoutTest.cpp

@@ -1849,7 +1849,7 @@ TEST(Checkout, concurrent_crawl_during_checkout) {
   auto result = std::move(fut).get(0ms);
   EXPECT_THAT(result.conflicts, UnorderedElementsAre());
 
-  mount.getEdenMount()->getPrjfsChannel()->stop();
+  mount.getEdenMount()->getPrjfsChannel()->unmount().get();
 }
 
 TEST(Checkout, concurrent_file_to_directory_during_checkout) {
@@ -1900,7 +1900,7 @@ TEST(Checkout, concurrent_file_to_directory_during_checkout) {
       UnorderedElementsAre(
           makeConflict(ConflictType::MODIFIED_REMOVED, "b.txt")));
 
-  mount.getEdenMount()->getPrjfsChannel()->stop();
+  mount.getEdenMount()->getPrjfsChannel()->unmount().get();
 }
 
 TEST(Checkout, concurrent_new_file_during_checkout) {
@@ -1952,7 +1952,7 @@ TEST(Checkout, concurrent_new_file_during_checkout) {
       UnorderedElementsAre(
           makeConflict(ConflictType::UNTRACKED_ADDED, "a/2.txt")));
 
-  mount.getEdenMount()->getPrjfsChannel()->stop();
+  mount.getEdenMount()->getPrjfsChannel()->unmount().get();
 }
 
 TEST(Checkout, concurrent_recreation_during_checkout) {
@@ -2010,7 +2010,7 @@ TEST(Checkout, concurrent_recreation_during_checkout) {
           makeConflict(ConflictType::REMOVED_MODIFIED, "a/1.txt"),
           makeConflict(ConflictType::MODIFIED_MODIFIED, "a/1.txt")));
 
-  mount.getEdenMount()->getPrjfsChannel()->stop();
+  mount.getEdenMount()->getPrjfsChannel()->unmount().get();
 }
 
 #endif

eden/fs/nfs/Nfsd3.cpp

@@ -2061,7 +2061,7 @@ void Nfsd3::initialize(folly::File connectedSocket) {
   server_->initializeConnectedSocket(std::move(connectedSocket));
 }
 
-folly::Future<folly::Unit> Nfsd3::unmount() {
+folly::SemiFuture<folly::Unit> Nfsd3::unmount() {
   return privHelper_->nfsUnmount(mountPath_.view());
 }
 

eden/fs/nfs/Nfsd3.h

@@ -161,7 +161,7 @@ class Nfsd3 final : public FsChannel {
    * shuts down the Nfsd3. The future returned by initialize() will be fulfilled
    * with a non-takeover StopData.
    */
-  FOLLY_NODISCARD folly::Future<folly::Unit> unmount();
+  FOLLY_NODISCARD folly::SemiFuture<folly::Unit> unmount() override;
 
   /**
    * Trigger an invalidation for the given path.

eden/fs/prjfs/PrjfsChannel.cpp

@@ -1458,7 +1458,7 @@ FsChannelInfo PrjfsChannel::StopData::extractTakeoverInfo() {
   return ProjFsChannelData{};
 }
 
-folly::SemiFuture<folly::Unit> PrjfsChannel::stop() {
+folly::SemiFuture<folly::Unit> PrjfsChannel::unmount() {
   XLOG(INFO) << "Stopping PrjfsChannel for: " << mountPath_;
   XCHECK(!stopPromise_.isFulfilled());
 

eden/fs/prjfs/PrjfsChannel.h

@@ -535,11 +535,6 @@ class PrjfsChannel : public FsChannel {
     return "prjfs";
   }
 
-  bool takeoverStop() override {
-    // ProjFS does not support takeover.
-    return false;
-  }
-
   /**
    * Stop the PrjfsChannel.
    *
@@ -549,7 +544,12 @@ class PrjfsChannel : public FsChannel {
    * PrjfsChannel must not be destructed until the returned future is
    * fulfilled.
    */
-  folly::SemiFuture<folly::Unit> stop();
+  folly::SemiFuture<folly::Unit> unmount() override;
+
+  bool takeoverStop() override {
+    // ProjFS does not support takeover.
+    return false;
+  }
 
   struct StopData : FsStopData {
     bool isUnmounted() override;