Fix null scratch buffer submitted to async read on aligned direct-IO path (#14907)

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

`RandomAccessFileReader::ReadAsync` took its "already aligned" fast path when the request offset and length were sector-aligned, forwarding the caller's `req` (including its `scratch` pointer) straight to the FileSystem. A null `scratch` trivially satisfied the alignment check (`uintptr_t(nullptr) & (alignment - 1) == 0`), so when a caller submitted an aligned request with `scratch == nullptr` and an `aligned_buf` out-parameter for the reader to allocate into, the null buffer was submitted to the async read. For direct IO with io_uring this becomes a null `iovec` base, which the kernel rejects with `EFAULT` (surfaced as `Req failed: Unknown error -14`).

This is exactly how `IODispatcher` submits MultiScan async reads for plain direct IO (`scratch == nullptr`, `aligned_buf` provided), so the failure manifested intermittently as `db_stress` iterator divergence ("Iterator diverged from control iterator") in crash tests with `use_direct_reads=1 --use_multiscan=1 --multiscan_use_async_io=1`. It is intermittent because it only triggers when the coalesced read's offset and length both happen to be sector-aligned. Task T267030385.

The fix excludes the aligned fast path when the caller provided no `scratch` but did provide an `aligned_buf`, forcing the allocating path so a valid backing buffer is always handed to the async read. This matches the existing behavior of the synchronous `Read`, which already guards its aligned fast path with `scratch != nullptr`.

Reviewed By: xingbowang

Differential Revision: D110361301

fbshipit-source-id: 3884f7ddbc746cf51b2566d74918e2fb97233dcb

file/random_access_file_reader.cc

@@ -584,7 +584,17 @@ IOStatus RandomAccessFileReader::ReadAsync(
   }
 
   size_t alignment = file_->GetRequiredBufferAlignment();
-  bool is_aligned = direct_io_buffer == nullptr &&
+  // The "already aligned" fast path forwards the caller's request (including
+  // its scratch pointer) straight to the FileSystem. It is only safe when the
+  // caller actually supplied a scratch buffer to read into. A null scratch
+  // combined with an `aligned_buf` out-parameter means the caller expects this
+  // reader to allocate the backing buffer (returned via `aligned_buf`), so we
+  // must take the allocating path below. Otherwise a null buffer would be
+  // submitted to the async read (e.g. a null iovec base to io_uring, which
+  // fails with EFAULT).
+  const bool caller_needs_allocation =
+      req.scratch == nullptr && aligned_buf != nullptr;
+  bool is_aligned = direct_io_buffer == nullptr && !caller_needs_allocation &&
                     (req.offset & (alignment - 1)) == 0 &&
                     (req.len & (alignment - 1)) == 0 &&
                     (uintptr_t(req.scratch) & (alignment - 1)) == 0;

file/random_access_file_reader_test.cc

@@ -55,7 +55,7 @@ class RandomAccessFileReaderTest : public testing::Test {
     }
   }
 
- private:
+ protected:
   Env* env_;
   std::shared_ptr<FileSystem> fs_;
   std::string test_dir_;
@@ -63,6 +63,46 @@ class RandomAccessFileReaderTest : public testing::Test {
   std::string Path(const std::string& fname) { return test_dir_ + "/" + fname; }
 };
 
+namespace {
+// Wraps an FSRandomAccessFile to observe the FSReadRequest that
+// RandomAccessFileReader::ReadAsync forwards to the FileSystem layer. It
+// records whether a request with a non-empty length but a null scratch buffer
+// was submitted. A null scratch means no backing buffer was provided for the
+// read; in production this surfaces as an EFAULT from io_uring (a null iovec
+// base), reported as "Req failed: Unknown error -14".
+class ScratchObservingRandomAccessFile : public FSRandomAccessFileOwnerWrapper {
+ public:
+  ScratchObservingRandomAccessFile(std::unique_ptr<FSRandomAccessFile>&& target,
+                                   bool* saw_null_scratch)
+      : FSRandomAccessFileOwnerWrapper(std::move(target)),
+        saw_null_scratch_(saw_null_scratch) {}
+
+  IOStatus ReadAsync(FSReadRequest& req, const IOOptions& opts,
+                     std::function<void(FSReadRequest&, void*)> cb,
+                     void* cb_arg, void** /*io_handle*/,
+                     IOHandleDeleter* /*del_fn*/,
+                     IODebugContext* dbg) override {
+    if (req.len > 0 && req.scratch == nullptr) {
+      *saw_null_scratch_ = true;
+      // A real direct-IO read with a null scratch would fault; synthesize an
+      // error completion instead so the test can observe the submission.
+      req.result = Slice();
+      req.status = IOStatus::IOError("null scratch buffer submitted");
+    } else {
+      // Complete synchronously so the test does not depend on io_uring runtime
+      // availability. Mirrors FSRandomAccessFile::ReadAsync's default fallback.
+      req.status =
+          Read(req.offset, req.len, opts, &req.result, req.scratch, dbg);
+    }
+    cb(req, cb_arg);
+    return IOStatus::OK();
+  }
+
+ private:
+  bool* saw_null_scratch_;
+};
+}  // namespace
+
 // Skip the following tests in lite mode since direct I/O is unsupported.
 
 TEST_F(RandomAccessFileReaderTest, ReadDirectIO) {
@@ -444,6 +484,79 @@ TEST_F(RandomAccessFileReaderTest, MultiReadDirectIOUsesExternalBuffer) {
   }
 }
 
+// Regression test for a direct-IO async-read buffer bug. When a caller submits
+// an already-aligned FSReadRequest with a null scratch and provides an
+// `aligned_buf` out-parameter for the reader to allocate the backing buffer
+// into (exactly how IODispatcher submits MultiScan async reads for plain direct
+// IO), RandomAccessFileReader::ReadAsync must NOT take its "already aligned"
+// fast path and forward the null scratch to the FileSystem. Doing so hands
+// io_uring a null iovec base, which fails with EFAULT ("Req failed: Unknown
+// error -14").
+TEST_F(RandomAccessFileReaderTest, ReadAsyncDirectIOAlignedNullScratch) {
+  std::string fname = "read-async-direct-io-aligned-null-scratch";
+  Random rand(0);
+  std::string content = rand.RandomString(kDefaultPageSize);
+  Write(fname, content);
+
+  FileOptions opts;
+  opts.use_direct_reads = true;
+  std::string fpath = Path(fname);
+  std::unique_ptr<FSRandomAccessFile> f;
+  ASSERT_OK(fs_->NewRandomAccessFile(fpath, opts, &f, nullptr));
+
+  bool saw_null_scratch = false;
+  std::unique_ptr<FSRandomAccessFile> wrapped(
+      new ScratchObservingRandomAccessFile(std::move(f), &saw_null_scratch));
+  std::unique_ptr<RandomAccessFileReader> r(new RandomAccessFileReader(
+      std::move(wrapped), fpath, env_->GetSystemClock().get()));
+  ASSERT_TRUE(r->use_direct_io());
+
+  const size_t page_size = r->file()->GetRequiredBufferAlignment();
+
+  // Offset and length are both alignment-aligned, so the reader would take its
+  // "already aligned" fast path. scratch is null and an aligned_buf is
+  // provided, so the reader is expected to allocate the backing buffer itself.
+  FSReadRequest req;
+  req.offset = 0;
+  req.len = page_size;
+  req.scratch = nullptr;
+
+  AlignedBuf aligned_buf;
+  bool completed = false;
+  IOStatus completed_status;
+  Slice completed_result;
+  auto cb = [&](FSReadRequest& done, void* /*arg*/) {
+    completed = true;
+    completed_status = done.status;
+    completed_result = done.result;
+  };
+
+  void* io_handle = nullptr;
+  IOHandleDeleter del_fn = nullptr;
+  ASSERT_OK(r->ReadAsync(req, IOOptions(), cb, /*cb_arg=*/nullptr, &io_handle,
+                         &del_fn, &aligned_buf, /*dbg=*/nullptr,
+                         /*direct_io_buffer_context=*/nullptr));
+  // The read result is delivered via the callback (checked below); the reader
+  // works on an internal copy and leaves this request's status untouched.
+  req.status.PermitUncheckedError();
+
+  if (io_handle != nullptr && del_fn != nullptr) {
+    std::vector<void*> handles{io_handle};
+    ASSERT_OK(fs_->Poll(handles, handles.size()));
+    del_fn(io_handle);
+  }
+
+  ASSERT_TRUE(completed);
+  // Core assertion: the reader must allocate a backing buffer for the aligned
+  // direct-IO async read rather than forwarding a null scratch to the FS.
+  EXPECT_FALSE(saw_null_scratch)
+      << "ReadAsync forwarded a null scratch on the aligned direct-IO fast "
+         "path; io_uring would fail this read with EFAULT";
+  ASSERT_OK(completed_status);
+  ASSERT_EQ(completed_result.size(), req.len);
+  ASSERT_EQ(completed_result.ToString(), content.substr(0, req.len));
+}
+
 TEST(FSReadRequest, Align) {
   FSReadRequest r;
   r.offset = 2000;

unreleased_history/bug_fixes/read_async_direct_io_null_scratch.md

@@ -0,0 +1 @@
+Fixed a bug in `RandomAccessFileReader::ReadAsync` where an already-aligned direct-IO read request with a null `scratch` and a caller-provided `aligned_buf` would take the "already aligned" fast path and submit the null buffer to the underlying async read (e.g. a null iovec base to io_uring, failing with EFAULT). This could surface as spurious iterator failures during async prefetch (MultiScan with async IO) on direct-IO databases. The async path now allocates a backing buffer in this case, matching the synchronous `Read` path.