os/bluestore: Hybrid Allocator might unexpectedly returned ENOSPC

This happened when secondary allocator returned no additional
extents while primary one still provided a few but less than enough.
This is rather a non-critical issue but it violated our informal
agreement for allocators which can return less space than requested.

Signed-off-by: Igor Fedotov <igor.fedotov@croit.io>

diff --git a/src/os/bluestore/HybridAllocator.cc b/src/os/bluestore/HybridAllocator.cc
index 2201d5958246b0072043a2d755aa584a4cf6e39d..448fdc7cfe944e250dca3e51e4ad3009628e52f2 100644 (file)
--- a/src/os/bluestore/HybridAllocator.cc
+++ b/src/os/bluestore/HybridAllocator.cc
@@ -39,68 +39,52 @@ int64_t HybridAllocator::allocate(
     max_alloc_size = p2align(uint64_t(cap), (uint64_t)get_block_size());
   }
 
-  std::lock_guard l(lock);
-
   int64_t res;
-  PExtentVector local_extents;
 
-  // preserve original 'extents' vector state
-  auto orig_size = extents->size();
-  auto orig_pos = extents->end();
-  if (orig_size) {
-    --orig_pos;
-  }
+  typedef
+    std::function<int64_t(uint64_t, uint64_t, uint64_t, int64_t, PExtentVector*)>
+    alloc_fn;
+  alloc_fn priA = [&](uint64_t _want,
+                      uint64_t _unit,
+                      uint64_t _max_alloc_size,
+                      int64_t  _hint,
+                      PExtentVector* _extents) {
+    return _allocate(_want, _unit, _max_alloc_size, _hint, _extents);
+  };
+  alloc_fn secA = [&](uint64_t _want,
+                      uint64_t _unit,
+                      uint64_t _max_alloc_size,
+                      int64_t  _hint,
+                      PExtentVector* _extents) {
+    return bmap_alloc ?
+      bmap_alloc->allocate(_want, _unit, _max_alloc_size, _hint, _extents) :
+      0;
+  };
 
+  std::lock_guard l(lock);
   // try bitmap first to avoid unneeded contiguous extents split if
   // desired amount is less than shortes range in AVL
   if (bmap_alloc && bmap_alloc->get_free() &&
     want < _lowest_size_available()) {
-    res = bmap_alloc->allocate(want, unit, max_alloc_size, hint, extents);
-    if (res < 0) {
-      // got a failure, release already allocated and
-      // start over allocation from avl
-      if (orig_size) {
-        local_extents.insert(
-          local_extents.end(), ++orig_pos, extents->end());
-        extents->resize(orig_size);
-      } else {
-        extents->swap(local_extents);
-      }
-      bmap_alloc->release(local_extents);
-      res = 0;
-    }
-    if ((uint64_t)res < want) {
-      auto res2 = _allocate(want - res, unit, max_alloc_size, hint, extents);
-      if (res2 < 0) {
-        res = res2; // caller to do the release
-      } else {
-        res += res2;
-      }
-    }
-  } else {
-    res = _allocate(want, unit, max_alloc_size, hint, extents);
+    std::swap(priA, secA);
+  }
+
+  {
+    auto orig_size = extents->size();
+    res = priA(want, unit, max_alloc_size, hint, extents);
     if (res < 0) {
-      // got a failure, release already allocated and
-      // start over allocation from bitmap
-      if (orig_size) {
-        local_extents.insert(
-          local_extents.end(), ++orig_pos, extents->end());
-        extents->resize(orig_size);
-      } else {
-        extents->swap(local_extents);
-      }
-      _release(local_extents);
+      // allocator shouldn't return new extents on error
+      ceph_assert(orig_size == extents->size());
       res = 0;
     }
-    if ((uint64_t)res < want ) {
-      auto res2 = bmap_alloc ?
-        bmap_alloc->allocate(want - res, unit, max_alloc_size, hint, extents) :
-        0;
-      if (res2 < 0 ) {
-        res = res2; // caller to do the release
-      } else {
-        res += res2;
-      }
+  }
+  if ((uint64_t)res < want) {
+    auto orig_size = extents->size();
+    auto res2 = secA(want - res, unit, max_alloc_size, hint, extents);
+    if (res2 > 0) {
+      res += res2;
+    } else {
+      ceph_assert(orig_size == extents->size());
     }
   }
   return res ? res : -ENOSPC;