From: Yin Congmin Date: Thu, 28 Jul 2022 05:43:07 +0000 (+0800) Subject: librbd/cache/pwl: move write_image_cache_state() out of m_lock X-Git-Tag: v17.2.4~36^2~1 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=2bd305811e88db7a99ae5e2dc76219e0f8265f93;p=ceph.git librbd/cache/pwl: move write_image_cache_state() out of m_lock periodic_stats() will get m_lock, then get owner_lock. It is opposite to the lock getting order of SnapshotCreateRequest::handle_notify_quiesce(). move write_image_cache_state() out of m_lock scope. After calling update_image_cache_state(), and m_lock auto released, then call write_image_cache_state() to update state in osds. Fixes: https://tracker.ceph.com/issues/56703 Signed-off-by: Yin Congmin (cherry picked from commit a0e2868d9473a9120c4c5d478f6a592859ce1aec) --- diff --git a/src/librbd/cache/pwl/AbstractWriteLog.cc b/src/librbd/cache/pwl/AbstractWriteLog.cc index 49f4161effe0..72fcf4fe95d4 100644 --- a/src/librbd/cache/pwl/AbstractWriteLog.cc +++ b/src/librbd/cache/pwl/AbstractWriteLog.cc @@ -314,8 +314,11 @@ void AbstractWriteLog::log_perf() { template void AbstractWriteLog::periodic_stats() { - std::lock_guard locker(m_lock); - update_image_cache_state(); + { + std::lock_guard locker(m_lock); + update_image_cache_state(); + } + write_image_cache_state(); 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 @@ -570,15 +573,15 @@ void AbstractWriteLog::pwl_init(Context *on_finish, DeferredContexts &later) } template -void AbstractWriteLog::update_image_cache_state() { +void AbstractWriteLog::write_image_cache_state() { using klass = AbstractWriteLog; Context *ctx = util::create_context_callback< - klass, &klass::handle_update_image_cache_state>(this); - update_image_cache_state(ctx); + klass, &klass::handle_write_image_cache_state>(this); + m_cache_state->write_image_cache_state(ctx); } template -void AbstractWriteLog::update_image_cache_state(Context *on_finish) { +void AbstractWriteLog::update_image_cache_state() { ldout(m_image_ctx.cct, 10) << dendl; ceph_assert(ceph_mutex_is_locked_by_me(m_lock)); @@ -593,11 +596,10 @@ void AbstractWriteLog::update_image_cache_state(Context *on_finish) { m_cache_state->hit_bytes = m_perfcounter->get(l_librbd_pwl_rd_hit_bytes); m_cache_state->miss_bytes = m_perfcounter->get(l_librbd_pwl_rd_bytes) - m_cache_state->hit_bytes; - m_cache_state->write_image_cache_state(on_finish); } template -void AbstractWriteLog::handle_update_image_cache_state(int r) { +void AbstractWriteLog::handle_write_image_cache_state(int r) { CephContext *cct = m_image_ctx.cct; ldout(cct, 10) << "r=" << r << dendl; @@ -622,8 +624,11 @@ 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(on_finish); + { + std::lock_guard locker(m_lock); + update_image_cache_state(); + } + m_cache_state->write_image_cache_state(on_finish); } else { on_finish->complete(r); } @@ -654,14 +659,17 @@ 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(next_ctx); + { + 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); }); ctx = new LambdaContext( [this, ctx](int r) { @@ -1305,6 +1313,7 @@ void AbstractWriteLog::complete_op_log_entries(GenericLogOperations &&ops, { GenericLogEntries dirty_entries; int published_reserves = 0; + bool need_update_state = false; ldout(m_image_ctx.cct, 20) << __func__ << ": completing" << dendl; for (auto &op : ops) { utime_t now = ceph_clock_now(); @@ -1327,6 +1336,7 @@ void AbstractWriteLog::complete_op_log_entries(GenericLogOperations &&ops, if (m_cache_state->clean && !this->m_dirty_log_entries.empty()) { m_cache_state->clean = false; update_image_cache_state(); + need_update_state = true; } } op->complete(result); @@ -1342,6 +1352,9 @@ void AbstractWriteLog::complete_op_log_entries(GenericLogOperations &&ops, log_entry->ram_entry.write_bytes); 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(); + } // New entries may be flushable { std::lock_guard locker(m_lock); @@ -1738,6 +1751,7 @@ void AbstractWriteLog::process_writeback_dirty_entries() { bool all_clean = false; int flushed = 0; bool has_write_entry = false; + bool need_update_state = false; ldout(cct, 20) << "Look for dirty entries" << dendl; { @@ -1760,6 +1774,7 @@ void AbstractWriteLog::process_writeback_dirty_entries() { if (!m_cache_state->clean && all_clean) { m_cache_state->clean = true; update_image_cache_state(); + need_update_state = true; } break; } @@ -1791,6 +1806,9 @@ 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(); + } if (all_clean) { /* All flushing complete, drain outside lock */ @@ -2005,6 +2023,7 @@ 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); @@ -2013,9 +2032,13 @@ void AbstractWriteLog::flush_dirty_entries(Context *on_finish) { if (!m_cache_state->clean && all_clean && !flushing) { m_cache_state->clean = true; update_image_cache_state(); + need_update_state = true; } stop_flushing = (m_shutting_down); } + if (need_update_state) { + write_image_cache_state(); + } if (!flushing && (all_clean || stop_flushing)) { /* Complete without holding m_lock */ diff --git a/src/librbd/cache/pwl/AbstractWriteLog.h b/src/librbd/cache/pwl/AbstractWriteLog.h index 4905edde6e77..5fece849abcc 100644 --- a/src/librbd/cache/pwl/AbstractWriteLog.h +++ b/src/librbd/cache/pwl/AbstractWriteLog.h @@ -233,8 +233,6 @@ private: void arm_periodic_stats(); void pwl_init(Context *on_finish, pwl::DeferredContexts &later); - void update_image_cache_state(Context *on_finish); - void handle_update_image_cache_state(int r); void check_image_cache_state_clean(); void flush_dirty_entries(Context *on_finish); @@ -399,6 +397,8 @@ protected: return 0; } void update_image_cache_state(void); + void write_image_cache_state(void); + void handle_write_image_cache_state(int r); }; } // namespace pwl diff --git a/src/librbd/cache/pwl/rwl/WriteLog.cc b/src/librbd/cache/pwl/rwl/WriteLog.cc index 41bdafe5b9e3..1fdd5d966b08 100644 --- a/src/librbd/cache/pwl/rwl/WriteLog.cc +++ b/src/librbd/cache/pwl/rwl/WriteLog.cc @@ -100,26 +100,33 @@ 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::lock_guard 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; + 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; + } } - if (m_cache_state->empty && !m_log_entries.empty()) { - m_cache_state->empty = false; - this->update_image_cache_state(); + if (need_update_state) { + this->write_image_cache_state(); } } @@ -547,6 +554,7 @@ bool WriteLog::retire_entries(const unsigned long int frees_per_tx) { m_perfcounter->hinc(l_librbd_pwl_retire_tx_t_hist, utime_t(tx_end - tx_start).to_nsec(), retiring_entries.size()); + bool need_update_state = false; /* Update runtime copy of first_valid, and free entries counts */ { std::lock_guard locker(m_lock); @@ -557,6 +565,7 @@ bool WriteLog::retire_entries(const unsigned long int frees_per_tx) { if (!m_cache_state->empty && m_log_entries.empty()) { m_cache_state->empty = true; this->update_image_cache_state(); + need_update_state = true; } for (auto &entry: retiring_entries) { if (entry->write_bytes()) { @@ -573,6 +582,9 @@ bool WriteLog::retire_entries(const unsigned long int frees_per_tx) { this->m_alloc_failed_since_retire = false; this->wake_up(); } + if (need_update_state) { + this->write_image_cache_state(); + } } else { ldout(cct, 20) << "Nothing to retire" << dendl; return false; diff --git a/src/librbd/cache/pwl/ssd/WriteLog.cc b/src/librbd/cache/pwl/ssd/WriteLog.cc index 2c0dc258b86f..1fee2d13e72e 100644 --- a/src/librbd/cache/pwl/ssd/WriteLog.cc +++ b/src/librbd/cache/pwl/ssd/WriteLog.cc @@ -531,17 +531,23 @@ void WriteLog::release_ram(std::shared_ptr log_entry) { template void WriteLog::alloc_op_log_entries(GenericLogOperations &ops) { - 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; + 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; + } } - if (m_cache_state->empty && !m_log_entries.empty()) { - m_cache_state->empty = false; - this->update_image_cache_state(); + if (need_update_state) { + this->write_image_cache_state(); } } @@ -807,6 +813,7 @@ bool WriteLog::retire_entries(const unsigned long int frees_per_tx) { allocated_bytes += entry->get_aligned_data_size(); } } + bool need_update_state = false; { std::lock_guard locker(m_lock); m_first_valid_entry = first_valid_entry; @@ -818,6 +825,7 @@ bool WriteLog::retire_entries(const unsigned long int frees_per_tx) { if (!m_cache_state->empty && m_log_entries.empty()) { m_cache_state->empty = true; this->update_image_cache_state(); + need_update_state = true; } ldout(m_image_ctx.cct, 20) @@ -832,6 +840,9 @@ bool WriteLog::retire_entries(const unsigned long int frees_per_tx) { this->m_alloc_failed_since_retire = false; this->wake_up(); } + if (need_update_state) { + this->write_image_cache_state(); + } this->dispatch_deferred_writes(); this->process_writeback_dirty_entries();