mirror of
https://github.com/facebook/sapling.git
synced 2024-10-07 07:17:55 +03:00
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
This commit is contained in:
parent
7c5c5e75d0
commit
cc1ede2ffa
@ -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 <folly/logging/xlog.h>
|
||||
#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
|
@ -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
|
||||
|
@ -105,14 +105,6 @@ class FileInode::LockedState {
|
||||
FileInodePtr inode,
|
||||
std::shared_ptr<EdenFileHandle>* 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<State>::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<EdenFileHandle> FileInode::LockedState::unlockAndCreateHandle(
|
||||
}
|
||||
|
||||
void FileInode::LockedState::createHandleInOuterScope(
|
||||
FileInodePtr inode,
|
||||
FileInodePtr /*inode*/,
|
||||
std::shared_ptr<EdenFileHandle>* outHandle) {
|
||||
if (!hasOpenRefcount_) {
|
||||
ptr_->incOpenCount();
|
||||
hasOpenRefcount_ = true;
|
||||
}
|
||||
|
||||
ptr_->checkInvariants();
|
||||
*outHandle =
|
||||
std::make_shared<EdenFileHandle>(std::move(inode), &hasOpenRefcount_);
|
||||
}
|
||||
|
||||
void FileInode::LockedState::incOpenCount() {
|
||||
CHECK(!hasOpenRefcount_);
|
||||
ptr_->incOpenCount();
|
||||
hasOpenRefcount_ = true;
|
||||
*outHandle = std::make_shared<EdenFileHandle>();
|
||||
}
|
||||
|
||||
std::shared_ptr<const Blob> FileInode::LockedState::getCachedBlob(
|
||||
@ -234,22 +188,7 @@ std::shared_ptr<const Blob> 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>(fn)(std::move(state), nullptr); });
|
||||
}
|
||||
@ -366,8 +304,6 @@ typename folly::futures::detail::callableResult<FileInode::LockedState, Fn>::
|
||||
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>(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<FileInodePtr, FileInode::FileHandlePtr> 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<std::string> FileInode::readlink(CacheHint cacheHint) {
|
||||
return readAll(cacheHint);
|
||||
}
|
||||
|
||||
void FileInode::fileHandleDidClose() {
|
||||
auto state = LockedState{this};
|
||||
state->decOpenCount();
|
||||
}
|
||||
|
||||
std::optional<bool> FileInode::isSameAsFast(
|
||||
const Hash& blobID,
|
||||
TreeEntryType entryType) {
|
||||
@ -782,7 +701,6 @@ folly::Future<struct stat> 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<size_t> 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<char*>(data.data());
|
||||
iov.iov_len = data.size();
|
||||
|
@ -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<FileInodeState> {
|
||||
@ -171,8 +155,6 @@ class FileInode final : public InodeBaseMetadata<FileInodeState> {
|
||||
|
||||
/**
|
||||
* 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<FileInodeState> {
|
||||
* 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<BufVec> read(size_t size, off_t off);
|
||||
|
||||
@ -297,7 +276,6 @@ class FileInode final : public InodeBaseMetadata<FileInodeState> {
|
||||
* 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<FileInodeState> {
|
||||
*
|
||||
* 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<FileInodeState> {
|
||||
*
|
||||
* 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<const Blob> blob);
|
||||
|
||||
@ -395,11 +364,6 @@ class FileInode final : public InodeBaseMetadata<FileInodeState> {
|
||||
*/
|
||||
void materializeInParent();
|
||||
|
||||
/**
|
||||
* Called as part of shutting down an open handle.
|
||||
*/
|
||||
void fileHandleDidClose();
|
||||
|
||||
/**
|
||||
* Helper function for isSameAs().
|
||||
*
|
||||
|
@ -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.
|
||||
|
Loading…
Reference in New Issue
Block a user