ObjectCacher: fix off-by-one error in split

This error left a completion that should have been attached
to the right BufferHead on the left BufferHead, which would
result in the completion never being called unless the buffers
were merged before it's original read completed. This would cause
a hang in any higher level waiting for a read to complete.

The existing loop went backwards (using a forward iterator),
but stopped when the iterator reached the beginning of the map,
or when a waiter belonged to the left BufferHead.

If the first list of waiters should have been moved to the right
BufferHead, it was skipped because at that point the iterator
was at the beginning of the map, which was the main condition
of the loop.

Restructure the waiters-moving loop to go forward in the map instead,
so it's harder to make an off-by-one error.

Possibly-fixes: #3286
Signed-off-by: Josh Durgin <josh.durgin@inktank.com>

diff --git a/src/osdc/ObjectCacher.cc b/src/osdc/ObjectCacher.cc
index 95a62ec2b10a163a853d6ee398e4a1aebd0c0530..0e8e04490c6a1776f9879ff6d6f8811e8acf8ac2 100644 (file)
--- a/src/osdc/ObjectCacher.cc
+++ b/src/osdc/ObjectCacher.cc
@@ -53,21 +53,22 @@ ObjectCacher::BufferHead *ObjectCacher::Object::split(BufferHead *left, loff_t o
     right->bl.substr_of(bl, left->length(), right->length());
     left->bl.substr_of(bl, 0, left->length());
   }
-  
+
   // move read waiters
   if (!left->waitfor_read.empty()) {
-    map<loff_t, list<Context*> >::iterator o, p = left->waitfor_read.end();
-    p--;
-    while (p != left->waitfor_read.begin()) {
-      if (p->first < right->start()) break;      
+    map<loff_t, list<Context*> >::iterator start_remove = left->waitfor_read.begin();
+    while (start_remove != left->waitfor_read.end() &&
+          start_remove->first < right->start())
+      ++start_remove;
+    for (map<loff_t, list<Context*> >::iterator p = start_remove;
+        p != left->waitfor_read.end(); ++p) {
       ldout(oc->cct, 0) << "split  moving waiters at byte " << p->first << " to right bh" << dendl;
       right->waitfor_read[p->first].swap( p->second );
-      o = p;
-      p--;
-      left->waitfor_read.erase(o);
+      assert(p->second.empty());
     }
+    left->waitfor_read.erase(start_remove, left->waitfor_read.end());
   }
-  
+
   ldout(oc->cct, 20) << "split    left is " << *left << dendl;
   ldout(oc->cct, 20) << "split   right is " << *right << dendl;
   return right;