immediately save inode numbers when allocating a TreeInode

Summary:
When we start storing permission changes and other inode
metadata keyed on inode number, we will need to remember inode numbers
before they're accessed to handle the case where the Eden process dies
before the inode is unloaded.  Otherwise, data could be left in an
inconsistent state and some writes could be missed.

This change depends on saving (and forgetting) directories being
cheap.

Reviewed By: simpkins

Differential Revision: D7598094

fbshipit-source-id: 9ab127b30a9c09ab62aa08b50c13b3eaa40be60d

eden/fs/inodes/InodeMap.cpp

@@ -822,16 +822,6 @@ Optional<InodeMap::UnloadedInode> InodeMap::updateOverlayForUnload(
     CHECK(treeContentsLock)
         << "TreeInode::Dir lock was held prior to unloadInode!";
 
-    // If the tree is materialized, its state in the overlay should already be
-    // up-to-date.  If the tree is unlinked, it has no children and will never
-    // have children again, so there's no need to write it into the overlay.
-    if (!treeContentsLock->isMaterialized()) {
-      XLOG(DBG5) << "saving non-materialized tree " << asTree->getNodeId()
-                 << " (" << asTree->getLogPath() << ") to overlay";
-      mount_->getOverlay()->saveOverlayDir(
-          inode->getNodeId(), *treeContentsLock);
-    }
-
     // If the fuse refcount is non-zero we have to remember this inode.
     if (fuseCount > 0) {
       XLOG(DBG5) << "unloading tree inode " << inode->getNodeId()

eden/fs/inodes/TreeInode.cpp

@@ -147,10 +147,7 @@ TreeInode::TreeInode(
           ino,
           parent,
           name,
-          buildDirFromTree(
-              tree.get(),
-              parent->getMount()->getLastCheckoutTime(),
-              parent->getInodeMap())) {}
+          saveDirFromTree(ino, tree.get(), parent->getMount())) {}
 
 TreeInode::TreeInode(
     InodeNumber ino,
@@ -162,12 +159,7 @@ TreeInode::TreeInode(
 }
 
 TreeInode::TreeInode(EdenMount* mount, std::shared_ptr<const Tree>&& tree)
-    : TreeInode(
-          mount,
-          buildDirFromTree(
-              tree.get(),
-              mount->getLastCheckoutTime(),
-              mount->getInodeMap())) {}
+    : TreeInode(mount, saveDirFromTree(kRootNodeId, tree.get(), mount)) {}
 
 TreeInode::TreeInode(EdenMount* mount, Dir&& dir)
     : InodeBase(mount), contents_(std::move(dir)) {}
@@ -794,6 +786,17 @@ void TreeInode::saveOverlayDir(InodeNumber inodeNumber, const Dir& contents)
   return getOverlay()->saveOverlayDir(inodeNumber, contents);
 }
 
+TreeInode::Dir TreeInode::saveDirFromTree(
+    InodeNumber inodeNumber,
+    const Tree* tree,
+    EdenMount* mount) {
+  auto dir = buildDirFromTree(
+      tree, mount->getLastCheckoutTime(), mount->getInodeMap());
+  // buildDirFromTree just allocated inode numbers; they should be saved.
+  mount->getOverlay()->saveOverlayDir(inodeNumber, dir);
+  return dir;
+}
+
 TreeInode::Dir TreeInode::buildDirFromTree(
     const Tree* tree,
     const struct timespec& lastCheckoutTime,
@@ -2727,16 +2730,8 @@ void TreeInode::saveOverlayPostCheckout(
                                       : "none")
                << " isMaterialized=" << isMaterialized;
 
-    if (contents->isMaterialized()) {
-      // If we are materialized, write out our state to the overlay.
-      // (It's possible our state is unchanged from what's already on disk,
-      // but for now we can't detect this, and just always write it out.)
-      saveOverlayDir(*contents);
-    } else {
-      // If we are not materialized now, but we were before we'll need to
-      // remove ourself from the overlay.  However, we wait to do this until
-      // later, after we have written out our parent's overlay data.
-    }
+    // Update the overlay to include the new entries, even if dematerialized.
+    saveOverlayDir(*contents);
   }
 
   if (deleteSelf) {
@@ -2778,13 +2773,6 @@ void TreeInode::saveOverlayPostCheckout(
             ctx->renameLock(), loc.name, tree->getHash());
       }
     }
-
-    // If we were dematerialized, remove our overlay data only after updating
-    // our parent.  This ensures that we always have overlay data on disk when
-    // our parent thinks we do.
-    if (!isMaterialized) {
-      getOverlay()->removeOverlayData(getNodeId());
-    }
   }
 }
 

eden/fs/inodes/TreeInode.h

@@ -634,6 +634,15 @@ class TreeInode : public InodeBase {
    */
   void saveOverlayDir(InodeNumber inodeNumber, const Dir& contents) const;
 
+  /**
+   * Converts a Tree to a Dir and saves it to the Overlay under the given inode
+   * number.
+   */
+  static Dir saveDirFromTree(
+    InodeNumber inodeNumber,
+    const Tree* tree,
+    EdenMount* mount);
+
   /** Translates a Tree object from our store into a Dir object
    * used to track the directory in the inode */
   static Dir buildDirFromTree(

eden/fs/inodes/test/CheckoutTest.cpp

@@ -910,6 +910,65 @@ TEST(Checkout, checkoutUpdatesUnlinkedStatusForLoadedTrees) {
   EXPECT_FALSE(dirContents->isMaterialized());
 }
 
+TEST(Checkout, checkoutRemembersInodeNumbersAfterCheckoutAndTakeover) {
+  auto builder1 = FakeTreeBuilder{};
+  builder1.setFile("dir/sub/file1.txt", "contents1");
+  TestMount testMount{builder1};
+
+  // Prepare a second commit, changing dir/sub.
+  auto builder2 = FakeTreeBuilder{};
+  builder2.setFile("dir/sub/file2.txt", "contents2");
+  builder2.finalize(testMount.getBackingStore(), true);
+  auto commit2 = testMount.getBackingStore()->putCommit("2", builder2);
+  commit2->setReady();
+
+  // Load "dir/sub" on behalf of a FUSE connection.
+  auto subTree = testMount.getEdenMount()
+                     ->getInode(RelativePathPiece{"dir/sub"})
+                     .get(1ms)
+                     .asTreePtr();
+  auto dirInodeNumber = subTree->getParentBuggy()->getNodeId();
+  auto subInodeNumber = subTree->getNodeId();
+  subTree->incFuseRefcount();
+  subTree.reset();
+
+  // Checkout to a revision with a new dir/sub tree.  The old data should be
+  // removed from the overlay.
+  auto checkoutResult =
+      testMount.getEdenMount()->checkout(makeTestHash("2")).get(1ms);
+  EXPECT_EQ(0, checkoutResult.size());
+
+  testMount.remountGracefully();
+
+  // Try to load the same tree by its inode number and verify its parents have
+  // the same inode numbers.
+  subTree = testMount.getEdenMount()
+                ->getInodeMap()
+                ->lookupInode(subInodeNumber)
+                .get(1ms)
+                .asTreePtr();
+  EXPECT_EQ(dirInodeNumber, subTree->getParentBuggy()->getNodeId());
+  EXPECT_EQ(subInodeNumber, subTree->getNodeId());
+
+  auto subTree2 = testMount.getEdenMount()
+                      ->getInode(RelativePathPiece{"dir/sub"})
+                      .get(1ms)
+                      .asTreePtr();
+  EXPECT_EQ(dirInodeNumber, subTree2->getParentBuggy()->getNodeId());
+  EXPECT_EQ(subInodeNumber, subTree2->getNodeId());
+
+  testMount.getEdenMount()->getInodeMap()->decFuseRefcount(subInodeNumber);
+  subTree.reset();
+  subTree2.reset();
+
+  subTree = testMount.getEdenMount()
+                ->getInode(RelativePathPiece{"dir/sub"})
+                .get(1ms)
+                .asTreePtr();
+  EXPECT_EQ(dirInodeNumber, subTree->getParentBuggy()->getNodeId());
+  EXPECT_EQ(subInodeNumber, subTree->getNodeId());
+}
+
 // TODO:
 // - remove subdirectory
 //   - with no untracked/ignored files, it should get removed entirely