diff --git a/eden/fs/inodes/EdenMount.cpp b/eden/fs/inodes/EdenMount.cpp index 1224cb060eeb7..76e31c95a5da5 100644 --- a/eden/fs/inodes/EdenMount.cpp +++ b/eden/fs/inodes/EdenMount.cpp @@ -196,10 +196,15 @@ folly::Future EdenMount::initialize( << "; max existing inode number is " << maxInodeNumber; } else { XLOG(DBG2) << "Initializing eden mount " << getPath() << " from takeover"; - overlay_->setNextInodeNumber( - InodeNumber::fromThrift(takeover->nextInodeNumber)); + if (!overlay_->hasInitializedNextInodeNumber()) { + XLOG(WARN) << "A clean shutdown before takeover did not leave an " + "initialized inode number! Rescanning..."; + overlay_->scanForNextInodeNumber(); + } } + CHECK(overlay_->hasInitializedNextInodeNumber()); + return createRootInode(*parents).then( [this, parents, takeover](TreeInodePtr initTreeNode) { if (takeover) { @@ -367,10 +372,8 @@ EdenMount::shutdownImpl(bool doTakeover) { // Close the Overlay object to make sure we have released its lock. // This is important during graceful restart to ensure that we have // released the lock before the new edenfs process begins to take over - // the mount piont. - inodeMap.nextInodeNumber = overlay_->close(); - CHECK(inodeMap.nextInodeNumber) - << "nextInodeNumber should always be nonzero"; + // the mount point. + overlay_->close(); state_.store(State::SHUT_DOWN); return std::make_tuple(fileHandleMap, inodeMap); }); diff --git a/eden/fs/inodes/InodeMap.cpp b/eden/fs/inodes/InodeMap.cpp index fb8e52b6fc124..cf6d74f080d86 100644 --- a/eden/fs/inodes/InodeMap.cpp +++ b/eden/fs/inodes/InodeMap.cpp @@ -614,7 +614,6 @@ Future InodeMap::shutdown(bool doTakeover) { } SerializedInodeMap result; - XLOG(DBG5) << "InodeMap::save nextInodeNumber: " << result.nextInodeNumber; result.unloadedInodes.reserve(data->unloadedInodes_.size()); for (const auto& it : data->unloadedInodes_) { const auto& entry = it.second; diff --git a/eden/fs/inodes/Overlay.cpp b/eden/fs/inodes/Overlay.cpp index 16a2e7ed8c639..4cbae77d058d1 100644 --- a/eden/fs/inodes/Overlay.cpp +++ b/eden/fs/inodes/Overlay.cpp @@ -95,11 +95,11 @@ Overlay::~Overlay() { close(); } -uint64_t Overlay::close() { +void Overlay::close() { CHECK_NE(std::this_thread::get_id(), gcThread_.get_id()); if (!infoFile_) { - return nextInodeNumber_.load(std::memory_order_relaxed); + return; } // Make sure everything is shut down in reverse of construction order. @@ -113,8 +113,12 @@ uint64_t Overlay::close() { inodeMetadataTable_.reset(); dirFile_.close(); infoFile_.close(); +} - return nextInodeNumber_.load(std::memory_order_relaxed); +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() { @@ -207,6 +211,8 @@ void Overlay::tryLoadNextInodeNumber() { } void Overlay::saveNextInodeNumber() { + // nextInodeNumber_ is either 0 (uninitialized) or nonzero (initialized). + // It's only initialized on one thread, so relaxed loads are okay. auto nextInodeNumber = nextInodeNumber_.load(std::memory_order_relaxed); if (!nextInodeNumber) { return; @@ -292,7 +298,7 @@ void Overlay::initNewOverlay() { infoPath.stringPiece(), ByteRange(infoHeader.data(), infoHeader.size())); // kRootNodeId is reserved - start at the next one. No scan is necessary. - setNextInodeNumber(InodeNumber{kRootNodeId.get() + 1}); + nextInodeNumber_.store(kRootNodeId.get() + 1, std::memory_order_relaxed); } void Overlay::ensureTmpDirectoryIsCreated() { @@ -314,22 +320,6 @@ void Overlay::ensureTmpDirectoryIsCreated() { } } -void Overlay::setNextInodeNumber(InodeNumber nextInodeNumber) { - if (auto ino = nextInodeNumber_.load(std::memory_order_relaxed)) { - // It's okay to allow setNextInodeNumber as long as the values are - // consistent. This code path will disappear when takeover transitions to - // relying on the Overlay efficiently remembering the next inode number - // itself. - DCHECK_EQ(ino, nextInodeNumber.get()) - << "Overlay nextInodeNumber already initialized with " << ino - << " so it should not be initialized with " << nextInodeNumber; - return; - } - - DCHECK_GT(nextInodeNumber, kRootNodeId); - nextInodeNumber_.store(nextInodeNumber.get(), std::memory_order_relaxed); -} - InodeNumber Overlay::allocateInodeNumber() { // InodeNumber should generally be 64-bits wide, in which case it isn't even // worth bothering to handle the case where nextInodeNumber_ wraps. We don't @@ -558,7 +548,7 @@ InodeNumber Overlay::scanForNextInodeNumber() { } } - setNextInodeNumber(InodeNumber{maxInode.get() + 1}); + nextInodeNumber_.store(maxInode.get() + 1, std::memory_order_relaxed); return maxInode; } diff --git a/eden/fs/inodes/Overlay.h b/eden/fs/inodes/Overlay.h index 3a445a25cfd71..dfe22f2e941e2 100644 --- a/eden/fs/inodes/Overlay.h +++ b/eden/fs/inodes/Overlay.h @@ -76,14 +76,14 @@ class Overlay { * Returns the next available InodeNumber to be passed to any process taking * over an Eden mount. */ - uint64_t close(); + void close(); /** - * For now, this method exists to populate the next inode number from takeover - * data. Eventually, it will be unnecessary - the Overlay, upon a clean - * shutdown, will remember its next inode number in a file on disk. + * 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(). */ - void setNextInodeNumber(InodeNumber nextInodeNumber); + bool hasInitializedNextInodeNumber() const; /** * Scans the Overlay for all inode numbers currently in use and sets the next diff --git a/eden/fs/takeover/takeover.thrift b/eden/fs/takeover/takeover.thrift index 2c570409ee43a..3cc8f8392b296 100644 --- a/eden/fs/takeover/takeover.thrift +++ b/eden/fs/takeover/takeover.thrift @@ -18,7 +18,6 @@ struct SerializedInodeMapEntry { } struct SerializedInodeMap { - 1: i64 nextInodeNumber 2: list unloadedInodes, }