From 251658471eab6d8cd968d678922bab437f72a9c7 Mon Sep 17 00:00:00 2001
From: Jason Dillaman <dillaman@redhat.com>
Date: Thu, 19 Oct 2017 13:52:56 -0400
Subject: [PATCH] librbd: default to sparse-reads for any IO operation over 64K

Testing BlueStore against both HDDs and OSDs with fully allocated
and sparse-allocated objects shows a performance improvement with
sparse-read between 32K and 64K.

Fixes: http://tracker.ceph.com/issues/21849
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
---
 src/common/options.cc                         |  12 ++
 src/librbd/ImageCtx.cc                        |   6 +
 src/librbd/ImageCtx.h                         |   1 +
 src/librbd/io/ImageRequest.cc                 |   2 +-
 src/librbd/io/ObjectRequest.cc                |   9 +-
 src/librbd/io/ObjectRequest.h                 |  10 +-
 src/test/librbd/CMakeLists.txt                |   1 +
 src/test/librbd/io/test_mock_ImageRequest.cc  |   3 +-
 src/test/librbd/io/test_mock_ObjectRequest.cc | 189 ++++++++++++++++++
 9 files changed, 219 insertions(+), 14 deletions(-)
 create mode 100644 src/test/librbd/io/test_mock_ObjectRequest.cc

diff --git a/src/common/options.cc b/src/common/options.cc
index dc37ad48a22ba..612a1f07ab6d5 100644
--- a/src/common/options.cc
+++ b/src/common/options.cc
@@ -5257,6 +5257,18 @@ static std::vector<Option> get_rbd_options() {
     .set_default(false)
     .set_description("localize parent requests to closest OSD"),
 
+    Option("rbd_sparse_read_threshold_bytes", Option::TYPE_UINT,
+           Option::LEVEL_ADVANCED)
+    .set_default(64_K)
+    .set_description("threshold for issuing a sparse-read")
+    .set_long_description("minimum number of sequential bytes to read against "
+                          "an object before issuing a sparse-read request to "
+                          "the cluster. 0 implies it must be a full object read"
+                          "to issue a sparse-read, 1 implies always use "
+                          "sparse-read, and any value larger than the maximum "
+                          "object size will disable sparse-read for all "
+                          "requests"),
+
     Option("rbd_readahead_trigger_requests", Option::TYPE_INT, Option::LEVEL_ADVANCED)
     .set_default(10)
     .set_description("number of sequential requests necessary to trigger readahead"),
diff --git a/src/librbd/ImageCtx.cc b/src/librbd/ImageCtx.cc
index 3918a76818190..411d34c110c88 100644
--- a/src/librbd/ImageCtx.cc
+++ b/src/librbd/ImageCtx.cc
@@ -981,6 +981,7 @@ struct C_InvalidateCache : public Context {
         "rbd_localize_snap_reads", false)(
         "rbd_balance_parent_reads", false)(
         "rbd_localize_parent_reads", false)(
+        "rbd_sparse_read_threshold_bytes", false)(
         "rbd_readahead_trigger_requests", false)(
         "rbd_readahead_max_bytes", false)(
         "rbd_readahead_disable_after_bytes", false)(
@@ -1039,6 +1040,7 @@ struct C_InvalidateCache : public Context {
     ASSIGN_OPTION(localize_snap_reads, bool);
     ASSIGN_OPTION(balance_parent_reads, bool);
     ASSIGN_OPTION(localize_parent_reads, bool);
+    ASSIGN_OPTION(sparse_read_threshold_bytes, uint64_t);
     ASSIGN_OPTION(readahead_trigger_requests, int64_t);
     ASSIGN_OPTION(readahead_max_bytes, int64_t);
     ASSIGN_OPTION(readahead_disable_after_bytes, int64_t);
@@ -1063,6 +1065,10 @@ struct C_InvalidateCache : public Context {
     if (thread_safe) {
       ASSIGN_OPTION(journal_pool, std::string);
     }
+
+    if (sparse_read_threshold_bytes == 0) {
+      sparse_read_threshold_bytes = get_object_size();
+    }
   }
 
   ExclusiveLock<ImageCtx> *ImageCtx::create_exclusive_lock() {
diff --git a/src/librbd/ImageCtx.h b/src/librbd/ImageCtx.h
index 9479bae30a9a5..625da49cd9836 100644
--- a/src/librbd/ImageCtx.h
+++ b/src/librbd/ImageCtx.h
@@ -176,6 +176,7 @@ namespace librbd {
     bool localize_snap_reads;
     bool balance_parent_reads;
     bool localize_parent_reads;
+    uint64_t sparse_read_threshold_bytes;
     uint32_t readahead_trigger_requests;
     uint64_t readahead_max_bytes;
     uint64_t readahead_disable_after_bytes;
diff --git a/src/librbd/io/ImageRequest.cc b/src/librbd/io/ImageRequest.cc
index cd87d92e07479..2105d74fd03af 100644
--- a/src/librbd/io/ImageRequest.cc
+++ b/src/librbd/io/ImageRequest.cc
@@ -355,7 +355,7 @@ void ImageReadRequest<I>::send_request() {
         aio_comp);
       ObjectReadRequest<I> *req = ObjectReadRequest<I>::create(
         &image_ctx, extent.oid.name, extent.objectno, extent.offset,
-        extent.length, extent.buffer_extents, snap_id, true, m_op_flags,
+        extent.length, extent.buffer_extents, snap_id, m_op_flags,
 	this->m_trace, req_comp);
       req_comp->request = req;
 
diff --git a/src/librbd/io/ObjectRequest.cc b/src/librbd/io/ObjectRequest.cc
index 16e8aabf20d13..3c55ccd07d942 100644
--- a/src/librbd/io/ObjectRequest.cc
+++ b/src/librbd/io/ObjectRequest.cc
@@ -205,14 +205,13 @@ template <typename I>
 ObjectReadRequest<I>::ObjectReadRequest(I *ictx, const std::string &oid,
                                         uint64_t objectno, uint64_t offset,
                                         uint64_t len, Extents& be,
-                                        librados::snap_t snap_id, bool sparse,
-					int op_flags,
+                                        librados::snap_t snap_id, int op_flags,
 					const ZTracer::Trace &parent_trace,
                                         Context *completion)
   : ObjectRequest<I>(ictx, oid, objectno, offset, len, snap_id, false, "read",
                      parent_trace, completion),
-    m_buffer_extents(be), m_tried_parent(false), m_sparse(sparse),
-    m_op_flags(op_flags), m_state(LIBRBD_AIO_READ_FLAT) {
+    m_buffer_extents(be), m_tried_parent(false), m_op_flags(op_flags),
+    m_state(LIBRBD_AIO_READ_FLAT) {
   guard_read();
 }
 
@@ -326,7 +325,7 @@ void ObjectReadRequest<I>::send() {
 
   librados::ObjectReadOperation op;
   int flags = image_ctx->get_read_flags(this->m_snap_id);
-  if (m_sparse) {
+  if (this->m_object_len >= image_ctx->sparse_read_threshold_bytes) {
     op.sparse_read(this->m_object_off, this->m_object_len, &m_ext_map,
                    &m_read_data, nullptr);
   } else {
diff --git a/src/librbd/io/ObjectRequest.h b/src/librbd/io/ObjectRequest.h
index 9c3551db31046..e3831802dcb7e 100644
--- a/src/librbd/io/ObjectRequest.h
+++ b/src/librbd/io/ObjectRequest.h
@@ -151,20 +151,19 @@ public:
   static ObjectReadRequest* create(ImageCtxT *ictx, const std::string &oid,
                                    uint64_t objectno, uint64_t offset,
                                    uint64_t len, Extents &buffer_extents,
-                                   librados::snap_t snap_id, bool sparse,
-				   int op_flags,
+                                   librados::snap_t snap_id, int op_flags,
 				   const ZTracer::Trace &parent_trace,
                                    Context *completion) {
     return new ObjectReadRequest(ictx, oid, objectno, offset, len,
-                                 buffer_extents, snap_id, sparse, op_flags,
+                                 buffer_extents, snap_id, op_flags,
 				 parent_trace, completion);
   }
 
   ObjectReadRequest(ImageCtxT *ictx, const std::string &oid,
                     uint64_t objectno, uint64_t offset, uint64_t len,
                     Extents& buffer_extents, librados::snap_t snap_id,
-                    bool sparse, int op_flags,
-		    const ZTracer::Trace &parent_trace, Context *completion);
+                    int op_flags, const ZTracer::Trace &parent_trace,
+                    Context *completion);
 
   bool should_complete(int r) override;
   void send() override;
@@ -197,7 +196,6 @@ public:
 private:
   Extents m_buffer_extents;
   bool m_tried_parent;
-  bool m_sparse;
   int m_op_flags;
   ceph::bufferlist m_read_data;
   ExtentMap m_ext_map;
diff --git a/src/test/librbd/CMakeLists.txt b/src/test/librbd/CMakeLists.txt
index 18396995d6273..1b32d714454ac 100644
--- a/src/test/librbd/CMakeLists.txt
+++ b/src/test/librbd/CMakeLists.txt
@@ -38,6 +38,7 @@ set(unittest_librbd_srcs
   image/test_mock_RemoveRequest.cc
   io/test_mock_ImageRequest.cc
   io/test_mock_ImageRequestWQ.cc
+  io/test_mock_ObjectRequest.cc
   journal/test_mock_OpenRequest.cc
   journal/test_mock_PromoteRequest.cc
   journal/test_mock_Replay.cc
diff --git a/src/test/librbd/io/test_mock_ImageRequest.cc b/src/test/librbd/io/test_mock_ImageRequest.cc
index 4aca8a8d9ce69..7925ce928f45c 100644
--- a/src/test/librbd/io/test_mock_ImageRequest.cc
+++ b/src/test/librbd/io/test_mock_ImageRequest.cc
@@ -136,8 +136,7 @@ struct ObjectReadRequest<librbd::MockTestImageCtx> : public ObjectRequest<librbd
                                    const std::string &oid,
                                    uint64_t objectno, uint64_t offset,
                                    uint64_t len, Extents &buffer_extents,
-                                   librados::snap_t snap_id, bool sparse,
-                                   int op_flags,
+                                   librados::snap_t snap_id, int op_flags,
                                    const ZTracer::Trace &parent_trace,
                                    Context *completion) {
     assert(s_instance != nullptr);
diff --git a/src/test/librbd/io/test_mock_ObjectRequest.cc b/src/test/librbd/io/test_mock_ObjectRequest.cc
new file mode 100644
index 0000000000000..616dbcf27fb3b
--- /dev/null
+++ b/src/test/librbd/io/test_mock_ObjectRequest.cc
@@ -0,0 +1,189 @@
+// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*-
+// vim: ts=8 sw=2 smarttab
+
+#include "test/librbd/test_mock_fixture.h"
+#include "test/librbd/test_support.h"
+#include "test/librbd/mock/MockImageCtx.h"
+#include "test/librbd/mock/MockObjectMap.h"
+#include "test/librbd/mock/cache/MockImageCache.h"
+#include "test/librados_test_stub/MockTestMemIoCtxImpl.h"
+#include "test/librados_test_stub/MockTestMemRadosClient.h"
+#include "librbd/io/CopyupRequest.h"
+#include "librbd/io/ImageRequest.h"
+#include "librbd/io/ObjectRequest.h"
+
+namespace librbd {
+namespace {
+
+struct MockTestImageCtx : public MockImageCtx {
+  MockTestImageCtx(ImageCtx &image_ctx) : MockImageCtx(image_ctx) {
+  }
+};
+
+} // anonymous namespace
+
+namespace util {
+
+inline ImageCtx *get_image_ctx(MockImageCtx *image_ctx) {
+  return image_ctx->image_ctx;
+}
+
+} // namespace util
+
+namespace io {
+
+template <>
+struct CopyupRequest<librbd::MockImageCtx> {
+  static CopyupRequest* create(librbd::MockImageCtx *ictx,
+                               const std::string &oid, uint64_t objectno,
+                               Extents &&image_extents,
+                               const ZTracer::Trace &parent_trace) {
+    return nullptr;
+  }
+
+  MOCK_METHOD0(send, void());
+};
+
+template <>
+struct ImageRequest<librbd::MockImageCtx> {
+  static void aio_read(librbd::MockImageCtx *ictx, AioCompletion *c,
+                       Extents &&image_extents, ReadResult &&read_result,
+                       int op_flags, const ZTracer::Trace &parent_trace) {
+  }
+};
+
+} // namespace io
+} // namespace librbd
+
+#include "librbd/io/ObjectRequest.cc"
+
+namespace librbd {
+namespace io {
+
+using ::testing::_;
+using ::testing::DoDefault;
+using ::testing::InSequence;
+using ::testing::Invoke;
+using ::testing::Return;
+using ::testing::WithArg;
+
+struct TestMockIoObjectRequest : public TestMockFixture {
+  typedef ObjectRequest<librbd::MockImageCtx> MockObjectRequest;
+  typedef ObjectReadRequest<librbd::MockImageCtx> MockObjectReadRequest;
+
+  void expect_object_may_exist(MockTestImageCtx &mock_image_ctx,
+                               uint64_t object_no, bool exists) {
+    if (mock_image_ctx.object_map != nullptr) {
+      EXPECT_CALL(*mock_image_ctx.object_map, object_may_exist(object_no))
+        .WillOnce(Return(exists));
+    }
+  }
+
+  void expect_get_parent_overlap(MockTestImageCtx &mock_image_ctx,
+                                 librados::snap_t snap_id, uint64_t overlap,
+                                 int r) {
+    EXPECT_CALL(mock_image_ctx, get_parent_overlap(snap_id, _))
+      .WillOnce(WithArg<1>(Invoke([overlap, r](uint64_t *o) {
+                             *o = overlap;
+                             return r;
+                           })));
+  }
+
+  void expect_prune_parent_extents(MockTestImageCtx &mock_image_ctx,
+                                   const MockObjectRequest::Extents& extents,
+                                   uint64_t overlap, uint64_t object_overlap) {
+    EXPECT_CALL(mock_image_ctx, prune_parent_extents(_, overlap))
+      .WillOnce(WithArg<0>(Invoke([extents, object_overlap](MockObjectRequest::Extents& e) {
+                             e = extents;
+                             return object_overlap;
+                           })));
+  }
+
+  void expect_get_read_flags(MockTestImageCtx &mock_image_ctx,
+                             librados::snap_t snap_id, int flags) {
+    EXPECT_CALL(mock_image_ctx, get_read_flags(snap_id))
+      .WillOnce(Return(flags));
+  }
+
+  void expect_read(MockTestImageCtx &mock_image_ctx,
+                   const std::string& oid, uint64_t off, uint64_t len,
+                   int r) {
+    auto& expect = EXPECT_CALL(get_mock_io_ctx(mock_image_ctx.data_ctx),
+                               read(oid, len, off, _));
+    if (r < 0) {
+      expect.WillOnce(Return(r));
+    } else {
+      expect.WillOnce(DoDefault());
+    }
+  }
+
+  void expect_sparse_read(MockTestImageCtx &mock_image_ctx,
+                          const std::string& oid, uint64_t off, uint64_t len,
+                          int r) {
+    auto& expect = EXPECT_CALL(get_mock_io_ctx(mock_image_ctx.data_ctx),
+                               sparse_read(oid, off, len, _, _));
+    if (r < 0) {
+      expect.WillOnce(Return(r));
+    } else {
+      expect.WillOnce(DoDefault());
+    }
+  }
+};
+
+TEST_F(TestMockIoObjectRequest, Read) {
+  librbd::ImageCtx *ictx;
+  ASSERT_EQ(0, open_image(m_image_name, &ictx));
+  ictx->sparse_read_threshold_bytes = 8096;
+
+  MockTestImageCtx mock_image_ctx(*ictx);
+  MockObjectMap mock_object_map;
+  if (ictx->test_features(RBD_FEATURE_OBJECT_MAP)) {
+    mock_image_ctx.object_map = &mock_object_map;
+  }
+
+  InSequence seq;
+  expect_get_parent_overlap(mock_image_ctx, CEPH_NOSNAP, 0, 0);
+  expect_prune_parent_extents(mock_image_ctx, {}, 0, 0);
+  expect_object_may_exist(mock_image_ctx, 0, true);
+  expect_get_read_flags(mock_image_ctx, CEPH_NOSNAP, 0);
+  expect_read(mock_image_ctx, "object0", 0, 4096, 0);
+
+  C_SaferCond ctx;
+  MockObjectReadRequest::Extents extents{{0, 4096}};
+  auto req = MockObjectReadRequest::create(
+    &mock_image_ctx, "object0", 0, 0, 4096, extents, CEPH_NOSNAP, 0, {}, &ctx);
+  req->send();
+  ASSERT_EQ(-ENOENT, ctx.wait());
+}
+
+TEST_F(TestMockIoObjectRequest, SparseReadThreshold) {
+  librbd::ImageCtx *ictx;
+  ASSERT_EQ(0, open_image(m_image_name, &ictx));
+  ictx->sparse_read_threshold_bytes = ictx->get_object_size();
+
+  MockTestImageCtx mock_image_ctx(*ictx);
+  MockObjectMap mock_object_map;
+  if (ictx->test_features(RBD_FEATURE_OBJECT_MAP)) {
+    mock_image_ctx.object_map = &mock_object_map;
+  }
+
+  InSequence seq;
+  expect_get_parent_overlap(mock_image_ctx, CEPH_NOSNAP, 0, 0);
+  expect_prune_parent_extents(mock_image_ctx, {}, 0, 0);
+  expect_object_may_exist(mock_image_ctx, 0, true);
+  expect_get_read_flags(mock_image_ctx, CEPH_NOSNAP, 0);
+  expect_sparse_read(mock_image_ctx, "object0", 0,
+                     ictx->sparse_read_threshold_bytes, 0);
+
+  C_SaferCond ctx;
+  MockObjectReadRequest::Extents extents{
+    {0, ictx->sparse_read_threshold_bytes}};
+  auto req = MockObjectReadRequest::create(
+    &mock_image_ctx, "object0", 0, 0, ictx->sparse_read_threshold_bytes,
+    extents, CEPH_NOSNAP, 0, {}, &ctx);
+  req->send();
+  ASSERT_EQ(-ENOENT, ctx.wait());
+}
+
+} // namespace io
+} // namespace librbd
-- 
2.39.5