Merge pull request #48416 from Rethan/wip-bluestore-discard-thread

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

Reviewed-by: Igor Fedotov <ifedotov@suse.com>

src/blk/BlockDevice.h

@@ -287,8 +287,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

src/blk/kernel/KernelDevice.cc

@@ -260,7 +260,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);
@@ -307,7 +309,9 @@ void KernelDevice::close()
 {
   dout(1) << __func__ << dendl;
   _aio_stop();
-  _discard_stop();
+  if (discard_thread.is_started()) {
+    _discard_stop();
+  }
   _pre_close();
 
   extblkdev::release_device(ebd_impl);
@@ -505,10 +509,9 @@ void KernelDevice::_aio_stop()
   }
 }
 
-int KernelDevice::_discard_start()
+void KernelDevice::_discard_start()
 {
     discard_thread.create("bstore_discard");
-    return 0;
 }
 
 void KernelDevice::_discard_stop()
@@ -695,7 +698,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));
@@ -708,9 +711,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())
@@ -722,6 +726,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,
@@ -1021,7 +1042,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) {
@@ -1029,13 +1050,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;
 }
 

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);
@@ -144,7 +145,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;

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

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();

src/os/bluestore/BlueStore.cc

@@ -5579,7 +5579,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()) {
@@ -13051,24 +13053,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: