invalidate directory inodes properly on checkout

Summary:
After the kernel added readdir caching, my testing uncovered that Eden
was invalidating TreeInode entries incorrectly when new entries were
added. Change TreeInode to distinguish between directory entry changes
and removals (FUSE_NOTIFY_INVAL_ENTRY) and additions
(FUSE_NOTIFY_INVAL_INODE).

Reviewed By: strager

Differential Revision: D13870422

fbshipit-source-id: 2a6f25bfd9e77436a5aae639fedbfd8a445b2e05

eden/fs/inodes/CheckoutAction.cpp

@@ -111,7 +111,7 @@ class CheckoutAction::LoadingRefcount {
   CheckoutAction* action_;
 };
 
-Future<Unit> CheckoutAction::run(
+Future<InvalidationRequired> CheckoutAction::run(
     CheckoutContext* /* ctx */,
     ObjectStore* store) {
   // Immediately create one LoadingRefcount, to ensure that our
@@ -237,8 +237,9 @@ void CheckoutAction::allLoadsComplete() noexcept {
   }
 
   try {
-    doAction().thenTry(
-        [this](folly::Try<Unit>&& t) { this->promise_.setTry(std::move(t)); });
+    doAction().thenTry([this](folly::Try<InvalidationRequired>&& t) {
+      this->promise_.setTry(std::move(t));
+    });
   } catch (const std::exception& ex) {
     exception_wrapper ew{std::current_exception(), ex};
     promise_.setException(ew);
@@ -283,38 +284,43 @@ bool CheckoutAction::ensureDataReady() noexcept {
   return true;
 }
 
-Future<Unit> CheckoutAction::doAction() {
+Future<InvalidationRequired> CheckoutAction::doAction() {
   // All the data is ready and we're ready to go!
 
   // Check for conflicts first.
-  return hasConflict().thenValue([this](bool conflictWasAddedToCtx) {
-    // Note that even if we know we are not going to apply the changes, we must
-    // still run hasConflict() first because we rely on its side-effects.
-    if (conflictWasAddedToCtx && !ctx_->forceUpdate()) {
-      // We only report conflicts for files, not directories. The only possible
-      // conflict that can occur here if this inode is a TreeInode is that the
-      // old source control state was for a file. There aren't really any other
-      // conflicts than this to report, even if we recurse. Anything inside this
-      // directory is basically just untracked (or possibly ignored) files.
-      return makeFuture();
-    }
+  return hasConflict().thenValue(
+      [this](
+          bool conflictWasAddedToCtx) -> folly::Future<InvalidationRequired> {
+        // Note that even if we know we are not going to apply the changes, we
+        // must still run hasConflict() first because we rely on its
+        // side-effects.
+        if (conflictWasAddedToCtx && !ctx_->forceUpdate()) {
+          // We only report conflicts for files, not directories. The only
+          // possible conflict that can occur here if this inode is a TreeInode
+          // is that the old source control state was for a file. There aren't
+          // really any other conflicts than this to report, even if we recurse.
+          // Anything inside this directory is basically just untracked (or
+          // possibly ignored) files.
+          return InvalidationRequired::No;
+        }
 
-    // Call TreeInode::checkoutUpdateEntry() to actually do the work.
-    //
-    // Note that we are moving most of our state into the checkoutUpdateEntry()
-    // arguments.  We have to be slightly careful here: getEntryName() returns a
-    // PathComponentPiece that is pointing into a PathComponent owned either by
-    // oldScmEntry_ or newScmEntry_.  Therefore don't move these scm entries,
-    // to make sure we don't invalidate the PathComponentPiece data.
-    auto parent = inode_->getParent(ctx_->renameLock());
-    return parent->checkoutUpdateEntry(
-        ctx_,
-        getEntryName(),
-        std::move(inode_),
-        std::move(oldTree_),
-        std::move(newTree_),
-        newScmEntry_);
-  });
+        // Call TreeInode::checkoutUpdateEntry() to actually do the work.
+        //
+        // Note that we are moving most of our state into the
+        // checkoutUpdateEntry() arguments.  We have to be slightly careful
+        // here: getEntryName() returns a PathComponentPiece that is pointing
+        // into a PathComponent owned either by oldScmEntry_ or newScmEntry_.
+        // Therefore don't move these scm entries, to make sure we don't
+        // invalidate the PathComponentPiece data.
+        auto parent = inode_->getParent(ctx_->renameLock());
+        return parent->checkoutUpdateEntry(
+            ctx_,
+            getEntryName(),
+            std::move(inode_),
+            std::move(oldTree_),
+            std::move(newTree_),
+            newScmEntry_);
+      });
 }
 
 Future<bool> CheckoutAction::hasConflict() {

eden/fs/inodes/CheckoutAction.h

@@ -28,6 +28,11 @@ class CheckoutContext;
 class ObjectStore;
 class Tree;
 
+enum class InvalidationRequired : bool {
+  No,
+  Yes,
+};
+
 /**
  * A helper class representing an action that must be taken as part of a
  * checkout operation.
@@ -90,7 +95,15 @@ class CheckoutAction {
 
   PathComponentPiece getEntryName() const;
 
-  FOLLY_NODISCARD folly::Future<folly::Unit> run(
+  /**
+   * Run the CheckoutAction.
+   *
+   * If this completes successfully, the result returned via the Future
+   * indicates if the change updated the parent directory's entries. Returns
+   * whether the caller is responsible for invalidating the directory's inode
+   * cache in the kernel.
+   */
+  FOLLY_NODISCARD folly::Future<InvalidationRequired> run(
       CheckoutContext* ctx,
       ObjectStore* store);
 
@@ -117,7 +130,12 @@ class CheckoutAction {
   void allLoadsComplete() noexcept;
   bool ensureDataReady() noexcept;
   folly::Future<bool> hasConflict();
-  FOLLY_NODISCARD folly::Future<folly::Unit> doAction();
+
+  /**
+   * Return whether the directory's contents have changed and the
+   * inode's readdir cache must be flushed.
+   */
+  FOLLY_NODISCARD folly::Future<InvalidationRequired> doAction();
 
   /**
    * The context for the in-progress checkout operation.
@@ -180,7 +198,7 @@ class CheckoutAction {
   /**
    * The promise that we will fulfil when the CheckoutAction is complete.
    */
-  folly::Promise<folly::Unit> promise_;
+  folly::Promise<InvalidationRequired> promise_;
 };
 } // namespace eden
 } // namespace facebook

eden/fs/inodes/TreeInode.cpp

@@ -903,7 +903,8 @@ FileInodePtr TreeInode::createImpl(
     contents.unlock();
   }
 
-  invalidateFuseCacheIfRequired(name);
+  invalidateFuseEntryCacheIfRequired(name);
+  invalidateFuseInodeCacheIfRequired();
 
   getMount()->getJournal().addDelta(
       std::make_unique<JournalDelta>(targetName, JournalDelta::CREATED));
@@ -1058,7 +1059,8 @@ TreeInodePtr TreeInode::mkdir(PathComponentPiece name, mode_t mode) {
     saveOverlayDir(contents->entries);
   }
 
-  invalidateFuseCacheIfRequired(name);
+  invalidateFuseEntryCacheIfRequired(name);
+  invalidateFuseInodeCacheIfRequired();
   getMount()->getJournal().addDelta(
       std::make_unique<JournalDelta>(targetName, JournalDelta::CREATED));
 
@@ -1238,7 +1240,8 @@ int TreeInode::tryRemoveChild(
   // We have successfully removed the entry.
   // Flush the kernel cache for this entry if requested.
   if (flushKernelCache) {
-    invalidateFuseCache(name);
+    invalidateFuseInodeCache();
+    invalidateFuseEntryCache(name);
   }
 
   return 0;
@@ -1597,6 +1600,15 @@ Future<Unit> TreeInode::doRename(
   locks.reset();
   deletedInode.reset();
 
+  // If the rename occurred outside of a FUSE request (unlikely), make sure to
+  // invalidate the kernel caches.
+  invalidateFuseInodeCacheIfRequired();
+  if (destParent.get() != this) {
+    destParent->invalidateFuseInodeCacheIfRequired();
+  }
+  invalidateFuseEntryCacheIfRequired(srcName);
+  destParent->invalidateFuseEntryCacheIfRequired(destName);
+
   return folly::unit;
 }
 
@@ -1876,7 +1888,7 @@ Future<Unit> TreeInode::diff(
     inode = inodeEntry->getInodePtr();
     if (!inode) {
       inodeFuture = loadChildLocked(
-          contents->entries, kIgnoreFilename, *inodeEntry, &pendingLoads);
+          contents->entries, kIgnoreFilename, *inodeEntry, pendingLoads);
     }
   }
 
@@ -2052,7 +2064,7 @@ Future<Unit> TreeInode::computeDiff(
                     entryIgnored));
           } else {
             auto inodeFuture = self->loadChildLocked(
-                contents->entries, name, *inodeEntry, &pendingLoads);
+                contents->entries, name, *inodeEntry, pendingLoads);
             deferredEntries.emplace_back(
                 DeferredDiffEntry::createUntrackedEntryFromInodeFuture(
                     context,
@@ -2123,7 +2135,7 @@ Future<Unit> TreeInode::computeDiff(
         // This inode is not loaded but is materialized.
         // We'll have to load it to confirm if it is the same or different.
         auto inodeFuture = self->loadChildLocked(
-            contents->entries, scmEntry.getName(), *inodeEntry, &pendingLoads);
+            contents->entries, scmEntry.getName(), *inodeEntry, pendingLoads);
         deferredEntries.emplace_back(
             DeferredDiffEntry::createModifiedEntryFromInodeFuture(
                 context,
@@ -2146,7 +2158,7 @@ Future<Unit> TreeInode::computeDiff(
         // This is a modified directory.  We have to load it then recurse
         // into it to find files with differences.
         auto inodeFuture = self->loadChildLocked(
-            contents->entries, scmEntry.getName(), *inodeEntry, &pendingLoads);
+            contents->entries, scmEntry.getName(), *inodeEntry, pendingLoads);
         deferredEntries.emplace_back(
             DeferredDiffEntry::createModifiedEntryFromInodeFuture(
                 context,
@@ -2292,11 +2304,18 @@ Future<Unit> TreeInode::checkout(
   XLOG(DBG4) << "checkout: starting update of " << getLogPath() << ": "
              << (fromTree ? fromTree->getHash().toString() : "<none>")
              << " --> " << (toTree ? toTree->getHash().toString() : "<none>");
+
   vector<unique_ptr<CheckoutAction>> actions;
   vector<IncompleteInodeLoad> pendingLoads;
+  bool wasDirectoryListModified = false;
 
   computeCheckoutActions(
-      ctx, fromTree.get(), toTree.get(), &actions, &pendingLoads);
+      ctx,
+      fromTree.get(),
+      toTree.get(),
+      actions,
+      pendingLoads,
+      wasDirectoryListModified);
 
   // Wire up the callbacks for any pending inode loads we started
   for (auto& load : pendingLoads) {
@@ -2304,36 +2323,44 @@ Future<Unit> TreeInode::checkout(
   }
 
   // Now start all of the checkout actions
-  vector<Future<Unit>> actionFutures;
+  vector<Future<InvalidationRequired>> actionFutures;
   for (const auto& action : actions) {
     actionFutures.emplace_back(action->run(ctx, getStore()));
   }
   // Wait for all of the actions, and record any errors.
   return folly::collectAllSemiFuture(actionFutures)
       .toUnsafeFuture()
-      .thenValue([ctx,
-                  self = inodePtrFromThis(),
-                  toTree = std::move(toTree),
-                  actions = std::move(actions)](
-                     vector<folly::Try<Unit>> actionResults) {
-        // Record any errors that occurred
-        size_t numErrors = 0;
-        for (size_t n = 0; n < actionResults.size(); ++n) {
-          auto& result = actionResults[n];
-          if (!result.hasException()) {
-            continue;
-          }
-          ++numErrors;
-          ctx->addError(
-              self.get(), actions[n]->getEntryName(), result.exception());
-        }
+      .thenValue(
+          [ctx,
+           self = inodePtrFromThis(),
+           toTree = std::move(toTree),
+           actions = std::move(actions),
+           wasDirectoryListModified](
+              vector<folly::Try<InvalidationRequired>> actionResults) mutable {
+            // Record any errors that occurred
+            size_t numErrors = 0;
+            for (size_t n = 0; n < actionResults.size(); ++n) {
+              auto& result = actionResults[n];
+              if (!result.hasException()) {
+                wasDirectoryListModified |=
+                    (result.value() == InvalidationRequired::Yes);
+                continue;
+              }
+              ++numErrors;
+              ctx->addError(
+                  self.get(), actions[n]->getEntryName(), result.exception());
+            }
 
-        // Update our state in the overlay
-        self->saveOverlayPostCheckout(ctx, toTree.get());
+            if (wasDirectoryListModified) {
+              self->invalidateFuseInodeCache();
+            }
 
-        XLOG(DBG4) << "checkout: finished update of " << self->getLogPath()
-                   << ": " << numErrors << " errors";
-      });
+            // Update our state in the overlay
+            self->saveOverlayPostCheckout(ctx, toTree.get());
+
+            XLOG(DBG4) << "checkout: finished update of " << self->getLogPath()
+                       << ": " << numErrors << " errors";
+          });
 }
 
 bool TreeInode::canShortCircuitCheckout(
@@ -2385,8 +2412,9 @@ void TreeInode::computeCheckoutActions(
     CheckoutContext* ctx,
     const Tree* fromTree,
     const Tree* toTree,
-    vector<unique_ptr<CheckoutAction>>* actions,
-    vector<IncompleteInodeLoad>* pendingLoads) {
+    vector<unique_ptr<CheckoutAction>>& actions,
+    vector<IncompleteInodeLoad>& pendingLoads,
+    bool& wasDirectoryListModified) {
   // Grab the contents_ lock for the duration of this function
   auto contents = contents_.wlock();
 
@@ -2421,20 +2449,40 @@ void TreeInode::computeCheckoutActions(
 
       // This entry is present in the new tree but not the old one.
       action = processCheckoutEntry(
-          ctx, contents->entries, nullptr, &newEntries[newIdx], pendingLoads);
+          ctx,
+          contents->entries,
+          nullptr,
+          &newEntries[newIdx],
+          pendingLoads,
+          wasDirectoryListModified);
       ++newIdx;
     } else if (newIdx >= newEntries.size()) {
       // This entry is present in the old tree but not the old one.
       action = processCheckoutEntry(
-          ctx, contents->entries, &oldEntries[oldIdx], nullptr, pendingLoads);
+          ctx,
+          contents->entries,
+          &oldEntries[oldIdx],
+          nullptr,
+          pendingLoads,
+          wasDirectoryListModified);
       ++oldIdx;
     } else if (oldEntries[oldIdx].getName() < newEntries[newIdx].getName()) {
       action = processCheckoutEntry(
-          ctx, contents->entries, &oldEntries[oldIdx], nullptr, pendingLoads);
+          ctx,
+          contents->entries,
+          &oldEntries[oldIdx],
+          nullptr,
+          pendingLoads,
+          wasDirectoryListModified);
       ++oldIdx;
     } else if (oldEntries[oldIdx].getName() > newEntries[newIdx].getName()) {
       action = processCheckoutEntry(
-          ctx, contents->entries, nullptr, &newEntries[newIdx], pendingLoads);
+          ctx,
+          contents->entries,
+          nullptr,
+          &newEntries[newIdx],
+          pendingLoads,
+          wasDirectoryListModified);
       ++newIdx;
     } else {
       action = processCheckoutEntry(
@@ -2442,13 +2490,14 @@ void TreeInode::computeCheckoutActions(
           contents->entries,
           &oldEntries[oldIdx],
           &newEntries[newIdx],
-          pendingLoads);
+          pendingLoads,
+          wasDirectoryListModified);
       ++oldIdx;
       ++newIdx;
     }
 
     if (action) {
-      actions->push_back(std::move(action));
+      actions.push_back(std::move(action));
     }
   }
 }
@@ -2458,7 +2507,8 @@ unique_ptr<CheckoutAction> TreeInode::processCheckoutEntry(
     DirContents& contents,
     const TreeEntry* oldScmEntry,
     const TreeEntry* newScmEntry,
-    vector<IncompleteInodeLoad>* pendingLoads) {
+    vector<IncompleteInodeLoad>& pendingLoads,
+    bool& wasDirectoryListModified) {
   XLOG(DBG5) << "processCheckoutEntry(" << getLogPath()
              << "): " << (oldScmEntry ? oldScmEntry->toLogString() : "(null)")
              << " -> " << (newScmEntry ? newScmEntry->toLogString() : "(null)");
@@ -2496,7 +2546,6 @@ unique_ptr<CheckoutAction> TreeInode::processCheckoutEntry(
             modeFromTreeEntryType(newScmEntry->getType()),
             getOverlay()->allocateInodeNumber(),
             newScmEntry->getHash());
-        invalidateFuseCache(newScmEntry->getName());
         contentsUpdated = true;
       }
     } else if (!newScmEntry) {
@@ -2518,19 +2567,18 @@ unique_ptr<CheckoutAction> TreeInode::processCheckoutEntry(
             modeFromTreeEntryType(newScmEntry->getType()),
             getOverlay()->allocateInodeNumber(),
             newScmEntry->getHash());
-        invalidateFuseCache(newScmEntry->getName());
         contentsUpdated = true;
       }
     }
 
     if (contentsUpdated) {
-      // Contents have changed and they need to be written out to the overlay.
-      // We should not do that here since this code runs per entry.  Today this
-      // ought to be reconciled in saveOverlayPostCheckout() after this inode
-      // processes all of its checkout actions.
-      // TODO: it is probably worth poking at this code to see if we can find
-      // cases where it does the wrong thing or fails to persist state after
-      // a checkout.
+      // Contents have changed and they need to be written out to the
+      // overlay.  We should not do that here since this code runs per
+      // entry. Today this is reconciled in saveOverlayPostCheckout()
+      // after this inode processes all of its checkout actions. But we
+      // do want to invalidate the kernel's dcache and inode caches.
+      wasDirectoryListModified = true;
+      invalidateFuseEntryCache(name);
     }
 
     // Nothing else to do when there is no local inode.
@@ -2615,6 +2663,8 @@ unique_ptr<CheckoutAction> TreeInode::processCheckoutEntry(
                      newScmEntry->getHash()};
   }
 
+  wasDirectoryListModified = true;
+
   // Contents have changed and the entry is not materialized, but we may have
   // allocated and remembered inode numbers for this tree.  It's much faster to
   // simply forget the inode numbers we allocated here -- if we were a real
@@ -2641,7 +2691,7 @@ unique_ptr<CheckoutAction> TreeInode::processCheckoutEntry(
   return nullptr;
 }
 
-Future<Unit> TreeInode::checkoutUpdateEntry(
+Future<InvalidationRequired> TreeInode::checkoutUpdateEntry(
     CheckoutContext* ctx,
     PathComponentPiece name,
     InodePtr inode,
@@ -2653,7 +2703,7 @@ Future<Unit> TreeInode::checkoutUpdateEntry(
     // If the target of the update is not a directory, then we know we do not
     // need to recurse into it, looking for more conflicts, so we can exit here.
     if (ctx->isDryRun()) {
-      return makeFuture();
+      return InvalidationRequired::No;
     }
 
     {
@@ -2667,13 +2717,13 @@ Future<Unit> TreeInode::checkoutUpdateEntry(
         auto bug = EDEN_BUG()
             << "entry removed while holding rename lock during checkout: "
             << inode->getLogPath();
-        return folly::makeFuture<Unit>(bug.toException());
+        return folly::makeFuture<InvalidationRequired>(bug.toException());
       }
       if (it->second.getInode() != inode.get()) {
         auto bug = EDEN_BUG()
             << "entry changed while holding rename lock during checkout: "
             << inode->getLogPath();
-        return folly::makeFuture<Unit>(bug.toException());
+        return folly::makeFuture<InvalidationRequired>(bug.toException());
       }
 
       // This is a file, so we can simply unlink it, and replace/remove the
@@ -2691,12 +2741,12 @@ Future<Unit> TreeInode::checkoutUpdateEntry(
     }
 
     // Tell FUSE to invalidate its cache for this entry.
-    invalidateFuseCache(name);
+    invalidateFuseEntryCache(name);
 
     // We don't save our own overlay data right now:
     // we'll wait to do that until the checkout operation finishes touching all
     // of our children in checkout().
-    return makeFuture();
+    return InvalidationRequired::Yes;
   }
 
   // If we are going from a directory to a directory, all we need to do
@@ -2706,7 +2756,8 @@ Future<Unit> TreeInode::checkoutUpdateEntry(
 
     CHECK(newScmEntry.has_value());
     CHECK(newScmEntry->isTree());
-    return treeInode->checkout(ctx, std::move(oldTree), std::move(newTree));
+    return treeInode->checkout(ctx, std::move(oldTree), std::move(newTree))
+        .thenValue([](folly::Unit) { return InvalidationRequired::No; });
   }
 
   if (ctx->isDryRun()) {
@@ -2715,7 +2766,7 @@ Future<Unit> TreeInode::checkoutUpdateEntry(
     // investigation to determine whether this is acceptible behavior.
     // Currently, the Hg extension ignores DIRECTORY_NOT_EMPTY conflicts, but
     // that may not be the right thing to do.
-    return makeFuture();
+    return InvalidationRequired::No;
   }
 
   // We need to remove this directory (and possibly replace it with a file).
@@ -2724,69 +2775,97 @@ Future<Unit> TreeInode::checkoutUpdateEntry(
   // exactly what we want.  checkout() will even remove the directory before it
   // returns if the directory is empty.
   return treeInode->checkout(ctx, std::move(oldTree), nullptr)
-      .thenValue([ctx,
-                  name = PathComponent{name},
-                  parentInode = inodePtrFromThis(),
-                  treeInode,
-                  newScmEntry](auto&&) {
-        // Make sure the treeInode was completely removed by the checkout.
-        // If there were still untracked files inside of it, it won't have
-        // been deleted, and we have a conflict that we cannot resolve.
-        if (!treeInode->isUnlinked()) {
-          ctx->addConflict(ConflictType::DIRECTORY_NOT_EMPTY, treeInode.get());
-          return;
-        }
+      .thenValue(
+          [ctx,
+           name = PathComponent{name},
+           parentInode = inodePtrFromThis(),
+           treeInode,
+           newScmEntry](auto &&) -> folly::Future<InvalidationRequired> {
+            // Make sure the treeInode was completely removed by the checkout.
+            // If there were still untracked files inside of it, it won't have
+            // been deleted, and we have a conflict that we cannot resolve.
+            if (!treeInode->isUnlinked()) {
+              ctx->addConflict(
+                  ConflictType::DIRECTORY_NOT_EMPTY, treeInode.get());
+              return InvalidationRequired::No;
+            }
 
-        if (!newScmEntry) {
-          // We're done
-          return;
-        }
+            if (!newScmEntry) {
+              // checkout() will invalidate the parent inode if it removes a
+              // child because it becomes an empty tree, so we don't need to
+              // invalidate here.
+              return InvalidationRequired::No;
+            }
 
-        // Add the new entry
-        bool inserted;
-        {
-          auto contents = parentInode->contents_.wlock();
-          DCHECK(!newScmEntry->isTree());
-          auto ret = contents->entries.emplace(
-              name,
-              modeFromTreeEntryType(newScmEntry->getType()),
-              parentInode->getOverlay()->allocateInodeNumber(),
-              newScmEntry->getHash());
-          inserted = ret.second;
-        }
-        if (inserted) {
-          parentInode->invalidateFuseCache(name);
-        } else {
-          // Hmm.  Someone else already created a new entry in this location
-          // before we had a chance to add our new entry.  We don't block new
-          // file or directory creations during a checkout operation, so this
-          // is possible.  Just report an error in this case.
-          ctx->addError(
-              parentInode.get(),
-              name,
-              InodeError(
-                  EEXIST,
-                  parentInode,
+            // Add the new entry
+            bool inserted;
+            {
+              auto contents = parentInode->contents_.wlock();
+              DCHECK(!newScmEntry->isTree());
+              auto ret = contents->entries.emplace(
                   name,
-                  "new file created with this name while checkout operation "
-                  "was in progress"));
-        }
-      });
+                  modeFromTreeEntryType(newScmEntry->getType()),
+                  parentInode->getOverlay()->allocateInodeNumber(),
+                  newScmEntry->getHash());
+              inserted = ret.second;
+            }
+
+            // This code is running asynchronously during checkout, so
+            // flush the readdir cache right here.
+            parentInode->invalidateFuseInodeCache();
+
+            if (!inserted) {
+              // Hmm.  Someone else already created a new entry in this location
+              // before we had a chance to add our new entry.  We don't block
+              // new file or directory creations during a checkout operation, so
+              // this is possible.  Just report an error in this case.
+              ctx->addError(
+                  parentInode.get(),
+                  name,
+                  InodeError(
+                      EEXIST,
+                      parentInode,
+                      name,
+                      "new file created with this name while checkout operation "
+                      "was in progress"));
+            }
+
+            // Return false because the code above has already invalidated
+            // this inode's readdir cache, so we don't technically need to
+            // do it again unless something else modifies the contents.
+            return InvalidationRequired::No;
+          });
 }
 
-void TreeInode::invalidateFuseCache(PathComponentPiece name) {
-  auto* fuseChannel = getMount()->getFuseChannel();
-  if (fuseChannel) {
-    fuseChannel->invalidateEntry(getNodeId(), name);
+void TreeInode::invalidateFuseInodeCache() {
+  if (auto* fuseChannel = getMount()->getFuseChannel()) {
+    // FUSE_NOTIFY_INVAL_ENTRY is the appropriate invalidation function
+    // when an entry is removed or modified. But when new entries are
+    // added, the inode itself must be invalidated.
+    fuseChannel->invalidateInode(getNodeId(), 0, 0);
   }
 }
 
-void TreeInode::invalidateFuseCacheIfRequired(PathComponentPiece name) {
+void TreeInode::invalidateFuseInodeCacheIfRequired() {
   if (RequestData::isFuseRequest()) {
     // no need to flush the cache if we are inside a FUSE request handler
     return;
   }
-  invalidateFuseCache(name);
+  invalidateFuseInodeCache();
+}
+
+void TreeInode::invalidateFuseEntryCache(PathComponentPiece name) {
+  if (auto* fuseChannel = getMount()->getFuseChannel()) {
+    fuseChannel->invalidateEntry(getNodeId(), name);
+  }
+}
+
+void TreeInode::invalidateFuseEntryCacheIfRequired(PathComponentPiece name) {
+  if (RequestData::isFuseRequest()) {
+    // no need to flush the cache if we are inside a FUSE request handler
+    return;
+  }
+  invalidateFuseEntryCache(name);
 }
 
 void TreeInode::saveOverlayPostCheckout(
@@ -2936,7 +3015,7 @@ folly::Future<InodePtr> TreeInode::loadChildLocked(
     DirContents& /* contents */,
     PathComponentPiece name,
     DirEntry& entry,
-    std::vector<IncompleteInodeLoad>* pendingLoads) {
+    std::vector<IncompleteInodeLoad>& pendingLoads) {
   DCHECK(!entry.getInode());
 
   folly::Promise<InodePtr> promise;
@@ -2946,7 +3025,7 @@ folly::Future<InodePtr> TreeInode::loadChildLocked(
       this, name, childNumber, std::move(promise));
   if (startLoad) {
     auto loadFuture = startLoadingInodeNoThrow(entry, name);
-    pendingLoads->emplace_back(
+    pendingLoads.emplace_back(
         this, std::move(loadFuture), name, entry.getInodeNumber());
   }
 
@@ -3274,7 +3353,7 @@ void TreeInode::prefetch() {
         // tree metadata, so just load all of the children so that lookup()
         // returns cheaply.
         inodeFutures.emplace_back(
-            self->loadChildLocked(contents->entries, name, entry, &pendingLoads)
+            self->loadChildLocked(contents->entries, name, entry, pendingLoads)
                 .unit());
       }
     }

eden/fs/inodes/TreeInode.h

@@ -32,6 +32,7 @@ class RenameLock;
 class Tree;
 class TreeEntry;
 class TreeInodeDebugInfo;
+enum class InvalidationRequired : bool;
 
 constexpr folly::StringPiece kDotEdenName{".eden"};
 
@@ -334,6 +335,9 @@ class TreeInode final : public InodeBaseMetadata<DirContents> {
   /*
    * Update a tree entry as part of a checkout operation.
    *
+   * Returns whether or not the tree's contents were updated and the inode's
+   * readdir cache must be flushed.
+   *
    * This helper function is only to be used by CheckoutAction.
    *
    * @param ctx The CheckoutContext for the current checkout operation.
@@ -355,7 +359,7 @@ class TreeInode final : public InodeBaseMetadata<DirContents> {
    *     This entry will refer to a tree if and only if the newTree parameter
    *     is non-null.
    */
-  FOLLY_NODISCARD folly::Future<folly::Unit> checkoutUpdateEntry(
+  FOLLY_NODISCARD folly::Future<InvalidationRequired> checkoutUpdateEntry(
       CheckoutContext* ctx,
       PathComponentPiece name,
       InodePtr inode,
@@ -553,7 +557,7 @@ class TreeInode final : public InodeBaseMetadata<DirContents> {
       DirContents& dir,
       PathComponentPiece name,
       DirEntry& entry,
-      std::vector<IncompleteInodeLoad>* pendingLoads);
+      std::vector<IncompleteInodeLoad>& pendingLoads);
 
   /**
    * Load the .gitignore file for this directory, then call computeDiff() once
@@ -607,25 +611,52 @@ class TreeInode final : public InodeBaseMetadata<DirContents> {
       CheckoutContext* ctx,
       const Tree* fromTree,
       const Tree* toTree,
-      std::vector<std::unique_ptr<CheckoutAction>>* actions,
-      std::vector<IncompleteInodeLoad>* pendingLoads);
+      std::vector<std::unique_ptr<CheckoutAction>>& actions,
+      std::vector<IncompleteInodeLoad>& pendingLoads,
+      bool& wasDirectoryListModified);
+  /**
+   * Sets wasDirectoryListModified true if this checkout entry operation has
+   * modified the directory contents, which implies the return value is nullptr.
+   *
+   * This function could return a std::variant of InvalidationRequired and
+   * std::unique_ptr<CheckoutAction> instead of setting a boolean.
+   */
   std::unique_ptr<CheckoutAction> processCheckoutEntry(
       CheckoutContext* ctx,
       DirContents& contents,
       const TreeEntry* oldScmEntry,
       const TreeEntry* newScmEntry,
-      std::vector<IncompleteInodeLoad>* pendingLoads);
+      std::vector<IncompleteInodeLoad>& pendingLoads,
+      bool& wasDirectoryListModified);
   void saveOverlayPostCheckout(CheckoutContext* ctx, const Tree* tree);
 
   /**
-   * Send a request to the kernel to invalidate the FUSE cache for the given
-   * child entry name.
+   * Send a request to the kernel to invalidate the pagecache for this inode,
+   * which flushes the readdir cache. This is required when the child entry list
+   * has changed. invalidateFuseEntryCache(name) only works if the entry name is
+   * known to FUSE, which is not true for new entries.
+   */
+  void invalidateFuseInodeCache();
+
+  /**
+   * If running outside of a FUSE request (in which case the kernel already
+   * knows to flush the appropriate caches), call invalidateFuseInodeCache().
+   */
+  void invalidateFuseInodeCacheIfRequired();
+
+  /**
+   * Send a request to the kernel to invalidate the dcache entry for the given
+   * child entry name. The dcache caches name lookups to child inodes.
+   *
+   * This should be called when an entry is added, removed, or changed.
+   * Invalidating upon removal is required because the kernel maintains a
+   * negative cache on lookup failures.
    *
    * This is safe to call while holding the contents_ lock, but it is not
    * required.  Calling it without the contents_ lock held is preferable when
    * possible.
    */
-  void invalidateFuseCache(PathComponentPiece name);
+  void invalidateFuseEntryCache(PathComponentPiece name);
 
   /**
    * Invalidate the kernel FUSE cache for this entry name only if we are not
@@ -634,7 +665,7 @@ class TreeInode final : public InodeBaseMetadata<DirContents> {
    * If we are being invoked because of a FUSE request for this entry we don't
    * need to tell the kernel about the change--it will automatically know.
    */
-  void invalidateFuseCacheIfRequired(PathComponentPiece name);
+  void invalidateFuseEntryCacheIfRequired(PathComponentPiece name);
 
   /**
    * Attempt to remove an empty directory during a checkout operation.

eden/integration/hg/update_test.py

@@ -12,7 +12,7 @@ import os
 import threading
 import unittest
 from textwrap import dedent
-from typing import Dict
+from typing import Dict, List, Set
 
 from eden.integration.hg.lib.hg_extension_test_base import EdenHgTestCase, hg_test
 from eden.integration.lib import hgrepo
@@ -468,3 +468,108 @@ class UpdateTest(EdenHgTestCase):
                 for thread_id in range(num_rename_threads)
             }
         )
+
+
+@hg_test
+class UpdateCacheInvalidationTest(EdenHgTestCase):
+    commit1: str
+
+    def edenfs_logging_settings(self) -> Dict[str, str]:
+        return {
+            "eden.fs.inodes.TreeInode": "DBG5",
+            "eden.fs.inodes.CheckoutAction": "DBG5",
+            "eden.fs.inodes.CheckoutContext": "DBG5",
+            "eden.fs.fuse.FuseChannel": "DBG3",
+        }
+
+    def populate_backing_repo(self, repo: hgrepo.HgRepository) -> None:
+        repo.write_file("dir/file1", "one")
+        repo.write_file("dir/file2", "two")
+        self.commit1 = repo.commit("Initial commit.")
+
+        repo.remove_file("dir/file1")
+        self.commit2 = repo.commit("Remove file1")
+
+        repo.write_file("dir/file3", "three")
+        self.commit3 = repo.commit("Add file3")
+
+        repo.update(self.commit1)
+        repo.write_file("dir/file2", "new two")
+        self.commit4 = repo.commit("Change file2")
+
+    def _populate_kernel_caches(self):
+        # Populate the kernel's readdir caches.
+        for _dirpath, _dirnames, _filenames in os.walk(self.repo.path):
+            pass
+
+    def _list_contents(self, path) -> Set[str]:
+        return set(os.listdir(os.path.join(self.repo.path, path)))
+
+    def _scan_contents(self, path) -> List[os.DirEntry]:
+        entries = list(os.scandir(os.path.join(self.repo.path, path)))
+        entries.sort(key=lambda entry: entry.name)
+        return entries
+
+    def test_update_adding_file_invalidates_tree_inode_caches(self):
+        self.repo.update(self.commit2)
+        self._populate_kernel_caches()
+        self.assertEqual({"file2"}, self._list_contents("dir"))
+
+        # The checkout operation should invalidate the kernel's caches.
+        self.repo.update(self.commit3)
+        self.assertEqual({"file2", "file3"}, self._list_contents("dir"))
+
+    def test_update_removing_file_invalidates_tree_inode_caches(self):
+        self.repo.update(self.commit1)
+        self._populate_kernel_caches()
+
+        self.assertEqual({"file1", "file2"}, self._list_contents("dir"))
+
+        # The checkout operation should invalidate the kernel's caches.
+        self.repo.update(self.commit2)
+        self.assertEqual({"file2"}, self._list_contents("dir"))
+
+    def test_changing_file_contents_creates_new_inode_and_flushes_dcache(self):
+        self.repo.update(self.commit1)
+        self._populate_kernel_caches()
+
+        before = self._scan_contents("dir")
+
+        self.repo.update(self.commit4)
+
+        after = self._scan_contents("dir")
+
+        self.assertEqual(["file1", "file2"], [x.name for x in before])
+        self.assertEqual(["file1", "file2"], [x.name for x in after])
+
+        self.assertEqual(before[0].inode(), after[0].inode())
+        self.assertNotEqual(before[1].inode(), after[1].inode())
+
+    def test_clean_update_removes_added_file(self) -> None:
+        self.repo.update(self.commit1)
+
+        self.write_file("dir/new_file.txt", "new file")
+        self.hg("add", "dir/new_file.txt")
+        self.assertTrue(os.path.isfile(self.get_path("dir/new_file.txt")))
+        self.assert_status({"dir/new_file.txt": "A"})
+
+        self._populate_kernel_caches()
+        self.repo.update(".", clean=True)
+        self.assert_status({"dir/new_file.txt": "?"})
+        self.assertTrue(os.path.isfile(self.get_path("dir/new_file.txt")))
+        self.assert_dirstate_empty()
+
+        self.assertEqual({"file1", "file2", "new_file.txt"}, self._list_contents("dir"))
+
+    def test_clean_update_adds_removed_file(self) -> None:
+        self.hg("remove", "dir/file1")
+        self.assertFalse(os.path.isfile(self.get_path("dir/file1")))
+        self.assert_status({"dir/file1": "R"})
+
+        self._populate_kernel_caches()
+        self.repo.update(".", clean=True)
+        self.assert_status({})
+        self.assertTrue(os.path.isfile(self.get_path("dir/file1")))
+        self.assert_dirstate_empty()
+
+        self.assertEqual({"file1", "file2"}, self._list_contents("dir"))