From 0cb485ecb391b1a42c47250f37cf4eb8d6180c58 Mon Sep 17 00:00:00 2001 From: Mykola Golub Date: Tue, 24 Oct 2017 20:38:59 +0300 Subject: [PATCH] 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 --- src/librbd/ImageCtx.cc | 14 ++- src/librbd/ImageCtx.h | 1 + src/librbd/image/OpenRequest.cc | 104 ++++-------------- src/librbd/image/OpenRequest.h | 23 ++-- src/librbd/image/RefreshRequest.cc | 4 +- .../librbd/image/test_mock_RefreshRequest.cc | 3 + src/test/librbd/mock/MockImageWatcher.h | 1 + 7 files changed, 46 insertions(+), 104 deletions(-) diff --git a/src/librbd/ImageCtx.cc b/src/librbd/ImageCtx.cc index 6286faf3998..2008a07a769 100644 --- 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 625da49cd98..ed77e339d64 100644 --- 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 bb1a3875ff4..73a2273e8a0 100644 --- 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 OpenRequest::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 @@ -75,7 +68,7 @@ Context *OpenRequest::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::handle_v2_get_data_pool(int *result) { } m_image_ctx->init_layout(); - send_v2_apply_metadata(); + send_refresh(); return nullptr; } template -void OpenRequest::send_v2_apply_metadata() { - CephContext *cct = m_image_ctx->cct; - ldout(cct, 10) << this << " " << __func__ << ": " - << "start_key=" << m_last_metadata_key << dendl; +void OpenRequest::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; - librados::AioCompletion *comp = - create_rados_callback(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 *req = RefreshRequest::create( + *m_image_ctx, false, m_skip_open_parent_image, + create_context_callback(this)); + req->send(); } template -Context *OpenRequest::handle_v2_apply_metadata(int *result) { +Context *OpenRequest::handle_refresh(int *result) { CephContext *cct = m_image_ctx->cct; - ldout(cct, 10) << this << " " << __func__ << ": r=" << *result << dendl; - - std::map 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 -void OpenRequest::send_register_watch() { - m_image_ctx->init(); - +Context *OpenRequest::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::send_register_watch() { Context *ctx = create_context_callback< klass, &klass::handle_register_watch>(this); m_image_ctx->register_watch(ctx); + return nullptr; } template @@ -503,37 +470,10 @@ Context *OpenRequest::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 -void OpenRequest::send_refresh() { - CephContext *cct = m_image_ctx->cct; - ldout(cct, 10) << this << " " << __func__ << dendl; - - using klass = OpenRequest; - RefreshRequest *req = RefreshRequest::create( - *m_image_ctx, false, m_skip_open_parent_image, - create_context_callback(this)); - req->send(); -} - -template -Context *OpenRequest::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 diff --git a/src/librbd/image/OpenRequest.h b/src/librbd/image/OpenRequest.h index 8ea1ff10a2e..45395f148d3 100644 --- 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 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 111cc133cf5..3dff35fe6d3 100644 --- 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::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 02ab30d4432..7747956e34b 100644 --- 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 d9d83427bdd..db3fafe3558 100644 --- 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 *)); -- 2.39.5