From 7b72f5a74d6b83206450126bd19ce0e15ee30c11 Mon Sep 17 00:00:00 2001
From: Igor Fedotov <ifedotov@mirantis.com>
Date: Thu, 19 May 2016 17:08:41 +0300
Subject: [PATCH] os/bluestore: Fixes improper length calculation in
 BufferSpace::read + adds simplified test case to highlight an issue for
 append to existing blob

Signed-off-by: Igor Fedotov <ifedotov@mirantis.com>
---
 src/os/bluestore/BlueStore.cc      |  2 +-
 src/os/bluestore/BlueStore.h       | 49 +++++++++++++++++-------------
 src/test/objectstore/store_test.cc | 40 +++++++++++++++++++++---
 3 files changed, 64 insertions(+), 27 deletions(-)

diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc
index 38abe49794722..4bb3e9d6dce12 100644
--- 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 7a89d4f9608a9..4bdabe0a80cee 100644
--- 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 8d9a1a5bb8123..97657252eea0a 100644
--- 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));
     }
-- 
2.39.5