From 378b810cbaadb1a12a7f0d21ed9a33e2a9640f55 Mon Sep 17 00:00:00 2001
From: Jason Dillaman <dillaman@redhat.com>
Date: Wed, 7 Dec 2016 16:28:22 -0500
Subject: [PATCH] librbd: update in-memory object map after on-disk update
 committed

Concurrent IO to the same object would previously result in the first
IO pausing to update the object map while the other IO would proceed
to directly update the object before the object map state was properly
updated.

Fixes: http://tracker.ceph.com/issues/16176
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
---
 src/librbd/object_map/Request.cc              |  5 +---
 src/librbd/object_map/ResizeRequest.cc        |  4 ++-
 src/librbd/object_map/UpdateRequest.cc        | 29 ++++++++++---------
 .../object_map/test_mock_UpdateRequest.cc     |  6 +++-
 4 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/src/librbd/object_map/Request.cc b/src/librbd/object_map/Request.cc
index 1725cbf9075..e54aed0b21e 100644
--- a/src/librbd/object_map/Request.cc
+++ b/src/librbd/object_map/Request.cc
@@ -28,10 +28,7 @@ bool Request::should_complete(int r) {
       return invalidate();
     }
 
-    {
-      RWLock::WLocker l2(m_image_ctx.object_map_lock);
-      finish_request();
-    }
+    finish_request();
     return true;
 
   case STATE_INVALIDATE:
diff --git a/src/librbd/object_map/ResizeRequest.cc b/src/librbd/object_map/ResizeRequest.cc
index e574a184729..5698fc80f7b 100644
--- a/src/librbd/object_map/ResizeRequest.cc
+++ b/src/librbd/object_map/ResizeRequest.cc
@@ -48,10 +48,12 @@ void ResizeRequest::send() {
 }
 
 void ResizeRequest::finish_request() {
-  CephContext *cct = m_image_ctx.cct;
+  RWLock::WLocker object_map_locker(m_image_ctx.object_map_lock);
 
+  CephContext *cct = m_image_ctx.cct;
   ldout(cct, 5) << this << " resizing in-memory object map: "
 		<< m_num_objs << dendl;
+
   resize(m_object_map, m_num_objs, m_default_object_state);
 }
 
diff --git a/src/librbd/object_map/UpdateRequest.cc b/src/librbd/object_map/UpdateRequest.cc
index 51dbc48827f..3edf32aa67f 100644
--- a/src/librbd/object_map/UpdateRequest.cc
+++ b/src/librbd/object_map/UpdateRequest.cc
@@ -33,20 +33,6 @@ void UpdateRequest::send() {
 		 << "->" << static_cast<uint32_t>(m_new_state)
 		 << dendl;
 
-  // rebuilding the object map might update on-disk only
-  if (m_snap_id == m_image_ctx.snap_id) {
-    assert(m_image_ctx.object_map_lock.is_wlocked());
-    for (uint64_t object_no = m_start_object_no;
-         object_no < MIN(m_end_object_no, m_object_map.size());
-         ++object_no) {
-      uint8_t state = m_object_map[object_no];
-      if (!m_current_state || state == *m_current_state ||
-          (*m_current_state == OBJECT_EXISTS && state == OBJECT_EXISTS_CLEAN)) {
-        m_object_map[object_no] = m_new_state;
-      }
-    }
-  }
-
   librados::ObjectWriteOperation op;
   if (m_snap_id == CEPH_NOSNAP) {
     rados::cls::lock::assert_locked(&op, RBD_LOCK_NAME, LOCK_EXCLUSIVE, "", "");
@@ -61,8 +47,23 @@ void UpdateRequest::send() {
 }
 
 void UpdateRequest::finish_request() {
+  RWLock::RLocker snap_locker(m_image_ctx.snap_lock);
+  RWLock::WLocker object_map_locker(m_image_ctx.object_map_lock);
   ldout(m_image_ctx.cct, 20) << this << " on-disk object map updated"
                              << dendl;
+
+  // rebuilding the object map might update on-disk only
+  if (m_snap_id == m_image_ctx.snap_id) {
+    for (uint64_t object_no = m_start_object_no;
+         object_no < MIN(m_end_object_no, m_object_map.size());
+         ++object_no) {
+      uint8_t state = m_object_map[object_no];
+      if (!m_current_state || state == *m_current_state ||
+          (*m_current_state == OBJECT_EXISTS && state == OBJECT_EXISTS_CLEAN)) {
+        m_object_map[object_no] = m_new_state;
+      }
+    }
+  }
 }
 
 } // namespace object_map
diff --git a/src/test/librbd/object_map/test_mock_UpdateRequest.cc b/src/test/librbd/object_map/test_mock_UpdateRequest.cc
index 45879f2f83e..ee18c189064 100644
--- a/src/test/librbd/object_map/test_mock_UpdateRequest.cc
+++ b/src/test/librbd/object_map/test_mock_UpdateRequest.cc
@@ -8,6 +8,7 @@
 #include "librbd/ImageState.h"
 #include "librbd/internal.h"
 #include "librbd/ObjectMap.h"
+#include "librbd/Operations.h"
 #include "librbd/object_map/UpdateRequest.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
@@ -56,10 +57,13 @@ TEST_F(TestMockObjectMapUpdateRequest, UpdateInMemory) {
 
   librbd::ImageCtx *ictx;
   ASSERT_EQ(0, open_image(m_image_name, &ictx));
+
+  librbd::NoOpProgressContext no_progress;
+  ASSERT_EQ(0, ictx->operations->resize(4 << ictx->order, true, no_progress));
   ASSERT_EQ(0, acquire_exclusive_lock(*ictx));
 
   ceph::BitVector<2> object_map;
-  object_map.resize(1024);
+  object_map.resize(4);
   for (uint64_t i = 0; i < object_map.size(); ++i) {
     object_map[i] = i % 4;
   }
-- 
2.39.5