diff --git a/db/flush_job.cc b/db/flush_job.cc index 38048b4e9..037128247 100644 --- a/db/flush_job.cc +++ b/db/flush_job.cc @@ -809,8 +809,6 @@ Status FlushJob::WriteLevel0Table() { { auto write_hint = cfd_->CalculateSSTWriteHint(0); - // https://fburl.com/code/zxh9nif4 : All of WriteController functions - // are to be called while holding DB mutex. Env::IOPriority io_priority = GetRateLimiterPriority(RateLimiter::OpType::kWrite); db_mutex_->Unlock(); diff --git a/file/writable_file_writer.cc b/file/writable_file_writer.cc index c96b31d87..c83670045 100644 --- a/file/writable_file_writer.cc +++ b/file/writable_file_writer.cc @@ -922,7 +922,7 @@ void WritableFileWriter::UpdateIOOptionsIfNeeded( Env::IOPriority rate_limiter_priority_used = WritableFileWriter::DecideRateLimiterPriority( writable_file_->GetIOPriority(), op_rate_limiter_priority); - if (rate_limiter_ != nullptr && rate_limiter_priority_used != Env::IO_TOTAL) { + if (rate_limiter_ == nullptr) { io_options.rate_limiter_priority = rate_limiter_priority_used; } } diff --git a/file/writable_file_writer.h b/file/writable_file_writer.h index ee2bb5e6c..9b49553a3 100644 --- a/file/writable_file_writer.h +++ b/file/writable_file_writer.h @@ -277,6 +277,8 @@ class WritableFileWriter { const char* GetFileChecksumFuncName() const; private: + friend class WritableFileWriterTest_RateLimiterPriority_Test; + static Env::IOPriority DecideRateLimiterPriority( Env::IOPriority writable_file_io_priority, Env::IOPriority op_rate_limiter_priority); diff --git a/util/file_reader_writer_test.cc b/util/file_reader_writer_test.cc index a0da9f8c4..fc82f6fa1 100644 --- a/util/file_reader_writer_test.cc +++ b/util/file_reader_writer_test.cc @@ -248,6 +248,153 @@ TEST_F(WritableFileWriterTest, BufferWithZeroCapacityDirectIO) { } } +TEST_F(WritableFileWriterTest, RateLimiterPriority) { + // This test is to check whether the rate limiter priority can be passed + // correctly from WritableFileWriter functions to FSWritableFile functions. + // Assume rate_limiter is not set. + // There are two major scenarios: + // 1. When op_rate_limiter_priority parameter in WritableFileWriter functions + // is the default (Env::IO_TOTAL). + // 2. When op_rate_limiter_priority parameter in WritableFileWriter + // functions is NOT the default. + class FakeWF : public FSWritableFile { + public: + // The op_rate_limiter_priority_ is to mock the op_rate_limiter_priority + // parameter in some WritableFileWriter functions, e.g. Append. + explicit FakeWF(Env::IOPriority io_priority, + Env::IOPriority op_rate_limiter_priority = Env::IO_TOTAL) + : op_rate_limiter_priority_(op_rate_limiter_priority) { + SetIOPriority(io_priority); + } + ~FakeWF() override {} + + IOStatus Append(const Slice& /*data*/, const IOOptions& options, + IODebugContext* /*dbg*/) override { + CheckRateLimiterPriority(options); + return IOStatus::OK(); + } + + IOStatus Append(const Slice& data, const IOOptions& options, + const DataVerificationInfo& /* verification_info */, + IODebugContext* dbg) override { + return Append(data, options, dbg); + } + + IOStatus Truncate(uint64_t /*size*/, const IOOptions& options, + IODebugContext* /*dbg*/) override { + CheckRateLimiterPriority(options); + return IOStatus::OK(); + } + IOStatus Close(const IOOptions& options, IODebugContext* /*dbg*/) override { + CheckRateLimiterPriority(options); + return IOStatus::OK(); + } + IOStatus Flush(const IOOptions& options, IODebugContext* /*dbg*/) override { + CheckRateLimiterPriority(options); + return IOStatus::OK(); + } + IOStatus Sync(const IOOptions& options, IODebugContext* /*dbg*/) override { + EXPECT_EQ(options.rate_limiter_priority, io_priority_); + return IOStatus::OK(); + } + IOStatus Fsync(const IOOptions& options, IODebugContext* /*dbg*/) override { + CheckRateLimiterPriority(options); + return IOStatus::OK(); + } + // void SetIOPriority(Env::IOPriority /*pri*/) override {} + uint64_t GetFileSize(const IOOptions& options, + IODebugContext* /*dbg*/) override { + CheckRateLimiterPriority(options); + return 0; + } + void GetPreallocationStatus(size_t* /*block_size*/, + size_t* /*last_allocated_block*/) override {} + size_t GetUniqueId(char* /*id*/, size_t /*max_size*/) const override { + return 0; + } + IOStatus InvalidateCache(size_t /*offset*/, size_t /*length*/) override { + return IOStatus::OK(); + } + + IOStatus Allocate(uint64_t /*offset*/, uint64_t /*len*/, + const IOOptions& options, + IODebugContext* /*dbg*/) override { + CheckRateLimiterPriority(options); + return IOStatus::OK(); + } + IOStatus RangeSync(uint64_t /*offset*/, uint64_t /*nbytes*/, + const IOOptions& options, + IODebugContext* /*dbg*/) override { + CheckRateLimiterPriority(options); + return IOStatus::OK(); + } + + void PrepareWrite(size_t /*offset*/, size_t /*len*/, + const IOOptions& options, + IODebugContext* /*dbg*/) override { + CheckRateLimiterPriority(options); + } + + protected: + void CheckRateLimiterPriority(const IOOptions& options) { + // The expected rate limiter priority is decided + // by WritableFileWriter::DecideRateLimiterPriority. + if (io_priority_ == Env::IO_TOTAL) { + EXPECT_EQ(options.rate_limiter_priority, op_rate_limiter_priority_); + } else if (op_rate_limiter_priority_ == Env::IO_TOTAL) { + EXPECT_EQ(options.rate_limiter_priority, io_priority_); + } else { + EXPECT_EQ(options.rate_limiter_priority, op_rate_limiter_priority_); + } + } + + Env::IOPriority op_rate_limiter_priority_; + }; + + FileOptions file_options; + + // When op_rate_limiter_priority parameter in WritableFileWriter functions + // is the default (Env::IO_TOTAL). + Env::IOPriority op_rate_limiter_priority = Env::IO_TOTAL; + std::unique_ptr wf{ + new FakeWF(Env::IO_HIGH, op_rate_limiter_priority)}; + std::unique_ptr writer( + new WritableFileWriter(std::move(wf), "" /* don't care */, file_options)); + writer->Append(Slice("abc")); + writer->Pad(10); + writer->Flush(); + writer->Close(); + writer->Sync(false); + writer->Sync(true); + writer->SyncInternal(false); + writer->SyncInternal(true); + writer->WriteBuffered("abc", 1, Env::IO_TOTAL); + writer->WriteBufferedWithChecksum("abc", 1, Env::IO_TOTAL); + writer->WriteDirect(Env::IO_TOTAL); + writer->WriteDirectWithChecksum(Env::IO_TOTAL); + writer->RangeSync(1, 10); + + // When op_rate_limiter_priority parameter in WritableFileWriter functions + // is NOT the default (Env::IO_TOTAL). + op_rate_limiter_priority = Env::IO_MID; + wf.reset(new FakeWF(Env::IO_USER, op_rate_limiter_priority)); + writer.reset( + new WritableFileWriter(std::move(wf), "" /* don't care */, file_options)); + writer->Append(Slice("abc"), 0, op_rate_limiter_priority); + writer->Pad(10, op_rate_limiter_priority); + writer->Flush(op_rate_limiter_priority); + writer->Close(); + writer->Sync(false); + writer->Sync(true); + writer->SyncInternal(false); + writer->SyncInternal(true); + writer->WriteBuffered("abc", 1, op_rate_limiter_priority); + writer->WriteBufferedWithChecksum("abc", 1, op_rate_limiter_priority); + writer->WriteDirect(op_rate_limiter_priority); + writer->WriteDirectWithChecksum(op_rate_limiter_priority); + writer->RangeSync(1, 10); +} + class DBWritableFileWriterTest : public DBTestBase { public: DBWritableFileWriterTest()