librbd/cache/pwl/ssd: solve competition between read and retire

SSD read is not like rwl's. SSD need aio read. Therefore,
we cannot guarantee that the data will not be retire
during the period from sending the read request to the SSD
and receiving the data to the memory, which may cause
the corresponding data on the SSD to be overwritten.

Fixes: https://tracker.ceph.com/issues/52236
Signed-off-by: Feng Hualong <hualong.feng@intel.com>

diff --git a/src/librbd/cache/pwl/AbstractWriteLog.cc b/src/librbd/cache/pwl/AbstractWriteLog.cc
index 20a03e6b350f69d612c8a0b56339cf058e52605f..b7341edd08f89197e2b6e834eb88660d8530fb0b 100644 (file)
--- a/src/librbd/cache/pwl/AbstractWriteLog.cc
+++ b/src/librbd/cache/pwl/AbstractWriteLog.cc
@@ -691,7 +691,7 @@ void AbstractWriteLog<I>::read(Extents&& image_extents,
   bl->clear();
   m_perfcounter->inc(l_librbd_pwl_rd_req, 1);
 
-  std::vector<WriteLogCacheEntry*> log_entries_to_read;
+  std::vector<std::shared_ptr<GenericWriteLogEntry>> log_entries_to_read;
   std::vector<bufferlist*> bls_to_read;
 
   m_async_op_tracker.start_op();

diff --git a/src/librbd/cache/pwl/AbstractWriteLog.h b/src/librbd/cache/pwl/AbstractWriteLog.h
index 2ca831f1f99cfc323a7ffcc17c5edc8ae5d7ee32..a0a20cd694579ce1a1a91118caf2943b5accb335 100644 (file)
--- a/src/librbd/cache/pwl/AbstractWriteLog.h
+++ b/src/librbd/cache/pwl/AbstractWriteLog.h
@@ -366,11 +366,11 @@ protected:
                                pwl::DeferredContexts &later) = 0;
   virtual void collect_read_extents(
       uint64_t read_buffer_offset, LogMapEntry<GenericWriteLogEntry> map_entry,
-      std::vector<WriteLogCacheEntry*> &log_entries_to_read,
+      std::vector<std::shared_ptr<GenericWriteLogEntry>> &log_entries_to_read,
       std::vector<bufferlist*> &bls_to_read, uint64_t entry_hit_length,
       Extent hit_extent, pwl::C_ReadRequest *read_ctx) = 0;
   virtual void complete_read(
-      std::vector<WriteLogCacheEntry*> &log_entries_to_read,
+      std::vector<std::shared_ptr<GenericWriteLogEntry>> &log_entries_to_read,
       std::vector<bufferlist*> &bls_to_read, Context *ctx) = 0;
   virtual void write_data_to_buffer(
       std::shared_ptr<pwl::WriteLogEntry> ws_entry,

diff --git a/src/librbd/cache/pwl/LogEntry.cc b/src/librbd/cache/pwl/LogEntry.cc
index 98224241b557e384e48da8e63ef4c507fec8786c..63873f748d9080ba91a51741bbcbcdf2b5a7ecbc 100644 (file)
--- a/src/librbd/cache/pwl/LogEntry.cc
+++ b/src/librbd/cache/pwl/LogEntry.cc
@@ -88,14 +88,6 @@ void WriteLogEntry::init(bool has_data,
   ram_entry.discard = 0;
 }
 
-unsigned int WriteLogEntry::reader_count() const {
-  if (cache_bp.have_raw()) {
-    return (cache_bp.raw_nref() - bl_refs - 1);
-  } else {
-    return 0;
-  }
-}
-
 std::ostream& WriteLogEntry::format(std::ostream &os) const {
   os << "(Write) ";
   GenericWriteLogEntry::format(os);

diff --git a/src/librbd/cache/pwl/LogEntry.h b/src/librbd/cache/pwl/LogEntry.h
index b29d7fb88bcb3439fd2da04f53d90ef19c3055b7..b0d27b5aefacc7f506cf08cbe43145d4dd79db37 100644 (file)
--- a/src/librbd/cache/pwl/LogEntry.h
+++ b/src/librbd/cache/pwl/LogEntry.h
@@ -217,7 +217,7 @@ public:
   virtual buffer::list &get_cache_bl() = 0;
 
   BlockExtent block_extent();
-  unsigned int reader_count() const;
+  virtual unsigned int reader_count() const = 0;
   /* Constructs a new bl containing copies of cache_bp */
   void copy_cache_bl(bufferlist *out_bl) override {};
   bool can_retire() const override {

diff --git a/src/librbd/cache/pwl/rwl/LogEntry.cc b/src/librbd/cache/pwl/rwl/LogEntry.cc
index 7325bef78a95892eb8839aad5668769133a8a1cc..056701fb514880d1902034eb72a30dc54d432e1e 100644 (file)
--- a/src/librbd/cache/pwl/rwl/LogEntry.cc
+++ b/src/librbd/cache/pwl/rwl/LogEntry.cc
@@ -80,6 +80,14 @@ void WriteLogEntry::copy_cache_bl(bufferlist *out_bl) {
   this->init_bl(cloned_bp, *out_bl);
 }
 
+unsigned int WriteLogEntry::reader_count() const {
+  if (cache_bp.have_raw()) {
+    return (cache_bp.raw_nref() - bl_refs - 1);
+  } else {
+    return 0;
+  }
+}
+
 void WriteSameLogEntry::writeback(
     librbd::cache::ImageWritebackInterface &image_writeback, Context *ctx) {
   bufferlist entry_bl;

diff --git a/src/librbd/cache/pwl/rwl/LogEntry.h b/src/librbd/cache/pwl/rwl/LogEntry.h
index 0eacb5aeec9ab6ce21411bcccc4985375e2bb791..a4675c5fbd824983357c6f153605206bf8c040ab 100644 (file)
--- a/src/librbd/cache/pwl/rwl/LogEntry.h
+++ b/src/librbd/cache/pwl/rwl/LogEntry.h
@@ -39,6 +39,7 @@ public:
       std::vector<WriteBufferAllocation>::iterator allocation) override;
   buffer::list &get_cache_bl() override;
   void copy_cache_bl(bufferlist *out_bl) override;
+  unsigned int reader_count() const override;
 };
 
 class WriteSameLogEntry : public WriteLogEntry {

diff --git a/src/librbd/cache/pwl/rwl/WriteLog.cc b/src/librbd/cache/pwl/rwl/WriteLog.cc
index ef0735ba65b903dbd8139852a31a7b79a514491e..d4bbe08b01e695f552f5948f069fbe81cd8c4b76 100644 (file)
--- a/src/librbd/cache/pwl/rwl/WriteLog.cc
+++ b/src/librbd/cache/pwl/rwl/WriteLog.cc
@@ -60,7 +60,7 @@ WriteLog<I>::~WriteLog() {
 template <typename I>
 void WriteLog<I>::collect_read_extents(
       uint64_t read_buffer_offset, LogMapEntry<GenericWriteLogEntry> map_entry,
-      std::vector<WriteLogCacheEntry*> &log_entries_to_read,
+      std::vector<std::shared_ptr<GenericWriteLogEntry>> &log_entries_to_read,
       std::vector<bufferlist*> &bls_to_read, uint64_t entry_hit_length,
       Extent hit_extent, pwl::C_ReadRequest *read_ctx) {
   /* Make a bl for this hit extent. This will add references to the
@@ -82,7 +82,7 @@ void WriteLog<I>::collect_read_extents(
 
 template <typename I>
 void WriteLog<I>::complete_read(
-    std::vector<WriteLogCacheEntry*> &log_entries_to_read,
+    std::vector<std::shared_ptr<GenericWriteLogEntry>> &log_entries_to_read,
     std::vector<bufferlist*> &bls_to_read, Context *ctx) {
   ctx->complete(0);
 }

diff --git a/src/librbd/cache/pwl/rwl/WriteLog.h b/src/librbd/cache/pwl/rwl/WriteLog.h
index 92989ac21a5d63cdb5248858a1636fed530caac9..a29047e8e375d7364ff6556475ea9686feb3f24f 100644 (file)
--- a/src/librbd/cache/pwl/rwl/WriteLog.h
+++ b/src/librbd/cache/pwl/rwl/WriteLog.h
@@ -88,11 +88,11 @@ protected:
                      bool &alloc_succeeds, bool &no_space) override;
   void collect_read_extents(
       uint64_t read_buffer_offset, LogMapEntry<GenericWriteLogEntry> map_entry,
-      std::vector<WriteLogCacheEntry*> &log_entries_to_read,
+      std::vector<std::shared_ptr<GenericWriteLogEntry>> &log_entries_to_read,
       std::vector<bufferlist*> &bls_to_read, uint64_t entry_hit_length,
       Extent hit_extent, pwl::C_ReadRequest *read_ctx) override;
   void complete_read(
-      std::vector<WriteLogCacheEntry*> &log_entries_to_read,
+      std::vector<std::shared_ptr<GenericWriteLogEntry>> &log_entries_to_read,
       std::vector<bufferlist*> &bls_to_read, Context *ctx) override;
   bool retire_entries(const unsigned long int frees_per_tx) override;
   void persist_last_flushed_sync_gen() override;

diff --git a/src/librbd/cache/pwl/ssd/LogEntry.h b/src/librbd/cache/pwl/ssd/LogEntry.h
index 666398440a22155ffbe57dfc68d2a54ef0452095..86121473b82edb07225a9f71c87880bc8db4c3f9 100644 (file)
--- a/src/librbd/cache/pwl/ssd/LogEntry.h
+++ b/src/librbd/cache/pwl/ssd/LogEntry.h
@@ -40,6 +40,11 @@ public:
   buffer::list &get_cache_bl() override;
   void remove_cache_bl() override;
   unsigned int get_aligned_data_size() const override;
+  void inc_bl_refs() { bl_refs++; };
+  void dec_bl_refs() { bl_refs--; };
+  unsigned int reader_count() const override {
+    return bl_refs;
+  }
 };
 
 class WriteSameLogEntry : public WriteLogEntry {

diff --git a/src/librbd/cache/pwl/ssd/WriteLog.cc b/src/librbd/cache/pwl/ssd/WriteLog.cc
index 69837d07310e15e4e4bf17a08319f364dcf6a584..774dae0a8edf0e786e719310a7721b08e8996700 100644 (file)
--- a/src/librbd/cache/pwl/ssd/WriteLog.cc
+++ b/src/librbd/cache/pwl/ssd/WriteLog.cc
@@ -69,7 +69,7 @@ WriteLog<I>::~WriteLog() {
 template <typename I>
 void WriteLog<I>::collect_read_extents(
     uint64_t read_buffer_offset, LogMapEntry<GenericWriteLogEntry> map_entry,
-    std::vector<WriteLogCacheEntry*> &log_entries_to_read,
+    std::vector<std::shared_ptr<GenericWriteLogEntry>> &log_entries_to_read,
     std::vector<bufferlist*> &bls_to_read,
     uint64_t entry_hit_length, Extent hit_extent,
     pwl::C_ReadRequest *read_ctx) {
@@ -87,14 +87,15 @@ void WriteLog<I>::collect_read_extents(
   if (!hit_bl.length()) {
     ldout(m_image_ctx.cct, 5) << "didn't hit RAM" << dendl;
     auto read_extent = read_ctx->read_extents.back();
-    log_entries_to_read.push_back(&write_entry->ram_entry);
+    write_entry->inc_bl_refs();
+    log_entries_to_read.push_back(std::move(write_entry));
     bls_to_read.push_back(&read_extent->m_bl);
   }
 }
 
 template <typename I>
 void WriteLog<I>::complete_read(
-    std::vector<WriteLogCacheEntry*> &log_entries_to_read,
+    std::vector<std::shared_ptr<GenericWriteLogEntry>> &log_entries_to_read,
     std::vector<bufferlist*> &bls_to_read,
     Context *ctx) {
   if (!log_entries_to_read.empty()) {
@@ -510,7 +511,9 @@ Context* WriteLog<I>::construct_flush_entry_ctx(
       });
     ctx = new LambdaContext(
       [this, log_entry, read_bl_ptr, ctx](int r) {
-        aio_read_data_block(&log_entry->ram_entry, read_bl_ptr, ctx);
+        auto write_entry = static_pointer_cast<WriteLogEntry>(log_entry);
+        write_entry->inc_bl_refs();
+        aio_read_data_block(std::move(write_entry), read_bl_ptr, ctx);
       });
     return ctx;
   } else {
@@ -957,29 +960,32 @@ int WriteLog<I>::update_pool_root_sync(
 }
 
 template <typename I>
-void WriteLog<I>::aio_read_data_block(WriteLogCacheEntry *log_entry,
+void WriteLog<I>::aio_read_data_block(std::shared_ptr<GenericWriteLogEntry> log_entry,
                                       bufferlist *bl, Context *ctx) {
-  std::vector<WriteLogCacheEntry*> log_entries {log_entry};
+  std::vector<std::shared_ptr<GenericWriteLogEntry>> log_entries = {std::move(log_entry)};
   std::vector<bufferlist *> bls {bl};
   aio_read_data_blocks(log_entries, bls, ctx);
 }
 
 template <typename I>
 void WriteLog<I>::aio_read_data_blocks(
-    std::vector<WriteLogCacheEntry*> &log_entries,
+    std::vector<std::shared_ptr<GenericWriteLogEntry>> &log_entries,
     std::vector<bufferlist *> &bls, Context *ctx) {
   ceph_assert(log_entries.size() == bls.size());
 
   //get the valid part
   Context *read_ctx = new LambdaContext(
-    [this, log_entries, bls, ctx](int r) {
+    [log_entries, bls, ctx](int r) {
       for (unsigned int i = 0; i < log_entries.size(); i++) {
         bufferlist valid_data_bl;
-        auto length = log_entries[i]->is_write() ? log_entries[i]->write_bytes :
-                                                   log_entries[i]->ws_datalen;
+        auto write_entry = static_pointer_cast<WriteLogEntry>(log_entries[i]);
+        auto length = write_entry->ram_entry.is_write() ? write_entry->ram_entry.write_bytes
+                                                        : write_entry->ram_entry.ws_datalen;
+
         valid_data_bl.substr_of(*bls[i], 0, length);
         bls[i]->clear();
         bls[i]->append(valid_data_bl);
+        write_entry->dec_bl_refs();
       }
       ctx->complete(r);
     });
@@ -987,7 +993,7 @@ void WriteLog<I>::aio_read_data_blocks(
   CephContext *cct = m_image_ctx.cct;
   AioTransContext *aio = new AioTransContext(cct, read_ctx);
   for (unsigned int i = 0; i < log_entries.size(); i++) {
-    auto log_entry = log_entries[i];
+    WriteLogCacheEntry *log_entry = &log_entries[i]->ram_entry;
 
     ceph_assert(log_entry->is_write() || log_entry->is_writesame());
     uint64_t len = log_entry->is_write() ? log_entry->write_bytes :

diff --git a/src/librbd/cache/pwl/ssd/WriteLog.h b/src/librbd/cache/pwl/ssd/WriteLog.h
index c4b4c178af606e379ca7d5634e13c7439cb9f2ad..f4750fd2c3747a63a5fdbf3852b32661d2075f8b 100644 (file)
--- a/src/librbd/cache/pwl/ssd/WriteLog.h
+++ b/src/librbd/cache/pwl/ssd/WriteLog.h
@@ -108,11 +108,11 @@ private:
   void load_existing_entries(pwl::DeferredContexts &later);
   void collect_read_extents(
       uint64_t read_buffer_offset, LogMapEntry<GenericWriteLogEntry> map_entry,
-      std::vector<WriteLogCacheEntry*> &log_entries_to_read,
+      std::vector<std::shared_ptr<GenericWriteLogEntry>> &log_entries_to_read,
       std::vector<bufferlist*> &bls_to_read, uint64_t entry_hit_length,
       Extent hit_extent, pwl::C_ReadRequest *read_ctx) override;
   void complete_read(
-      std::vector<WriteLogCacheEntry*> &log_entries_to_read,
+      std::vector<std::shared_ptr<GenericWriteLogEntry>> &log_entries_to_read,
       std::vector<bufferlist*> &bls_to_read, Context *ctx) override;
   void enlist_op_appender();
   bool retire_entries(const unsigned long int frees_per_tx);
@@ -132,9 +132,9 @@ private:
   int update_pool_root_sync(std::shared_ptr<pwl::WriteLogPoolRoot> root);
   void update_pool_root(std::shared_ptr<WriteLogPoolRoot> root,
                                           AioTransContext *aio);
-  void aio_read_data_block(WriteLogCacheEntry *log_entry, bufferlist *bl,
-                           Context *ctx);
-  void aio_read_data_blocks(std::vector<WriteLogCacheEntry*> &log_entries,
+  void aio_read_data_block(std::shared_ptr<GenericWriteLogEntry> log_entry,
+                           bufferlist *bl, Context *ctx);
+  void aio_read_data_blocks(std::vector<std::shared_ptr<GenericWriteLogEntry>> &log_entries,
                             std::vector<bufferlist *> &bls, Context *ctx);
   static void aio_cache_cb(void *priv, void *priv2) {
     AioTransContext *c = static_cast<AioTransContext*>(priv2);