From dd203ed6041000ba728a7ad92b3d568ccd4d4e21 Mon Sep 17 00:00:00 2001 From: Yanqin Jin Date: Thu, 27 Jan 2022 19:29:17 -0800 Subject: [PATCH] Disallow a combination of options (#9348) Summary: Disallow `immutable_db_opts.use_direct_io_for_flush_and_compaction == true` and `mutable_db_opts.writable_file_max_buffer_size == 0`, since it causes `WritableFileWriter::Append()` to loop forever and does not make much sense in direct IO. This combination of options itself does not make much sense: asking RocksDB to do direct IO but not allowing RocksDB to allocate a buffer. We should detect this false combination and warn user early, no matter whether the application is running on a platform that supports direct IO or not. In the case of platform **not** supporting direct IO, it's ok if the user learns about this and then finds that direct IO is not supported. One tricky thing: the constructor of `WritableFileWriter` is being used in our unit tests, and it's impossible to return status code from constructor. Since we do not throw, I put an assertion for now. Fortunately, the constructor is not exposed to external applications. Closing https://github.com/facebook/rocksdb/issues/7109 Pull Request resolved: https://github.com/facebook/rocksdb/pull/9348 Test Plan: make check Reviewed By: ajkr Differential Revision: D33371924 Pulled By: riversand963 fbshipit-source-id: 2a3701ab541cee23bffda8a36cdf37b2d235edfa --- HISTORY.md | 3 +++ db/db_basic_test.cc | 18 ++++++++++++++++++ db/db_impl/db_impl_open.cc | 6 ++++++ file/writable_file_writer.cc | 5 +++++ file/writable_file_writer.h | 1 + util/file_reader_writer_test.cc | 26 ++++++++++++++++++++------ 6 files changed, 53 insertions(+), 6 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 2865b64e8..4adc0300b 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -14,6 +14,9 @@ * Remove deprecated API AdvancedColumnFamilyOptions::soft_rate_limit. * Remove deprecated API AdvancedColumnFamilyOptions::hard_rate_limit. +### Behavior Changes +* Disallow the combination of DBOptions.use_direct_io_for_flush_and_compaction == true and DBOptions.writable_file_max_buffer_size == 0. This combination can cause WritableFileWriter::Append() to loop forever, and it does not make much sense in direct IO. + ## 6.29.0 (01/21/2022) Note: The next release will be major release 7.0. See https://github.com/facebook/rocksdb/issues/9390 for more info. ### Public API change diff --git a/db/db_basic_test.cc b/db/db_basic_test.cc index 96687ba74..f27532c11 100644 --- a/db/db_basic_test.cc +++ b/db/db_basic_test.cc @@ -51,6 +51,24 @@ TEST_F(DBBasicTest, OpenWhenOpen) { delete db2; } +TEST_F(DBBasicTest, EnableDirectIOWithZeroBuf) { + if (!IsDirectIOSupported()) { + ROCKSDB_GTEST_BYPASS("Direct IO not supported"); + return; + } + Options options = GetDefaultOptions(); + options.create_if_missing = true; + options.use_direct_io_for_flush_and_compaction = true; + options.writable_file_max_buffer_size = 0; + ASSERT_TRUE(TryReopen(options).IsInvalidArgument()); + + options.writable_file_max_buffer_size = 1024; + Reopen(options); + const std::unordered_map new_db_opts = { + {"writable_file_max_buffer_size", "0"}}; + ASSERT_TRUE(db_->SetDBOptions(new_db_opts).IsInvalidArgument()); +} + TEST_F(DBBasicTest, UniqueSession) { Options options = CurrentOptions(); std::string sid1, sid2, sid3, sid4; diff --git a/db/db_impl/db_impl_open.cc b/db/db_impl/db_impl_open.cc index f9be6f425..ccd245786 100644 --- a/db/db_impl/db_impl_open.cc +++ b/db/db_impl/db_impl_open.cc @@ -294,6 +294,12 @@ Status DBImpl::ValidateOptions(const DBOptions& db_options) { "atomic_flush is currently incompatible with best-efforts recovery"); } + if (db_options.use_direct_io_for_flush_and_compaction && + 0 == db_options.writable_file_max_buffer_size) { + return Status::InvalidArgument( + "writes in direct IO require writable_file_max_buffer_size > 0"); + } + return Status::OK(); } diff --git a/file/writable_file_writer.cc b/file/writable_file_writer.cc index d79f74fa7..f345c6572 100644 --- a/file/writable_file_writer.cc +++ b/file/writable_file_writer.cc @@ -28,6 +28,11 @@ IOStatus WritableFileWriter::Create(const std::shared_ptr& fs, const FileOptions& file_opts, std::unique_ptr* writer, IODebugContext* dbg) { + if (file_opts.use_direct_writes && + 0 == file_opts.writable_file_max_buffer_size) { + return IOStatus::InvalidArgument( + "Direct write requires writable_file_max_buffer_size > 0"); + } std::unique_ptr file; IOStatus io_s = fs->NewWritableFile(fname, file_opts, &file, dbg); if (io_s.ok()) { diff --git a/file/writable_file_writer.h b/file/writable_file_writer.h index 0ff76e0a6..ca59a0b43 100644 --- a/file/writable_file_writer.h +++ b/file/writable_file_writer.h @@ -190,6 +190,7 @@ class WritableFileWriter { perform_data_verification_(perform_data_verification), buffered_data_crc32c_checksum_(0), buffered_data_with_checksum_(buffered_data_with_checksum) { + assert(!use_direct_io() || max_buffer_size_ > 0); TEST_SYNC_POINT_CALLBACK("WritableFileWriter::WritableFileWriter:0", reinterpret_cast(max_buffer_size_)); buf_.Alignment(writable_file_->GetRequiredBufferAlignment()); diff --git a/util/file_reader_writer_test.cc b/util/file_reader_writer_test.cc index edd71b899..a0da9f8c4 100644 --- a/util/file_reader_writer_test.cc +++ b/util/file_reader_writer_test.cc @@ -25,7 +25,7 @@ namespace ROCKSDB_NAMESPACE { class WritableFileWriterTest : public testing::Test {}; -const uint32_t kMb = 1 << 20; +constexpr uint32_t kMb = static_cast(1) << 20; TEST_F(WritableFileWriterTest, RangeSync) { class FakeWF : public FSWritableFile { @@ -234,6 +234,20 @@ TEST_F(WritableFileWriterTest, IncrementalBuffer) { } } +TEST_F(WritableFileWriterTest, BufferWithZeroCapacityDirectIO) { + EnvOptions env_opts; + env_opts.use_direct_writes = true; + env_opts.writable_file_max_buffer_size = 0; + { + std::unique_ptr writer; + const Status s = + WritableFileWriter::Create(FileSystem::Default(), /*fname=*/"dont_care", + FileOptions(env_opts), &writer, + /*dbg=*/nullptr); + ASSERT_TRUE(s.IsInvalidArgument()); + } +} + class DBWritableFileWriterTest : public DBTestBase { public: DBWritableFileWriterTest() @@ -251,7 +265,7 @@ TEST_F(DBWritableFileWriterTest, AppendWithChecksum) { Options options = GetDefaultOptions(); options.create_if_missing = true; DestroyAndReopen(options); - std::string fname = this->dbname_ + "/test_file"; + std::string fname = dbname_ + "/test_file"; std::unique_ptr writable_file_ptr; ASSERT_OK(fault_fs_->NewWritableFile(fname, file_options, &writable_file_ptr, /*dbg*/ nullptr)); @@ -291,7 +305,7 @@ TEST_F(DBWritableFileWriterTest, AppendVerifyNoChecksum) { Options options = GetDefaultOptions(); options.create_if_missing = true; DestroyAndReopen(options); - std::string fname = this->dbname_ + "/test_file"; + std::string fname = dbname_ + "/test_file"; std::unique_ptr writable_file_ptr; ASSERT_OK(fault_fs_->NewWritableFile(fname, file_options, &writable_file_ptr, /*dbg*/ nullptr)); @@ -334,7 +348,7 @@ TEST_F(DBWritableFileWriterTest, AppendWithChecksumRateLimiter) { Options options = GetDefaultOptions(); options.create_if_missing = true; DestroyAndReopen(options); - std::string fname = this->dbname_ + "/test_file"; + std::string fname = dbname_ + "/test_file"; std::unique_ptr writable_file_ptr; ASSERT_OK(fault_fs_->NewWritableFile(fname, file_options, &writable_file_ptr, /*dbg*/ nullptr)); @@ -376,7 +390,7 @@ TEST_F(DBWritableFileWriterTest, AppendWithChecksumRateLimiter) { FileOptions file_options1 = FileOptions(); file_options1.rate_limiter = NewGenericRateLimiter(static_cast(0.5 * raw_rate)); - fname = this->dbname_ + "/test_file_1"; + fname = dbname_ + "/test_file_1"; std::unique_ptr writable_file_ptr1; ASSERT_OK(fault_fs_->NewWritableFile(fname, file_options1, &writable_file_ptr1, @@ -857,7 +871,7 @@ TEST_F(DBWritableFileWriterTest, IOErrorNotification) { DestroyAndReopen(options); ImmutableOptions ioptions(options); - std::string fname = this->dbname_ + "/test_file"; + std::string fname = dbname_ + "/test_file"; std::unique_ptr writable_file_ptr(new FakeWF); std::unique_ptr file_writer;