From cc1ede2ffabc19289c57fc51d3a649c24a1a8ad9 Mon Sep 17 00:00:00 2001 From: Chad Austin Date: Wed, 12 Dec 2018 16:48:09 -0800 Subject: [PATCH] stop counting open FileInode handles Summary: Now that FileInode read and write operations are stateless via BlobAccess and OverlayFileAccess, EdenFileHandle no longer provides any value. Remove it. This also fixes eden's shutdown timeout when a file handle is open and paves the way for FUSE_NO_OPEN_SUPPORT. Reviewed By: strager Differential Revision: D13325137 fbshipit-source-id: 71ed47a7c997f5035b4394ccb311f94332ecd8c2 --- eden/fs/inodes/EdenFileHandle.cpp | 26 --------- eden/fs/inodes/EdenFileHandle.h | 42 --------------- eden/fs/inodes/FileInode.cpp | 87 +------------------------------ eden/fs/inodes/FileInode.h | 38 +------------- eden/integration/mount_test.py | 1 - 5 files changed, 3 insertions(+), 191 deletions(-) delete mode 100644 eden/fs/inodes/EdenFileHandle.cpp diff --git a/eden/fs/inodes/EdenFileHandle.cpp b/eden/fs/inodes/EdenFileHandle.cpp deleted file mode 100644 index fabd4c51c1..0000000000 --- a/eden/fs/inodes/EdenFileHandle.cpp +++ /dev/null @@ -1,26 +0,0 @@ -/* - * Copyright (c) 2016-present, Facebook, Inc. - * All rights reserved. - * - * This source code is licensed under the BSD-style license found in the - * LICENSE file in the root directory of this source tree. An additional grant - * of patent rights can be found in the PATENTS file in the same directory. - * - */ -#include "eden/fs/inodes/EdenFileHandle.h" - -#include -#include "eden/fs/inodes/EdenMount.h" -#include "eden/fs/inodes/FileInode.h" -#include "eden/fs/inodes/TreeInode.h" -#include "eden/fs/store/LocalStore.h" - -namespace facebook { -namespace eden { - -EdenFileHandle::~EdenFileHandle() { - inode_->fileHandleDidClose(); -} - -} // namespace eden -} // namespace facebook diff --git a/eden/fs/inodes/EdenFileHandle.h b/eden/fs/inodes/EdenFileHandle.h index f536dc60bc..c1c75d03a9 100644 --- a/eden/fs/inodes/EdenFileHandle.h +++ b/eden/fs/inodes/EdenFileHandle.h @@ -9,53 +9,11 @@ */ #pragma once #include "eden/fs/fuse/FileHandle.h" -#include "eden/fs/inodes/InodePtr.h" namespace facebook { namespace eden { -class Blob; -class FileInode; -class LocalStore; - class EdenFileHandle : public FileHandle { - public: - /** - * Construct an EdenFileHandle object. - * - * This should only be called by FileInode. The caller is responsible for - * acquiring an open refcount on the FileInode before constructing an - * EdenFileHandle object. The EdenFileHandle destructor will decrement the - * FileInode's open refcount. - * - * *callerHasRefcount will be set to false by this constructor. This makes - * it easier for the caller to correctly decide if they still own the - * refcount or not in case allocating the EdenFileHandle object throws an - * exception. - */ - explicit EdenFileHandle(FileInodePtr inode, bool* callerHasRefcount) - : inode_(std::move(inode)) { - *callerHasRefcount = false; - } - - /** - * EdenFileHandle destructor. - * - * This calls fileHandleDidClose on the associated inode to decrement its - * open count. Beware that fileHandleDidClose() acquires the FileInode lock, - * so callers must ensure that EdenFileHandle objects can never be destroyed - * while they are already holding an inode lock. - */ - ~EdenFileHandle() override; - - private: - EdenFileHandle() = delete; - EdenFileHandle(const EdenFileHandle&) = delete; - EdenFileHandle(EdenFileHandle&&) = delete; - EdenFileHandle& operator=(const EdenFileHandle&) = delete; - EdenFileHandle& operator=(EdenFileHandle&&) = delete; - - FileInodePtr inode_; }; } // namespace eden diff --git a/eden/fs/inodes/FileInode.cpp b/eden/fs/inodes/FileInode.cpp index 2e75f3f286..acc1c0950c 100644 --- a/eden/fs/inodes/FileInode.cpp +++ b/eden/fs/inodes/FileInode.cpp @@ -105,14 +105,6 @@ class FileInode::LockedState { FileInodePtr inode, std::shared_ptr* outHandle); - /** - * Ensure that state->file is an open File object. - * - * This method may only be called when the state tag is - * MATERIALIZED_IN_OVERLAY. - */ - void ensureFileOpen(const FileInode* inode); - /** * Move the file into the MATERIALIZED_IN_OVERLAY state. * @@ -120,25 +112,6 @@ class FileInode::LockedState { */ void setMaterialized(); - /** - * Increment the state's open count. - * - * This should generally be called when setting the blob or file object in - * the state, to ensure that the blob or file is destroyed when the state - * lock is released if it is not still referenced by an EdenFileHandle - * object. - * - * This reference count will automatically be decremented again when the - * LockedState is destroyed. This can only be called at most once on a - * LockedState object--it is not valid to call incOpenCount() on a - * LockedState that already has a reference count. - */ - void incOpenCount(); - - bool hasOpenCount() const { - return hasOpenRefcount_; - } - /** * If this inode still has access to a cached blob, return it. * @@ -150,24 +123,17 @@ class FileInode::LockedState { private: folly::Synchronized::LockedPtr ptr_; - bool hasOpenRefcount_{false}; }; FileInode::LockedState::~LockedState() { if (!ptr_) { return; } - if (hasOpenRefcount_) { - ptr_->decOpenCount(); - } // Check the state invariants every time we release the lock ptr_->checkInvariants(); } void FileInode::LockedState::unlock() { - if (hasOpenRefcount_) { - ptr_->decOpenCount(); - } ptr_->checkInvariants(); ptr_.unlock(); } @@ -185,22 +151,10 @@ std::shared_ptr FileInode::LockedState::unlockAndCreateHandle( } void FileInode::LockedState::createHandleInOuterScope( - FileInodePtr inode, + FileInodePtr /*inode*/, std::shared_ptr* outHandle) { - if (!hasOpenRefcount_) { - ptr_->incOpenCount(); - hasOpenRefcount_ = true; - } - ptr_->checkInvariants(); - *outHandle = - std::make_shared(std::move(inode), &hasOpenRefcount_); -} - -void FileInode::LockedState::incOpenCount() { - CHECK(!hasOpenRefcount_); - ptr_->incOpenCount(); - hasOpenRefcount_ = true; + *outHandle = std::make_shared(); } std::shared_ptr FileInode::LockedState::getCachedBlob( @@ -234,22 +188,7 @@ std::shared_ptr FileInode::LockedState::getCachedBlob( return nullptr; } -void FileInode::LockedState::ensureFileOpen(const FileInode* inode) { - DCHECK(ptr_->isMaterialized()) - << "must only be called for materialized files"; - - if (!hasOpenRefcount_) { - ptr_->incOpenCount(); - hasOpenRefcount_ = true; - } -} - void FileInode::LockedState::setMaterialized() { - if (!hasOpenRefcount_) { - ptr_->incOpenCount(); - hasOpenRefcount_ = true; - } - ptr_->hash.reset(); ptr_->tag = State::MATERIALIZED_IN_OVERLAY; @@ -290,7 +229,6 @@ ReturnType FileInode::runWhileDataLoaded( state.unlock(); break; case State::MATERIALIZED_IN_OVERLAY: - state.ensureFileOpen(this); return folly::makeFutureWith( [&] { return std::forward(fn)(std::move(state), nullptr); }); } @@ -366,8 +304,6 @@ typename folly::futures::detail::callableResult:: state.unlock(); break; case State::MATERIALIZED_IN_OVERLAY: - // Open the file, then run the function - state.ensureFileOpen(this); return folly::makeFutureWith( [&] { return std::forward(fn)(LockedState{std::move(state)}); }); } @@ -490,15 +426,6 @@ void FileInodeState::checkInvariants() { XLOG(FATAL) << "Unexpected tag value: " << tag; } -void FileInodeState::incOpenCount() { - ++openCount; -} - -void FileInodeState::decOpenCount() { - DCHECK_GT(openCount, 0); - --openCount; -} - /********************************************************************* * FileInode methods ********************************************************************/ @@ -515,9 +442,6 @@ std::tuple FileInode::create( FileInodePtr::makeNew(ino, parentInode, name, mode, initialTimestamps); auto state = LockedState{inode}; - state.incOpenCount(); - DCHECK_EQ(state->openCount, 1) - << "open count cannot be anything other than 1"; return std::make_tuple(inode, state.unlockAndCreateHandle(inode)); } @@ -618,11 +542,6 @@ folly::Future FileInode::readlink(CacheHint cacheHint) { return readAll(cacheHint); } -void FileInode::fileHandleDidClose() { - auto state = LockedState{this}; - state->decOpenCount(); -} - std::optional FileInode::isSameAsFast( const Hash& blobID, TreeEntryType entryType) { @@ -782,7 +701,6 @@ folly::Future FileInode::stat() { }); case State::MATERIALIZED_IN_OVERLAY: - state.ensureFileOpen(this); st.st_size = getOverlayFileAccess(state)->getFileSize(getNodeId(), *this); updateBlockCount(st); return st; @@ -939,7 +857,6 @@ folly::Future FileInode::write(folly::StringPiece data, off_t off) { // If we are currently materialized we don't need to copy the input data. if (state->tag == State::MATERIALIZED_IN_OVERLAY) { - state.ensureFileOpen(this); struct iovec iov; iov.iov_base = const_cast(data.data()); iov.iov_len = data.size(); diff --git a/eden/fs/inodes/FileInode.h b/eden/fs/inodes/FileInode.h index c3827cd94d..4f2483ca92 100644 --- a/eden/fs/inodes/FileInode.h +++ b/eden/fs/inodes/FileInode.h @@ -46,7 +46,7 @@ class OverlayFileAccess; * - not loading: the blob may be in cache, but it is not currently being * loaded * - loading: fetching data from backing store, but it's not available yet - * - materialized: contents are written into overlay and file handle is open + * - materialized: contents are written into overlay * * Valid state transitions: * - not loading -> loading @@ -81,17 +81,6 @@ struct FileInodeState { return tag == MATERIALIZED_IN_OVERLAY; } - /** - * Decrement the openCount, closing any open overlay file handles if the open - * count is now zero. - */ - void decOpenCount(); - - /** - * Increment the open count. - */ - void incOpenCount(); - Tag tag; /** @@ -127,11 +116,6 @@ struct FileInodeState { * Records the ranges that have been read() when not materialized. */ CoverageSet readByteRanges; - - /** - * Number of open file handles referencing us. - */ - size_t openCount{0}; }; class FileInode final : public InodeBaseMetadata { @@ -171,8 +155,6 @@ class FileInode final : public InodeBaseMetadata { /** * Construct an inode using a freshly created overlay file. - * file must be moved in and must have been created by a call to - * Overlay::openFile. */ FileInode( InodeNumber ino, @@ -259,9 +241,6 @@ class FileInode final : public InodeBaseMetadata { * that the end of the file was reached. * * May throw exceptions on error. - * - * Precondition: openCount > 0. This is held because read is only called by - * FileInode or FileHandle. */ folly::Future read(size_t size, off_t off); @@ -297,7 +276,6 @@ class FileInode final : public InodeBaseMetadata { * Run a function with the FileInode materialized. * * fn(state) will be invoked when state->tag is MATERIALIZED_IN_OVERLAY. - * state->file will also be open. * * Returns a Future with the result of fn(state_.wlock()) */ @@ -347,9 +325,6 @@ class FileInode final : public InodeBaseMetadata { * * This should normally only be invoked by truncateAndRun(). Most callers * should use truncateAndRun() instead of calling this function directly. - * - * The input LockedState object will be updated to hold an open refcount, - * and state->file will be valid when this function returns. */ void materializeAndTruncate(LockedState& state); @@ -360,18 +335,12 @@ class FileInode final : public InodeBaseMetadata { * * This should normally only be invoked by truncateAndRun(). Most callers * should use truncateAndRun() instead of calling this function directly. - * - * The input LockedState object will be updated to hold an open refcount, - * and state->file will be valid when this function returns. */ void truncateInOverlay(LockedState& state); /** * Transition from NOT_LOADING to MATERIALIZED_IN_OVERLAY by copying the * blob into the overlay. - * - * The input LockedState object will be updated to hold an open refcount, - * and state->file will be valid when this function returns. */ void materializeNow(LockedState& state, std::shared_ptr blob); @@ -395,11 +364,6 @@ class FileInode final : public InodeBaseMetadata { */ void materializeInParent(); - /** - * Called as part of shutting down an open handle. - */ - void fileHandleDidClose(); - /** * Helper function for isSameAs(). * diff --git a/eden/integration/mount_test.py b/eden/integration/mount_test.py index 9a77a6e5be..9ca6e97d5a 100644 --- a/eden/integration/mount_test.py +++ b/eden/integration/mount_test.py @@ -93,7 +93,6 @@ class MountTest(testcase.EdenRepoTest): entries = sorted(os.listdir(self.mount)) self.assertEqual([".eden", "adir", "bdir", "hello", "slink"], entries) - @unittest.skip("not passing yet") def test_unmount_succeeds_while_file_handle_is_open(self) -> None: fd = os.open(os.path.join(self.mount, "hello"), os.O_RDWR) # This test will fail or time out if unmounting times out.