librbd: don't read metadata twice on image open

After adding get_metadata to RefreshRequest it has become redundant
in OpenRequest.

Signed-off-by: Mykola Golub <to.my.trociny@gmail.com>

diff --git a/src/librbd/ImageCtx.cc b/src/librbd/ImageCtx.cc
index 6286faf3998e9a855b97e1e3ae3704ef33f5deb9..2008a07a7695b3eaeda6544a42fe5093ac6b379f 100644 (file)
--- a/src/librbd/ImageCtx.cc
+++ b/src/librbd/ImageCtx.cc
@@ -274,6 +274,11 @@ struct C_InvalidateCache : public Context {
     trace_endpoint.copy_name(pname);
     perf_start(pname);
 
+    assert(image_watcher == NULL);
+    image_watcher = new ImageWatcher<>(*this);
+  }
+
+  void ImageCtx::init_cache() {
     if (cache) {
       Mutex::Locker l(cache_lock);
       ldout(cct, 20) << "enabling caching..." << dendl;
@@ -290,9 +295,9 @@ struct C_InvalidateCache : public Context {
                     << " max_dirty_age="
                     << cache_max_dirty_age << dendl;
 
-      object_cacher = new ObjectCacher(cct, pname, *writeback_handler, cache_lock,
-                                      NULL, NULL,
-                                      cache_size,
+      object_cacher = new ObjectCacher(cct, perfcounter->get_name(),
+                                       *writeback_handler, cache_lock, NULL,
+                                       NULL, cache_size,
                                       10,  /* reset this in init */
                                       init_max_dirty,
                                       cache_target_dirty,
@@ -845,8 +850,7 @@ struct C_InvalidateCache : public Context {
   }
 
   void ImageCtx::register_watch(Context *on_finish) {
-    assert(image_watcher == NULL);
-    image_watcher = new ImageWatcher<>(*this);
+    assert(image_watcher != NULL);
     image_watcher->register_watch(on_finish);
   }
 

diff --git a/src/librbd/ImageCtx.h b/src/librbd/ImageCtx.h
index 625da49cd9836a77a3e5f5197b14f4bd584a98ee..ed77e339d64421377a23c59703f674ea3874f159 100644 (file)
--- a/src/librbd/ImageCtx.h
+++ b/src/librbd/ImageCtx.h
@@ -228,6 +228,7 @@ namespace librbd {
             const char *snap, IoCtx& p, bool read_only);
     ~ImageCtx();
     void init();
+    void init_cache();
     void shutdown();
     void init_layout();
     void perf_start(std::string name);

diff --git a/src/librbd/image/OpenRequest.cc b/src/librbd/image/OpenRequest.cc
index bb1a3875ff408ba5287c26ebfcb1744334bb81cc..73a2273e8a03d5cb06cdace47b1e36552235ba75 100644 (file)
--- a/src/librbd/image/OpenRequest.cc
+++ b/src/librbd/image/OpenRequest.cc
@@ -20,12 +20,6 @@
 namespace librbd {
 namespace image {
 
-namespace {
-
-static uint64_t MAX_METADATA_ITEMS = 128;
-
-}
-
 using util::create_context_callback;
 using util::create_rados_callback;
 
@@ -33,8 +27,7 @@ template <typename I>
 OpenRequest<I>::OpenRequest(I *image_ctx, bool skip_open_parent,
                             Context *on_finish)
   : m_image_ctx(image_ctx), m_skip_open_parent_image(skip_open_parent),
-    m_on_finish(on_finish), m_error_result(0),
-    m_last_metadata_key(ImageCtx::METADATA_CONF_PREFIX) {
+    m_on_finish(on_finish), m_error_result(0) {
 }
 
 template <typename I>
@@ -75,7 +68,7 @@ Context *OpenRequest<I>::handle_v1_detect_header(int *result) {
     m_image_ctx->header_oid = util::old_header_name(m_image_ctx->name);
     m_image_ctx->apply_metadata({}, true);
 
-    send_register_watch();
+    send_refresh();
   }
   return nullptr;
 }
@@ -418,71 +411,44 @@ Context *OpenRequest<I>::handle_v2_get_data_pool(int *result) {
   }
 
   m_image_ctx->init_layout();
-  send_v2_apply_metadata();
+  send_refresh();
   return nullptr;
 }
 
 template <typename I>
-void OpenRequest<I>::send_v2_apply_metadata() {
-  CephContext *cct = m_image_ctx->cct;
-  ldout(cct, 10) << this << " " << __func__ << ": "
-                 << "start_key=" << m_last_metadata_key << dendl;
+void OpenRequest<I>::send_refresh() {
+  m_image_ctx->init();
 
-  librados::ObjectReadOperation op;
-  cls_client::metadata_list_start(&op, m_last_metadata_key, MAX_METADATA_ITEMS);
+  CephContext *cct = m_image_ctx->cct;
+  ldout(cct, 10) << this << " " << __func__ << dendl;
 
   using klass = OpenRequest<I>;
-  librados::AioCompletion *comp =
-    create_rados_callback<klass, &klass::handle_v2_apply_metadata>(this);
-  m_out_bl.clear();
-  m_image_ctx->md_ctx.aio_operate(m_image_ctx->header_oid, comp, &op,
-                                  &m_out_bl);
-  comp->release();
+  RefreshRequest<I> *req = RefreshRequest<I>::create(
+    *m_image_ctx, false, m_skip_open_parent_image,
+    create_context_callback<klass, &klass::handle_refresh>(this));
+  req->send();
 }
 
 template <typename I>
-Context *OpenRequest<I>::handle_v2_apply_metadata(int *result) {
+Context *OpenRequest<I>::handle_refresh(int *result) {
   CephContext *cct = m_image_ctx->cct;
-  ldout(cct, 10) << this << " " << __func__ << ": r=" << *result << dendl;
-
-  std::map<std::string, bufferlist> metadata;
-  if (*result == 0) {
-    bufferlist::iterator it = m_out_bl.begin();
-    *result = cls_client::metadata_list_finish(&it, &metadata);
-  }
+  ldout(cct, 10) << __func__ << ": r=" << *result << dendl;
 
-  if (*result == -EOPNOTSUPP || *result == -EIO) {
-    ldout(cct, 10) << "config metadata not supported by OSD" << dendl;
-  } else if (*result < 0) {
-    lderr(cct) << "failed to retrieve metadata: " << cpp_strerror(*result)
+  if (*result < 0) {
+    lderr(cct) << "failed to refresh image: " << cpp_strerror(*result)
                << dendl;
     send_close_image(*result);
     return nullptr;
   }
 
-  if (!metadata.empty()) {
-    m_metadata.insert(metadata.begin(), metadata.end());
-    m_last_metadata_key = metadata.rbegin()->first;
-    if (boost::starts_with(m_last_metadata_key,
-                           ImageCtx::METADATA_CONF_PREFIX)) {
-      send_v2_apply_metadata();
-      return nullptr;
-    }
-  }
-
-  m_image_ctx->apply_metadata(m_metadata, true);
-
-  send_register_watch();
-  return nullptr;
+  m_image_ctx->init_cache();
+  return send_register_watch(result);
 }
 
 template <typename I>
-void OpenRequest<I>::send_register_watch() {
-  m_image_ctx->init();
-
+Context *OpenRequest<I>::send_register_watch(int *result) {
   if (m_image_ctx->read_only) {
-    send_refresh();
-    return;
+    return send_set_snap(result);
   }
 
   CephContext *cct = m_image_ctx->cct;
@@ -492,6 +458,7 @@ void OpenRequest<I>::send_register_watch() {
   Context *ctx = create_context_callback<
     klass, &klass::handle_register_watch>(this);
   m_image_ctx->register_watch(ctx);
+  return nullptr;
 }
 
 template <typename I>
@@ -503,37 +470,10 @@ Context *OpenRequest<I>::handle_register_watch(int *result) {
     lderr(cct) << "failed to register watch: " << cpp_strerror(*result)
                << dendl;
     send_close_image(*result);
-  } else {
-    send_refresh();
-  }
-  return nullptr;
-}
-
-template <typename I>
-void OpenRequest<I>::send_refresh() {
-  CephContext *cct = m_image_ctx->cct;
-  ldout(cct, 10) << this << " " << __func__ << dendl;
-
-  using klass = OpenRequest<I>;
-  RefreshRequest<I> *req = RefreshRequest<I>::create(
-    *m_image_ctx, false, m_skip_open_parent_image,
-    create_context_callback<klass, &klass::handle_refresh>(this));
-  req->send();
-}
-
-template <typename I>
-Context *OpenRequest<I>::handle_refresh(int *result) {
-  CephContext *cct = m_image_ctx->cct;
-  ldout(cct, 10) << __func__ << ": r=" << *result << dendl;
-
-  if (*result < 0) {
-    lderr(cct) << "failed to refresh image: " << cpp_strerror(*result)
-               << dendl;
-    send_close_image(*result);
     return nullptr;
-  } else {
-    return send_set_snap(result);
   }
+
+  return send_set_snap(result);
 }
 
 template <typename I>

diff --git a/src/librbd/image/OpenRequest.h b/src/librbd/image/OpenRequest.h
index 8ea1ff10a2e8411f674796b2d29e4e38b75746c9..45395f148d3bf02da483595d50396fa8489f7f01 100644 (file)
--- a/src/librbd/image/OpenRequest.h
+++ b/src/librbd/image/OpenRequest.h
@@ -55,15 +55,12 @@ private:
    *            V2_GET_CREATE_TIMESTAMP             |
    *                |                               |
    *                v                               |
-   *            V2_GET_DATA_POOL                    |
-   *                |                               |
-   *                v                               |
-   *      /---> V2_APPLY_METADATA -------------> REGISTER_WATCH (skip if
-   *      |         |                               |            read-only)
-   *      \---------/                               v
-   *                                             REFRESH
+   *            V2_GET_DATA_POOL --------------> REFRESH
    *                                                |
    *                                                v
+   *                                             REGISTER_WATCH (skip if
+   *                                                |            read-only)
+   *                                                v
    *                                             SET_SNAP (skip if no snap)
    *                                                |
    *                                                v
@@ -84,9 +81,6 @@ private:
   bufferlist m_out_bl;
   int m_error_result;
 
-  std::string m_last_metadata_key;
-  std::map<std::string, bufferlist> m_metadata;
-
   void send_v1_detect_header();
   Context *handle_v1_detect_header(int *result);
 
@@ -114,15 +108,12 @@ private:
   void send_v2_get_data_pool();
   Context *handle_v2_get_data_pool(int *result);
 
-  void send_v2_apply_metadata();
-  Context *handle_v2_apply_metadata(int *result);
-
-  void send_register_watch();
-  Context *handle_register_watch(int *result);
-
   void send_refresh();
   Context *handle_refresh(int *result);
 
+  Context *send_register_watch(int *result);
+  Context *handle_register_watch(int *result);
+
   Context *send_set_snap(int *result);
   Context *handle_set_snap(int *result);
 

diff --git a/src/librbd/image/RefreshRequest.cc b/src/librbd/image/RefreshRequest.cc
index 111cc133cf574a1810a6290b78b42035f2f65636..3dff35fe6d3803baa29a2026d91788b786e1f606 100644 (file)
--- a/src/librbd/image/RefreshRequest.cc
+++ b/src/librbd/image/RefreshRequest.cc
@@ -11,6 +11,7 @@
 #include "cls/rbd/cls_rbd_client.h"
 #include "librbd/ExclusiveLock.h"
 #include "librbd/ImageCtx.h"
+#include "librbd/ImageWatcher.h"
 #include "librbd/Journal.h"
 #include "librbd/ObjectMap.h"
 #include "librbd/Utils.h"
@@ -345,7 +346,8 @@ Context *RefreshRequest<I>::handle_v2_get_metadata(int *result) {
     }
   }
 
-  m_image_ctx.apply_metadata(m_metadata, false);
+  bool thread_safe = m_image_ctx.image_watcher->is_unregistered();
+  m_image_ctx.apply_metadata(m_metadata, thread_safe);
 
   send_v2_get_flags();
   return nullptr;

diff --git a/src/test/librbd/image/test_mock_RefreshRequest.cc b/src/test/librbd/image/test_mock_RefreshRequest.cc
index 02ab30d4432e215869a736a015a9b32f61b83f5d..7747956e34b1e2e35a956b527ff77ab6fc00b034 100644 (file)
--- a/src/test/librbd/image/test_mock_RefreshRequest.cc
+++ b/src/test/librbd/image/test_mock_RefreshRequest.cc
@@ -4,6 +4,7 @@
 #include "test/librbd/test_mock_fixture.h"
 #include "test/librbd/test_support.h"
 #include "test/librbd/mock/MockImageCtx.h"
+#include "test/librbd/mock/MockImageWatcher.h"
 #include "test/librbd/mock/MockJournal.h"
 #include "test/librbd/mock/MockJournalPolicy.h"
 #include "test/librbd/mock/MockObjectMap.h"
@@ -162,6 +163,8 @@ public:
       expect.WillOnce(Return(r));
     } else {
       expect.WillOnce(DoDefault());
+      EXPECT_CALL(*mock_image_ctx.image_watcher, is_unregistered())
+        .WillOnce(Return(false));
     }
   }
 

diff --git a/src/test/librbd/mock/MockImageWatcher.h b/src/test/librbd/mock/MockImageWatcher.h
index d9d83427bdd6d4e2e50ecf566b771026d01a8277..db3fafe3558234cf86a2eb33bab5634bc728939f 100644 (file)
--- a/src/test/librbd/mock/MockImageWatcher.h
+++ b/src/test/librbd/mock/MockImageWatcher.h
@@ -12,6 +12,7 @@ namespace librbd {
 
 struct MockImageWatcher {
   MOCK_METHOD0(is_registered, bool());
+  MOCK_METHOD0(is_unregistered, bool());
   MOCK_METHOD0(unregister_watch, void());
   MOCK_METHOD1(flush, void(Context *));