From adce4eba1e5c7e80b27f74a1fd3b3f6eb866af69 Mon Sep 17 00:00:00 2001 From: Chad Austin Date: Wed, 24 Jan 2018 15:18:45 -0800 Subject: [PATCH] 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 | 12 ++----- eden/fs/inodes/TreeInode.cpp | 47 +++++++++++--------------- eden/fs/inodes/TreeInode.h | 18 +++------- eden/fs/service/EdenServer.cpp | 7 ++-- eden/fs/service/EdenServiceHandler.cpp | 10 +++--- 5 files changed, 38 insertions(+), 56 deletions(-) diff --git a/eden/cli/debug.py b/eden/cli/debug.py index 62df07a554..ea4647221c 100644 --- a/eden/cli/debug.py +++ b/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): diff --git a/eden/fs/inodes/TreeInode.cpp b/eden/fs/inodes/TreeInode.cpp index 834a89d6f6..2114308470 100644 --- a/eden/fs/inodes/TreeInode.cpp +++ b/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(getNow()); - auto timePointRel = now - age; - auto epochTime = timePointRel.time_since_epoch(); - auto sec = std::chrono::duration_cast(epochTime); - auto nsec = - std::chrono::duration_cast(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 toUnload; + std::unordered_set 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 treeChildren; std::vector 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(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; diff --git a/eden/fs/inodes/TreeInode.h b/eden/fs/inodes/TreeInode.h index 6e7eae48fd..10447408e6 100644 --- a/eden/fs/inodes/TreeInode.h +++ b/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 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 contents_; }; } // namespace eden diff --git a/eden/fs/service/EdenServer.cpp b/eden/fs/service/EdenServer.cpp index 4376b31bed..de6b2e7b6b 100644 --- a/eden/fs/service/EdenServer.cpp +++ b/eden/fs/service/EdenServer.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include @@ -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(cutoff); + totalUnloaded += rootInode->unloadChildrenLastAccessedBefore(cutoff_ts); } serviceData->setCounter(kPeriodicUnloadCounterKey, totalUnloaded); } diff --git a/eden/fs/service/EdenServiceHandler.cpp b/eden/fs/service/EdenServiceHandler.cpp index 2aebd7af9c..f18fbc25c1 100644 --- a/eden/fs/service/EdenServiceHandler.cpp +++ b/eden/fs/service/EdenServiceHandler.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -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(cutoff); + return inode->unloadChildrenLastAccessedBefore(cutoff_ts); } void EdenServiceHandler::getStatInfo(InternalStats& result) {