From 64f741ecebf3ff9a31d99f9f3d121ffeff54173b Mon Sep 17 00:00:00 2001 From: Yin Congmin Date: Tue, 26 Apr 2022 01:10:18 +0800 Subject: [PATCH] librbd/cache/pwl: fix endianness issue fix endianness issue with WriteLogCacheEntry encoding. abandon the use of bits in the union. make '&' operation with the whole union filed(flags) to get the bit information. Fixes: https://tracker.ceph.com/issues/55389 Signed-off-by: Yin Congmin --- src/librbd/cache/pwl/LogEntry.cc | 12 ++-- src/librbd/cache/pwl/LogEntry.h | 10 ++-- src/librbd/cache/pwl/Types.cc | 36 +++++------ src/librbd/cache/pwl/Types.h | 90 +++++++++++++++++++++------- src/librbd/cache/pwl/rwl/WriteLog.cc | 2 +- src/librbd/cache/pwl/ssd/WriteLog.cc | 2 +- 6 files changed, 101 insertions(+), 51 deletions(-) diff --git a/src/librbd/cache/pwl/LogEntry.cc b/src/librbd/cache/pwl/LogEntry.cc index c9af236889428..8a050eb792114 100644 --- a/src/librbd/cache/pwl/LogEntry.cc +++ b/src/librbd/cache/pwl/LogEntry.cc @@ -46,7 +46,7 @@ std::ostream &operator<<(std::ostream &os, bool GenericWriteLogEntry::can_writeback() const { return (this->completed && - (ram_entry.sequenced || + (ram_entry.is_sequenced() || (sync_point_entry && sync_point_entry->completed))); } @@ -71,7 +71,7 @@ std::ostream &operator<<(std::ostream &os, void WriteLogEntry::init(bool has_data, uint64_t current_sync_gen, uint64_t last_op_sequence_num, bool persist_on_flush) { - ram_entry.has_data = 1; + ram_entry.set_has_data(has_data); ram_entry.sync_gen_number = current_sync_gen; if (persist_on_flush) { /* Persist on flush. Sequence #0 is never used. */ @@ -79,10 +79,10 @@ void WriteLogEntry::init(bool has_data, } else { /* Persist on write */ ram_entry.write_sequence_number = last_op_sequence_num; - ram_entry.sequenced = 1; + ram_entry.set_sequenced(true); } - ram_entry.sync_point = 0; - ram_entry.discard = 0; + ram_entry.set_sync_point(false); + ram_entry.set_discard(false); } std::ostream& WriteLogEntry::format(std::ostream &os) const { @@ -115,7 +115,7 @@ void DiscardLogEntry::init(uint64_t current_sync_gen, bool persist_on_flush, } else { /* Persist on write */ ram_entry.write_sequence_number = last_op_sequence_num; - ram_entry.sequenced = 1; + ram_entry.set_sequenced(true); } } diff --git a/src/librbd/cache/pwl/LogEntry.h b/src/librbd/cache/pwl/LogEntry.h index 78eb4a6de6acb..ecaca0b7b70d6 100644 --- a/src/librbd/cache/pwl/LogEntry.h +++ b/src/librbd/cache/pwl/LogEntry.h @@ -93,7 +93,7 @@ public: std::shared_ptr next_sync_point_entry = nullptr; SyncPointLogEntry(uint64_t sync_gen_number) { ram_entry.sync_gen_number = sync_gen_number; - ram_entry.sync_point = 1; + ram_entry.set_sync_point(true); }; ~SyncPointLogEntry() override {}; SyncPointLogEntry(const SyncPointLogEntry&) = delete; @@ -185,14 +185,14 @@ public: uint64_t image_offset_bytes, uint64_t write_bytes, uint32_t data_length) : WriteLogEntry(sync_point_entry, image_offset_bytes, write_bytes) { - ram_entry.writesame = 1; + ram_entry.set_writesame(true); ram_entry.ws_datalen = data_length; is_writesame = true; }; WriteLogEntry(uint64_t image_offset_bytes, uint64_t write_bytes, uint32_t data_length) : WriteLogEntry(nullptr, image_offset_bytes, write_bytes) { - ram_entry.writesame = 1; + ram_entry.set_writesame(true); ram_entry.ws_datalen = data_length; is_writesame = true; }; @@ -241,11 +241,11 @@ public: uint32_t discard_granularity_bytes) : GenericWriteLogEntry(sync_point_entry, image_offset_bytes, write_bytes), m_discard_granularity_bytes(discard_granularity_bytes) { - ram_entry.discard = 1; + ram_entry.set_discard(true); }; DiscardLogEntry(uint64_t image_offset_bytes, uint64_t write_bytes) : GenericWriteLogEntry(nullptr, image_offset_bytes, write_bytes) { - ram_entry.discard = 1; + ram_entry.set_discard(true); }; DiscardLogEntry(const DiscardLogEntry&) = delete; DiscardLogEntry &operator=(const DiscardLogEntry&) = delete; diff --git a/src/librbd/cache/pwl/Types.cc b/src/librbd/cache/pwl/Types.cc index bc1bd22f00049..c29305eecaf0d 100644 --- a/src/librbd/cache/pwl/Types.cc +++ b/src/librbd/cache/pwl/Types.cc @@ -60,12 +60,12 @@ void WriteLogCacheEntry::dump(Formatter *f) const { f->dump_unsigned("image_offset_bytes", image_offset_bytes); f->dump_unsigned("write_bytes", write_bytes); f->dump_unsigned("write_data_pos", write_data_pos); - f->dump_unsigned("entry_valid", entry_valid); - f->dump_unsigned("sync_point", sync_point); - f->dump_unsigned("sequenced", sequenced); - f->dump_unsigned("has_data", has_data); - f->dump_unsigned("discard", discard); - f->dump_unsigned("writesame", writesame); + f->dump_bool("entry_valid", is_entry_valid()); + f->dump_bool("sync_point", is_sync_point()); + f->dump_bool("sequenced", is_sequenced()); + f->dump_bool("has_data", has_data()); + f->dump_bool("discard", is_discard()); + f->dump_bool("writesame", is_writesame()); f->dump_unsigned("ws_datalen", ws_datalen); f->dump_unsigned("entry_index", entry_index); } @@ -78,12 +78,12 @@ void WriteLogCacheEntry::generate_test_instances(std::list& ls.back()->image_offset_bytes = 1; ls.back()->write_bytes = 1; ls.back()->write_data_pos = 1; - ls.back()->entry_valid = 1; - ls.back()->sync_point = 1; - ls.back()->sequenced = 1; - ls.back()->has_data = 1; - ls.back()->discard = 1; - ls.back()->writesame = 1; + ls.back()->set_entry_valid(true); + ls.back()->set_sync_point(true); + ls.back()->set_sequenced(true); + ls.back()->set_has_data(true); + ls.back()->set_discard(true); + ls.back()->set_writesame(true); ls.back()->ws_datalen = 1; ls.back()->entry_index = 1; } @@ -115,12 +115,12 @@ void WriteLogPoolRoot::generate_test_instances(std::list& ls) std::ostream& operator<<(std::ostream& os, const WriteLogCacheEntry &entry) { - os << "entry_valid=" << (bool)entry.entry_valid - << ", sync_point=" << (bool)entry.sync_point - << ", sequenced=" << (bool)entry.sequenced - << ", has_data=" << (bool)entry.has_data - << ", discard=" << (bool)entry.discard - << ", writesame=" << (bool)entry.writesame + os << "entry_valid=" << entry.is_entry_valid() + << ", sync_point=" << entry.is_sync_point() + << ", sequenced=" << entry.is_sequenced() + << ", has_data=" << entry.has_data() + << ", discard=" << entry.is_discard() + << ", writesame=" << entry.is_writesame() << ", sync_gen_number=" << entry.sync_gen_number << ", write_sequence_number=" << entry.write_sequence_number << ", image_offset_bytes=" << entry.image_offset_bytes diff --git a/src/librbd/cache/pwl/Types.h b/src/librbd/cache/pwl/Types.h index 62ce9ea1e6e00..0d8c93a24c316 100644 --- a/src/librbd/cache/pwl/Types.h +++ b/src/librbd/cache/pwl/Types.h @@ -150,6 +150,16 @@ enum { l_librbd_pwl_last, }; +enum { + WRITE_LOG_CACHE_ENTRY_VALID = 1U << 0, /* if 0, this entry is free */ + WRITE_LOG_CACHE_ENTRY_SYNC_POINT = 1U << 1, /* No data. No write sequence number. + Marks sync point for this sync gen number */ + WRITE_LOG_CACHE_ENTRY_SEQUENCED = 1U << 2, /* write sequence number is valid */ + WRITE_LOG_CACHE_ENTRY_HAS_DATA = 1U << 3, /* write_data field is valid (else ignore) */ + WRITE_LOG_CACHE_ENTRY_DISCARD = 1U << 4, /* has_data will be 0 if this is a discard */ + WRITE_LOG_CACHE_ENTRY_WRITESAME = 1U << 5, /* ws_datalen indicates length of data at write_bytes */ +}; + namespace librbd { namespace cache { namespace pwl { @@ -220,18 +230,7 @@ struct WriteLogCacheEntry { #ifdef WITH_RBD_SSD_CACHE uint64_t write_data_pos = 0; /* SSD data offset */ #endif - union { - uint8_t flags = 0; - struct { - uint8_t entry_valid :1; /* if 0, this entry is free */ - uint8_t sync_point :1; /* No data. No write sequence number. Marks sync - point for this sync gen number */ - uint8_t sequenced :1; /* write sequence number is valid */ - uint8_t has_data :1; /* write_data field is valid (else ignore) */ - uint8_t discard :1; /* has_data will be 0 if this is a discard */ - uint8_t writesame :1; /* ws_datalen indicates length of data at write_bytes */ - }; - }; + uint8_t flags = 0; uint32_t ws_datalen = 0; /* Length of data buffer (writesame only) */ uint32_t entry_index = 0; /* For debug consistency check. Can be removed if * we need the space */ @@ -240,23 +239,74 @@ struct WriteLogCacheEntry { BlockExtent block_extent(); uint64_t get_offset_bytes(); uint64_t get_write_bytes(); - bool is_sync_point() { - return sync_point; + bool is_entry_valid() const { + return flags & WRITE_LOG_CACHE_ENTRY_VALID; + } + bool is_sync_point() const { + return flags & WRITE_LOG_CACHE_ENTRY_SYNC_POINT; + } + bool is_sequenced() const { + return flags & WRITE_LOG_CACHE_ENTRY_SEQUENCED; } - bool is_discard() { - return discard; + bool has_data() const { + return flags & WRITE_LOG_CACHE_ENTRY_HAS_DATA; } - bool is_writesame() { - return writesame; + bool is_discard() const { + return flags & WRITE_LOG_CACHE_ENTRY_DISCARD; } - bool is_write() { + bool is_writesame() const { + return flags & WRITE_LOG_CACHE_ENTRY_WRITESAME; + } + bool is_write() const { /* Log entry is a basic write */ return !is_sync_point() && !is_discard() && !is_writesame(); } - bool is_writer() { + bool is_writer() const { /* Log entry is any type that writes data */ return is_write() || is_discard() || is_writesame(); } + void set_entry_valid(bool flag) { + if (flag) { + flags |= WRITE_LOG_CACHE_ENTRY_VALID; + } else { + flags &= ~WRITE_LOG_CACHE_ENTRY_VALID; + } + } + void set_sync_point(bool flag) { + if (flag) { + flags |= WRITE_LOG_CACHE_ENTRY_SYNC_POINT; + } else { + flags &= ~WRITE_LOG_CACHE_ENTRY_SYNC_POINT; + } + } + void set_sequenced(bool flag) { + if (flag) { + flags |= WRITE_LOG_CACHE_ENTRY_SEQUENCED; + } else { + flags &= ~WRITE_LOG_CACHE_ENTRY_SEQUENCED; + } + } + void set_has_data(bool flag) { + if (flag) { + flags |= WRITE_LOG_CACHE_ENTRY_HAS_DATA; + } else { + flags &= ~WRITE_LOG_CACHE_ENTRY_HAS_DATA; + } + } + void set_discard(bool flag) { + if (flag) { + flags |= WRITE_LOG_CACHE_ENTRY_DISCARD; + } else { + flags &= ~WRITE_LOG_CACHE_ENTRY_DISCARD; + } + } + void set_writesame(bool flag) { + if (flag) { + flags |= WRITE_LOG_CACHE_ENTRY_WRITESAME; + } else { + flags &= ~WRITE_LOG_CACHE_ENTRY_WRITESAME; + } + } friend std::ostream& operator<<(std::ostream& os, const WriteLogCacheEntry &entry); #ifdef WITH_RBD_SSD_CACHE diff --git a/src/librbd/cache/pwl/rwl/WriteLog.cc b/src/librbd/cache/pwl/rwl/WriteLog.cc index eb096f9187679..41bdafe5b9e3d 100644 --- a/src/librbd/cache/pwl/rwl/WriteLog.cc +++ b/src/librbd/cache/pwl/rwl/WriteLog.cc @@ -113,7 +113,7 @@ void WriteLog::alloc_op_log_entries(GenericLogOperations &ops) 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.entry_valid = 1; + log_entry->ram_entry.set_entry_valid(true); m_log_entries.push_back(log_entry); ldout(m_image_ctx.cct, 20) << "operation=[" << *operation << "]" << dendl; } diff --git a/src/librbd/cache/pwl/ssd/WriteLog.cc b/src/librbd/cache/pwl/ssd/WriteLog.cc index 8bee4ee255f33..2c0dc258b86fb 100644 --- a/src/librbd/cache/pwl/ssd/WriteLog.cc +++ b/src/librbd/cache/pwl/ssd/WriteLog.cc @@ -535,7 +535,7 @@ void WriteLog::alloc_op_log_entries(GenericLogOperations &ops) { for (auto &operation : ops) { auto &log_entry = operation->get_log_entry(); - log_entry->ram_entry.entry_valid = 1; + log_entry->ram_entry.set_entry_valid(true); m_log_entries.push_back(log_entry); ldout(m_image_ctx.cct, 20) << "operation=[" << *operation << "]" << dendl; } -- 2.39.5