fix determining which inodes can be unloaded

Summary:
I'm not sure what was wrong with the old code, but I
simplified and clarified all of the time math and now `eden debug
unload` behaves as I'd expect it should.

Reviewed By: simpkins

Differential Revision: D6764962

fbshipit-source-id: 3ed359d4ab4652e95d1538a0982c24185999351c

eden/cli/debug.py

@@ -455,21 +455,13 @@ def do_unload_inodes(args: argparse.Namespace):
     mount, rel_path = get_mount_path(args.path)
 
     with config.get_thrift_client() as client:
-        inodeInfo_before_unload = client.debugInodeStatus(mount, rel_path)
-        inodeCount_before_unload = get_loaded_inode_count(
-            inodeInfo_before_unload)
-
         # set the age in nanoSeconds
         age = TimeSpec()
         age.seconds = int(args.age)
         age.nanoSeconds = int((args.age - age.seconds) * 10**9)
-        client.unloadInodeForPath(mount, rel_path, age)
+        count = client.unloadInodeForPath(mount, rel_path, age)
 
-        inodeInfo_after_unload = client.debugInodeStatus(mount, rel_path)
-        inodeCount_after_unload = get_loaded_inode_count(inodeInfo_after_unload)
-        count = inodeCount_before_unload - inodeCount_after_unload
-        print('Unloaded %s Inodes under the directory : %s' %
-              (count, args.path))
+        print(f'Unloaded {count} inodes under {mount}/{rel_path}')
 
 
 def do_flush_cache(args: argparse.Namespace):

eden/fs/inodes/TreeInode.cpp

@@ -2862,21 +2862,7 @@ void TreeInode::unloadChildrenNow() {
   // all of our children trees, which may result in them being destroyed.
 }
 
-uint64_t TreeInode::unloadChildrenNow(std::chrono::nanoseconds age) {
-  // Get the timepoint and convert to timespec.
-  auto now = folly::to<std::chrono::system_clock::time_point>(getNow());
-  auto timePointRel = now - age;
-  auto epochTime = timePointRel.time_since_epoch();
-  auto sec = std::chrono::duration_cast<std::chrono::seconds>(epochTime);
-  auto nsec =
-      std::chrono::duration_cast<std::chrono::nanoseconds>(epochTime - sec);
-  timespec timePoint;
-  timePoint.tv_sec = sec.count();
-  timePoint.tv_nsec = nsec.count();
-  return unloadChildrenNow(timePoint);
-}
-
-uint64_t TreeInode::unloadChildrenNow(const timespec& timePointAge) {
+uint64_t TreeInode::unloadChildrenLastAccessedBefore(const timespec& cutoff) {
   // Unloading inodes from the InodeMap requires to lock contents_ of TreeInode.
   // Getting atime of an inode requires a lock on the inode.
 
@@ -2908,12 +2894,18 @@ uint64_t TreeInode::unloadChildrenNow(const timespec& timePointAge) {
   // We are intentionally making toUnload as a list of FileInode* rather than
   // FileInodePtr to make raw pointer comparision to check if the pointer in
   // toUnload exists in contents_->entries.
-  std::unordered_set<FileInode*> toUnload;
+  std::unordered_set<InodeBase*> toUnload;
   {
     for (const auto& inode : potentialUnload) {
-      auto timeStamps = inode->getTimestamps();
-      auto atime = timeStamps.atime;
-      if (atime >= timePointAge) {
+      // Is atime the right thing to check here?  If a read is served from
+      // the kernel's cache, the cached atime is updated, but FUSE does not
+      // tell us.  That said, if we update atime whenever FUSE forwards a
+      // read request on to Eden, then atime ought to be a suitable proxy
+      // for whether it's a good idea to unload the inode or not.
+      //
+      // https://sourceforge.net/p/fuse/mailman/message/34448996/
+      auto atime = inode->getTimestamps().atime;
+      if (atime < cutoff) {
         toUnload.insert(inode.get());
       }
     }
@@ -2928,7 +2920,6 @@ uint64_t TreeInode::unloadChildrenNow(const timespec& timePointAge) {
   // than the required age.
   std::vector<TreeInodePtr> treeChildren;
   std::vector<InodeBase*> toDelete;
-  uint64_t unloadCount = 0;
   {
     auto* inodeMap = getInodeMap();
     auto contents = contents_.wlock();
@@ -2944,17 +2935,15 @@ uint64_t TreeInode::unloadChildrenNow(const timespec& timePointAge) {
       } else {
         // Check if the entry is present in the toUnload list(atime greater than
         // age)
-        auto asFile = dynamic_cast<FileInode*>(entry.second->getInode());
-        if (toUnload.find(asFile) != toUnload.end() &&
-            entry.second->getInode()->isPtrAcquireCountZero()) {
+        auto entryInode = entry.second->getInode();
+        if (toUnload.count(entryInode) && entryInode->isPtrAcquireCountZero()) {
           // Unload the inode
           inodeMap->unloadInode(
-              entry.second->getInode(), this, entry.first, false, inodeMapLock);
+              entryInode, this, entry.first, false, inodeMapLock);
           // Record that we should now delete this inode after releasing
           // the locks.
-          toDelete.push_back(entry.second->getInode());
+          toDelete.push_back(entryInode);
           entry.second->clearInode();
-          unloadCount++;
         }
       }
     }
@@ -2963,10 +2952,14 @@ uint64_t TreeInode::unloadChildrenNow(const timespec& timePointAge) {
   for (auto* child : toDelete) {
     delete child;
   }
+
+  // Note that unloadCount only includes released FileInodes at the moment.
+  // It should include TreeInodes too.
+  uint64_t unloadCount = toDelete.size();
   for (auto& child : treeChildren) {
     // TODO(T21096505): Currently unloadChildrenNow is now unloading TreeInodes,
     // we have to make sure that even treeChildren also gets unloaded.
-    unloadCount += child->unloadChildrenNow(timePointAge);
+    unloadCount += child->unloadChildrenLastAccessedBefore(cutoff);
   }
 
   return unloadCount;

eden/fs/inodes/TreeInode.h

@@ -485,13 +485,12 @@ class TreeInode : public InodeBase {
   void unloadChildrenNow();
 
   /**
-   * unloadChildrenNow function is used to unload inodes from a directory based
-   * on the age of the inode.
-   * Age of the inode is determined by the last access time of the inode, if
-   * time since last access time is greater than the 'age' parameter, we will
-   * unload the inode.
+   * Unload all unreferenced inodes under this tree whose last access time is
+   * older than the specified cutoff.
+   *
+   * Returns the number of inodes unloaded.
    */
-  uint64_t unloadChildrenNow(std::chrono::nanoseconds age);
+  uint64_t unloadChildrenLastAccessedBefore(const timespec& cutoff);
 
   /**
    * Load all materialized children underneath this TreeInode.
@@ -769,13 +768,6 @@ class TreeInode : public InodeBase {
   folly::Future<fusell::Dispatcher::Attr> setInodeAttr(
       const fuse_setattr_in& attr) override;
 
-  /**
-   * Internal function used by unloadChildrenNow(std::chrono::nanoseconds age)
-   * which takes struct timespec as parameter. We will unload inodes under a
-   * directory whose atime lies before 'timePointAge'.
-   */
-  uint64_t unloadChildrenNow(const timespec& timePointAge);
-
   folly::Synchronized<Dir> contents_;
 };
 } // namespace eden

eden/fs/service/EdenServer.cpp

@@ -13,6 +13,7 @@
 #include <folly/FileUtil.h>
 #include <folly/SocketAddress.h>
 #include <folly/String.h>
+#include <folly/chrono/Conv.h>
 #include <folly/experimental/logging/xlog.h>
 #include <folly/io/async/AsyncSignalHandler.h>
 #include <gflags/gflags.h>
@@ -229,8 +230,10 @@ void EdenServer::unloadInodes() {
 
     uint64_t totalUnloaded = serviceData->getCounter(kPeriodicUnloadCounterKey);
     for (auto& rootInode : roots) {
-      totalUnloaded += rootInode->unloadChildrenNow(
-          std::chrono::minutes(FLAGS_unload_age_minutes));
+      auto cutoff = std::chrono::system_clock::now() -
+          std::chrono::minutes(FLAGS_unload_age_minutes);
+      auto cutoff_ts = folly::to<timespec>(cutoff);
+      totalUnloaded += rootInode->unloadChildrenLastAccessedBefore(cutoff_ts);
     }
     serviceData->setCounter(kPeriodicUnloadCounterKey, totalUnloaded);
   }

eden/fs/service/EdenServiceHandler.cpp

@@ -12,6 +12,7 @@
 #include <folly/Conv.h>
 #include <folly/FileUtil.h>
 #include <folly/String.h>
+#include <folly/chrono/Conv.h>
 #include <folly/container/Access.h>
 #include <folly/experimental/logging/Logger.h>
 #include <folly/experimental/logging/LoggerDB.h>
@@ -593,10 +594,11 @@ int64_t EdenServiceHandler::unloadInodeForPath(
   } else {
     inode = edenMount->getInode(RelativePathPiece{*path}).get().asTreePtr();
   }
-  // Convert age to std::chrono::nanoseconds.
-  std::chrono::seconds sec(age->seconds);
-  std::chrono::nanoseconds nsec(age->nanoSeconds);
-  return inode->unloadChildrenNow(sec + nsec);
+  auto cutoff = std::chrono::system_clock::now() -
+      std::chrono::seconds(age->seconds) -
+      std::chrono::nanoseconds(age->nanoSeconds);
+  auto cutoff_ts = folly::to<timespec>(cutoff);
+  return inode->unloadChildrenLastAccessedBefore(cutoff_ts);
 }
 
 void EdenServiceHandler::getStatInfo(InternalStats& result) {