From b1d712d29d57c08a484688381b8a0dc092c4e4e0 Mon Sep 17 00:00:00 2001 From: Yin Congmin Date: Mon, 29 Nov 2021 11:47:52 +0800 Subject: [PATCH] librbd/cache/pwl: clean code and update comments some comments are outdated, some are no longger needed, update them. remove redundant ";". remove redundant log. remove redundant "". Signed-off-by: Yin Congmin --- src/librbd/cache/pwl/AbstractWriteLog.cc | 17 +++++++------- src/librbd/cache/pwl/Request.cc | 28 +++++++++++++----------- src/librbd/cache/pwl/SyncPoint.cc | 2 +- src/librbd/cache/pwl/Types.h | 17 +++++++------- src/librbd/cache/pwl/rwl/Request.cc | 8 +++---- src/librbd/cache/pwl/rwl/WriteLog.cc | 16 ++++++-------- src/librbd/cache/pwl/ssd/Request.cc | 8 +++---- src/librbd/cache/pwl/ssd/WriteLog.cc | 12 +++++----- 8 files changed, 54 insertions(+), 54 deletions(-) diff --git a/src/librbd/cache/pwl/AbstractWriteLog.cc b/src/librbd/cache/pwl/AbstractWriteLog.cc index 7c1fa44d724c9..d19a6f4a26b90 100644 --- a/src/librbd/cache/pwl/AbstractWriteLog.cc +++ b/src/librbd/cache/pwl/AbstractWriteLog.cc @@ -412,7 +412,7 @@ void AbstractWriteLog::update_sync_points(std::map &missing_s ceph_assert(kv.first == m_current_sync_gen+1); init_flush_new_sync_point(later); ceph_assert(kv.first == m_current_sync_gen); - sync_point_entries[kv.first] = m_current_sync_point->log_entry;; + sync_point_entries[kv.first] = m_current_sync_point->log_entry; } /* @@ -519,7 +519,7 @@ void AbstractWriteLog::pwl_init(Context *on_finish, DeferredContexts &later) ldout(cct, 5) << "There's an existing pool file " << m_log_pool_name << ", While there's no cache in the image metatata." << dendl; if (remove(m_log_pool_name.c_str()) != 0) { - lderr(cct) << "Failed to remove the pool file " << m_log_pool_name + lderr(cct) << "failed to remove the pool file " << m_log_pool_name << dendl; on_finish->complete(-errno); return; @@ -528,7 +528,8 @@ void AbstractWriteLog::pwl_init(Context *on_finish, DeferredContexts &later) } } else if ((m_cache_state->present) && (access(m_log_pool_name.c_str(), F_OK) != 0)) { - ldout(cct, 5) << "Can't find the existed pool file " << m_log_pool_name << dendl; + lderr(cct) << "can't find the existed pool file: " << m_log_pool_name + << ". error: " << cpp_strerror(-errno) << dendl; on_finish->complete(-errno); return; } @@ -828,11 +829,9 @@ void AbstractWriteLog::write(Extents &&image_extents, ceph_assert(m_initialized); - /* Split images because PMDK's space management is not perfect, there are - * fragment problems. The larger the block size difference of the block, - * the easier the fragmentation problem will occur, resulting in the - * remaining space can not be allocated in large size. We plan to manage - * pmem space and allocation by ourselves in the future. + /* Split image extents larger than 1M. This isn't strictly necessary but + * makes libpmemobj allocator's job easier and reduces pmemobj_defrag() cost. + * We plan to manage pmem space and allocation by ourselves in the future. */ Extents split_image_extents; uint64_t max_extent_size = get_max_extent(); @@ -2136,7 +2135,7 @@ bool AbstractWriteLog::can_retire_entry(std::shared_ptr log_ template void AbstractWriteLog::check_image_cache_state_clean() { ceph_assert(m_deferred_ios.empty()); - ceph_assert(m_ops_to_append.empty());; + ceph_assert(m_ops_to_append.empty()); ceph_assert(m_async_flush_ops == 0); ceph_assert(m_async_append_ops == 0); ceph_assert(m_dirty_log_entries.empty()); diff --git a/src/librbd/cache/pwl/Request.cc b/src/librbd/cache/pwl/Request.cc index 4770e498fba54..d69ea725379c7 100644 --- a/src/librbd/cache/pwl/Request.cc +++ b/src/librbd/cache/pwl/Request.cc @@ -35,16 +35,16 @@ C_BlockIORequest::~C_BlockIORequest() { template std::ostream &operator<<(std::ostream &os, const C_BlockIORequest &req) { - os << "image_extents=[" << req.image_extents << "], " - << "image_extents_summary=[" << req.image_extents_summary << "], " - << "bl=" << req.bl << ", " - << "user_req=" << req.user_req << ", " - << "m_user_req_completed=" << req.m_user_req_completed << ", " - << "m_deferred=" << req.m_deferred << ", " - << "detained=" << req.detained << ", " - << "waited_lanes=" << req.waited_lanes << ", " - << "waited_entries=" << req.waited_entries << ", " - << "waited_buffers=" << req.waited_buffers << ""; + os << "image_extents=" << req.image_extents << "," + << " image_extents_summary=[" << req.image_extents_summary << "]," + << " bl=" << req.bl << "," + << " user_req=" << req.user_req << "," + << " m_user_req_completed=" << req.m_user_req_completed << "," + << " m_deferred=" << req.m_deferred << "," + << " detained=" << req.detained << "," + << " waited_lanes=" << req.waited_lanes << "," + << " waited_entries=" << req.waited_entries << "," + << " waited_buffers=" << req.waited_buffers; return os; } @@ -141,7 +141,7 @@ std::ostream &operator<<(std::ostream &os, os << (C_BlockIORequest&)req << " m_resources.allocated=" << req.m_resources.allocated; if (req.op_set) { - os << "op_set=" << *req.op_set; + os << " op_set=" << *req.op_set; } return os; } @@ -211,7 +211,8 @@ void C_WriteRequest::setup_log_operations(DeferredContexts &on_exit) { current_sync_point, pwl.get_persist_on_flush(), pwl.get_context(), this); - ldout(pwl.get_context(), 20) << "write_req=" << *this << " op_set=" << op_set.get() << dendl; + ldout(pwl.get_context(), 20) << "write_req=[" << *this << "]" + << " op_set=" << op_set.get() << dendl; ceph_assert(m_resources.allocated); /* op_set->operations initialized differently for plain write or write same */ auto allocation = m_resources.buffers.begin(); @@ -222,7 +223,8 @@ void C_WriteRequest::setup_log_operations(DeferredContexts &on_exit) { this->op_set->operations.emplace_back(operation); /* A WS is also a write */ - ldout(pwl.get_context(), 20) << "write_req=" << *this << " op_set=" << op_set.get() + ldout(pwl.get_context(), 20) << "write_req=[" << *this << "]" + << " op_set=" << op_set.get() << " operation=" << operation << dendl; log_entries.emplace_back(operation->log_entry); if (!op_set->persist_on_flush) { diff --git a/src/librbd/cache/pwl/SyncPoint.cc b/src/librbd/cache/pwl/SyncPoint.cc index 8fb2f82052e63..e0e13edf6fa17 100644 --- a/src/librbd/cache/pwl/SyncPoint.cc +++ b/src/librbd/cache/pwl/SyncPoint.cc @@ -38,7 +38,7 @@ std::ostream &operator<<(std::ostream &os, << "m_append_scheduled=" << p.m_append_scheduled << ", " << "appending=" << p.appending << ", " << "on_sync_point_appending=" << p.on_sync_point_appending.size() << ", " - << "on_sync_point_persisted=" << p.on_sync_point_persisted.size() << ""; + << "on_sync_point_persisted=" << p.on_sync_point_persisted.size(); return os; } diff --git a/src/librbd/cache/pwl/Types.h b/src/librbd/cache/pwl/Types.h index 863292e161172..445be8262732c 100644 --- a/src/librbd/cache/pwl/Types.h +++ b/src/librbd/cache/pwl/Types.h @@ -283,7 +283,7 @@ struct WriteLogPoolRoot { #ifdef WITH_RBD_RWL union { struct { - uint8_t layout_version; /* Version of this structure (RWL_LAYOUT_VERSION) */ + uint8_t layout_version; }; uint64_t _u64; } header; @@ -291,15 +291,16 @@ struct WriteLogPoolRoot { #endif #ifdef WITH_RBD_SSD_CACHE uint64_t layout_version = 0; - uint64_t cur_sync_gen = 0; + uint64_t cur_sync_gen = 0; /* TODO: remove it when changing disk format */ #endif uint64_t pool_size; - uint64_t flushed_sync_gen; /* All writing entries with this or a lower - * sync gen number are flushed. */ - uint32_t block_size; /* block size */ + uint64_t flushed_sync_gen; /* All writing entries with this or a lower + * sync gen number are flushed. */ + uint32_t block_size; uint32_t num_log_entries; - uint64_t first_free_entry; /* Entry following the newest valid entry */ - uint64_t first_valid_entry; /* Index of the oldest valid entry in the log */ + uint64_t first_free_entry; /* The free entry following the latest valid + * entry, which is going to be written */ + uint64_t first_valid_entry; /* The oldest valid entry to be retired */ #ifdef WITH_RBD_SSD_CACHE DENC(WriteLogPoolRoot, v, p) { @@ -346,7 +347,7 @@ public: const ExtentsSummary &s) { os << "total_bytes=" << s.total_bytes << ", " << "first_image_byte=" << s.first_image_byte << ", " - << "last_image_byte=" << s.last_image_byte << ""; + << "last_image_byte=" << s.last_image_byte; return os; } BlockExtent block_extent() { diff --git a/src/librbd/cache/pwl/rwl/Request.cc b/src/librbd/cache/pwl/rwl/Request.cc index 09158127241a3..093e09a94f5c6 100644 --- a/src/librbd/cache/pwl/rwl/Request.cc +++ b/src/librbd/cache/pwl/rwl/Request.cc @@ -49,10 +49,10 @@ template std::ostream &operator<<(std::ostream &os, const C_CompAndWriteRequest &req) { os << (C_WriteRequest&)req - << "cmp_bl=" << req.cmp_bl << ", " - << "read_bl=" << req.read_bl << ", " - << "compare_succeeded=" << req.compare_succeeded << ", " - << "mismatch_offset=" << req.mismatch_offset; + << " cmp_bl=" << req.cmp_bl << "," + << " read_bl=" << req.read_bl << "," + << " compare_succeeded=" << req.compare_succeeded << "," + << " mismatch_offset=" << req.mismatch_offset; return os; } diff --git a/src/librbd/cache/pwl/rwl/WriteLog.cc b/src/librbd/cache/pwl/rwl/WriteLog.cc index c340fe6a8c8d7..44ca037f82e1b 100644 --- a/src/librbd/cache/pwl/rwl/WriteLog.cc +++ b/src/librbd/cache/pwl/rwl/WriteLog.cc @@ -160,9 +160,6 @@ int WriteLog::append_op_log_entries(GenericLogOperations &ops) << "from " << &operation->get_log_entry()->ram_entry << " " << "to " << operation->get_log_entry()->cache_entry << " " << "operation=[" << *operation << "]" << dendl; - ldout(m_image_ctx.cct, 05) << "APPENDING: index=" - << operation->get_log_entry()->log_entry_index << " " - << "operation=[" << *operation << "]" << dendl; operation->log_append_start_time = now; *operation->get_log_entry()->cache_entry = operation->get_log_entry()->ram_entry; ldout(m_image_ctx.cct, 20) << "APPENDING: index=" @@ -270,8 +267,8 @@ bool WriteLog::initialize_pool(Context *on_finish, pwl::DeferredContexts &lat this->m_pwl_pool_layout_name, this->m_log_pool_size, (S_IWUSR | S_IRUSR))) == NULL) { - lderr(cct) << "failed to create pool (" << this->m_log_pool_name << ")" - << pmemobj_errormsg() << dendl; + lderr(cct) << "failed to create pool: " << this->m_log_pool_name + << ". error: " << pmemobj_errormsg() << dendl; m_cache_state->present = false; m_cache_state->clean = true; m_cache_state->empty = true; @@ -317,7 +314,8 @@ bool WriteLog::initialize_pool(Context *on_finish, pwl::DeferredContexts &lat } TX_ONABORT { this->m_total_log_entries = 0; this->m_free_log_entries = 0; - lderr(cct) << "failed to initialize pool (" << this->m_log_pool_name << ")" << dendl; + lderr(cct) << "failed to initialize pool: " << this->m_log_pool_name + << ". pmemobj TX errno: " << pmemobj_tx_errno() << dendl; r = -pmemobj_tx_errno(); goto err_close_pool; } TX_FINALLY { @@ -336,13 +334,13 @@ bool WriteLog::initialize_pool(Context *on_finish, pwl::DeferredContexts &lat pool_root = POBJ_ROOT(m_log_pool, struct WriteLogPoolRoot); if (D_RO(pool_root)->header.layout_version != RWL_LAYOUT_VERSION) { // TODO: will handle upgrading version in the future - lderr(cct) << "Pool layout version is " + lderr(cct) << "pool layout version is " << D_RO(pool_root)->header.layout_version << " expected " << RWL_LAYOUT_VERSION << dendl; goto err_close_pool; } if (D_RO(pool_root)->block_size != MIN_WRITE_ALLOC_SIZE) { - lderr(cct) << "Pool block size is " << D_RO(pool_root)->block_size + lderr(cct) << "pool block size is " << D_RO(pool_root)->block_size << " expected " << MIN_WRITE_ALLOC_SIZE << dendl; goto err_close_pool; } @@ -482,7 +480,7 @@ bool WriteLog::retire_entries(const unsigned long int frees_per_tx) { this->can_retire_entry(m_log_entries.front())) { auto entry = m_log_entries.front(); if (entry->log_entry_index != first_valid_entry) { - lderr(cct) << "Retiring entry index (" << entry->log_entry_index + lderr(cct) << "retiring entry index (" << entry->log_entry_index << ") and first valid log entry index (" << first_valid_entry << ") must be ==." << dendl; } diff --git a/src/librbd/cache/pwl/ssd/Request.cc b/src/librbd/cache/pwl/ssd/Request.cc index e92e547c8e607..308db78f7c363 100644 --- a/src/librbd/cache/pwl/ssd/Request.cc +++ b/src/librbd/cache/pwl/ssd/Request.cc @@ -34,10 +34,10 @@ template std::ostream &operator<<(std::ostream &os, const C_CompAndWriteRequest &req) { os << (C_WriteRequest&)req - << "cmp_bl=" << req.cmp_bl << ", " - << "read_bl=" << req.read_bl << ", " - << "compare_succeeded=" << req.compare_succeeded << ", " - << "mismatch_offset=" << req.mismatch_offset; + << " cmp_bl=" << req.cmp_bl << "," + << " read_bl=" << req.read_bl << "," + << " compare_succeeded=" << req.compare_succeeded << "," + << " mismatch_offset=" << req.mismatch_offset; return os; } diff --git a/src/librbd/cache/pwl/ssd/WriteLog.cc b/src/librbd/cache/pwl/ssd/WriteLog.cc index b537ce948a65d..8ffdb7e94d2c3 100644 --- a/src/librbd/cache/pwl/ssd/WriteLog.cc +++ b/src/librbd/cache/pwl/ssd/WriteLog.cc @@ -209,7 +209,7 @@ bool WriteLog::initialize_pool(Context *on_finish, ::IOContext ioctx(cct, nullptr); r = bdev->read(0, MIN_WRITE_ALLOC_SSD_SIZE, &bl, &ioctx, false); if (r < 0) { - lderr(cct) << "Read ssd cache superblock failed " << dendl; + lderr(cct) << "read ssd cache superblock failed " << dendl; goto err_close_bdev; } auto p = bl.cbegin(); @@ -222,14 +222,14 @@ bool WriteLog::initialize_pool(Context *on_finish, << dendl; ceph_assert(is_valid_pool_root(pool_root)); if (pool_root.layout_version != SSD_LAYOUT_VERSION) { - lderr(cct) << "Pool layout version is " + lderr(cct) << "pool layout version is " << pool_root.layout_version << " expected " << SSD_LAYOUT_VERSION << dendl; goto err_close_bdev; } if (pool_root.block_size != MIN_WRITE_ALLOC_SSD_SIZE) { - lderr(cct) << "Pool block size is " << pool_root.block_size + lderr(cct) << "pool block size is " << pool_root.block_size << " expected " << MIN_WRITE_ALLOC_SSD_SIZE << dendl; goto err_close_bdev; @@ -446,8 +446,8 @@ void WriteLog::append_scheduled_ops(void) { GenericLogOperations ops; ldout(m_image_ctx.cct, 20) << dendl; - bool ops_remain = false; //no-op variable for SSD - bool appending = false; //no-op variable for SSD + bool ops_remain = false; // unused, no-op variable for SSD + bool appending = false; // unused, no-op variable for SSD this->append_scheduled(ops, ops_remain, appending); if (ops.size()) { @@ -1026,7 +1026,7 @@ void WriteLog::update_root_scheduled_ops() { }); Context *append_ctx = new LambdaContext([this, ctx](int r) { ldout(m_image_ctx.cct, 15) << "Finish the update of pool root." << dendl; - bool need_finisher = false;; + bool need_finisher = false; assert(r == 0); { std::lock_guard locker(m_lock); -- 2.39.5