diff --git a/file/writable_file_writer.cc b/file/writable_file_writer.cc index 6680c5bf7..fd7cb39eb 100644 --- a/file/writable_file_writer.cc +++ b/file/writable_file_writer.cc @@ -10,7 +10,6 @@ #include "file/writable_file_writer.h" #include -#include #include #include "db/version_edit.h" @@ -56,7 +55,8 @@ IOStatus WritableFileWriter::Append(const Slice& data, uint32_t crc32c_checksum, { IOOptions io_options; - UpdateIOOptionsIfNeeded(io_options, op_rate_limiter_priority); + WritableFileWriter::DecideRateLimiterPriority( + writable_file_->GetIOPriority(), op_rate_limiter_priority, io_options); IOSTATS_TIMER_GUARD(prepare_write_nanos); TEST_SYNC_POINT("WritableFileWriter::Append:BeforePrepareWrite"); writable_file_->PrepareWrite(static_cast(GetFileSize()), left, @@ -337,7 +337,8 @@ IOStatus WritableFileWriter::Flush(Env::IOPriority op_rate_limiter_priority) { } #endif IOOptions io_options; - UpdateIOOptionsIfNeeded(io_options, op_rate_limiter_priority); + WritableFileWriter::DecideRateLimiterPriority( + writable_file_->GetIOPriority(), op_rate_limiter_priority, io_options); s = writable_file_->Flush(io_options, nullptr); #ifndef ROCKSDB_LITE if (ShouldNotifyListeners()) { @@ -445,10 +446,9 @@ IOStatus WritableFileWriter::SyncInternal(bool use_fsync) { start_ts = FileOperationInfo::StartNow(); } #endif - std::cout << "SyncInternal()" << std::endl; + IOOptions io_options; io_options.rate_limiter_priority = writable_file_->GetIOPriority(); - std::cout << "SyncInternal() - 2" << std::endl; if (use_fsync) { s = writable_file_->Fsync(io_options, nullptr); } else { @@ -506,11 +506,11 @@ IOStatus WritableFileWriter::WriteBuffered( size_t left = size; DataVerificationInfo v_info; char checksum_buf[sizeof(uint32_t)]; + IOOptions io_options; Env::IOPriority rate_limiter_priority_used = WritableFileWriter::DecideRateLimiterPriority( - writable_file_->GetIOPriority(), op_rate_limiter_priority); - IOOptions io_options; - io_options.rate_limiter_priority = rate_limiter_priority_used; + writable_file_->GetIOPriority(), op_rate_limiter_priority, + io_options); while (left > 0) { size_t allowed = left; @@ -595,11 +595,11 @@ IOStatus WritableFileWriter::WriteBufferedWithChecksum( size_t left = size; DataVerificationInfo v_info; char checksum_buf[sizeof(uint32_t)]; + IOOptions io_options; Env::IOPriority rate_limiter_priority_used = WritableFileWriter::DecideRateLimiterPriority( - writable_file_->GetIOPriority(), op_rate_limiter_priority); - IOOptions io_options; - io_options.rate_limiter_priority = rate_limiter_priority_used; + writable_file_->GetIOPriority(), op_rate_limiter_priority, + io_options); // Check how much is allowed. Here, we loop until the rate limiter allows to // write the entire buffer. // TODO: need to be improved since it sort of defeats the purpose of the rate @@ -725,11 +725,11 @@ IOStatus WritableFileWriter::WriteDirect( size_t left = buf_.CurrentSize(); DataVerificationInfo v_info; char checksum_buf[sizeof(uint32_t)]; + IOOptions io_options; Env::IOPriority rate_limiter_priority_used = WritableFileWriter::DecideRateLimiterPriority( - writable_file_->GetIOPriority(), op_rate_limiter_priority); - IOOptions io_options; - io_options.rate_limiter_priority = rate_limiter_priority_used; + writable_file_->GetIOPriority(), op_rate_limiter_priority, + io_options); while (left > 0) { // Check how much is allowed @@ -826,11 +826,11 @@ IOStatus WritableFileWriter::WriteDirectWithChecksum( DataVerificationInfo v_info; char checksum_buf[sizeof(uint32_t)]; + IOOptions io_options; Env::IOPriority rate_limiter_priority_used = WritableFileWriter::DecideRateLimiterPriority( - writable_file_->GetIOPriority(), op_rate_limiter_priority); - IOOptions io_options; - io_options.rate_limiter_priority = rate_limiter_priority_used; + writable_file_->GetIOPriority(), op_rate_limiter_priority, + io_options); // Check how much is allowed. Here, we loop until the rate limiter allows to // write the entire buffer. // TODO: need to be improved since it sort of defeats the purpose of the rate @@ -898,24 +898,20 @@ IOStatus WritableFileWriter::WriteDirectWithChecksum( #endif // !ROCKSDB_LITE Env::IOPriority WritableFileWriter::DecideRateLimiterPriority( Env::IOPriority writable_file_io_priority, - Env::IOPriority op_rate_limiter_priority) { + Env::IOPriority op_rate_limiter_priority, IOOptions& io_options) { + Env::IOPriority rate_limiter_priority{Env::IO_TOTAL}; if (writable_file_io_priority == Env::IO_TOTAL && op_rate_limiter_priority == Env::IO_TOTAL) { - return Env::IO_TOTAL; + rate_limiter_priority = Env::IO_TOTAL; } else if (writable_file_io_priority == Env::IO_TOTAL) { - return op_rate_limiter_priority; + rate_limiter_priority = op_rate_limiter_priority; } else if (op_rate_limiter_priority == Env::IO_TOTAL) { - return writable_file_io_priority; + rate_limiter_priority = writable_file_io_priority; } else { - return op_rate_limiter_priority; + rate_limiter_priority = op_rate_limiter_priority; } + io_options.rate_limiter_priority = rate_limiter_priority; + return rate_limiter_priority; } -void WritableFileWriter::UpdateIOOptionsIfNeeded( - IOOptions& io_options, const Env::IOPriority op_rate_limiter_priority) { - Env::IOPriority rate_limiter_priority_used = - WritableFileWriter::DecideRateLimiterPriority( - writable_file_->GetIOPriority(), op_rate_limiter_priority); - io_options.rate_limiter_priority = rate_limiter_priority_used; -} } // namespace ROCKSDB_NAMESPACE diff --git a/file/writable_file_writer.h b/file/writable_file_writer.h index 9b49553a3..7663a360e 100644 --- a/file/writable_file_writer.h +++ b/file/writable_file_writer.h @@ -277,15 +277,10 @@ class WritableFileWriter { const char* GetFileChecksumFuncName() const; private: - friend class WritableFileWriterTest_RateLimiterPriority_Test; - + // Decide the Rate Limiter priority and update io_options. static Env::IOPriority DecideRateLimiterPriority( Env::IOPriority writable_file_io_priority, - Env::IOPriority op_rate_limiter_priority); - - // If rate_limiter_ is nullptr, the rate_limiter_priority may be updated. - void UpdateIOOptionsIfNeeded(IOOptions& io_options, - const Env::IOPriority op_rate_limiter_priority); + Env::IOPriority op_rate_limiter_priority, IOOptions& io_options); // Used when os buffering is OFF and we are writing // DMA such as in Direct I/O mode diff --git a/util/writable_file_io_priority_test.cc b/util/writable_file_io_priority_test.cc index 4fa5e73c4..3dc3e3dbe 100644 --- a/util/writable_file_io_priority_test.cc +++ b/util/writable_file_io_priority_test.cc @@ -4,22 +4,11 @@ // (found in the LICENSE.Apache file in the root directory). // #include -#include -#include "db/db_test_util.h" -#include "env/mock_env.h" -#include "file/line_file_reader.h" -#include "file/random_access_file_reader.h" -#include "file/read_write_util.h" -#include "file/readahead_raf.h" -#include "file/sequence_file_reader.h" #include "file/writable_file_writer.h" #include "rocksdb/file_system.h" #include "test_util/testharness.h" #include "test_util/testutil.h" -#include "util/crc32c.h" -#include "util/random.h" -#include "utilities/fault_injection_fs.h" namespace ROCKSDB_NAMESPACE { @@ -27,6 +16,24 @@ class WritableFileWriterIOPriorityTest : public testing::Test { public: WritableFileWriterIOPriorityTest() = default; ~WritableFileWriterIOPriorityTest() = default; + + void SetUp() override { + FileOptions file_options; + + // When op_rate_limiter_priority parameter in WritableFileWriter functions + // is the default (Env::IO_TOTAL). + std::unique_ptr wf_1{new FakeWF(Env::IO_HIGH, Env::IO_TOTAL)}; + writer_without_op_priority_.reset(new WritableFileWriter( + std::move(wf_1), "" /* don't care */, file_options)); + + // When op_rate_limiter_priority parameter in WritableFileWriter functions + // is NOT the default (Env::IO_TOTAL). + // std::unique_ptr wf_2{ + // new FakeWF(Env::IO_USER, Env::IO_MID)}; + // writer_with_op_priority_.reset(new WritableFileWriter( + // std::move(wf_2), "" /* don't care */, file_options)); + } + protected: // This test is to check whether the rate limiter priority can be passed // correctly from WritableFileWriter functions to FSWritableFile functions. @@ -144,72 +151,42 @@ protected: Env::IOPriority op_rate_limiter_priority_; }; + std::shared_ptr writer_without_op_priority_; + std::shared_ptr writer_with_op_priority_; }; -TEST_F(WritableFileWriterIOPriorityTest, RateLimiterPriority) { - // When op_rate_limiter_priority parameter in WritableFileWriter functions - // is the default (Env::IO_TOTAL). - FileOptions file_options; - 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(); +// 1. When op_rate_limiter_priority parameter in WritableFileWriter functions +// is the default (Env::IO_TOTAL). - // writer->SyncInternal(false); - // std::cout << "SyncInternal(false)" << std::endl; - // 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->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); +TEST_F(WritableFileWriterIOPriorityTest, Append_Default) { + writer_without_op_priority_->Append(Slice("abc")); } -TEST_F(WritableFileWriterIOPriorityTest, RateLimiterPriority2) { - // When op_rate_limiter_priority parameter in WritableFileWriter functions - // is NOT the default (Env::IO_TOTAL). - FileOptions file_options; - Env::IOPriority op_rate_limiter_priority = Env::IO_MID; - std::unique_ptr wf{ - new FakeWF(Env::IO_USER, op_rate_limiter_priority)}; - std::unique_ptr writer( - 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->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); +TEST_F(WritableFileWriterIOPriorityTest, Pad_Default) { + writer_without_op_priority_->Pad(10); } +TEST_F(WritableFileWriterIOPriorityTest, Flush_Default) { + writer_without_op_priority_->Flush(); +} + +TEST_F(WritableFileWriterIOPriorityTest, Close_Default) { + writer_without_op_priority_->Close(); +} + +TEST_F(WritableFileWriterIOPriorityTest, Sync_Default) { + writer_without_op_priority_->Sync(false); + writer_without_op_priority_->Sync(true); +} + +TEST_F(WritableFileWriterIOPriorityTest, SyncWithoutFlush_Default) { + writer_without_op_priority_->SyncWithoutFlush(false); + writer_without_op_priority_->SyncWithoutFlush(true); +} + +// 2. When op_rate_limiter_priority parameter in WritableFileWriter +// functions is NOT the default. + } // namespace ROCKSDB_NAMESPACE int main(int argc, char** argv) {