From: Igor Fedotov <igor.fedotov@croit.io>
Date: Fri, 4 Jul 2025 12:15:26 +0000 (+0300)
Subject: blk/kernel: improve DiscardThread life cycle.
X-Git-Tag: v20.2.1~38^2~36^2
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=26cc64f53aeec18bfe79e53e8d19507397f1e68e;p=ceph.git

blk/kernel: improve DiscardThread life cycle.

This will eliminate a potential race between thread startup and its
removal.

Relates-to: https://tracker.ceph.com/issues/71800
Signed-off-by: Igor Fedotov <igor.fedotov@croit.io>
(cherry picked from commit 69369b151b96ca74bffb9d72f4c249f48fde2845)
---

diff --git a/src/blk/kernel/KernelDevice.cc b/src/blk/kernel/KernelDevice.cc
index db06d3052863..009876ac0638 100644
--- a/src/blk/kernel/KernelDevice.cc
+++ b/src/blk/kernel/KernelDevice.cc
@@ -592,12 +592,12 @@ void KernelDevice::_discard_update_threads(bool discard_stop)
     for(uint64_t i = oldcount; i < newcount; i++)
     {
       // All threads created with the same name
-      discard_threads.emplace_back(new DiscardThread(this, i));
+      discard_threads.emplace_back(new DiscardThread(this));
       discard_threads.back()->create("bstore_discard");
     }
   // Decrease? Signal threads after telling them to stop
   } else if (newcount < oldcount) {
-    std::vector<std::shared_ptr<DiscardThread>> discard_threads_to_stop;
+    std::vector<DiscardThread*> discard_threads_to_stop;
     dout(10) << __func__ << " stopping " << (oldcount - newcount) << " existing discard threads" << dendl;
 
     // Signal the last threads to quit, and stop tracking them
@@ -608,8 +608,9 @@ void KernelDevice::_discard_update_threads(bool discard_stop)
     discard_cond.notify_all();
     discard_threads.resize(newcount);
     l.unlock();
-    for (auto &t : discard_threads_to_stop) {
+    for (auto t : discard_threads_to_stop) {
       t->join();
+      delete t;
     }
   }
   logger->set(l_blk_kernel_discard_threads, discard_threads.size());
@@ -779,9 +780,9 @@ void KernelDevice::swap_discard_queued(interval_set<uint64_t>& other)
   discard_queued.swap(other);
 }
 
-void KernelDevice::_discard_thread(uint64_t tid)
+void KernelDevice::_discard_thread(DiscardThread* thr)
 {
-  dout(10) << __func__ << " thread " << tid << " start" << dendl;
+  dout(10) << __func__ << " thread " << thr << " start" << dendl;
 
   // Thread-local list of processing discards
   interval_set<uint64_t> discard_processing;
@@ -789,10 +790,6 @@ void KernelDevice::_discard_thread(uint64_t tid)
   std::unique_lock l(discard_lock);
   discard_cond.notify_all();
 
-  // Keeps the shared pointer around until erased from the vector
-  // and until we leave this function
-  auto thr = discard_threads[tid];
-
   while (true) {
     ceph_assert(discard_processing.empty());
     if (discard_queued.empty()) {
@@ -843,7 +840,7 @@ void KernelDevice::_discard_thread(uint64_t tid)
     }
   }
 
-  dout(10) << __func__ << " thread " << tid << " finish" << dendl;
+  dout(10) << __func__ << " thread " << thr << " finish" << dendl;
 }
 
 // this is private and is expected that the caller checks that discard
diff --git a/src/blk/kernel/KernelDevice.h b/src/blk/kernel/KernelDevice.h
index 25aa2710ce3f..f10458b4aff7 100644
--- a/src/blk/kernel/KernelDevice.h
+++ b/src/blk/kernel/KernelDevice.h
@@ -78,15 +78,15 @@ private:
 
   struct DiscardThread : public Thread {
     KernelDevice *bdev;
-    const uint64_t id;
     bool stop = false;
-    explicit DiscardThread(KernelDevice *b, uint64_t id) : bdev(b), id(id) {}
+    explicit DiscardThread(KernelDevice *b) : bdev(b) {
+    }
     void *entry() override {
-      bdev->_discard_thread(id);
+      bdev->_discard_thread(this);
       return NULL;
     }
   };
-  std::vector<std::shared_ptr<DiscardThread>> discard_threads;
+  std::vector<DiscardThread*> discard_threads;
 
   std::atomic_int injecting_crash;
 
@@ -94,7 +94,7 @@ private:
   virtual void  _pre_close() { }  // hook for child implementations
 
   void _aio_thread();
-  void _discard_thread(uint64_t tid);
+  void _discard_thread(DiscardThread* thr);
   bool _queue_discard(interval_set<uint64_t> &to_release);
   bool try_discard(interval_set<uint64_t> &to_release, bool async = true) override;