blk/KernelDevice: don't start discard thread if device not support_discard

Only create discard thread if the device support discard,
otherwise we will have some threads which does nothing.
Also extract queue_discard/discard logic to device, make it cleaner when
calling discard from bluefs and bluestore.

Signed-off-by: haoyixing <haoyixing@kuaishou.com>

diff --git a/src/blk/BlockDevice.h b/src/blk/BlockDevice.h
index 67b4b330b6c45fd970e36f6a85f60e304db47d85..7e87f3a128c7bc3792ebdfe572d1cd84f03409fc 100644 (file)
--- a/src/blk/BlockDevice.h
+++ b/src/blk/BlockDevice.h
@@ -286,8 +286,7 @@ public:
     bool buffered,
     int write_hint = WRITE_LIFE_NOT_SET) = 0;
   virtual int flush() = 0;
-  virtual int discard(uint64_t offset, uint64_t len) { return 0; }
-  virtual int queue_discard(interval_set<uint64_t> &to_release) { return -1; }
+  virtual bool try_discard(interval_set<uint64_t> &to_release, bool async=true) { return false; }
   virtual void discard_drain() { return; }
 
   // for managing buffered readers/writers

diff --git a/src/blk/kernel/KernelDevice.cc b/src/blk/kernel/KernelDevice.cc
index 687aff06c6d66cef2e91794895c3d5f238c682de..3ed42cb528c73986e8aa1d58f441956cff40b694 100644 (file)
--- a/src/blk/kernel/KernelDevice.cc
+++ b/src/blk/kernel/KernelDevice.cc
@@ -255,7 +255,9 @@ int KernelDevice::open(const string& p)
   if (r < 0) {
     goto out_fail;
   }
-  _discard_start();
+  if (support_discard && cct->_conf->bdev_enable_discard && cct->_conf->bdev_async_discard) {
+    _discard_start();
+  }
 
   // round size down to an even block
   size &= ~(block_size - 1);
@@ -302,7 +304,9 @@ void KernelDevice::close()
 {
   dout(1) << __func__ << dendl;
   _aio_stop();
-  _discard_stop();
+  if (discard_thread.is_started()) {
+    _discard_stop();
+  }
   _pre_close();
 
   if (vdo_fd >= 0) {
@@ -514,10 +518,9 @@ void KernelDevice::_aio_stop()
   }
 }
 
-int KernelDevice::_discard_start()
+void KernelDevice::_discard_start()
 {
     discard_thread.create("bstore_discard");
-    return 0;
 }
 
 void KernelDevice::_discard_stop()
@@ -704,7 +707,7 @@ void KernelDevice::_discard_thread()
       l.unlock();
       dout(20) << __func__ << " finishing" << dendl;
       for (auto p = discard_finishing.begin();p != discard_finishing.end(); ++p) {
-       discard(p.get_start(), p.get_len());
+       _discard(p.get_start(), p.get_len());
       }
 
       discard_callback(discard_callback_priv, static_cast<void*>(&discard_finishing));
@@ -717,9 +720,10 @@ void KernelDevice::_discard_thread()
   discard_started = false;
 }
 
-int KernelDevice::queue_discard(interval_set<uint64_t> &to_release)
+int KernelDevice::_queue_discard(interval_set<uint64_t> &to_release)
 {
-  if (!support_discard)
+  // if bdev_async_discard enabled on the fly, discard_thread is not started here, fallback to sync discard
+  if (!discard_thread.is_started())
     return -1;
 
   if (to_release.empty())
@@ -731,6 +735,23 @@ int KernelDevice::queue_discard(interval_set<uint64_t> &to_release)
   return 0;
 }
 
+// return true only if _queue_discard succeeded, so caller won't have to do alloc->release
+// otherwise false
+bool KernelDevice::try_discard(interval_set<uint64_t> &to_release, bool async)
+{
+  if (!support_discard || !cct->_conf->bdev_enable_discard)
+    return false;
+
+  if (async && discard_thread.is_started()) {
+    return 0 == _queue_discard(to_release);
+  } else {
+    for (auto p = to_release.begin(); p != to_release.end(); ++p) {
+      _discard(p.get_start(), p.get_len());
+    }
+  }
+  return false;
+}
+
 void KernelDevice::_aio_log_start(
   IOContext *ioc,
   uint64_t offset,
@@ -1030,7 +1051,7 @@ int KernelDevice::aio_write(
   return 0;
 }
 
-int KernelDevice::discard(uint64_t offset, uint64_t len)
+int KernelDevice::_discard(uint64_t offset, uint64_t len)
 {
   int r = 0;
   if (cct->_conf->objectstore_blackhole) {
@@ -1038,13 +1059,10 @@ int KernelDevice::discard(uint64_t offset, uint64_t len)
               << dendl;
     return 0;
   }
-  if (support_discard) {
-      dout(10) << __func__
-              << " 0x" << std::hex << offset << "~" << len << std::dec
-              << dendl;
-
-      r = BlkDev{fd_directs[WRITE_LIFE_NOT_SET]}.discard((int64_t)offset, (int64_t)len);
-  }
+  dout(10) << __func__
+          << " 0x" << std::hex << offset << "~" << len << std::dec
+          << dendl;
+  r = BlkDev{fd_directs[WRITE_LIFE_NOT_SET]}.discard((int64_t)offset, (int64_t)len);
   return r;
 }
 

diff --git a/src/blk/kernel/KernelDevice.h b/src/blk/kernel/KernelDevice.h
index 613a9bbfed9e4229d5a866a547d1b93e3aa03635..b5960c70899086ee1f1559aab31a1a0024cc0682 100644 (file)
--- a/src/blk/kernel/KernelDevice.h
+++ b/src/blk/kernel/KernelDevice.h
@@ -84,12 +84,13 @@ private:
 
   void _aio_thread();
   void _discard_thread();
-  int queue_discard(interval_set<uint64_t> &to_release) override;
+  int _queue_discard(interval_set<uint64_t> &to_release);
+  bool try_discard(interval_set<uint64_t> &to_release, bool async = true) override;
 
   int _aio_start();
   void _aio_stop();
 
-  int _discard_start();
+  void _discard_start();
   void _discard_stop();
 
   void _aio_log_start(IOContext *ioc, uint64_t offset, uint64_t length);
@@ -145,7 +146,7 @@ public:
                bool buffered,
                int write_hint = WRITE_LIFE_NOT_SET) override;
   int flush() override;
-  int discard(uint64_t offset, uint64_t len) override;
+  int _discard(uint64_t offset, uint64_t len);
 
   // for managing buffered readers/writers
   int invalidate_cache(uint64_t off, uint64_t len) override;

diff --git a/src/blk/zoned/HMSMRDevice.h b/src/blk/zoned/HMSMRDevice.h
index 326f9ab27f4ba15214d70db0c4a824275761ca56..edf18b5f0ba35ad1be76052e534d469a5c7335c4 100644 (file)
--- a/src/blk/zoned/HMSMRDevice.h
+++ b/src/blk/zoned/HMSMRDevice.h
@@ -47,11 +47,6 @@ public:
   void reset_zone(uint64_t zone) override;
   std::vector<uint64_t> get_zones() override;
 
-  int discard(uint64_t offset, uint64_t len) override {
-    // discard is a no-op on a zoned device
-    return 0;
-  }
-
 };
 
 #endif //CEPH_BLK_HMSMRDEVICE_H

diff --git a/src/os/bluestore/BlueFS.cc b/src/os/bluestore/BlueFS.cc
index e72f24bbc7048a60a4a2474a8ff3b73603c41444..fc5285d135bbe64bdcbb81ba4e06f3759f1a4445 100644 (file)
--- a/src/os/bluestore/BlueFS.cc
+++ b/src/os/bluestore/BlueFS.cc
@@ -420,7 +420,9 @@ int BlueFS::add_block_device(unsigned id, const string& path, bool trim,
     return r;
   }
   if (trim) {
-    b->discard(0, b->get_size());
+    interval_set<uint64_t> whole_device;
+    whole_device.insert(0, b->get_size());
+    b->try_discard(whole_device, false);
   }
 
   dout(1) << __func__ << " bdev " << id << " path " << path
@@ -2912,18 +2914,13 @@ void BlueFS::_clear_dirty_set_stable_D(uint64_t seq)
 void BlueFS::_release_pending_allocations(vector<interval_set<uint64_t>>& to_release)
 {
   for (unsigned i = 0; i < to_release.size(); ++i) {
-    if (!to_release[i].empty()) {
-      /* OK, now we have the guarantee alloc[i] won't be null. */
-      int r = 0;
-      if (cct->_conf->bdev_enable_discard && cct->_conf->bdev_async_discard) {
-       r = bdev[i]->queue_discard(to_release[i]);
-       if (r == 0)
-         continue;
-      } else if (cct->_conf->bdev_enable_discard) {
-       for (auto p = to_release[i].begin(); p != to_release[i].end(); ++p) {
-         bdev[i]->discard(p.get_start(), p.get_len());
-       }
-      }
+    if (to_release[i].empty()) {
+        continue;
+    }
+    /* OK, now we have the guarantee alloc[i] won't be null. */
+
+    bool discard_queued = bdev[i]->try_discard(to_release[i]);
+    if (!discard_queued) {
       alloc[i]->release(to_release[i]);
       if (is_shared_alloc(i)) {
         shared_alloc->bluefs_used -= to_release[i].size();

diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc
index 4fab7de0378fcfbd5dc8ded6433f551e763d75f6..4499a6d1b20dfdfe76cac1e01c28e2320a85922a 100644 (file)
--- a/src/os/bluestore/BlueStore.cc
+++ b/src/os/bluestore/BlueStore.cc
@@ -5520,7 +5520,9 @@ int BlueStore::_open_bdev(bool create)
     goto fail;
 
   if (create && cct->_conf->bdev_enable_discard) {
-    bdev->discard(0, bdev->get_size());
+    interval_set<uint64_t> whole_device;
+    whole_device.insert(0, bdev->get_size());
+    bdev->try_discard(whole_device, false);
   }
 
   if (bdev->supported_bdev_label()) {
@@ -12951,24 +12953,18 @@ void BlueStore::_txc_finish(TransContext *txc)
 
 void BlueStore::_txc_release_alloc(TransContext *txc)
 {
+  bool discard_queued = false;
   // it's expected we're called with lazy_release_lock already taken!
-  if (likely(!cct->_conf->bluestore_debug_no_reuse_blocks)) {
-    int r = 0;
-    if (cct->_conf->bdev_enable_discard && cct->_conf->bdev_async_discard) {
-      r = bdev->queue_discard(txc->released);
-      if (r == 0) {
-       dout(10) << __func__ << "(queued) " << txc << " " << std::hex
-                << txc->released << std::dec << dendl;
-       goto out;
-      }
-    } else if (cct->_conf->bdev_enable_discard) {
-      for (auto p = txc->released.begin(); p != txc->released.end(); ++p) {
-         bdev->discard(p.get_start(), p.get_len());
-      }
-    }
-    dout(10) << __func__ << "(sync) " << txc << " " << std::hex
-             << txc->released << std::dec << dendl;
-    alloc->release(txc->released);
+  if (unlikely(cct->_conf->bluestore_debug_no_reuse_blocks)) {
+      goto out;
+  }
+  discard_queued = bdev->try_discard(txc->released);
+  // if async discard succeeded, will do alloc->release when discard callback
+  // else we should release here
+  if (!discard_queued) {
+      dout(10) << __func__ << "(sync) " << txc << " " << std::hex
+               << txc->released << std::dec << dendl;
+      alloc->release(txc->released);
   }
 
 out: