From 0f0b0b6b4dd75a25a8a5c307ffbb32a6559cd3a9 Mon Sep 17 00:00:00 2001 From: Chad Austin Date: Wed, 3 Jan 2018 17:18:32 -0800 Subject: [PATCH] remove unnecessary code and a faulty assertion Summary: It is no longer correct to assert that state->file is set if O_TRUNC happened before blob import from hg finished. It surprises me we never saw a crash because of that. Also, the O_TRUNC path after blob import finishes can never complete a future, so don't try. Reviewed By: wez Differential Revision: D6656699 fbshipit-source-id: 5e245fc46185714e5f5d81c2680835a3497747ff --- eden/fs/inodes/FileInode.cpp | 40 ++++++++++----------------- eden/fs/inodes/test/FileInodeTest.cpp | 32 +++++++++++++++++++++ 2 files changed, 46 insertions(+), 26 deletions(-) diff --git a/eden/fs/inodes/FileInode.cpp b/eden/fs/inodes/FileInode.cpp index 18442da383..c59dd63d67 100644 --- a/eden/fs/inodes/FileInode.cpp +++ b/eden/fs/inodes/FileInode.cpp @@ -676,7 +676,7 @@ folly::Future FileInode::write(folly::StringPiece data, off_t off) { // Waits until inode is either in 'loaded' or 'materialized' state. Future FileInode::ensureDataLoaded() { - Future resultFuture{FileHandlePtr{nullptr}}; + folly::Optional> resultFuture; auto blobFuture = Future>::makeEmpty(); { @@ -720,11 +720,13 @@ Future FileInode::ensureDataLoaded() { } } + // Execution only gets here in the NOT_LOADED case, in which case resultFuture + // is initialized. + CHECK(resultFuture); + auto self = inodePtrFromThis(); // separate line for formatting blobFuture .then([self](folly::Try> tryBlob) { - folly::SharedPromise promise; - auto state = self->state_.wlock(); state->checkInvariants(); @@ -736,8 +738,8 @@ Future FileInode::ensureDataLoaded() { // with a concurrent materializeForWrite(O_TRUNC), in which case the // state would have transitioned to 'materialized' before this // callback runs. - case State::BLOB_LOADING: - promise = std::move(*state->blobLoadingPromise); + case State::BLOB_LOADING: { + auto promise = std::move(*state->blobLoadingPromise); state->blobLoadingPromise.clear(); if (tryBlob.hasValue()) { @@ -766,26 +768,14 @@ Future FileInode::ensureDataLoaded() { promise.setException(tryBlob.exception()); } break; + } - case State::MATERIALIZED_IN_OVERLAY: { + case State::MATERIALIZED_IN_OVERLAY: // The load raced with a materializeForWrite(O_TRUNC). Nothing left // to do here: ensureDataLoaded() guarantees `blob` or `file` is // defined after its completion, and the // materializeForWrite(O_TRUNC) fulfilled the promise. - CHECK(state->file); - // The FileHandle must be allocated while the lock is held so the - // blob field is set and the openCount incremented atomically, so - // that no other thread can cause the blob to get unset before - // openCount is incremented. - auto result = std::make_shared( - self, [&state] { fileHandleDidOpen(*state); }); - // Call the Future's subscribers while the state_ lock is not held. - // Even if the FileInode has transitioned to a materialized state, - // any pending loads must be unblocked. - state.unlock(); - promise.setValue(result); break; - } default: EDEN_BUG() @@ -794,7 +784,7 @@ Future FileInode::ensureDataLoaded() { }) .onError([](const std::exception&) { // We get here if EDEN_BUG() didn't terminate the process, or if we - // threw in the preceeding block. Both are bad because we won't + // threw in the preceding block. Both are bad because we won't // automatically propagate the exception to resultFuture and we // can't trust the state of anything if we get here. // Rather than leaving something hanging, we suicide. @@ -803,7 +793,7 @@ Future FileInode::ensureDataLoaded() { << "Failed to propagate failure in getBlob(), no choice but to die"; }); - return resultFuture; + return std::move(*resultFuture); } namespace { @@ -894,7 +884,7 @@ Future FileInode::materializeForWrite() { void FileInode::materializeAndTruncate() { // Set if in 'loading' state. Fulfilled outside of the scopes of any locks. - folly::Optional> sharedPromise; + folly::SharedPromise sharedPromise; // Notifying the parent of materialization must occur outside of the lock. bool didMaterialize = false; @@ -934,7 +924,7 @@ void FileInode::materializeAndTruncate() { // Transition to `loaded`. if (state->blobLoadingPromise) { // Loading. // Move the promise out so it's fulfilled outside of the lock. - sharedPromise.emplace(std::move(*state->blobLoadingPromise)); + sharedPromise = std::move(*state->blobLoadingPromise); state->blobLoadingPromise.reset(); } else if (state->blob) { // Loaded. state->blob.reset(); @@ -961,9 +951,7 @@ void FileInode::materializeAndTruncate() { } // Fulfill outside of the lock. - if (sharedPromise) { - sharedPromise->setTry(std::move(try_)); - } + sharedPromise.setTry(std::move(try_)); } ObjectStore* FileInode::getObjectStore() const { diff --git a/eden/fs/inodes/test/FileInodeTest.cpp b/eden/fs/inodes/test/FileInodeTest.cpp index 0cda90e334..8dfe59f91a 100644 --- a/eden/fs/inodes/test/FileInodeTest.cpp +++ b/eden/fs/inodes/test/FileInodeTest.cpp @@ -410,6 +410,38 @@ TEST_F(FileInodeTest, truncatingMaterializesParent) { EXPECT_EQ(true, isInodeMaterialized(parent)); } +TEST(FileInodeTest_, truncatingDuringLoad) { + FakeTreeBuilder builder; + builder.setFiles({{"notready.txt", "Contents not ready.\n"}}); + + TestMount mount_; + mount_.initialize(builder, false); + + auto inode = mount_.getFileInode("notready.txt"); + + auto backingStore = mount_.getBackingStore(); + auto storedBlob = backingStore->getStoredBlob(*inode->getBlobHash()); + + auto readAllFuture = inode->readAll(); + EXPECT_EQ(false, readAllFuture.isReady()); + + { + // Synchronously truncate the file while the load is in progress. + auto handleFuture = inode->open(O_TRUNC); + EXPECT_EQ(true, handleFuture.isReady()); + EXPECT_EQ(true, handleFuture.hasValue()); + // Deallocate the handle here, closing the open file. + } + + // Verify, from the caller's perspective, the load is complete (but empty). + EXPECT_EQ(true, readAllFuture.isReady()); + EXPECT_EQ("", readAllFuture.value()); + + // Now finish the ObjectStore load request to make sure the FileInode + // handles the state correctly. + storedBlob->setReady(); +} + // TODO: test multiple flags together // TODO: ensure ctime is updated after every call to setattr() // TODO: ensure mtime is updated after opening a file, writing to it, then