Update to FlexibleRead

file/file_prefetch_buffer.cc

@@ -92,10 +92,9 @@ void FilePrefetchBuffer::PrepareBufferForRead(
 
 Status FilePrefetchBuffer::Read(BufferInfo* buf, const IOOptions& opts,
                                 RandomAccessFileReader* reader,
-                                const uint64_t original_length,
                                 uint64_t read_len, uint64_t aligned_useful_len,
-                                uint64_t start_offset, bool use_fs_buffer,
-                                bool for_compaction) {
+                                uint64_t optional_read_size,
+                                uint64_t start_offset, bool use_fs_buffer) {
   Slice result;
   Status s;
   char* to_buf = nullptr;
@@ -104,11 +103,9 @@ Status FilePrefetchBuffer::Read(BufferInfo* buf, const IOOptions& opts,
                            read_len, result);
   } else {
     to_buf = buf->buffer_.BufferStart() + aligned_useful_len;
-    bool issue_flexible_read = for_compaction && !reader->use_direct_io() &&
-                               original_length < read_len;
-    if (issue_flexible_read) {
-      s = FlexibleMultiRead(reader, opts, start_offset + aligned_useful_len,
-                            original_length, read_len, to_buf, result);
+    if (0 < optional_read_size) {
+      s = FlexibleRead(reader, opts, start_offset + aligned_useful_len,
+                       read_len, optional_read_size, to_buf, result);
     } else {
       s = reader->Read(opts, start_offset + aligned_useful_len, read_len,
                        &result, to_buf, /*aligned_buf=*/nullptr);
@@ -205,9 +202,13 @@ Status FilePrefetchBuffer::Prefetch(const IOOptions& opts,
 
   Status s;
   if (read_len > 0) {
-    s = Read(buf, opts, reader, /*original_length=*/n, read_len,
-             aligned_useful_len, rounddown_offset, use_fs_buffer,
-             /*for_compaction=*/false);
+    // Currently FilePrefetchBuffer::Prefetch is used in
+    // BlockBasedTable::PrefetchTail. Our optimization for FlexibleRead is meant
+    // for when we want to start from the beginning of the file in compaction
+    // and read the whole file sequentially. It is probably not worth setting
+    // optimal_read_size > 0 in this case.
+    s = Read(buf, opts, reader, read_len, aligned_useful_len,
+             /*optional_read_size=*/0, rounddown_offset, use_fs_buffer);
   }
 
   if (usage_ == FilePrefetchBufferUsage::kTableOpenPrefetchTail && s.ok()) {
@@ -745,8 +746,12 @@ Status FilePrefetchBuffer::PrefetchInternal(const IOOptions& opts,
   }
 
   if (read_len1 > 0) {
-    s = Read(buf, opts, reader, original_length, read_len1, aligned_useful_len1,
-             start_offset1, use_fs_buffer, for_compaction);
+    size_t optional_read_length = for_compaction && !reader->use_direct_io() &&
+                                          original_length < read_len1
+                                      ? read_len1 - original_length
+                                      : 0;
+    s = Read(buf, opts, reader, read_len1, aligned_useful_len1,
+             optional_read_length, start_offset1, use_fs_buffer);
     if (!s.ok()) {
       AbortAllIOs();
       FreeAllBuffers();

file/file_prefetch_buffer.h

@@ -437,9 +437,9 @@ class FilePrefetchBuffer {
                           bool for_compaction, bool& copy_to_third_buffer);
 
   Status Read(BufferInfo* buf, const IOOptions& opts,
-              RandomAccessFileReader* reader, const uint64_t original_length,
-              uint64_t read_len, uint64_t aligned_useful_len,
-              uint64_t start_offset, bool use_fs_buffer, bool for_compaction);
+              RandomAccessFileReader* reader, uint64_t read_len,
+              uint64_t aligned_useful_len, uint64_t optional_read_size,
+              uint64_t start_offset, bool use_fs_buffer);
 
   Status ReadAsync(BufferInfo* buf, const IOOptions& opts,
                    RandomAccessFileReader* reader, uint64_t read_len,
@@ -541,16 +541,18 @@ class FilePrefetchBuffer {
     return s;
   }
 
-  IOStatus FlexibleMultiRead(RandomAccessFileReader* reader,
-                             const IOOptions& opts, uint64_t offset,
-                             size_t original_length, size_t read_length,
-                             char* scratch, Slice& result) {
+  // FlexibleRead enables the result size to be in the range of
+  // [read_length - optional_read_size, read_length]
+  IOStatus FlexibleRead(RandomAccessFileReader* reader, const IOOptions& opts,
+                        uint64_t offset, size_t read_length,
+                        size_t optional_read_size, char* scratch,
+                        Slice& result) {
     FSReadRequest read_req;
     read_req.offset = offset;
     read_req.len = read_length;
     read_req.scratch = scratch;
-    assert(original_length <= read_length);
-    read_req.optional_read_size = read_length - original_length;
+    assert(optional_read_size <= read_length);
+    read_req.optional_read_size = optional_read_size;
     IOStatus s = reader->MultiRead(opts, &read_req, 1, nullptr);
     if (!s.ok()) {
       return s;

include/rocksdb/file_system.h

@@ -818,7 +818,7 @@ struct FSReadRequest {
   // returned when the end of file has been reached or an error has occurred.
   //
   // When optional_read_size > 0, if the end of the file is not reached, the
-  // returned data can have size in the range [len - optional_read_size, len]
+  // returned data's size can be in the range [len - optional_read_size, len]
   //
   // Note that optional_read_size should never exceed len
   //
@@ -883,10 +883,11 @@ struct FSReadRequest {
   // WARNING 2: Since fs_scratch is a unique pointer, FSReadRequest's copy
   // constructor is implicitly disabled. This turns out to be very useful
   // because we want users to be explicit when setting offset, len, and
-  // optional_read_size. If you end up wanting to delete this field, be very
-  // careful and consider explicitly deleting the copy constructor, since the
-  // lack of a copy constructor is likely acting as a good protective measure
-  // against bugs
+  // optional_read_size. Consider the possibility where optional_read_size ends
+  // up exceeding the request length because it was copied over by mistake. If
+  // you end up wanting to delete this field, be very careful and consider
+  // explicitly deleting the copy constructor, since the lack of a copy
+  // constructor is likely acting as a good protective measure against bugs
   FSAllocationPtr fs_scratch;
 };