os/bluestore: Fixes improper length calculation in BufferSpace::read + adds simplifie...

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

diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc
index 38abe49794722e569ebdbc478cd319c78ae37cc4..4bb3e9d6dce12662ef08a1f763d238803a859c70 100644 (file)
--- a/src/os/bluestore/BlueStore.cc
+++ b/src/os/bluestore/BlueStore.cc
@@ -2897,7 +2897,7 @@ int BlueStore::_do_read(
        continue;
       }
       if (rr0_it!=rr0_end && (rr0_it->first < rr_it->first + r_len)) {
-       r_len = rr_it->first - rr0_it->first;
+       r_len = rr0_it->first - rr_it->first;
       }
 
       if (off > rr_it->first && r_len) {

diff --git a/src/os/bluestore/BlueStore.h b/src/os/bluestore/BlueStore.h
index 7a89d4f9608a93a39b30420114b757a04d8a88da..4bdabe0a80cee0cf64a0187b0cc77bfe63b9ea73 100644 (file)
--- a/src/os/bluestore/BlueStore.h
+++ b/src/os/bluestore/BlueStore.h
@@ -275,32 +275,39 @@ public:
       uint64_t end = offset + length;
       while (i != buffer_map.end() && offset < end && i->first < end) {
        Buffer *b = i->second.get();
-
-       if (b->offset < offset) {
-         uint64_t head = offset - b->offset;
-         uint64_t l = MIN(length, b->length - head);
-         if (b->is_writing()) {//?? should we use in is_cleaning state too?
-           res[offset].substr_of(b->data, head, l);
-           res_intervals.insert( offset, l);
-         }
-         offset += l;
-         length -= l;
-       } else if (b->offset > offset) {
-         uint64_t head = b->offset - offset;
-         uint64_t l = MIN(length - head, b->length);
-         if (b->is_writing()) {
+        if(b->is_writing()) {
+         if (b->offset < offset) {
+           uint64_t head = offset - b->offset;
+           uint64_t l = b->length > head ? b->length - head : 0;
+           l = MIN(length, b->length - head);
+           if(l>0){
+             res[offset].substr_of(b->data, head, l);
+             res_intervals.insert( offset, l);
+             offset += l;
+             length -= l;
+           }
+         } else if (b->offset > offset) {
+           uint64_t head = b->offset - offset;
+           uint64_t l = length > head ? length - head : 0;
+           l = MIN(l, b->length);
+           if(l>0){
+             res[b->offset].substr_of(b->data, 0, l);
+             res_intervals.insert(b->offset, l);
+           }
+           offset += l + head;
+           length -= l + head;
+         } else if(b->length != length) {
+           uint64_t l = MIN(length, b->length);
            res[b->offset].substr_of(b->data, 0, l);
            res_intervals.insert(b->offset, l);
-         }
-         offset += l + head;
-         length -= l + head;
-       } else {
-         if (b->is_writing()) {
+           offset += l;
+           length -= l;
+         } else {
            res[b->offset].append(b->data);
            res_intervals.insert(b->offset, b->length);
+           offset += b->length;
+           length -= b->length;
          }
-         offset += b->length;
-         length -= b->length;
        }
        ++i;
       }

diff --git a/src/test/objectstore/store_test.cc b/src/test/objectstore/store_test.cc
index 8d9a1a5bb8123a10240423031bc74059785ddf0b..97657252eea0a7759a65826f315f707544e61caa 100644 (file)
--- a/src/test/objectstore/store_test.cc
+++ b/src/test/objectstore/store_test.cc
@@ -603,31 +603,61 @@ TEST_P(StoreTest, BufferCacheReadTest) {
       ASSERT_TRUE(newdata.contents_equal(expected));
     }
   }
+  //additional write to an unused region of some blob
+  {
+    ObjectStore::Transaction t;
+    bufferlist bl2, newdata;
+    bl2.append("1234567890");
+
+    t.write(cid, hoid, 20, bl2.length(), bl2);
+    cerr << "Append" << 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, 30);
+    {
+      bufferlist expected;
+      expected.append("edcba");
+      expected.append_zero(5);
+      expected.append("edcba");
+      expected.append_zero(5);
+      expected.append(bl2);
+
+      if(!newdata.contents_equal(expected)){
+        newdata.hexdump(cerr);
+        cerr<<std::endl;
+      }
+      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");
+    bl3.append("BA");
 
-    t.write(cid, hoid, 20, bl2.length(), bl2);
+    t.write(cid, hoid, 40, 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);
+    r = store->read(cid, hoid, 0, 40, newdata);
+    ASSERT_EQ(r, 40);
+//newdata.hexdump(cerr);
+//cerr<<std::endl;
     {
       bufferlist expected;
       expected.append("eDCBa");
       expected.append_zero(5);
-      expected.append("edCBa");
+      expected.append("edcBA");
       expected.append_zero(5);
       expected.append(bl2);
+      expected.append(bl2);
 
       ASSERT_TRUE(newdata.contents_equal(expected));
     }