From 04d373b26034464a8ad08e7123c3911609e20adc Mon Sep 17 00:00:00 2001 From: Yi Wu Date: Thu, 20 Sep 2018 16:50:07 -0700 Subject: [PATCH] BlobDB: handle IO error on read (#4410) Summary: Fix IO error on read not being handle and crashing the DB. With the fix we properly return the error. Pull Request resolved: https://github.com/facebook/rocksdb/pull/4410 Differential Revision: D9979246 Pulled By: yiwu-arbug fbshipit-source-id: 111a85675067a29c03cb60e9a34103f4ff636694 --- util/fault_injection_test_env.cc | 9 +++++++++ util/fault_injection_test_env.h | 4 ++++ utilities/blob_db/blob_db_impl.cc | 23 +++++++++++++++-------- utilities/blob_db/blob_db_impl.h | 7 ++----- utilities/blob_db/blob_db_test.cc | 21 ++++++++++++++++++++- utilities/blob_db/blob_file.cc | 26 +++++++++++++++++++------- utilities/blob_db/blob_file.h | 7 ++++--- 7 files changed, 73 insertions(+), 24 deletions(-) diff --git a/util/fault_injection_test_env.cc b/util/fault_injection_test_env.cc index 46e1e0d77..3b3dbbe99 100644 --- a/util/fault_injection_test_env.cc +++ b/util/fault_injection_test_env.cc @@ -197,6 +197,15 @@ Status FaultInjectionTestEnv::NewWritableFile(const std::string& fname, return s; } +Status FaultInjectionTestEnv::NewRandomAccessFile( + const std::string& fname, std::unique_ptr* result, + const EnvOptions& soptions) { + if (!IsFilesystemActive()) { + return GetError(); + } + return target()->NewRandomAccessFile(fname, result, soptions); +} + Status FaultInjectionTestEnv::DeleteFile(const std::string& f) { if (!IsFilesystemActive()) { return GetError(); diff --git a/util/fault_injection_test_env.h b/util/fault_injection_test_env.h index a8a8e38dc..563986e29 100644 --- a/util/fault_injection_test_env.h +++ b/util/fault_injection_test_env.h @@ -110,6 +110,10 @@ class FaultInjectionTestEnv : public EnvWrapper { unique_ptr* result, const EnvOptions& soptions) override; + Status NewRandomAccessFile(const std::string& fname, + std::unique_ptr* result, + const EnvOptions& soptions) override; + virtual Status DeleteFile(const std::string& f) override; virtual Status RenameFile(const std::string& s, diff --git a/utilities/blob_db/blob_db_impl.cc b/utilities/blob_db/blob_db_impl.cc index 8ea4bacd9..1a32bd562 100644 --- a/utilities/blob_db/blob_db_impl.cc +++ b/utilities/blob_db/blob_db_impl.cc @@ -304,13 +304,17 @@ void BlobDBImpl::CloseRandomAccessLocked( open_file_count_--; } -std::shared_ptr BlobDBImpl::GetOrOpenRandomAccessReader( - const std::shared_ptr& bfile, Env* env, - const EnvOptions& env_options) { +Status BlobDBImpl::GetBlobFileReader( + const std::shared_ptr& blob_file, + std::shared_ptr* reader) { + assert(reader != nullptr); bool fresh_open = false; - auto rar = bfile->GetOrOpenRandomAccessReader(env, env_options, &fresh_open); - if (fresh_open) open_file_count_++; - return rar; + Status s = blob_file->GetReader(env_, env_options_, reader, &fresh_open); + if (s.ok() && fresh_open) { + assert(*reader != nullptr); + open_file_count_++; + } + return s; } std::shared_ptr BlobDBImpl::NewBlobFile(const std::string& reason) { @@ -998,8 +1002,11 @@ Status BlobDBImpl::GetBlobValue(const Slice& key, const Slice& index_entry, } // takes locks when called - std::shared_ptr reader = - GetOrOpenRandomAccessReader(bfile, env_, env_options_); + std::shared_ptr reader; + s = GetBlobFileReader(bfile, &reader); + if (!s.ok()) { + return s; + } assert(blob_index.offset() > key.size() + sizeof(uint32_t)); uint64_t record_offset = blob_index.offset() - key.size() - sizeof(uint32_t); diff --git a/utilities/blob_db/blob_db_impl.h b/utilities/blob_db/blob_db_impl.h index b565ea845..4296d5c6a 100644 --- a/utilities/blob_db/blob_db_impl.h +++ b/utilities/blob_db/blob_db_impl.h @@ -296,11 +296,8 @@ class BlobDBImpl : public BlobDB { // Open all blob files found in blob_dir. Status OpenAllBlobFiles(); - // hold write mutex on file and call - // creates a Random Access reader for GET call - std::shared_ptr GetOrOpenRandomAccessReader( - const std::shared_ptr& bfile, Env* env, - const EnvOptions& env_options); + Status GetBlobFileReader(const std::shared_ptr& blob_file, + std::shared_ptr* reader); // hold write mutex on file and call. // Close the above Random Access reader diff --git a/utilities/blob_db/blob_db_test.cc b/utilities/blob_db/blob_db_test.cc index c99362863..cf8f1217a 100644 --- a/utilities/blob_db/blob_db_test.cc +++ b/utilities/blob_db/blob_db_test.cc @@ -16,6 +16,7 @@ #include "port/port.h" #include "rocksdb/utilities/debug.h" #include "util/cast_util.h" +#include "util/fault_injection_test_env.h" #include "util/random.h" #include "util/string_util.h" #include "util/sync_point.h" @@ -40,6 +41,7 @@ class BlobDBTest : public testing::Test { BlobDBTest() : dbname_(test::PerThreadDBPath("blob_db_test")), mock_env_(new MockTimeEnv(Env::Default())), + fault_injection_env_(new FaultInjectionTestEnv(Env::Default())), blob_db_(nullptr) { Status s = DestroyBlobDB(dbname_, Options(), BlobDBOptions()); assert(s.ok()); @@ -236,6 +238,7 @@ class BlobDBTest : public testing::Test { const std::string dbname_; std::unique_ptr mock_env_; + std::unique_ptr fault_injection_env_; BlobDB *blob_db_; }; // class BlobDBTest @@ -354,6 +357,23 @@ TEST_F(BlobDBTest, GetExpiration) { ASSERT_EQ(300 /* = 100 + 200 */, expiration); } +TEST_F(BlobDBTest, GetIOError) { + Options options; + options.env = fault_injection_env_.get(); + BlobDBOptions bdb_options; + bdb_options.min_blob_size = 0; // Make sure value write to blob file + bdb_options.disable_background_tasks = true; + Open(bdb_options, options); + ColumnFamilyHandle *column_family = blob_db_->DefaultColumnFamily(); + PinnableSlice value; + ASSERT_OK(Put("foo", "bar")); + fault_injection_env_->SetFilesystemActive(false, Status::IOError()); + Status s = blob_db_->Get(ReadOptions(), column_family, "foo", &value); + ASSERT_TRUE(s.IsIOError()); + // Reactivate file system to allow test to close DB. + fault_injection_env_->SetFilesystemActive(true); +} + TEST_F(BlobDBTest, WriteBatch) { Random rnd(301); BlobDBOptions bdb_options; @@ -461,7 +481,6 @@ TEST_F(BlobDBTest, DecompressAfterReopen) { Reopen(bdb_options); VerifyDB(data); } - #endif TEST_F(BlobDBTest, MultipleWriters) { diff --git a/utilities/blob_db/blob_file.cc b/utilities/blob_db/blob_file.cc index c34ad9098..6e70bdcb0 100644 --- a/utilities/blob_db/blob_file.cc +++ b/utilities/blob_db/blob_file.cc @@ -191,36 +191,48 @@ void BlobFile::CloseRandomAccessLocked() { last_access_ = -1; } -std::shared_ptr BlobFile::GetOrOpenRandomAccessReader( - Env* env, const EnvOptions& env_options, bool* fresh_open) { +Status BlobFile::GetReader(Env* env, const EnvOptions& env_options, + std::shared_ptr* reader, + bool* fresh_open) { + assert(reader != nullptr); + assert(fresh_open != nullptr); *fresh_open = false; int64_t current_time = 0; env->GetCurrentTime(¤t_time); last_access_.store(current_time); + Status s; { ReadLock lockbfile_r(&mutex_); - if (ra_file_reader_) return ra_file_reader_; + if (ra_file_reader_) { + *reader = ra_file_reader_; + return s; + } } WriteLock lockbfile_w(&mutex_); - if (ra_file_reader_) return ra_file_reader_; + // Double check. + if (ra_file_reader_) { + *reader = ra_file_reader_; + return s; + } std::unique_ptr rfile; - Status s = env->NewRandomAccessFile(PathName(), &rfile, env_options); + s = env->NewRandomAccessFile(PathName(), &rfile, env_options); if (!s.ok()) { ROCKS_LOG_ERROR(info_log_, "Failed to open blob file for random-read: %s status: '%s'" " exists: '%s'", PathName().c_str(), s.ToString().c_str(), env->FileExists(PathName()).ToString().c_str()); - return nullptr; + return s; } ra_file_reader_ = std::make_shared(std::move(rfile), PathName()); + *reader = ra_file_reader_; *fresh_open = true; - return ra_file_reader_; + return s; } Status BlobFile::ReadMetadata(Env* env, const EnvOptions& env_options) { diff --git a/utilities/blob_db/blob_file.h b/utilities/blob_db/blob_file.h index 288523e77..668a03722 100644 --- a/utilities/blob_db/blob_file.h +++ b/utilities/blob_db/blob_file.h @@ -181,6 +181,10 @@ class BlobFile { // footer_valid_ to false and return Status::OK. Status ReadMetadata(Env* env, const EnvOptions& env_options); + Status GetReader(Env* env, const EnvOptions& env_options, + std::shared_ptr* reader, + bool* fresh_open); + private: std::shared_ptr OpenRandomAccessReader( Env* env, const DBOptions& db_options, @@ -190,9 +194,6 @@ class BlobFile { Status WriteFooterAndCloseLocked(); - std::shared_ptr GetOrOpenRandomAccessReader( - Env* env, const EnvOptions& env_options, bool* fresh_open); - void CloseRandomAccessLocked(); // this is used, when you are reading only the footer of a