From 9c65adeb23fbb03b31bb0455b601af2c517baad5 Mon Sep 17 00:00:00 2001
From: Yite Gu <yitegu0@gmail.com>
Date: Wed, 28 Aug 2024 17:48:40 +0800
Subject: [PATCH] blk/KernelDevice: using join() to wait thread end is more
 safe

Using join() to wait discard thread end is more safe, it can
ensure that resource releases are sequential, to avoid potential
race conditions.

Signed-off-by: Yite Gu <yitegu0@gmail.com>
---
 src/blk/kernel/KernelDevice.cc | 22 ++++++++++------------
 src/blk/kernel/KernelDevice.h  |  3 +--
 2 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/src/blk/kernel/KernelDevice.cc b/src/blk/kernel/KernelDevice.cc
index 1985c85435ee5..72921e6d9f08b 100644
--- a/src/blk/kernel/KernelDevice.cc
+++ b/src/blk/kernel/KernelDevice.cc
@@ -65,7 +65,6 @@ KernelDevice::KernelDevice(CephContext* cct, aio_callback_t cb, void *cbpriv, ai
     discard_callback(d_cb),
     discard_callback_priv(d_cbpriv),
     aio_stop(false),
-    discard_stop(false),
     aio_thread(this),
     injecting_crash(0)
 {
@@ -548,7 +547,7 @@ void KernelDevice::_aio_stop()
   }
 }
 
-void KernelDevice::_discard_update_threads()
+void KernelDevice::_discard_update_threads(bool discard_stop)
 {
   std::unique_lock l(discard_lock);
 
@@ -570,28 +569,27 @@ void KernelDevice::_discard_update_threads()
     }
   // Decrease? Signal threads after telling them to stop
   } else if (newcount < oldcount) {
+    std::vector<std::shared_ptr<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
-    for(uint64_t i = oldcount; i > newcount; i--)
-    {
+    for(uint64_t i = oldcount; i > newcount; i--) {
       discard_threads[i-1]->stop = true;
-      discard_threads[i-1]->detach();
+      discard_threads_to_stop.push_back(discard_threads[i-1]);
     }
-    discard_threads.resize(newcount);
-
     discard_cond.notify_all();
+    discard_threads.resize(newcount);
+    l.unlock();
+    for (auto &t : discard_threads_to_stop) {
+      t->join();
+    }
   }
 }
 
 void KernelDevice::_discard_stop()
 {
   dout(10) << __func__ << dendl;
-
-  discard_stop = true;
-  _discard_update_threads();
-  discard_drain();
-
+  _discard_update_threads(true);
   dout(10) << __func__ << " stopped" << dendl;
 }
 
diff --git a/src/blk/kernel/KernelDevice.h b/src/blk/kernel/KernelDevice.h
index 42e542a6cc8da..ac555cdd3daf3 100644
--- a/src/blk/kernel/KernelDevice.h
+++ b/src/blk/kernel/KernelDevice.h
@@ -58,7 +58,6 @@ private:
   aio_callback_t discard_callback;
   void *discard_callback_priv;
   bool aio_stop;
-  bool discard_stop;
   std::unique_ptr<PerfCounters> logger;
 
   ceph::mutex discard_lock = ceph::make_mutex("KernelDevice::discard_lock");
@@ -100,7 +99,7 @@ private:
   int _aio_start();
   void _aio_stop();
 
-  void _discard_update_threads();
+  void _discard_update_threads(bool discard_stop = false);
   void _discard_stop();
   bool _discard_started();
 
-- 
2.39.5