os/bluestore: Added asserts for allocator regions

Functions release/init_add_free/init_rm_free did not check its input against device size.
It is incorrect and had been a problem when you shrink device.

Signed-off-by: Adam Kupczyk <akupczyk@redhat.com>

diff --git a/src/os/bluestore/Allocator.cc b/src/os/bluestore/Allocator.cc
index 91a001b5c9749ca07256d16457db2e03dddf1a7f..75f3172ca55bc6d474265a87b0d8e49c10f3b7ca 100644 (file)
--- a/src/os/bluestore/Allocator.cc
+++ b/src/os/bluestore/Allocator.cc
@@ -109,7 +109,7 @@ public:
 Allocator::Allocator(const std::string& name,
                      int64_t _capacity,
                      int64_t _block_size)
-  : capacity(_capacity), block_size(_block_size)
+  : device_size(_capacity), block_size(_block_size)
 {
   asok_hook = new SocketHook(this, name);
 }
@@ -129,7 +129,7 @@ Allocator *Allocator::create(CephContext* cct, string type,
 {
   Allocator* alloc = nullptr;
   if (type == "stupid") {
-    alloc = new StupidAllocator(cct, name, size, block_size);
+    alloc = new StupidAllocator(cct, size, block_size, name);
   } else if (type == "bitmap") {
     alloc = new BitmapAllocator(cct, size, block_size, name);
   } else if (type == "avl") {

diff --git a/src/os/bluestore/Allocator.h b/src/os/bluestore/Allocator.h
index 177438025d73cbc8da4130e33aeac69e2b408d48..db778a988838cc5b1df24c236f4b9c0afb8aa516 100644 (file)
--- a/src/os/bluestore/Allocator.h
+++ b/src/os/bluestore/Allocator.h
@@ -79,7 +79,7 @@ public:
   const string& get_name() const;
   int64_t get_capacity() const
   {
-    return capacity;
+    return device_size;
   }
   int64_t get_block_size() const
   {
@@ -89,9 +89,9 @@ public:
 private:
   class SocketHook;
   SocketHook* asok_hook = nullptr;
-
-  int64_t capacity = 0;
-  int64_t block_size = 0;
+protected:
+  const int64_t device_size = 0;
+  const int64_t block_size = 0;
 };
 
 #endif

diff --git a/src/os/bluestore/AvlAllocator.cc b/src/os/bluestore/AvlAllocator.cc
index 8c43b37dc26827399b5fd4a35631a9369c2a3eb9..f6a11b4f9bebe88fc70e0205c6ff1a686eb34ce6 100644 (file)
--- a/src/os/bluestore/AvlAllocator.cc
+++ b/src/os/bluestore/AvlAllocator.cc
@@ -268,6 +268,7 @@ void AvlAllocator::_release(const interval_set<uint64_t>& release_set)
   for (auto p = release_set.begin(); p != release_set.end(); ++p) {
     const auto offset = p.get_start();
     const auto length = p.get_len();
+    ceph_assert(offset + length <= uint64_t(num_total));
     ldout(cct, 10) << __func__ << std::hex
       << " offset 0x" << offset
       << " length 0x" << length
@@ -406,6 +407,7 @@ void AvlAllocator::dump(std::function<void(uint64_t offset, uint64_t length)> no
 void AvlAllocator::init_add_free(uint64_t offset, uint64_t length)
 {
   std::lock_guard l(lock);
+  ceph_assert(offset + length <= uint64_t(num_total));
   ldout(cct, 10) << __func__ << std::hex
                  << " offset 0x" << offset
                  << " length 0x" << length
@@ -416,6 +418,7 @@ void AvlAllocator::init_add_free(uint64_t offset, uint64_t length)
 void AvlAllocator::init_rm_free(uint64_t offset, uint64_t length)
 {
   std::lock_guard l(lock);
+  ceph_assert(offset + length <= uint64_t(num_total));
   ldout(cct, 10) << __func__ << std::hex
                  << " offset 0x" << offset
                  << " length 0x" << length

diff --git a/src/os/bluestore/BitmapAllocator.cc b/src/os/bluestore/BitmapAllocator.cc
index 75f909c071e60750061c790b98ecdcc9ac094cc3..3571ddd219279e7c2b57d6b0809a49533effadd2 100644 (file)
--- a/src/os/bluestore/BitmapAllocator.cc
+++ b/src/os/bluestore/BitmapAllocator.cc
@@ -52,9 +52,10 @@ void BitmapAllocator::release(
   const interval_set<uint64_t>& release_set)
 {
   if (cct->_conf->subsys.should_gather<dout_subsys, 10>()) {
-    for (auto r : release_set) {
-      ldout(cct, 10) << __func__ << " 0x" << std::hex << r.first << "~" << r.second
-                   << std::dec << dendl;
+    for (auto& [offset, len] : release_set) {
+      ldout(cct, 10) << __func__ << " 0x" << std::hex << offset << "~" << len
+                     << std::dec << dendl;
+      ceph_assert(offset + len <= (uint64_t)device_size);
     }
   }
   _free_l2(release_set);
@@ -70,6 +71,7 @@ void BitmapAllocator::init_add_free(uint64_t offset, uint64_t length)
   auto mas = get_min_alloc_size();
   uint64_t offs = round_up_to(offset, mas);
   uint64_t l = p2align(offset + length - offs, mas);
+  ceph_assert(offs + l <= (uint64_t)device_size);
 
   _mark_free(offs, l);
   ldout(cct, 10) << __func__ << " done" << dendl;
@@ -81,6 +83,7 @@ void BitmapAllocator::init_rm_free(uint64_t offset, uint64_t length)
   auto mas = get_min_alloc_size();
   uint64_t offs = round_up_to(offset, mas);
   uint64_t l = p2align(offset + length - offs, mas);
+  ceph_assert(offs + l <= (uint64_t)device_size);
   _mark_allocated(offs, l);
   ldout(cct, 10) << __func__ << " done" << dendl;
 }

diff --git a/src/os/bluestore/BitmapAllocator.h b/src/os/bluestore/BitmapAllocator.h
index 5c768a4ac2fb1ede2d42c8175aa343df365b0268..ec61e210823ec7ef560c38f8ad67610afe84bb0f 100644 (file)
--- a/src/os/bluestore/BitmapAllocator.h
+++ b/src/os/bluestore/BitmapAllocator.h
@@ -15,7 +15,6 @@
 class BitmapAllocator : public Allocator,
   public AllocatorLevel02<AllocatorLevel01Loose> {
   CephContext* cct;
-
 public:
   BitmapAllocator(CephContext* _cct, int64_t capacity, int64_t alloc_unit, const std::string& name);
   ~BitmapAllocator() override

diff --git a/src/os/bluestore/StupidAllocator.cc b/src/os/bluestore/StupidAllocator.cc
index 533f279d7801d65a47080650fd19999da16c9231..51d444170c4ce992177bfe24198ea114cabb17b3 100644 (file)
--- a/src/os/bluestore/StupidAllocator.cc
+++ b/src/os/bluestore/StupidAllocator.cc
@@ -11,14 +11,15 @@
 #define dout_prefix *_dout << "stupidalloc 0x" << this << " "
 
 StupidAllocator::StupidAllocator(CephContext* cct,
-                                 const std::string& name,
-                                 int64_t _size,
-                                 int64_t _block_size)
-  : Allocator(name, _size, _block_size), cct(cct), num_free(0),
+                                 int64_t capacity,
+                                 int64_t _block_size,
+                                 const std::string& name)
+  : Allocator(name, capacity, _block_size),
+    cct(cct), num_free(0),
     free(10)
 {
   ceph_assert(cct != nullptr);
-  bdev_block_size = cct->_conf->bdev_block_size;
+  ceph_assert(block_size > 0);
 }
 
 StupidAllocator::~StupidAllocator()
@@ -27,8 +28,7 @@ StupidAllocator::~StupidAllocator()
 
 unsigned StupidAllocator::_choose_bin(uint64_t orig_len)
 {
-  ceph_assert(bdev_block_size > 0);
-  uint64_t len = orig_len / bdev_block_size;
+  uint64_t len = orig_len / block_size;
   int bin = std::min((int)cbits(len), (int)free.size() - 1);
   ldout(cct, 30) << __func__ << " len 0x" << std::hex << orig_len
                 << std::dec << " -> " << bin << dendl;

diff --git a/src/os/bluestore/StupidAllocator.h b/src/os/bluestore/StupidAllocator.h
index bd99a7c835fcddd9576f03d952a5047db36942e8..9b18e4bb674a16d9bf3fa0c4ac12a95bff803d33 100644 (file)
--- a/src/os/bluestore/StupidAllocator.h
+++ b/src/os/bluestore/StupidAllocator.h
@@ -18,7 +18,6 @@ class StupidAllocator : public Allocator {
   ceph::mutex lock = ceph::make_mutex("StupidAllocator::lock");
 
   int64_t num_free;     ///< total bytes in freelist
-  uint64_t bdev_block_size;
 
   template <typename K, typename V> using allocator_t =
     mempool::bluestore_alloc::pool_allocator<std::pair<const K, V>>;
@@ -38,9 +37,9 @@ class StupidAllocator : public Allocator {
 
 public:
   StupidAllocator(CephContext* cct,
-                  const std::string& name,
                   int64_t size,
-                  int64_t block_size);
+                  int64_t block_size,
+                 const std::string& name);
   ~StupidAllocator() override;
   const char* get_type() const override
   {