os/bluestore: Fixes some issues when using Buffer Cache from _do_read and improves...

Signed-off-by: Igor Fedotov <ifedotov@mirantis.com>

diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc
index 41b804d69063e7bf5e08a33b8ac0aa615436f17f..c9f2047cb5faddd2bd483dad72dfdbacbe883916 100644 (file)
--- a/src/os/bluestore/BlueStore.cc
+++ b/src/os/bluestore/BlueStore.cc
@@ -2804,6 +2804,7 @@ int BlueStore::_do_read(
   ready_regions_t ready_regions_in_cache, ready_regions;
   interval_set<uint64_t> ready_intervals_in_cache;
   o->bc.read(off, length, ready_regions_in_cache, ready_intervals_in_cache);
+  dout(20) << __func__ << " regions in cache " << ready_regions_in_cache.size() << " " << ready_intervals_in_cache << dendl;
 
   //build blob list to read
   blobs2read_t blobs2read;
@@ -2887,12 +2888,13 @@ int BlueStore::_do_read(
   auto rr0_end = ready_regions_in_cache.end();
 
   off = offset;
-  while (rr_it != rr_end || rr0_it != rr0_end) {
+  while ((rr_it != rr_end || rr0_it != rr0_end) && off < offset + length) {
     ready_regions_t::iterator it;
     if (rr_it != rr_end && (rr0_it == rr0_end || rr_it->first < rr0_it->first)) {
-
       uint64_t r_off = 0;
       uint64_t r_len = rr_it->second.length();
+      dout(30) << __func__ << " read region " << rr_it->first << "~" << rr_it->second.length() <<
+        " off " << off << " " << dendl;
       if (off > rr_it->first + r_len) {
        ++rr_it;
        continue;
@@ -2921,17 +2923,22 @@ int BlueStore::_do_read(
        bl.claim_append(tmp);
       }
       off = rr_it->first + r_off + r_len;
+      dout(30) << __func__ << " used read region " << rr_it->first + r_off <<
+        "~" << r_len << " new off " << off << dendl;
       ++rr_it;
     } else if(rr0_it != rr0_end) {
       if (off < rr0_it->first)
        bl.append_zero(rr0_it->first + off);
-      bl.claim_append(rr0_it->second);
+      dout(30) << __func__ << " used from cached buffer " <<
+        rr0_it->first << "~" << rr0_it->second.length() << dendl;
       off = rr0_it->first + rr0_it->second.length();
+      bl.claim_append(rr0_it->second);
       ++rr0_it;
     }
   }
   assert(offset + length >= off);
   bl.append_zero(offset + length - off);
+  assert(bl.length() == length);
 
   r = bl.length();
   return r;

diff --git a/src/os/bluestore/BlueStore.h b/src/os/bluestore/BlueStore.h
index 56e3eb971068cd4681a301c84af1a77f7ec40d2e..425ba97e3145937a8eff96a9e71bcfa59f14f435 100644 (file)
--- a/src/os/bluestore/BlueStore.h
+++ b/src/os/bluestore/BlueStore.h
@@ -270,7 +270,7 @@ public:
       }
     }
 
-    void read(uint64_t offset, uint64_t length, BlueStore::ready_regions_t& res, interval_set<uint64_t> res_intervals) {
+    void read(uint64_t offset, uint64_t length, BlueStore::ready_regions_t& res, interval_set<uint64_t>& res_intervals) {
       res.clear();
       auto i = _data_lower_bound(offset);
       uint64_t end = offset + length;

diff --git a/src/test/objectstore/store_test.cc b/src/test/objectstore/store_test.cc
index 855c8ac3b0cdf2629816c9dd7f611968650845b1..8d9a1a5bb8123a10240423031bc74059785ddf0b 100644 (file)
--- a/src/test/objectstore/store_test.cc
+++ b/src/test/objectstore/store_test.cc
@@ -572,16 +572,27 @@ TEST_P(StoreTest, BufferCacheReadTest) {
     r = apply_transaction(store, &osr, std::move(t));
     ASSERT_EQ(r, 0);
 
-    newdata.clear();
-    r = store->read(cid, hoid, 0, 5, newdata);
-    ASSERT_EQ(r, 5);
-    ASSERT_TRUE(newdata.contents_equal(bl));
+    r = store->read(cid, hoid, 0, 15, newdata);
+    ASSERT_EQ(r, 15);
+    {
+      bufferlist expected;
+      expected.append(bl);
+      expected.append_zero(5);
+      expected.append(bl);
+      ASSERT_TRUE(newdata.contents_equal(expected));
+    }
+  }
+  //overwrite over the same extents
+  {
+    ObjectStore::Transaction t;
+    bufferlist bl, newdata;
+    bl.append("edbca");
+    t.write(cid, hoid, 0, 5, bl);
+    t.write(cid, hoid, 10, 5, bl);
+    cerr << "TwinWrite" << std::endl;
+    r = apply_transaction(store, &osr, std::move(t));
+    ASSERT_EQ(r, 0);
 
-    newdata.clear();
-    r = store->read(cid, hoid, 10, 15, newdata);
-    ASSERT_EQ(r, 5);
-    ASSERT_TRUE(newdata.contents_equal(bl));
-    newdata.clear();
     r = store->read(cid, hoid, 0, 15, newdata);
     ASSERT_EQ(r, 15);
     {
@@ -592,6 +603,35 @@ TEST_P(StoreTest, BufferCacheReadTest) {
       ASSERT_TRUE(newdata.contents_equal(expected));
     }
   }
+
+  //additional write to an unused region of some blob and partial owerite over existing extents
+  {
+    ObjectStore::Transaction t;
+    bufferlist bl, bl2, bl3, newdata;
+    bl.append("DCB");
+    bl2.append("1234567890");
+    bl3.append("CB");
+
+    t.write(cid, hoid, 20, bl2.length(), bl2);
+    t.write(cid, hoid, 1, bl.length(), bl);
+    t.write(cid, hoid, 13, bl3.length(), bl3);
+    cerr << "TripleWrite" << std::endl;
+    r = apply_transaction(store, &osr, std::move(t));
+    ASSERT_EQ(r, 0);
+
+    r = store->read(cid, hoid, 0, 30, newdata);
+    ASSERT_EQ(r, 15);
+    {
+      bufferlist expected;
+      expected.append("eDCBa");
+      expected.append_zero(5);
+      expected.append("edCBa");
+      expected.append_zero(5);
+      expected.append(bl2);
+
+      ASSERT_TRUE(newdata.contents_equal(expected));
+    }
+  }
 }
 
 TEST_P(StoreTest, SimpleObjectTest) {