Rewrite removeRecursively to fast remove unloaded children if possible (try #3)

Reviewed By: chadaustin

Differential Revision: D37157670

fbshipit-source-id: 3fa90023b22e66fb7b7e3fa3fc3ee6a1dff68fb7

Author: miaoyipu

eden/fs/inodes/DirEntry.cpp

@@ -49,6 +49,15 @@ TreeInodePtr DirEntry::asTreePtrOrNull() const {
   return TreeInodePtr{};
 }
 
+TreeInode* FOLLY_NULLABLE DirEntry::asTreeOrNull() const {
+  if (hasInodePointer_) {
+    if (auto tree = dynamic_cast<TreeInode*>(inode_)) {
+      return tree;
+    }
+  }
+  return nullptr;
+}
+
 void DirEntry::setInode(InodeBase* inode) {
   XDCHECK(!hasInodePointer_);
   XDCHECK(inode);

eden/fs/inodes/DirEntry.h

@@ -153,6 +153,12 @@ class DirEntry {
    */
   TreeInodePtr asTreePtrOrNull() const;
 
+  /**
+   * Similar to asTreePtrOrNull() except it returns TreeInode* to avoid
+   * dereferring TreeInodePtr that could potentially deadlock.
+   */
+  TreeInode* FOLLY_NULLABLE asTreeOrNull() const;
+
   /**
    * Associates a loaded inode pointer with this entry. Does not take ownership.
    */

eden/fs/inodes/EdenMount.cpp

@@ -668,7 +668,7 @@ ImmediateFuture<SetPathObjectIdResultAndTimes> EdenMount::setPathObjectId(
    */
   auto oldParent = getWorkingCopyParent();
   XLOG(DBG3) << "adding " << rootId << " to Eden mount " << this->getPath()
-             << " at path" << path << " on top of " << oldParent;
+             << " at path " << path << " on top of " << oldParent;
 
   auto ctx = std::make_shared<CheckoutContext>(
       this,

eden/fs/inodes/Overlay.cpp

@@ -622,6 +622,10 @@ void Overlay::removeChild(
   }
 }
 
+void Overlay::removeChildren(InodeNumber parent, const DirContents& content) {
+  saveOverlayDir(parent, content);
+}
+
 void Overlay::renameChild(
     InodeNumber src,
     InodeNumber dst,

eden/fs/inodes/Overlay.h

@@ -242,6 +242,7 @@ class Overlay : public std::enable_shared_from_this<Overlay> {
       InodeNumber parent,
       PathComponentPiece childName,
       const DirContents& content);
+  void removeChildren(InodeNumber parent, const DirContents& content);
 
   void renameChild(
       InodeNumber src,

eden/fs/inodes/PrjfsDispatcherImpl.cpp

@@ -372,22 +372,20 @@ ImmediateFuture<folly::Unit> handleNotPresentFileNotification(
         return createDirInode(mount, dirname.copy(), context)
             .thenValue([basename = basename.copy(),
                         &context](const TreeInodePtr treeInode) {
-              return treeInode
-                  ->removeRecursively(
-                      basename, InvalidationRequired::No, context)
-                  .thenTry([](folly::Try<folly::Unit> try_) {
-                    if (auto* exc =
-                            try_.tryGetExceptionObject<std::system_error>()) {
-                      if (isEnoent(*exc)) {
-                        // ProjectedFS can sometimes send multiple deletion
-                        // notification for the same file, in which case a
-                        // previous deletion will have removed the file already.
-                        // We can safely ignore the error here.
-                        return folly::Try{folly::unit};
-                      }
-                    }
-                    return try_;
-                  });
+              return treeInode->removeRecursively(
+                  basename, InvalidationRequired::No, context);
+            })
+            .thenTry([](folly::Try<folly::Unit> try_) {
+              if (auto* exc = try_.tryGetExceptionObject<std::system_error>()) {
+                if (isEnoent(*exc)) {
+                  // ProjectedFS can sometimes send multiple deletion
+                  // notification for the same file, in which case a
+                  // previous deletion will have removed the file already.
+                  // We can safely ignore the error here.
+                  return folly::Try{folly::unit};
+                }
+              }
+              return try_;
             });
       })
       .ensure([path = std::move(path)] {});

eden/fs/inodes/TreeInode.cpp

@@ -1207,47 +1207,158 @@ ImmediateFuture<folly::Unit> TreeInode::rmdir(
       });
 }
 
-ImmediateFuture<folly::Unit> TreeInode::removeRecursively(
+void TreeInode::removeAllChildrenRecursively(
+    InvalidationRequired invalidate,
+    ObjectFetchContext& context,
+    const RenameLock& renameLock) {
+  // TODO: Unconditional materialization is slightly conservative. If the
+  // BackingStore Tree is empty, then this function can return without
+  // materializing.
+  materialize(&renameLock);
+#ifndef _WIN32
+  if (getNodeId() == getMount()->getDotEdenInodeNumber()) {
+    throw InodeError(EPERM, inodePtrFromThis());
+  }
+#endif
+
+  std::vector<TreeInodePtr> loadedTreeNodes;
+  // Step 1, collect children nodes who are tree and loaded
+  {
+    auto contents = contents_.rlock();
+    for (auto& entry : contents->entries) {
+      if (auto asTreePtr = entry.second.asTreePtrOrNull()) {
+        loadedTreeNodes.push_back(std::move(asTreePtr));
+      }
+    }
+  }
+
+  // Step 2, Clear contents in the child folders
+  for (auto& treeNode : loadedTreeNodes) {
+    treeNode->removeAllChildrenRecursively(invalidate, context, renameLock);
+  }
+
+  loadedTreeNodes.clear();
+
+  // Step 3, Now all child nodes are removable, unless one of the directories
+  // had a new entry added while the contents lock was not held.
+  auto contents = contents_.wlock();
+  auto it = contents->entries.begin();
+  while (it != contents->entries.end()) {
+    auto inodeNum = it->second.getInodeNumber();
+    bool isDir = it->second.isDirectory();
+    if (it->second.getInode()) {
+      // If a treeInode is not empty, i.e. files were added to the tree
+      // between step2 and step3, an exception will be thrown.
+
+      // TODO: There's a race here: checkPreRemove acquires the child's
+      // contents lock but then releases it after the check. Thus, there's a
+      // window where the child can gain an entry being unlinked, which breaks
+      // EdenFS's internal data model. This code should acquire the child's
+      // contents lock and hold it across the unlink operation.
+      //
+      // TODO: Have checkPreRemove take a DirContents& to ensure the contents
+      // lock is acquired by the parent, and encourage holding it across the
+      // unlink operation.
+      //
+      // Be careful, here we obtains TreeInode* instead of TreeIndePtr to avoid
+      // deference of TreeInodePtr, otherwise there could be a deadlock on
+      // getting the parent location info when deference.
+      if (TreeInode* asTreePtr = it->second.asTreeOrNull()) {
+        int checkResult = checkPreRemove(*asTreePtr);
+        if (checkResult != 0) {
+          throw InodeError(checkResult, InodePtr::newPtrLocked(asTreePtr));
+        }
+      }
+
+      auto inode = it->second.getInode();
+      inode->markUnlinked(this, it->first, renameLock);
+    }
+    // Erase from contents must happen right after markUnlink
+    it = contents->entries.erase(it);
+
+    if (isDir) {
+      getOverlay()->recursivelyRemoveOverlayData(inodeNum);
+    } else {
+      getOverlay()->removeOverlayData(inodeNum);
+    }
+  }
+
+  if (InvalidationRequired::Yes == invalidate) {
+    invalidateChannelDirCache(*contents).get();
+  }
+  updateMtimeAndCtimeLocked(contents->entries, getNow());
+  getOverlay()->removeChildren(getNodeId(), contents->entries);
+}
+
+InodePtr TreeInode::tryRemoveUnloadedChild(
+    PathComponentPiece name,
+    InvalidationRequired invalidate) {
+#ifndef _WIN32
+  if (getNodeId() == getMount()->getDotEdenInodeNumber()) {
+    throw InodeError(EPERM, inodePtrFromThis());
+  }
+#endif
+  auto contents = contents_.wlock();
+
+  auto it = contents->entries.find(name);
+  if (it == contents->entries.end()) {
+    throw InodeError(ENOENT, inodePtrFromThis(), name);
+  }
+
+  auto inodeName = copyCanonicalInodeName(it);
+  auto inodeNumber = it->second.getInodeNumber();
+
+  if (auto node = it->second.getInodePtr()) {
+    // The child has a loaded! Fall back to the slow path.
+    return node;
+  }
+
+  contents->entries.erase(it);
+  if (InvalidationRequired::Yes == invalidate) {
+    invalidateChannelEntryCache(*contents, inodeName, inodeNumber)
+        .throwUnlessValue();
+    invalidateChannelDirCache(*contents).get();
+  }
+
+  updateMtimeAndCtimeLocked(contents->entries, getNow());
+  if (it->second.isDirectory()) {
+    getOverlay()->recursivelyRemoveOverlayData(inodeNumber);
+  } else {
+    getOverlay()->removeOverlayData(inodeNumber);
+  }
+  getOverlay()->removeChild(getNodeId(), name, contents->entries);
+  return nullptr;
+}
+
+ImmediateFuture<folly::Unit> TreeInode::removeRecursivelyNoFlushInvalidation(
     PathComponentPiece name,
     InvalidationRequired invalidate,
     ObjectFetchContext& context) {
-  return getOrLoadChild(name, context)
-      .thenValue([self = inodePtrFromThis(),
-                  name = name.copy(),
-                  invalidate,
-                  &context](InodePtr child) mutable {
-        auto asFileInode = child.asSubclassPtrOrNull<FileInodePtr>();
-        if (asFileInode) {
-          return self->removeImpl<FileInodePtr>(
-              std::move(name), std::move(child), invalidate, 1, context);
-        } else {
-          auto tree = child.asTreePtr();
+  // Fast return if the node is unloaded and removed
+  auto child = tryRemoveUnloadedChild(name, invalidate);
+  if (!child) {
+    return folly::unit;
+  }
 
-          std::vector<PathComponent> names;
-          {
-            auto contents = tree->contents_.rlock();
-            for (const auto& entry : contents->entries) {
-              names.emplace_back(entry.first);
-            }
-          }
+  if (child.asFilePtrOrNull()) {
+    return inodePtrFromThis()->removeImpl<FileInodePtr>(
+        PathComponent{name}, std::move(child), invalidate, 1, context);
+  } else {
+    {
+      auto renameLock = inodePtrFromThis()->getMount()->acquireRenameLock();
+      child.asTreePtr()->removeAllChildrenRecursively(
+          invalidate, context, renameLock);
+    }
+    return inodePtrFromThis()->removeImpl<TreeInodePtr>(
+        PathComponent{name}, std::move(child), invalidate, 1, context);
+  }
+}
 
-          std::vector<ImmediateFuture<folly::Unit>> childRemovalFutures;
-          childRemovalFutures.reserve(names.size());
-          for (const auto& name : names) {
-            childRemovalFutures.push_back(
-                tree->removeRecursively(name, invalidate, context));
-          }
-          return collectAllSafe(std::move(childRemovalFutures))
-              .thenValue([self,
-                          name = std::move(name),
-                          invalidate,
-                          child = std::move(child),
-                          &context](std::vector<folly::Unit>&&) mutable {
-                return self->removeImpl<TreeInodePtr>(
-                    std::move(name), std::move(child), invalidate, 1, context);
-              });
-        }
-      })
+ImmediateFuture<folly::Unit> TreeInode::removeRecursively(
+    PathComponentPiece name,
+    InvalidationRequired invalidate,
+    ObjectFetchContext& context) {
+  return this->removeRecursivelyNoFlushInvalidation(name, invalidate, context)
       .thenValue([self = inodePtrFromThis(), invalidate](folly::Unit&&) {
         if (invalidate == InvalidationRequired::Yes) {
           return self->getMount()->flushInvalidations();
@@ -1271,7 +1382,7 @@ ImmediateFuture<folly::Unit> TreeInode::removeImpl(
   }
 
   // Verify that we can remove the child before we materialize ourself
-  int checkResult = checkPreRemove(child);
+  int checkResult = checkPreRemove(*child);
   if (checkResult != 0) {
     return ImmediateFuture<Unit>{
         folly::Try<Unit>{InodeError{checkResult, child}}};
@@ -1398,7 +1509,7 @@ int TreeInode::tryRemoveChild(
     }
 
     // Verify that the child is still in a good state to remove
-    auto checkError = checkPreRemove(child);
+    auto checkError = checkPreRemove(*child);
     if (checkError != 0) {
       return checkError;
     }
@@ -1436,16 +1547,16 @@ int TreeInode::tryRemoveChild(
   return 0;
 }
 
-int TreeInode::checkPreRemove(const TreeInodePtr& child) {
+int TreeInode::checkPreRemove(const TreeInode& child) {
   // Lock the child contents, and make sure they are empty
-  auto childContents = child->contents_.rlock();
+  auto childContents = child.contents_.rlock();
   if (!childContents->entries.empty()) {
     return ENOTEMPTY;
   }
   return 0;
 }
 
-int TreeInode::checkPreRemove(const FileInodePtr& /* child */) {
+int TreeInode::checkPreRemove(const FileInode& /* child */) {
   // Nothing to do
   return 0;
 }

eden/fs/inodes/TreeInode.h

@@ -15,6 +15,7 @@
 #include "eden/fs/inodes/DirEntry.h"
 #include "eden/fs/inodes/InodeBase.h"
 #include "eden/fs/inodes/InodeOrTreeOrEntry.h"
+#include "eden/fs/utils/PathFuncs.h"
 
 namespace facebook::eden {
 
@@ -239,6 +240,37 @@ class TreeInode final : public InodeBaseMetadata<DirContents> {
       InvalidationRequired invalidate,
       ObjectFetchContext& context);
 
+  /**
+   * Internal method intended for removeRecursively to use. This method does not
+   * flush invalidation so the caller won't see the up-to-date content after
+   * return. Call EdenMount::flushInvalidations to ensure any changes to the
+   * inode will be visible after it returns.
+   */
+  ImmediateFuture<folly::Unit> removeRecursivelyNoFlushInvalidation(
+      PathComponentPiece name,
+      InvalidationRequired invalidate,
+      ObjectFetchContext& context);
+
+  /**
+   * Attempts to remove and unlink children of this inode. Under concurrent
+   * modification, it is not guaranteed that TreeInode is empty after this
+   * function returns.
+   */
+  void removeAllChildrenRecursively(
+      InvalidationRequired invalidate,
+      ObjectFetchContext& context,
+      const RenameLock& renameLock);
+
+  /**
+   * For unloaded nodes, the removal should be simpler: remove the node
+   * from entries and update the overlay.
+   * If the return value is valid, the entry was not removed, and the child's
+   * loaded inode was returned.
+   */
+  InodePtr tryRemoveUnloadedChild(
+      PathComponentPiece name,
+      InvalidationRequired invalidate);
+
   /**
    * Create a filesystem node.
    * Only unix domain sockets and regular files are supported; attempting to
@@ -635,8 +667,8 @@ class TreeInode final : public InodeBaseMetadata<DirContents> {
    * checkPreRemove() is called by tryRemoveChild() for file or directory
    * specific checks before unlinking an entry.  Returns an errno value or 0.
    */
-  FOLLY_NODISCARD static int checkPreRemove(const TreeInodePtr& child);
-  FOLLY_NODISCARD static int checkPreRemove(const FileInodePtr& child);
+  FOLLY_NODISCARD static int checkPreRemove(const TreeInode& child);
+  FOLLY_NODISCARD static int checkPreRemove(const FileInode& child);
 
   /**
    * This helper function starts loading a currently unloaded child inode.