From ad504b10f60290cc7461ea96eaada1fb3f7639d7 Mon Sep 17 00:00:00 2001 From: Ilya Dryomov Date: Thu, 18 Aug 2022 18:48:39 +0200 Subject: [PATCH] librbd/cache/pwl: generate image cache state json under m_lock The previous commit moved the entirety of write_image_cache_state() from under m_lock. This was a step too far because the generated image cache state json is no longer guaranteed to be consistent. Arrange for m_lock to still be held during image cache json generation but released before owner_lock is grabbed. Signed-off-by: Ilya Dryomov --- src/librbd/cache/pwl/AbstractWriteLog.cc | 57 ++++++++----------- src/librbd/cache/pwl/AbstractWriteLog.h | 2 +- src/librbd/cache/pwl/ImageCacheState.cc | 7 ++- src/librbd/cache/pwl/ImageCacheState.h | 3 +- src/librbd/cache/pwl/rwl/WriteLog.cc | 41 ++++++------- src/librbd/cache/pwl/ssd/WriteLog.cc | 30 +++++----- .../cache/pwl/test_mock_ReplicatedWriteLog.cc | 5 +- .../librbd/cache/pwl/test_mock_SSDWriteLog.cc | 5 +- 8 files changed, 72 insertions(+), 78 deletions(-) diff --git a/src/librbd/cache/pwl/AbstractWriteLog.cc b/src/librbd/cache/pwl/AbstractWriteLog.cc index 72fcf4fe95d..580726ddffc 100644 --- a/src/librbd/cache/pwl/AbstractWriteLog.cc +++ b/src/librbd/cache/pwl/AbstractWriteLog.cc @@ -314,11 +314,7 @@ void AbstractWriteLog::log_perf() { template void AbstractWriteLog::periodic_stats() { - { - std::lock_guard locker(m_lock); - update_image_cache_state(); - } - write_image_cache_state(); + std::unique_lock locker(m_lock); ldout(m_image_ctx.cct, 5) << "STATS: m_log_entries=" << m_log_entries.size() << ", m_dirty_log_entries=" << m_dirty_log_entries.size() << ", m_free_log_entries=" << m_free_log_entries @@ -331,6 +327,9 @@ void AbstractWriteLog::periodic_stats() { << ", m_current_sync_gen=" << m_current_sync_gen << ", m_flushed_sync_gen=" << m_flushed_sync_gen << dendl; + + update_image_cache_state(); + write_image_cache_state(locker); } template @@ -573,11 +572,11 @@ void AbstractWriteLog::pwl_init(Context *on_finish, DeferredContexts &later) } template -void AbstractWriteLog::write_image_cache_state() { +void AbstractWriteLog::write_image_cache_state(std::unique_lock& locker) { using klass = AbstractWriteLog; Context *ctx = util::create_context_callback< klass, &klass::handle_write_image_cache_state>(this); - m_cache_state->write_image_cache_state(ctx); + m_cache_state->write_image_cache_state(locker, ctx); } template @@ -624,11 +623,9 @@ void AbstractWriteLog::init(Context *on_finish) { Context *ctx = new LambdaContext( [this, on_finish](int r) { if (r >= 0) { - { - std::lock_guard locker(m_lock); - update_image_cache_state(); - } - m_cache_state->write_image_cache_state(on_finish); + std::unique_lock locker(m_lock); + update_image_cache_state(); + m_cache_state->write_image_cache_state(locker, on_finish); } else { on_finish->complete(r); } @@ -659,17 +656,15 @@ void AbstractWriteLog::shut_down(Context *on_finish) { Context *next_ctx = override_ctx(r, ctx); periodic_stats(); - { - std::lock_guard locker(m_lock); - check_image_cache_state_clean(); - m_wake_up_enabled = false; - m_log_entries.clear(); - m_cache_state->clean = true; - m_cache_state->empty = true; - remove_pool_file(); - update_image_cache_state(); - } - m_cache_state->write_image_cache_state(next_ctx); + std::unique_lock locker(m_lock); + check_image_cache_state_clean(); + m_wake_up_enabled = false; + m_log_entries.clear(); + m_cache_state->clean = true; + m_cache_state->empty = true; + remove_pool_file(); + update_image_cache_state(); + m_cache_state->write_image_cache_state(locker, next_ctx); }); ctx = new LambdaContext( [this, ctx](int r) { @@ -1353,7 +1348,8 @@ void AbstractWriteLog::complete_op_log_entries(GenericLogOperations &&ops, m_perfcounter->tinc(l_librbd_pwl_log_op_app_to_cmp_t, now - op->log_append_start_time); } if (need_update_state) { - write_image_cache_state(); + std::unique_lock locker(m_lock); + write_image_cache_state(locker); } // New entries may be flushable { @@ -1807,7 +1803,8 @@ void AbstractWriteLog::process_writeback_dirty_entries() { construct_flush_entries(entries_to_flush, post_unlock, has_write_entry); } if (need_update_state) { - write_image_cache_state(); + std::unique_lock locker(m_lock); + write_image_cache_state(locker); } if (all_clean) { @@ -2023,21 +2020,17 @@ void AbstractWriteLog::flush_dirty_entries(Context *on_finish) { bool all_clean; bool flushing; bool stop_flushing; - bool need_update_state = false; { - std::lock_guard locker(m_lock); + std::unique_lock locker(m_lock); flushing = (0 != m_flush_ops_in_flight); all_clean = m_dirty_log_entries.empty(); + stop_flushing = (m_shutting_down); if (!m_cache_state->clean && all_clean && !flushing) { m_cache_state->clean = true; update_image_cache_state(); - need_update_state = true; + write_image_cache_state(locker); } - stop_flushing = (m_shutting_down); - } - if (need_update_state) { - write_image_cache_state(); } if (!flushing && (all_clean || stop_flushing)) { diff --git a/src/librbd/cache/pwl/AbstractWriteLog.h b/src/librbd/cache/pwl/AbstractWriteLog.h index 5fece849abc..ffe299c37d3 100644 --- a/src/librbd/cache/pwl/AbstractWriteLog.h +++ b/src/librbd/cache/pwl/AbstractWriteLog.h @@ -397,7 +397,7 @@ protected: return 0; } void update_image_cache_state(void); - void write_image_cache_state(void); + void write_image_cache_state(std::unique_lock& locker); void handle_write_image_cache_state(int r); }; diff --git a/src/librbd/cache/pwl/ImageCacheState.cc b/src/librbd/cache/pwl/ImageCacheState.cc index fe6e1087d05..ab941df0f65 100644 --- a/src/librbd/cache/pwl/ImageCacheState.cc +++ b/src/librbd/cache/pwl/ImageCacheState.cc @@ -60,9 +60,10 @@ bool ImageCacheState::init_from_metadata(json_spirit::mValue& json_root) { } template -void ImageCacheState::write_image_cache_state(Context *on_finish) { +void ImageCacheState::write_image_cache_state(std::unique_lock& locker, + Context *on_finish) { + ceph_assert(ceph_mutex_is_locked_by_me(*locker.mutex())); stats_timestamp = ceph_clock_now(); - std::shared_lock owner_lock{m_image_ctx->owner_lock}; json_spirit::mObject o; o["present"] = present; o["empty"] = empty; @@ -82,7 +83,9 @@ void ImageCacheState::write_image_cache_state(Context *on_finish) { o["hit_bytes"] = hit_bytes; o["miss_bytes"] = miss_bytes; std::string image_state_json = json_spirit::write(o); + locker.unlock(); + std::shared_lock owner_lock{m_image_ctx->owner_lock}; ldout(m_image_ctx->cct, 20) << __func__ << " Store state: " << image_state_json << dendl; m_plugin_api.execute_image_metadata_set(m_image_ctx, PERSISTENT_CACHE_STATE, diff --git a/src/librbd/cache/pwl/ImageCacheState.h b/src/librbd/cache/pwl/ImageCacheState.h index c2fd4b77825..5be5f73ac15 100644 --- a/src/librbd/cache/pwl/ImageCacheState.h +++ b/src/librbd/cache/pwl/ImageCacheState.h @@ -63,7 +63,8 @@ public: void init_from_config(); bool init_from_metadata(json_spirit::mValue& json_root); - void write_image_cache_state(Context *on_finish); + void write_image_cache_state(std::unique_lock& locker, + Context *on_finish); void clear_image_cache_state(Context *on_finish); diff --git a/src/librbd/cache/pwl/rwl/WriteLog.cc b/src/librbd/cache/pwl/rwl/WriteLog.cc index 1fdd5d966b0..e922ba543aa 100644 --- a/src/librbd/cache/pwl/rwl/WriteLog.cc +++ b/src/librbd/cache/pwl/rwl/WriteLog.cc @@ -100,33 +100,27 @@ void WriteLog::alloc_op_log_entries(GenericLogOperations &ops) TOID(struct WriteLogPoolRoot) pool_root; pool_root = POBJ_ROOT(m_log_pool, struct WriteLogPoolRoot); struct WriteLogCacheEntry *pmem_log_entries = D_RW(D_RW(pool_root)->log_entries); - bool need_update_state = false; ceph_assert(ceph_mutex_is_locked_by_me(this->m_log_append_lock)); - { - /* Allocate the (already reserved) log entries */ - std::lock_guard locker(m_lock); + /* Allocate the (already reserved) log entries */ + std::unique_lock locker(m_lock); - for (auto &operation : ops) { - uint32_t entry_index = this->m_first_free_entry; - this->m_first_free_entry = (this->m_first_free_entry + 1) % this->m_total_log_entries; - auto &log_entry = operation->get_log_entry(); - log_entry->log_entry_index = entry_index; - log_entry->ram_entry.entry_index = entry_index; - log_entry->cache_entry = &pmem_log_entries[entry_index]; - log_entry->ram_entry.set_entry_valid(true); - m_log_entries.push_back(log_entry); - ldout(m_image_ctx.cct, 20) << "operation=[" << *operation << "]" << dendl; - } - if (m_cache_state->empty && !m_log_entries.empty()) { - m_cache_state->empty = false; - this->update_image_cache_state(); - need_update_state = true; - } + for (auto &operation : ops) { + uint32_t entry_index = this->m_first_free_entry; + this->m_first_free_entry = (this->m_first_free_entry + 1) % this->m_total_log_entries; + auto &log_entry = operation->get_log_entry(); + log_entry->log_entry_index = entry_index; + log_entry->ram_entry.entry_index = entry_index; + log_entry->cache_entry = &pmem_log_entries[entry_index]; + log_entry->ram_entry.set_entry_valid(true); + m_log_entries.push_back(log_entry); + ldout(m_image_ctx.cct, 20) << "operation=[" << *operation << "]" << dendl; } - if (need_update_state) { - this->write_image_cache_state(); + if (m_cache_state->empty && !m_log_entries.empty()) { + m_cache_state->empty = false; + this->update_image_cache_state(); + this->write_image_cache_state(locker); } } @@ -583,7 +577,8 @@ bool WriteLog::retire_entries(const unsigned long int frees_per_tx) { this->wake_up(); } if (need_update_state) { - this->write_image_cache_state(); + std::unique_lock locker(m_lock); + this->write_image_cache_state(locker); } } else { ldout(cct, 20) << "Nothing to retire" << dendl; diff --git a/src/librbd/cache/pwl/ssd/WriteLog.cc b/src/librbd/cache/pwl/ssd/WriteLog.cc index 1fee2d13e72..753b15b69f7 100644 --- a/src/librbd/cache/pwl/ssd/WriteLog.cc +++ b/src/librbd/cache/pwl/ssd/WriteLog.cc @@ -531,23 +531,18 @@ void WriteLog::release_ram(std::shared_ptr log_entry) { template void WriteLog::alloc_op_log_entries(GenericLogOperations &ops) { - bool need_update_state = false; - { - std::lock_guard locker(m_lock); - for (auto &operation : ops) { - auto &log_entry = operation->get_log_entry(); - log_entry->ram_entry.set_entry_valid(true); - m_log_entries.push_back(log_entry); - ldout(m_image_ctx.cct, 20) << "operation=[" << *operation << "]" << dendl; - } - if (m_cache_state->empty && !m_log_entries.empty()) { - m_cache_state->empty = false; - this->update_image_cache_state(); - need_update_state = true; - } + std::unique_lock locker(m_lock); + + for (auto &operation : ops) { + auto &log_entry = operation->get_log_entry(); + log_entry->ram_entry.set_entry_valid(true); + m_log_entries.push_back(log_entry); + ldout(m_image_ctx.cct, 20) << "operation=[" << *operation << "]" << dendl; } - if (need_update_state) { - this->write_image_cache_state(); + if (m_cache_state->empty && !m_log_entries.empty()) { + m_cache_state->empty = false; + this->update_image_cache_state(); + this->write_image_cache_state(locker); } } @@ -841,7 +836,8 @@ bool WriteLog::retire_entries(const unsigned long int frees_per_tx) { this->wake_up(); } if (need_update_state) { - this->write_image_cache_state(); + std::unique_lock locker(m_lock); + this->write_image_cache_state(locker); } this->dispatch_deferred_writes(); diff --git a/src/test/librbd/cache/pwl/test_mock_ReplicatedWriteLog.cc b/src/test/librbd/cache/pwl/test_mock_ReplicatedWriteLog.cc index fc372b0d8c7..a37f5803897 100644 --- a/src/test/librbd/cache/pwl/test_mock_ReplicatedWriteLog.cc +++ b/src/test/librbd/cache/pwl/test_mock_ReplicatedWriteLog.cc @@ -118,10 +118,13 @@ TEST_F(TestMockCacheReplicatedWriteLog, init_state_write) { image_cache_state.empty = false; image_cache_state.clean = false; + ceph::mutex lock = ceph::make_mutex("MockImageCacheStateRWL lock"); MockContextRWL finish_ctx; expect_metadata_set(mock_image_ctx); expect_context_complete(finish_ctx, 0); - image_cache_state.write_image_cache_state(&finish_ctx); + std::unique_lock locker(lock); + image_cache_state.write_image_cache_state(locker, &finish_ctx); + ASSERT_FALSE(locker.owns_lock()); ASSERT_EQ(0, finish_ctx.wait()); } diff --git a/src/test/librbd/cache/pwl/test_mock_SSDWriteLog.cc b/src/test/librbd/cache/pwl/test_mock_SSDWriteLog.cc index b1fc4726461..72a44dcc9e1 100644 --- a/src/test/librbd/cache/pwl/test_mock_SSDWriteLog.cc +++ b/src/test/librbd/cache/pwl/test_mock_SSDWriteLog.cc @@ -120,10 +120,13 @@ TEST_F(TestMockCacheSSDWriteLog, init_state_write) { image_cache_state.empty = false; image_cache_state.clean = false; + ceph::mutex lock = ceph::make_mutex("MockImageCacheStateSSD lock"); MockContextSSD finish_ctx; expect_metadata_set(mock_image_ctx); expect_context_complete(finish_ctx, 0); - image_cache_state.write_image_cache_state(&finish_ctx); + std::unique_lock locker(lock); + image_cache_state.write_image_cache_state(locker, &finish_ctx); + ASSERT_FALSE(locker.owns_lock()); ASSERT_EQ(0, finish_ctx.wait()); } -- 2.39.5