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

@@ -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

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<std::string, std::string> 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;

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();
 }
 

file/writable_file_writer.cc

@@ -28,6 +28,11 @@ IOStatus WritableFileWriter::Create(const std::shared_ptr<FileSystem>& fs,
                                     const FileOptions& file_opts,
                                     std::unique_ptr<WritableFileWriter>* 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<FSWritableFile> file;
   IOStatus io_s = fs->NewWritableFile(fname, file_opts, &file, dbg);
   if (io_s.ok()) {

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<void*>(max_buffer_size_));
     buf_.Alignment(writable_file_->GetRequiredBufferAlignment());

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<uint32_t>(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<WritableFileWriter> 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<FSWritableFile> 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<FSWritableFile> 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<FSWritableFile> 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<int64_t>(0.5 * raw_rate));
-  fname = this->dbname_ + "/test_file_1";
+  fname = dbname_ + "/test_file_1";
   std::unique_ptr<FSWritableFile> 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<FakeWF> writable_file_ptr(new FakeWF);
 
   std::unique_ptr<WritableFileWriter> file_writer;