Add more tests for RandomAccessFileReader::MultiRead (#7157)

Summary:
There is a typo in TryMerge which may cause MultiRead to internally read more data than expected, but won't affect MultiRead results' correctness.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/7157

Test Plan: make random_access_file_reader_test && ./random_access_file_reader_test

Reviewed By: siying

Differential Revision: D22670257

Pulled By: cheng-chang

fbshipit-source-id: d261289455a65aa496b348c6e5582b48b12963b7

file/random_access_file_reader.cc

@@ -183,12 +183,6 @@ FSReadRequest Align(const FSReadRequest& r, size_t alignment) {
   return req;
 }
 
-// Try to merge src to dest if they have overlap.
-//
-// Each request represents an inclusive interval [offset, offset + len].
-// If the intervals have overlap, update offset and len to represent the
-// merged interval, and return true.
-// Otherwise, do nothing and return false.
 bool TryMerge(FSReadRequest* dest, const FSReadRequest& src) {
   size_t dest_offset = static_cast<size_t>(dest->offset);
   size_t src_offset = static_cast<size_t>(src.offset);
@@ -234,6 +228,8 @@ Status RandomAccessFileReader::MultiRead(const IOOptions& opts,
           aligned_reqs.push_back(r);
         }
       }
+      TEST_SYNC_POINT_CALLBACK("RandomAccessFileReader::MultiRead:AlignedReqs",
+                               &aligned_reqs);
 
       // Allocate aligned buffer and let scratch buffers point to it.
       size_t total_len = 0;

file/random_access_file_reader.h

@@ -26,6 +26,17 @@ class HistogramImpl;
 
 using AlignedBuf = std::unique_ptr<char[]>;
 
+// Align the request r according to alignment and return the aligned result.
+FSReadRequest Align(const FSReadRequest& r, size_t alignment);
+
+// Try to merge src to dest if they have overlap.
+//
+// Each request represents an inclusive interval [offset, offset + len].
+// If the intervals have overlap, update offset and len to represent the
+// merged interval, and return true.
+// Otherwise, do nothing and return false.
+bool TryMerge(FSReadRequest* dest, const FSReadRequest& src);
+
 // RandomAccessFileReader is a wrapper on top of Env::RnadomAccessFile. It is
 // responsible for:
 // - Handling Buffered and Direct reads appropriately.

file/random_access_file_reader_test.cc

@@ -5,10 +5,13 @@
 
 #include "file/random_access_file_reader.h"
 
+#include <algorithm>
+
 #include "file/file_util.h"
 #include "port/port.h"
 #include "port/stack_trace.h"
 #include "rocksdb/file_system.h"
+#include "test_util/sync_point.h"
 #include "test_util/testharness.h"
 #include "test_util/testutil.h"
 #include "util/random.h"
@@ -101,6 +104,15 @@ TEST_F(RandomAccessFileReaderTest, ReadDirectIO) {
 }
 
 TEST_F(RandomAccessFileReaderTest, MultiReadDirectIO) {
+  std::vector<FSReadRequest> aligned_reqs;
+  ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->SetCallBack(
+      "RandomAccessFileReader::MultiRead:AlignedReqs", [&](void* reqs) {
+        // Copy reqs, since it's allocated on stack inside MultiRead, which will
+        // be deallocated after MultiRead returns.
+        aligned_reqs = *reinterpret_cast<std::vector<FSReadRequest>*>(reqs);
+      });
+  ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->EnableProcessing();
+
   // Creates a file with 3 pages.
   std::string fname = "multi-read-direct-io";
   Random rand(0);
@@ -139,6 +151,12 @@ TEST_F(RandomAccessFileReaderTest, MultiReadDirectIO) {
         r->MultiRead(IOOptions(), reqs.data(), reqs.size(), &aligned_buf));
 
     AssertResult(content, reqs);
+
+    // Reads the first page internally.
+    ASSERT_EQ(aligned_reqs.size(), 1);
+    const FSReadRequest& aligned_r = aligned_reqs[0];
+    ASSERT_EQ(aligned_r.offset, 0);
+    ASSERT_EQ(aligned_r.len, alignment());
   }
 
   {
@@ -177,6 +195,12 @@ TEST_F(RandomAccessFileReaderTest, MultiReadDirectIO) {
         r->MultiRead(IOOptions(), reqs.data(), reqs.size(), &aligned_buf));
 
     AssertResult(content, reqs);
+
+    // Reads the first two pages in one request internally.
+    ASSERT_EQ(aligned_reqs.size(), 1);
+    const FSReadRequest& aligned_r = aligned_reqs[0];
+    ASSERT_EQ(aligned_r.offset, 0);
+    ASSERT_EQ(aligned_r.len, 2 * alignment());
   }
 
   {
@@ -215,6 +239,12 @@ TEST_F(RandomAccessFileReaderTest, MultiReadDirectIO) {
         r->MultiRead(IOOptions(), reqs.data(), reqs.size(), &aligned_buf));
 
     AssertResult(content, reqs);
+
+    // Reads the first 3 pages in one request internally.
+    ASSERT_EQ(aligned_reqs.size(), 1);
+    const FSReadRequest& aligned_r = aligned_reqs[0];
+    ASSERT_EQ(aligned_r.offset, 0);
+    ASSERT_EQ(aligned_r.len, 3 * alignment());
   }
 
   {
@@ -245,11 +275,170 @@ TEST_F(RandomAccessFileReaderTest, MultiReadDirectIO) {
         r->MultiRead(IOOptions(), reqs.data(), reqs.size(), &aligned_buf));
 
     AssertResult(content, reqs);
+
+    // Reads the 1st and 3rd pages in two requests internally.
+    ASSERT_EQ(aligned_reqs.size(), 2);
+    const FSReadRequest& aligned_r0 = aligned_reqs[0];
+    const FSReadRequest& aligned_r1 = aligned_reqs[1];
+    ASSERT_EQ(aligned_r0.offset, 0);
+    ASSERT_EQ(aligned_r0.len, alignment());
+    ASSERT_EQ(aligned_r1.offset, 2 * alignment());
+    ASSERT_EQ(aligned_r1.len, alignment());
   }
+
+  ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->DisableProcessing();
+  ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->ClearAllCallBacks();
 }
 
 #endif  // ROCKSDB_LITE
 
+TEST(FSReadRequest, Align) {
+  FSReadRequest r;
+  r.offset = 2000;
+  r.len = 2000;
+  r.scratch = nullptr;
+
+  FSReadRequest aligned_r = Align(r, 1024);
+  ASSERT_EQ(aligned_r.offset, 1024);
+  ASSERT_EQ(aligned_r.len, 3072);
+}
+
+TEST(FSReadRequest, TryMerge) {
+  // reverse means merging dest into src.
+  for (bool reverse : {true, false}) {
+    {
+      // dest: [ ]
+      //  src:      [ ]
+      FSReadRequest dest;
+      dest.offset = 0;
+      dest.len = 10;
+      dest.scratch = nullptr;
+
+      FSReadRequest src;
+      src.offset = 15;
+      src.len = 10;
+      src.scratch = nullptr;
+
+      if (reverse) std::swap(dest, src);
+      ASSERT_FALSE(TryMerge(&dest, src));
+    }
+
+    {
+      // dest: [ ]
+      //  src:   [ ]
+      FSReadRequest dest;
+      dest.offset = 0;
+      dest.len = 10;
+      dest.scratch = nullptr;
+
+      FSReadRequest src;
+      src.offset = 10;
+      src.len = 10;
+      src.scratch = nullptr;
+
+      if (reverse) std::swap(dest, src);
+      ASSERT_TRUE(TryMerge(&dest, src));
+      ASSERT_EQ(dest.offset, 0);
+      ASSERT_EQ(dest.len, 20);
+    }
+
+    {
+      // dest: [    ]
+      //  src:   [    ]
+      FSReadRequest dest;
+      dest.offset = 0;
+      dest.len = 10;
+      dest.scratch = nullptr;
+
+      FSReadRequest src;
+      src.offset = 5;
+      src.len = 10;
+      src.scratch = nullptr;
+
+      if (reverse) std::swap(dest, src);
+      ASSERT_TRUE(TryMerge(&dest, src));
+      ASSERT_EQ(dest.offset, 0);
+      ASSERT_EQ(dest.len, 15);
+    }
+
+    {
+      // dest: [    ]
+      //  src:   [  ]
+      FSReadRequest dest;
+      dest.offset = 0;
+      dest.len = 10;
+      dest.scratch = nullptr;
+
+      FSReadRequest src;
+      src.offset = 5;
+      src.len = 5;
+      src.scratch = nullptr;
+
+      if (reverse) std::swap(dest, src);
+      ASSERT_TRUE(TryMerge(&dest, src));
+      ASSERT_EQ(dest.offset, 0);
+      ASSERT_EQ(dest.len, 10);
+    }
+
+    {
+      // dest: [     ]
+      //  src:   [ ]
+      FSReadRequest dest;
+      dest.offset = 0;
+      dest.len = 10;
+      dest.scratch = nullptr;
+
+      FSReadRequest src;
+      src.offset = 5;
+      src.len = 1;
+      src.scratch = nullptr;
+
+      if (reverse) std::swap(dest, src);
+      ASSERT_TRUE(TryMerge(&dest, src));
+      ASSERT_EQ(dest.offset, 0);
+      ASSERT_EQ(dest.len, 10);
+    }
+
+    {
+      // dest: [ ]
+      //  src: [ ]
+      FSReadRequest dest;
+      dest.offset = 0;
+      dest.len = 10;
+      dest.scratch = nullptr;
+
+      FSReadRequest src;
+      src.offset = 0;
+      src.len = 10;
+      src.scratch = nullptr;
+
+      if (reverse) std::swap(dest, src);
+      ASSERT_TRUE(TryMerge(&dest, src));
+      ASSERT_EQ(dest.offset, 0);
+      ASSERT_EQ(dest.len, 10);
+    }
+
+    {
+      // dest: [   ]
+      //  src: [ ]
+      FSReadRequest dest;
+      dest.offset = 0;
+      dest.len = 10;
+      dest.scratch = nullptr;
+
+      FSReadRequest src;
+      src.offset = 0;
+      src.len = 5;
+      src.scratch = nullptr;
+
+      if (reverse) std::swap(dest, src);
+      ASSERT_TRUE(TryMerge(&dest, src));
+      ASSERT_EQ(dest.offset, 0);
+      ASSERT_EQ(dest.len, 10);
+    }
+  }
+}
+
 }  // namespace ROCKSDB_NAMESPACE
 
 int main(int argc, char** argv) {