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 <congmin.yin@intel.com>
(cherry picked from commit 64f741ecebf3ff9a31d99f9f3d121ffeff54173b)

Conflicts:
	src/librbd/cache/pwl/Types.cc [ commit e96cdcb65851
	  ("librbd/cache/pwl: merge multiple output "<<" calls") not
	  in pacific ]

diff --git a/src/librbd/cache/pwl/LogEntry.cc b/src/librbd/cache/pwl/LogEntry.cc
index 63873f748d9080ba91a51741bbcbcdf2b5a7ecbc..504d21051d9e891c9ec65f61071f1991874b096b 100644 (file)
--- a/src/librbd/cache/pwl/LogEntry.cc
+++ b/src/librbd/cache/pwl/LogEntry.cc
@@ -47,7 +47,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)));
 }
@@ -74,7 +74,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. */
@@ -82,10 +82,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 {
@@ -120,7 +120,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 78eb4a6de6acb14c3ed119f9772104e12571d547..ecaca0b7b70d6157fcf3fcaddcda1f88cd14272e 100644 (file)
--- a/src/librbd/cache/pwl/LogEntry.h
+++ b/src/librbd/cache/pwl/LogEntry.h
@@ -93,7 +93,7 @@ public:
   std::shared_ptr<SyncPointLogEntry> 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 67745d7de376adfa178aadc30f6f7344e6b11adb..505f5d57ba248d2a434377ed8b61fafa5ea1dd79 100644 (file)
--- 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(list<WriteLogCacheEntry*>& ls)
   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(list<WriteLogPoolRoot*>& 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 15a2fe80b67c4a4bd3ed4a70120f9716c9b74339..dc067612bfcff3d992631fe763f9a009324fb0f9 100644 (file)
--- 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 f25094750a5540c4eb6f6fb4614b1ef420ec8db5..04d605e7be585202d56faa87e800530f98b03983 100644 (file)
--- a/src/librbd/cache/pwl/rwl/WriteLog.cc
+++ b/src/librbd/cache/pwl/rwl/WriteLog.cc
@@ -112,7 +112,7 @@ void WriteLog<I>::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 493f9019136d1027da9989136fa015a0bd1a8b9d..245c0932afa99af4899f7f64b779ed4beb986610 100644 (file)
--- a/src/librbd/cache/pwl/ssd/WriteLog.cc
+++ b/src/librbd/cache/pwl/ssd/WriteLog.cc
@@ -533,7 +533,7 @@ void WriteLog<I>::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;
   }