From 2448f80375368ef5e1daf8843b848513c9bbc3a1 Mon Sep 17 00:00:00 2001
From: Jay Edgar <jkedgar@fb.com>
Date: Wed, 6 Apr 2016 15:53:52 -0700
Subject: [PATCH] Make sure that if use_mmap_reads is on use_os_buffer is also
 on

Summary: The code assumes that if use_mmap_reads is on then use_os_buffer is also on.  This make sense as by using memory mapped files for reading you are expecting the OS to cache what it needs.  Add code to make sure the user does not turn off use_os_buffer when they turn on use_mmap_reads

Test Plan: New test: DBTest.MMapAndBufferOptions

Reviewers: sdong

Reviewed By: sdong

Subscribers: andrewkr, dhruba

Differential Revision: https://reviews.facebook.net/D56397
---
 db/db_impl.cc | 62 ++++++++++++++++++++++++++++++++++-----------------
 db/db_test.cc | 20 +++++++++++++++++
 2 files changed, 61 insertions(+), 21 deletions(-)

diff --git a/db/db_impl.cc b/db/db_impl.cc
index 1dd29eae5..4970d7792 100644
--- a/db/db_impl.cc
+++ b/db/db_impl.cc
@@ -201,6 +201,44 @@ Status SanitizeOptionsByTable(
   return Status::OK();
 }
 
+static Status ValidateOptions(
+    const DBOptions& db_options,
+    const std::vector<ColumnFamilyDescriptor>& column_families) {
+  Status s;
+
+  for (auto& cfd : column_families) {
+    s = CheckCompressionSupported(cfd.options);
+    if (s.ok() && db_options.allow_concurrent_memtable_write) {
+      s = CheckConcurrentWritesSupported(cfd.options);
+    }
+    if (!s.ok()) {
+      return s;
+    }
+    if (db_options.db_paths.size() > 1) {
+      if ((cfd.options.compaction_style != kCompactionStyleUniversal) &&
+          (cfd.options.compaction_style != kCompactionStyleLevel)) {
+        return Status::NotSupported(
+            "More than one DB paths are only supported in "
+            "universal and level compaction styles. ");
+      }
+    }
+  }
+
+  if (db_options.db_paths.size() > 4) {
+    return Status::NotSupported(
+        "More than four DB paths are not supported yet. ");
+  }
+
+  if (db_options.allow_mmap_reads && !db_options.allow_os_buffer) {
+    // Protect against assert in PosixMMapReadableFile constructor
+    return Status::NotSupported(
+        "If memory mapped reads (allow_mmap_reads) are enabled "
+        "then os caching (allow_os_buffer) must also be enabled. ");
+  }
+
+  return Status::OK();
+}
+
 CompressionType GetCompressionFlush(const ImmutableCFOptions& ioptions) {
   // Compressing memtable flushes might not help unless the sequential load
   // optimization is used for leveled compaction. Otherwise the CPU and
@@ -5388,27 +5426,9 @@ Status DB::Open(const DBOptions& db_options, const std::string& dbname,
     return s;
   }
 
-  for (auto& cfd : column_families) {
-    s = CheckCompressionSupported(cfd.options);
-    if (s.ok() && db_options.allow_concurrent_memtable_write) {
-      s = CheckConcurrentWritesSupported(cfd.options);
-    }
-    if (!s.ok()) {
-      return s;
-    }
-    if (db_options.db_paths.size() > 1) {
-      if ((cfd.options.compaction_style != kCompactionStyleUniversal) &&
-          (cfd.options.compaction_style != kCompactionStyleLevel)) {
-        return Status::NotSupported(
-            "More than one DB paths are only supported in "
-            "universal and level compaction styles. ");
-      }
-    }
-  }
-
-  if (db_options.db_paths.size() > 4) {
-    return Status::NotSupported(
-        "More than four DB paths are not supported yet. ");
+  s = ValidateOptions(db_options, column_families);
+  if (!s.ok()) {
+    return s;
   }
 
   *dbptr = nullptr;
diff --git a/db/db_test.cc b/db/db_test.cc
index ae67c074b..86500483f 100644
--- a/db/db_test.cc
+++ b/db/db_test.cc
@@ -5876,6 +5876,26 @@ TEST_F(DBTest, TableOptionsSanitizeTest) {
   ASSERT_OK(TryReopen(options));
 }
 
+TEST_F(DBTest, MmapAndBufferOptions) {
+  Options options = CurrentOptions();
+
+  // If allow_mmap_reads is on allow_os_buffer must also be on
+  options.allow_os_buffer = false;
+  options.allow_mmap_reads = true;
+  ASSERT_NOK(TryReopen(options));
+
+  // All other combinations are acceptable
+  options.allow_os_buffer = true;
+  ASSERT_OK(TryReopen(options));
+
+  options.allow_os_buffer = false;
+  options.allow_mmap_reads = false;
+  ASSERT_OK(TryReopen(options));
+
+  options.allow_os_buffer = true;
+  ASSERT_OK(TryReopen(options));
+}
+
 TEST_F(DBTest, ConcurrentMemtableNotSupported) {
   Options options = CurrentOptions();
   options.allow_concurrent_memtable_write = true;