From 489faf99876e234888de02b35dd54a1d9682a450 Mon Sep 17 00:00:00 2001
From: shangdehao1 <dehao.shang@intel.com>
Date: Fri, 19 Apr 2019 08:24:41 +0800
Subject: [PATCH] librbd: modify RO hook implements

modify read and handle_read_cache method, and cleanup.

Signed-off-by: Dehao Shang <dehao.shang@intel.com>
---
 .../cache/SharedReadOnlyObjectDispatch.cc     | 55 ++++++++-----------
 .../cache/SharedReadOnlyObjectDispatch.h      | 10 ++--
 .../cache/test_mock_ParentImageCache.cc       |  1 -
 3 files changed, 29 insertions(+), 37 deletions(-)

diff --git a/src/librbd/cache/SharedReadOnlyObjectDispatch.cc b/src/librbd/cache/SharedReadOnlyObjectDispatch.cc
index 499da1cbcb3..d763439e3ea 100644
--- a/src/librbd/cache/SharedReadOnlyObjectDispatch.cc
+++ b/src/librbd/cache/SharedReadOnlyObjectDispatch.cc
@@ -25,7 +25,7 @@ namespace cache {
 template <typename I, typename C>
 SharedReadOnlyObjectDispatch<I, C>::SharedReadOnlyObjectDispatch(
     I* image_ctx) : m_image_ctx(image_ctx), m_cache_client(nullptr),
-    m_initialzed(false) {
+    m_initialzed(false), m_object_store(nullptr) {
 }
 
 template <typename I, typename C>
@@ -90,12 +90,11 @@ bool SharedReadOnlyObjectDispatch<I, C>::read(
   ldout(cct, 20) << "object_no=" << object_no << " " << object_off << "~"
                  << object_len << dendl;
 
-  // if any session failed, reads will go to rados
-  if(!m_cache_client->is_session_work()) {
+  // if any failse, reads will go to rados
+  if(!m_cache_client->is_session_work() || m_cache_client == nullptr ||
+     m_object_store == nullptr || !m_initialzed) {
     ldout(cct, 5) << "SRO cache client session failed " << dendl;
-    *dispatch_result = io::DISPATCH_RESULT_CONTINUE;
-    on_dispatched->complete(0);
-    return true;
+    return false;
   }
 
   CacheGenContextURef ctx = make_gen_lambda_context<ObjectCacheRequest*,
@@ -106,48 +105,42 @@ bool SharedReadOnlyObjectDispatch<I, C>::read(
                        dispatch_result, on_dispatched);
   });
 
-  if (m_cache_client && m_cache_client->is_session_work() && m_object_store) {
-
-    m_cache_client->lookup_object(m_image_ctx->data_ctx.get_namespace(),
-                                  m_image_ctx->data_ctx.get_id(),
-                                  (uint64_t)snap_id, oid, std::move(ctx));
-  }
+  m_cache_client->lookup_object(m_image_ctx->data_ctx.get_namespace(),
+                                m_image_ctx->data_ctx.get_id(),
+                                (uint64_t)snap_id, oid, std::move(ctx));
   return true;
 }
 
 template <typename I, typename C>
-int SharedReadOnlyObjectDispatch<I, C>::handle_read_cache(
+void SharedReadOnlyObjectDispatch<I, C>::handle_read_cache(
     ObjectCacheRequest* ack, uint64_t read_off,
     uint64_t read_len, ceph::bufferlist* read_data,
     io::DispatchResult* dispatch_result, Context* on_dispatched) {
-  std::string file_path;
-  if (ack->type == RBDSC_READ_REPLY) {
-    file_path = ((ObjectCacheReadReplyData*)ack)->cache_path;
-    ceph_assert(file_path != "");
-  } else {
+  auto cct = m_image_ctx->cct;
+  ldout(cct, 20) << dendl;
+
+  if(ack->type != RBDSC_READ_REPLY) {
     // go back to read rados
     *dispatch_result = io::DISPATCH_RESULT_CONTINUE;
     on_dispatched->complete(0);
-    return false;
+    return;
   }
 
-  auto cct = m_image_ctx->cct;
-  ldout(cct, 20) << dendl;
+  ceph_assert(ack->type == RBDSC_READ_REPLY);
+  std::string file_path = ((ObjectCacheReadReplyData*)ack)->cache_path;
+  ceph_assert(file_path != "");
 
   // try to read from parent image cache
   int r = m_object_store->read_object(file_path, read_data, read_off, read_len, on_dispatched);
-  if (r == read_len) {
-    *dispatch_result = io::DISPATCH_RESULT_COMPLETE;
-    //TODO(): complete in syncfile
-    on_dispatched->complete(r);
-    ldout(cct, 20) << "read cache: " << *dispatch_result <<dendl;
-    return true;
+  if(r < 0) {
+    // cache read error, fall back to read rados
+    *dispatch_result = io::DISPATCH_RESULT_CONTINUE;
+    on_dispatched->complete(0);
   }
 
-  // cache read error, fall back to read rados
-  *dispatch_result = io::DISPATCH_RESULT_CONTINUE;
-  on_dispatched->complete(0);
-  return false;
+  *dispatch_result = io::DISPATCH_RESULT_COMPLETE;
+  //TODO(): complete in syncfile
+  on_dispatched->complete(r);
 }
 
 template <typename I, typename C>
diff --git a/src/librbd/cache/SharedReadOnlyObjectDispatch.h b/src/librbd/cache/SharedReadOnlyObjectDispatch.h
index 3c7e934f07b..cd1d566c332 100644
--- a/src/librbd/cache/SharedReadOnlyObjectDispatch.h
+++ b/src/librbd/cache/SharedReadOnlyObjectDispatch.h
@@ -113,11 +113,11 @@ public:
 
 private:
 
-  int handle_read_cache(
-      ObjectCacheRequest* ack, uint64_t read_off,
-      uint64_t read_len, ceph::bufferlist* read_data,
-      io::DispatchResult* dispatch_result,
-      Context* on_dispatched);
+  void handle_read_cache(
+         ObjectCacheRequest* ack, uint64_t read_off,
+         uint64_t read_len, ceph::bufferlist* read_data,
+         io::DispatchResult* dispatch_result,
+         Context* on_dispatched);
   int handle_register_client(bool reg);
   void client_handle_request(std::string msg);
 
diff --git a/src/test/librbd/cache/test_mock_ParentImageCache.cc b/src/test/librbd/cache/test_mock_ParentImageCache.cc
index f26383cabaf..68e53e58061 100644
--- a/src/test/librbd/cache/test_mock_ParentImageCache.cc
+++ b/src/test/librbd/cache/test_mock_ParentImageCache.cc
@@ -296,7 +296,6 @@ TEST_F(TestMockParentImageCache, test_read) {
   Context* on_finish = &cond;
 
   auto& expect = EXPECT_CALL(*(mock_parent_image_cache->m_cache_client), is_session_work())
-                   .WillOnce(Return(true))
                    .WillOnce(Return(true));
   expect_cache_lookup_object(*mock_parent_image_cache, on_finish); 
 
-- 
2.39.5