From: haoyixing Date: Mon, 10 Oct 2022 06:58:41 +0000 (+0800) Subject: blk/KernelDevice: don't start discard thread if device not support_discard X-Git-Tag: v18.1.0~639^2 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=755f3e03b5bf547cfd940b52a53833f66285062b;p=ceph.git 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 --- diff --git a/src/blk/BlockDevice.h b/src/blk/BlockDevice.h index 67b4b330b6c4..7e87f3a128c7 100644 --- 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 &to_release) { return -1; } + virtual bool try_discard(interval_set &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 687aff06c6d6..3ed42cb528c7 100644 --- 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(&discard_finishing)); @@ -717,9 +720,10 @@ void KernelDevice::_discard_thread() discard_started = false; } -int KernelDevice::queue_discard(interval_set &to_release) +int KernelDevice::_queue_discard(interval_set &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 &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 &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 613a9bbfed9e..b5960c708990 100644 --- 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 &to_release) override; + int _queue_discard(interval_set &to_release); + bool try_discard(interval_set &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 326f9ab27f4b..edf18b5f0ba3 100644 --- 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 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 e72f24bbc704..fc5285d135bb 100644 --- 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 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>& 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 4fab7de0378f..4499a6d1b20d 100644 --- 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 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: