From 42b4ffd194c1a5ebeef97182168a0916467afee8 Mon Sep 17 00:00:00 2001 From: Adam Simpkins Date: Wed, 6 Mar 2019 20:30:45 -0800 Subject: [PATCH] add an Overlay::initialize() function Summary: Add an Overlay::initialize() function, and consolidate all Overlay initializtion logic in this function. Previously some initialization was done by the Overlay constructor, and some was done in `EdenMount::initialize()` Overlay::initialize() returns a folly::SemiFuture as it may perform a non-trivial amount of disk I/O. However, at the moment it currently performs all I/O in the current thread before it returns. I plan to move this work to a separate thread in a subsequent diff. Reviewed By: strager Differential Revision: D13981140 fbshipit-source-id: b59eaef88012a8e74fcb770a9c93ca3f9bde32a0 --- eden/fs/inodes/EdenMount.cpp | 47 +++++-------- eden/fs/inodes/EdenMount.h | 8 +-- eden/fs/inodes/Overlay.cpp | 85 +++++++++++++++--------- eden/fs/inodes/Overlay.h | 51 ++++++++------ eden/fs/inodes/OverlayFileAccess.cpp | 8 +-- eden/fs/inodes/OverlayFileAccess.h | 3 +- eden/fs/inodes/test/OverlayBenchmark.cpp | 2 +- eden/fs/inodes/test/OverlayTest.cpp | 37 ++++++----- 8 files changed, 127 insertions(+), 114 deletions(-) diff --git a/eden/fs/inodes/EdenMount.cpp b/eden/fs/inodes/EdenMount.cpp index cd4daf000a..c261360bf9 100644 --- a/eden/fs/inodes/EdenMount.cpp +++ b/eden/fs/inodes/EdenMount.cpp @@ -193,6 +193,8 @@ EdenMount::EdenMount( objectStore_{std::move(objectStore)}, blobCache_{std::move(blobCache)}, blobAccess_{objectStore_, blobCache_}, + overlay_{std::make_unique(config_->getOverlayPath())}, + overlayFileAccess_{overlay_.get()}, bindMounts_{config_->getBindMounts()}, mountGeneration_{globalProcessGeneration | ++mountGeneration}, straceLogger_{kEdenStracePrefix.str() + config_->getMountPath().value()}, @@ -207,45 +209,25 @@ folly::Future EdenMount::initialize( return serverState_->getFaultInjector() .checkAsync("mount", getPath().stringPiece()) .via(serverState_->getThreadPool().get()) - .thenValue([this, doingTakeover = takeover.has_value()](auto&&) { - // Open the overlay. This acquires the overlay lock and makes sure - // that no other process is using this mount. - overlay_ = std::make_unique(config_->getOverlayPath()); - overlayFileAccess_.initialize(overlay_.get()); - - auto parents = - std::make_shared(config_->getParentCommits()); - parentInfo_.wlock()->parents.setParents(*parents); - - // Do this before the root TreeInode is allocated in case it needs to - // allocate any inode numbers. - if (!doingTakeover) { - auto maxInodeNumber = overlay_->scanForNextInodeNumber(); - XLOG(DBG2) << "Initializing eden mount " << getPath() - << "; max existing inode number is " << maxInodeNumber; - } else { - XLOG(DBG2) << "Initializing eden mount " << getPath() - << " from takeover"; - if (!overlay_->hasInitializedNextInodeNumber()) { - XLOG(WARN) << "A clean shutdown before takeover did not leave an " - "initialized inode number! Rescanning..."; - overlay_->scanForNextInodeNumber(); - } - } - CHECK(overlay_->hasInitializedNextInodeNumber()); + .thenValue([this](auto&&) { + auto parents = config_->getParentCommits(); + parentInfo_.wlock()->parents.setParents(parents); // Record the transition from no snapshot to the current snapshot in // the journal. This also sets things up so that we can carry the // snapshot id forward through subsequent journal entries. - // TODO: It would be nice if the .eden inode was created before - // allocating inode numbers for the Tree's entries. This would give the - // .eden directory inode number 2. auto delta = std::make_unique(); - delta->toHash = parents->parent1(); + delta->toHash = parents.parent1(); journal_.addDelta(std::move(delta)); - return createRootInode(*parents); + // Initialize the overlay. + // This must be performed before we do any operations that may allocate + // inode numbers, including creating the root TreeInode. + return overlay_->initialize().deferValue( + [parents](auto&&) { return parents; }); }) + .thenValue( + [this](ParentCommits&& parents) { return createRootInode(parents); }) .thenValue([this, takeover](TreeInodePtr initTreeNode) { if (takeover) { inodeMap_->initializeFromTakeover(std::move(initTreeNode), *takeover); @@ -253,6 +235,9 @@ folly::Future EdenMount::initialize( inodeMap_->initialize(std::move(initTreeNode)); } + // TODO: It would be nice if the .eden inode was created before + // allocating inode numbers for the Tree's entries. This would give the + // .eden directory inode number 2. return setupDotEden(getRootInode()); }) .thenValue([this](auto&&) { diff --git a/eden/fs/inodes/EdenMount.h b/eden/fs/inodes/EdenMount.h index 89feb8e791..2e8a23e3a7 100644 --- a/eden/fs/inodes/EdenMount.h +++ b/eden/fs/inodes/EdenMount.h @@ -106,11 +106,9 @@ class EdenMount { /** * Create a shared_ptr to an EdenMount. * - * Create an EdenMount instance Using an EdenMountDeleter. - * The caller must call initialize() after creating the EdenMount - * instance. This is not done implicitly because the graceful - * restart code needs to take the opportunity to update the InodeMap - * prior to the logic in initialize() running. + * The caller must call initialize() after creating the EdenMount to load data + * required to access the mount's inodes. No inode-related methods may be + * called on the EdenMount until initialize() has successfully completed. */ static std::shared_ptr create( std::unique_ptr config, diff --git a/eden/fs/inodes/Overlay.cpp b/eden/fs/inodes/Overlay.cpp index 4b48e58ec3..d247f50c96 100644 --- a/eden/fs/inodes/Overlay.cpp +++ b/eden/fs/inodes/Overlay.cpp @@ -85,10 +85,6 @@ constexpr uint32_t Overlay::kHeaderVersion; constexpr size_t Overlay::kHeaderLength; Overlay::Overlay(AbsolutePathPiece localDir) : localDir_(localDir) { - initOverlay(); - tryLoadNextInodeNumber(); - - gcThread_ = std::thread([this] { gcThread(); }); } Overlay::~Overlay() { @@ -98,16 +94,17 @@ Overlay::~Overlay() { void Overlay::close() { CHECK_NE(std::this_thread::get_id(), gcThread_.get_id()); + gcQueue_.lock()->stop = true; + gcCondVar_.notify_one(); + if (gcThread_.joinable()) { + gcThread_.join(); + } + if (!infoFile_) { return; } // Make sure everything is shut down in reverse of construction order. - - gcQueue_.lock()->stop = true; - gcCondVar_.notify_one(); - gcThread_.join(); - saveNextInodeNumber(); inodeMetadataTable_.reset(); @@ -115,16 +112,11 @@ void Overlay::close() { infoFile_.close(); } -bool Overlay::hasInitializedNextInodeNumber() const { - // nextInodeNumber_ is either 0 (uninitialized) or nonzero (initialized). - // It's only initialized on one thread, so relaxed loads are okay. - return 0 != nextInodeNumber_.load(std::memory_order_relaxed); -} - void Overlay::initOverlay() { // Read the info file. auto infoPath = localDir_ + PathComponentPiece{kInfoFile}; int fd = folly::openNoInt(infoPath.value().c_str(), O_RDONLY | O_CLOEXEC); + bool newOverlay = false; if (fd >= 0) { // This is an existing overlay directory. // Read the info file and make sure we are compatible with its version. @@ -137,6 +129,7 @@ void Overlay::initOverlay() { // This is a brand new overlay directory. initNewOverlay(); infoFile_ = File{infoPath.value().c_str(), O_RDONLY | O_CLOEXEC}; + newOverlay = true; } if (!infoFile_.try_lock()) { @@ -155,6 +148,13 @@ void Overlay::initOverlay() { dirFd, "error opening overlay directory handle for ", localDir_.value()); dirFile_ = File{dirFd, /* ownsFd */ true}; + if (newOverlay) { + nextInodeNumber_.store(kRootNodeId.get() + 1, std::memory_order_relaxed); + } else { + auto nextInodeNumber = loadNextInodeNumber(); + nextInodeNumber_.store(nextInodeNumber.get(), std::memory_order_relaxed); + } + // To support migrating from an older Overlay format, unconditionally create // tmp/. // TODO: It would be a bit expensive, but it might be worth checking @@ -167,7 +167,7 @@ void Overlay::initOverlay() { (localDir_ + PathComponentPiece{kMetadataFile}).c_str()); } -void Overlay::tryLoadNextInodeNumber() { +std::optional Overlay::tryLoadNextInodeNumberFile() { // If we ever want to extend this file, it should be renamed and a proper // header with version number added. In the meantime, we enforce the file is // 8 bytes. @@ -177,9 +177,8 @@ void Overlay::tryLoadNextInodeNumber() { if (errno == ENOENT) { // No max inode number file was written which usually means either Eden // was not shut down cleanly or an old overlay is being loaded. - // Either way, a full scan of the overlay is necessary, so leave - // nextInodeNumber_ at 0. - return; + // Either way, a full scan of the overlay is necessary. + return std::nullopt; } else { folly::throwSystemError("Failed to open ", kNextInodeNumberFile); } @@ -203,16 +202,16 @@ void Overlay::tryLoadNextInodeNumber() { if (readResult != sizeof(nextInodeNumber)) { XLOG(WARN) << "Failed to read entire inode number. Only read " << readResult << " bytes. Full overlay scan required."; - return; + return std::nullopt; } if (nextInodeNumber <= kRootNodeId.get()) { XLOG(WARN) << "Invalid max inode number " << nextInodeNumber << ". Full overlay scan required."; - return; + return std::nullopt; } - nextInodeNumber_.store(nextInodeNumber, std::memory_order_relaxed); + return InodeNumber{nextInodeNumber}; } void Overlay::saveNextInodeNumber() { @@ -301,9 +300,6 @@ void Overlay::initNewOverlay() { auto infoPath = localDir_ + PathComponentPiece{kInfoFile}; folly::writeFileAtomic( infoPath.stringPiece(), ByteRange(infoHeader.data(), infoHeader.size())); - - // kRootNodeId is reserved - start at the next one. No scan is necessary. - nextInodeNumber_.store(kRootNodeId.get() + 1, std::memory_order_relaxed); } void Overlay::ensureTmpDirectoryIsCreated() { @@ -486,13 +482,38 @@ bool Overlay::hasOverlayData(InodeNumber inodeNumber) { } } -InodeNumber Overlay::scanForNextInodeNumber() { - if (auto ino = nextInodeNumber_.load(std::memory_order_relaxed)) { - // Already defined. - CHECK_GT(ino, 1); - return InodeNumber{ino - 1}; +folly::SemiFuture Overlay::initialize() { + // TODO: Perform on-disk initialization in a separate thread, + // to avoid blocking the current thread. + // + // We potentially could just do this in the gc thread before running + // the gcThread() function. + initOverlay(); + gcThread_ = std::thread([this] { gcThread(); }); + return folly::makeSemiFuture(); +} + +InodeNumber Overlay::getMaxInodeNumber() { + auto ino = nextInodeNumber_.load(std::memory_order_relaxed); + CHECK_GT(ino, 1); + return InodeNumber{ino - 1}; +} + +InodeNumber Overlay::loadNextInodeNumber() { + auto numberFromFile = tryLoadNextInodeNumberFile(); + if (numberFromFile.has_value()) { + return numberFromFile.value(); } + XLOG(WARN) << "Overlay " << localDir_ + << " was not shut down cleanly. Will rescan."; + auto numberFromScan = scanForNextInodeNumber(); + XLOG(DBG2) << "Finished scanning overlay " << localDir_ + << "; max existing inode number is " << numberFromScan; + return numberFromScan; +} + +InodeNumber Overlay::scanForNextInodeNumber() { // Walk the root directory downwards to find all (non-unlinked) directory // inodes stored in the overlay. // @@ -552,9 +573,7 @@ InodeNumber Overlay::scanForNextInodeNumber() { } } - nextInodeNumber_.store(maxInode.get() + 1, std::memory_order_relaxed); - - return maxInode; + return InodeNumber{maxInode.get() + 1}; } Overlay::InodePath Overlay::getFilePath(InodeNumber inodeNumber) { diff --git a/eden/fs/inodes/Overlay.h b/eden/fs/inodes/Overlay.h index 9c348b7cb7..9d0745e48b 100644 --- a/eden/fs/inodes/Overlay.h +++ b/eden/fs/inodes/Overlay.h @@ -10,6 +10,7 @@ #pragma once #include #include +#include #include #include #include @@ -58,6 +59,12 @@ class Overlay { public: class InodePath; + /** + * Create a new Overlay object. + * + * The caller must call initialize() after creating the Overlay and wait for + * it to succeed before using any other methods. + */ explicit Overlay(AbsolutePathPiece localDir); ~Overlay(); @@ -66,6 +73,26 @@ class Overlay { Overlay& operator=(const Overlay&) = delete; Overlay& operator=(Overlay&&) = delete; + /** + * Initialize the overlay. + * + * This must be called after the Overlay constructor, before performing + * operations on the overlay. + * + * This may be a slow operation and may perform significant amounts of + * disk I/O. + * + * The initialization operation may include: + * - Acquiring a lock to ensure no other processes are accessing the on-disk + * overlay state + * - Creating the initial on-disk overlay data structures if necessary. + * - Verifying and fixing the on-disk data if the Overlay was not shut down + * cleanly the last time it was opened. + * - Upgrading the on-disk data from older formats if the Overlay was created + * by an older version of the software. + */ + folly::SemiFuture initialize(); + /** * Closes the overlay. It is undefined behavior to access the * InodeMetadataTable concurrently or call any other Overlay method @@ -78,22 +105,9 @@ class Overlay { void close(); /** - * Returns true if the next inode number was initialized, either upon - * construction by loading the file left by a cleanly-closed Overlay, or by - * calling scanForNextInodeNumber(). + * Get the maximum inode number that has ever been allocated to an inode. */ - bool hasInitializedNextInodeNumber() const; - - /** - * Scans the Overlay for all inode numbers currently in use and sets the next - * inode number to the maximum plus one. Either this or setNextInodeNumber - * should be called when opening a mount point to ensure that any future - * allocated inode numbers are always greater than those already tracked in - * the overlay. - * - * Returns the maximum existing inode number. - */ - InodeNumber scanForNextInodeNumber(); + InodeNumber getMaxInodeNumber(); /** * allocateInodeNumber() should only be called by TreeInode. @@ -106,9 +120,6 @@ class Overlay { * inodeCreated() should be called immediately afterwards to register the * new child Inode object. * - * It is illegal to call allocateInodeNumber prior to - * setNextInodeNumber or scanForNextInodeNumber. - * * TODO: It would be easy to extend this function to allocate a range of * inode values in one atomic operation. */ @@ -216,7 +227,9 @@ class Overlay { }; void initOverlay(); - void tryLoadNextInodeNumber(); + InodeNumber loadNextInodeNumber(); + std::optional tryLoadNextInodeNumberFile(); + InodeNumber scanForNextInodeNumber(); void saveNextInodeNumber(); void readExistingOverlay(int infoFD); void initNewOverlay(); diff --git a/eden/fs/inodes/OverlayFileAccess.cpp b/eden/fs/inodes/OverlayFileAccess.cpp index a8dae9aae4..62d922b734 100644 --- a/eden/fs/inodes/OverlayFileAccess.cpp +++ b/eden/fs/inodes/OverlayFileAccess.cpp @@ -43,12 +43,8 @@ OverlayFileAccess::State::State(size_t cacheSize) : entries{cacheSize} { } } -OverlayFileAccess::OverlayFileAccess() - : state_{folly::in_place, FLAGS_overlayFileCacheSize} {} - -void OverlayFileAccess::initialize(Overlay* overlay) { - overlay_ = overlay; -} +OverlayFileAccess::OverlayFileAccess(Overlay* overlay) + : overlay_{overlay}, state_{folly::in_place, FLAGS_overlayFileCacheSize} {} OverlayFileAccess::~OverlayFileAccess() = default; diff --git a/eden/fs/inodes/OverlayFileAccess.h b/eden/fs/inodes/OverlayFileAccess.h index 5c38f3bde5..13b9cf8e70 100644 --- a/eden/fs/inodes/OverlayFileAccess.h +++ b/eden/fs/inodes/OverlayFileAccess.h @@ -31,9 +31,8 @@ class Overlay; */ class OverlayFileAccess { public: - OverlayFileAccess(); + explicit OverlayFileAccess(Overlay* overlay); ~OverlayFileAccess(); - void initialize(Overlay* overlay); /** * Creates a new empty file in the overlay. diff --git a/eden/fs/inodes/test/OverlayBenchmark.cpp b/eden/fs/inodes/test/OverlayBenchmark.cpp index 384088ddbb..b28d57b1f4 100644 --- a/eden/fs/inodes/test/OverlayBenchmark.cpp +++ b/eden/fs/inodes/test/OverlayBenchmark.cpp @@ -29,7 +29,7 @@ void benchmarkOverlayTreeWrites(AbsolutePathPiece overlayPath) { // overlayPath is parameterized to measure on different filesystem types. Overlay overlay{overlayPath}; - overlay.scanForNextInodeNumber(); + overlay.initialize().get(); Hash hash1{folly::ByteRange{"abcdabcdabcdabcdabcd"_sp}}; Hash hash2{folly::ByteRange{"01234012340123401234"_sp}}; diff --git a/eden/fs/inodes/test/OverlayTest.cpp b/eden/fs/inodes/test/OverlayTest.cpp index caab8d1393..87bef793e9 100644 --- a/eden/fs/inodes/test/OverlayTest.cpp +++ b/eden/fs/inodes/test/OverlayTest.cpp @@ -69,6 +69,7 @@ TEST(OverlayGoldMasterTest, can_load_overlay_v2) { tarProcess.waitChecked(); Overlay overlay{realpath(tmpdir.path().string()) + "overlay-v2"_pc}; + overlay.initialize().get(); Hash hash1{folly::ByteRange{"abcdabcdabcdabcdabcd"_sp}}; Hash hash2{folly::ByteRange{"01234012340123401234"_sp}}; @@ -290,6 +291,7 @@ class RawOverlayTest : public ::testing::TestWithParam { void loadOverlay() { overlay = std::make_unique(getLocalDir()); + overlay->initialize().get(); } void corruptOverlayFile(InodeNumber inodeNumber) { @@ -320,17 +322,17 @@ class RawOverlayTest : public ::testing::TestWithParam { }; TEST_P(RawOverlayTest, max_inode_number_is_1_if_overlay_is_empty) { - EXPECT_EQ(kRootNodeId, overlay->scanForNextInodeNumber()); + EXPECT_EQ(kRootNodeId, overlay->getMaxInodeNumber()); EXPECT_EQ(2_ino, overlay->allocateInodeNumber()); recreate(OverlayRestartMode::CLEAN); - EXPECT_EQ(2_ino, overlay->scanForNextInodeNumber()); + EXPECT_EQ(2_ino, overlay->getMaxInodeNumber()); EXPECT_EQ(3_ino, overlay->allocateInodeNumber()); recreate(OverlayRestartMode::UNCLEAN); - EXPECT_EQ(kRootNodeId, overlay->scanForNextInodeNumber()); + EXPECT_EQ(kRootNodeId, overlay->getMaxInodeNumber()); EXPECT_EQ(2_ino, overlay->allocateInodeNumber()); } @@ -343,7 +345,7 @@ TEST_P(RawOverlayTest, remembers_max_inode_number_of_tree_inodes) { recreate(); - EXPECT_EQ(2_ino, overlay->scanForNextInodeNumber()); + EXPECT_EQ(2_ino, overlay->getMaxInodeNumber()); } TEST_P(RawOverlayTest, remembers_max_inode_number_of_tree_entries) { @@ -360,7 +362,7 @@ TEST_P(RawOverlayTest, remembers_max_inode_number_of_tree_entries) { recreate(); SCOPED_TRACE("Inodes:\n" + debugDumpOverlayInodes(*overlay, kRootNodeId)); - EXPECT_EQ(4_ino, overlay->scanForNextInodeNumber()); + EXPECT_EQ(4_ino, overlay->getMaxInodeNumber()); } TEST_P(RawOverlayTest, remembers_max_inode_number_of_file) { @@ -375,7 +377,7 @@ TEST_P(RawOverlayTest, remembers_max_inode_number_of_file) { recreate(); - EXPECT_EQ(3_ino, overlay->scanForNextInodeNumber()); + EXPECT_EQ(3_ino, overlay->getMaxInodeNumber()); } TEST_P( @@ -395,7 +397,7 @@ TEST_P( corruptOverlayFile(subdirectoryIno); loadOverlay(); - EXPECT_EQ(subdirectoryIno, overlay->scanForNextInodeNumber()); + EXPECT_EQ(subdirectoryIno, overlay->getMaxInodeNumber()); } TEST_P( @@ -468,13 +470,14 @@ TEST_P( corruptOverlayFileByDeleting(corruptedByDeletionIno); loadOverlay(); - EXPECT_EQ(maxIno, overlay->scanForNextInodeNumber()); + EXPECT_EQ(maxIno, overlay->getMaxInodeNumber()); } } TEST_P(RawOverlayTest, inode_number_scan_logs_corrupt_directories_once) { SKIP_IF(GetParam() == OverlayRestartMode::CLEAN) - << "scanForNextInodeNumber should not log about corrupt directories on clean restart."; + << "scanForNextInodeNumber should not log about corrupt " + "directories on clean restart."; constexpr auto subdirCount = 5; static_assert( @@ -500,21 +503,21 @@ TEST_P(RawOverlayTest, inode_number_scan_logs_corrupt_directories_once) { for (auto subdirIno : subdirInos) { corruptOverlayFile(subdirIno); } - loadOverlay(); auto logHandler = std::make_shared(); folly::LoggerDB::get() .getCategory("eden.fs.inodes.Overlay") ->addHandler(logHandler); - overlay->scanForNextInodeNumber(); + loadOverlay(); SCOPED_TRACE(prettifyLogMessages(*logHandler)); auto corruptOverlayFileMessages = filterLogMessages( *logHandler, "Ignoring failure to load directory inode"_sp); EXPECT_EQ(1, corruptOverlayFileMessages.size()) - << "scanForNextInodeNumber should have logged about corrupt directory inodes only once"; + << "scanForNextInodeNumber should have logged about " + "corrupt directory inodes only once"; } TEST_P(RawOverlayTest, inode_numbers_not_reused_after_unclean_shutdown) { @@ -540,7 +543,7 @@ TEST_P(RawOverlayTest, inode_numbers_not_reused_after_unclean_shutdown) { SCOPED_TRACE( "Inodes from subdir:\n" + debugDumpOverlayInodes(*overlay, ino4)); - EXPECT_EQ(5_ino, overlay->scanForNextInodeNumber()); + EXPECT_EQ(5_ino, overlay->getMaxInodeNumber()); } TEST_P(RawOverlayTest, inode_numbers_after_takeover) { @@ -563,8 +566,6 @@ TEST_P(RawOverlayTest, inode_numbers_after_takeover) { recreate(); - overlay->scanForNextInodeNumber(); - // Rewrite the root (say, after a takeover) without the file. DirContents newroot; @@ -576,7 +577,7 @@ TEST_P(RawOverlayTest, inode_numbers_after_takeover) { SCOPED_TRACE("Inodes:\n" + debugDumpOverlayInodes(*overlay, kRootNodeId)); // Ensure an inode in the overlay but not referenced by the previous session // counts. - EXPECT_EQ(5_ino, overlay->scanForNextInodeNumber()); + EXPECT_EQ(5_ino, overlay->getMaxInodeNumber()); } INSTANTIATE_TEST_CASE_P( @@ -598,7 +599,9 @@ class DebugDumpOverlayInodesTest : public ::testing::Test { public: DebugDumpOverlayInodesTest() : testDir_{makeTempDir("eden_DebugDumpOverlayInodesTest")}, - overlay{AbsolutePathPiece{testDir_.path().string()}} {} + overlay{AbsolutePathPiece{testDir_.path().string()}} { + overlay.initialize().get(); + } folly::test::TemporaryDirectory testDir_; Overlay overlay;