From 0c69672fce55362a7e6eb8ed6955f163afe85482 Mon Sep 17 00:00:00 2001
From: Igor Fedotov <ifedotov@suse.com>
Date: Thu, 10 May 2018 18:54:36 +0300
Subject: [PATCH] os/bluestore: fix improper access to a
 BitmapFastAllocator::last_pos from multiple threads.

Signed-off-by: Igor Fedotov <ifedotov@suse.com>
---
 src/os/bluestore/BitmapFastAllocator.cc       |  8 ++-----
 src/os/bluestore/BitmapFastAllocator.h        |  1 -
 src/os/bluestore/fastbmap_allocator_impl.h    | 24 ++++++++++++-------
 .../objectstore/fastbmap_allocator_test.cc    |  4 ++--
 4 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/src/os/bluestore/BitmapFastAllocator.cc b/src/os/bluestore/BitmapFastAllocator.cc
index 9139e817e65c0..2733ccd060d43 100755
--- a/src/os/bluestore/BitmapFastAllocator.cc
+++ b/src/os/bluestore/BitmapFastAllocator.cc
@@ -24,11 +24,7 @@ int64_t BitmapFastAllocator::allocate(
 {
   uint64_t allocated = 0;
 
-  if (hint != 0) {
-    last_pos = hint;
-  }
-
-  _allocate_l2(want_size, alloc_unit, max_alloc_size, &last_pos,
+  _allocate_l2(want_size, alloc_unit, max_alloc_size, hint,
     &allocated, extents);
   if (!allocated) {
     return -ENOSPC;
@@ -69,5 +65,5 @@ void BitmapFastAllocator::init_rm_free(uint64_t offset, uint64_t length)
 void BitmapFastAllocator::shutdown()
 {
   ldout(cct, 1) << __func__ << dendl;
-  last_pos = 0;
+  _shutdown();
 }
diff --git a/src/os/bluestore/BitmapFastAllocator.h b/src/os/bluestore/BitmapFastAllocator.h
index ac58f2a5a6368..9807780b50c8d 100755
--- a/src/os/bluestore/BitmapFastAllocator.h
+++ b/src/os/bluestore/BitmapFastAllocator.h
@@ -15,7 +15,6 @@
 class BitmapFastAllocator : public Allocator,
   public AllocatorLevel02<AllocatorLevel01Loose> {
   CephContext* cct;
-  uint64_t last_pos = 0;
 
 public:
   BitmapFastAllocator(CephContext* _cct, int64_t capacity, int64_t alloc_unit);
diff --git a/src/os/bluestore/fastbmap_allocator_impl.h b/src/os/bluestore/fastbmap_allocator_impl.h
index d8f7b1d8b76bd..fe8e94e33b744 100755
--- a/src/os/bluestore/fastbmap_allocator_impl.h
+++ b/src/os/bluestore/fastbmap_allocator_impl.h
@@ -544,6 +544,7 @@ protected:
   slot_vector_t l2;
   uint64_t l2_granularity = 0; // space per entry
   uint64_t available = 0;
+  uint64_t last_pos = 0;
 
   enum {
     CHILD_PER_SLOT = bits_per_slot, // 64
@@ -640,28 +641,30 @@ protected:
   void _allocate_l2(uint64_t length,
     uint64_t min_length,
     uint64_t max_length,
-    uint64_t* pos_start,
+    uint64_t hint,
+    
     uint64_t* allocated,
     interval_vector_t* res)
   {
     uint64_t prev_allocated = *allocated;
     uint64_t d = CHILD_PER_SLOT;
-    assert((*pos_start % d) == 0);
     assert(min_length <= l2_granularity);
     assert(max_length == 0 || max_length >= min_length);
     assert(max_length == 0 || (max_length % min_length) == 0);
 
     uint64_t l1_w = slotset_width * l1._children_per_slot();
 
-    auto last_pos = *pos_start;
-
-    auto l2_pos = *pos_start;
     std::lock_guard<std::mutex> l(lock);
 
     if (available < min_length) {
       return;
     }
-    auto pos = *pos_start / d;
+    if (hint != 0) {
+      last_pos = (hint / d) < l2.size() ? p2align(hint, d) : 0;
+    }
+    auto l2_pos = last_pos;
+    auto last_pos0 = last_pos;
+    auto pos = last_pos / d;
     auto pos_end = l2.size();
     // outer loop below is intended to optimize the performance by
     // avoiding 'modulo' operations inside the internal loop.
@@ -687,7 +690,7 @@ protected:
 	  bool empty = l1._allocate_l1(length,
 	    min_length,
 	    max_length,
-	    (l2_pos + free_pos)* l1_w,
+	    (l2_pos + free_pos) * l1_w,
 	    (l2_pos + free_pos + 1) * l1_w,
 	    allocated,
 	    res);
@@ -707,14 +710,13 @@ protected:
       }
       l2_pos = 0;
       pos = 0;
-      pos_end = *pos_start;
+      pos_end = last_pos0 / d;
     }
 
     ++l2_allocs;
     auto allocated_here = *allocated - prev_allocated;
     assert(available >= allocated_here);
     available -= allocated_here;
-    *pos_start = last_pos;
   }
 
 #ifndef NON_CEPH_BUILD
@@ -770,6 +772,10 @@ protected:
     available += l1._free_l1(o, len);
     _mark_l2_free(l2_pos, l2_pos_end);
   }
+  void _shutdown()
+  {
+    last_pos = 0;
+  }
 };
 
 #endif
diff --git a/src/test/objectstore/fastbmap_allocator_test.cc b/src/test/objectstore/fastbmap_allocator_test.cc
index acddb3f3723a5..64db85f2044eb 100755
--- a/src/test/objectstore/fastbmap_allocator_test.cc
+++ b/src/test/objectstore/fastbmap_allocator_test.cc
@@ -36,8 +36,8 @@ public:
     interval_vector_t* res)
   {
     uint64_t allocated = 0;
-    uint64_t last_pos = 0; // do hint support so far
-    _allocate_l2(length, min_length, 0, &last_pos, &allocated, res);
+    uint64_t hint = 0; // trigger internal l2 hint support
+    _allocate_l2(length, min_length, 0, hint, &allocated, res);
     *allocated0 += allocated;
   }
   void free_l2(const interval_vector_t& r)
-- 
2.39.5