librbd: removed 'ImageCtx::object_map_lock'

The 'image_lock' now protects the 'object_map' pointer and
the contents of the object-map itself are protected by a
private lock.

Signed-off-by: Jason Dillaman <dillaman@redhat.com>

diff --git a/src/librbd/DeepCopyRequest.cc b/src/librbd/DeepCopyRequest.cc
index 4939bcaac995fbd2a318b66980d3f5cba715d494..07f4e79044eaf064f91fa92877597e80d9d25394 100644 (file)
--- a/src/librbd/DeepCopyRequest.cc
+++ b/src/librbd/DeepCopyRequest.cc
@@ -214,7 +214,6 @@ void DeepCopyRequest<I>::send_copy_object_map() {
   }
 
   // rollback the object map (copy snapshot object map to HEAD)
-  RWLock::WLocker object_map_locker(m_dst_image_ctx->object_map_lock);
   auto ctx = new FunctionContext([this, finish_op_ctx](int r) {
       handle_copy_object_map(r);
       finish_op_ctx->complete(0);
@@ -281,7 +280,6 @@ void DeepCopyRequest<I>::handle_refresh_object_map(int r) {
 
   {
     RWLock::WLocker image_locker(m_dst_image_ctx->image_lock);
-    RWLock::WLocker object_map_locker(m_dst_image_ctx->object_map_lock);
     std::swap(m_dst_image_ctx->object_map, m_object_map);
   }
   delete m_object_map;

diff --git a/src/librbd/ImageCtx.cc b/src/librbd/ImageCtx.cc
index 0f7ea92017cc94da07e37254273a1340fef86b5c..b1c81ed8d48d189030470b56f0e96d99c7a242f1 100644 (file)
--- a/src/librbd/ImageCtx.cc
+++ b/src/librbd/ImageCtx.cc
@@ -108,7 +108,6 @@ public:
       owner_lock(util::unique_lock_name("librbd::ImageCtx::owner_lock", this)),
       image_lock(util::unique_lock_name("librbd::ImageCtx::image_lock", this)),
       parent_lock(util::unique_lock_name("librbd::ImageCtx::parent_lock", this)),
-      object_map_lock(util::unique_lock_name("librbd::ImageCtx::object_map_lock", this)),
       timestamp_lock(util::unique_lock_name("librbd::ImageCtx::timestamp_lock", this)),
       async_ops_lock(util::unique_lock_name("librbd::ImageCtx::async_ops_lock", this)),
       copyup_list_lock(util::unique_lock_name("librbd::ImageCtx::copyup_list_lock", this)),

diff --git a/src/librbd/ImageCtx.h b/src/librbd/ImageCtx.h
index 60f143578a579b61663b2598fb0d4fa5aea15bc7..0adf860d71bb69b61d62d57b1c1c98b46086be63 100644 (file)
--- a/src/librbd/ImageCtx.h
+++ b/src/librbd/ImageCtx.h
@@ -99,7 +99,7 @@ namespace librbd {
      * Lock ordering:
      *
      * owner_lock, image_lock, parent_lock,
-     * object_map_lock, async_op_lock, timestamp_lock
+     * async_op_lock, timestamp_lock
      */
     RWLock owner_lock; // protects exclusive lock leadership updates
     RWLock image_lock; // protects snapshot-related member variables,
@@ -112,8 +112,8 @@ namespace librbd {
                        // exclusive_locked
                        // lock_tag
                        // lockers
+                       // object_map
     RWLock parent_lock; // protects parent_md and parent
-    RWLock object_map_lock; // protects object map updates and object_map itself
 
     RWLock timestamp_lock; // protects (create/access/modify)_timestamp
     Mutex async_ops_lock; // protects async_ops and async_requests

diff --git a/src/librbd/deep_copy/ObjectCopyRequest.cc b/src/librbd/deep_copy/ObjectCopyRequest.cc
index 2fe039b23c62c2bf648751cef1b1ff2e8d40511e..03517fb5a9a1b92b9892a0be81f1921a3eabad3b 100644 (file)
--- a/src/librbd/deep_copy/ObjectCopyRequest.cc
+++ b/src/librbd/deep_copy/ObjectCopyRequest.cc
@@ -475,13 +475,11 @@ void ObjectCopyRequest<I>::send_update_object_map() {
     });
 
   auto dst_image_ctx = m_dst_image_ctx;
-  dst_image_ctx->object_map_lock.get_write();
   bool sent = dst_image_ctx->object_map->template aio_update<
     Context, &Context::complete>(dst_snap_id, m_dst_object_number, object_state,
                                  {}, {}, false, ctx);
 
   // NOTE: state machine might complete before we reach here
-  dst_image_ctx->object_map_lock.put_write();
   dst_image_ctx->image_lock.put_read();
   dst_image_ctx->owner_lock.put_read();
   if (!sent) {

diff --git a/src/librbd/io/CopyupRequest.cc b/src/librbd/io/CopyupRequest.cc
index 4c8f19945a0f77107972b0ac89a15f6130e0543e..cf29df82accc411515e36eb26782d1fd07003bd1 100644 (file)
--- a/src/librbd/io/CopyupRequest.cc
+++ b/src/librbd/io/CopyupRequest.cc
@@ -70,7 +70,8 @@ public:
 
   int update_head() {
     auto& image_ctx = this->m_image_ctx;
-    RWLock::WLocker object_map_locker(image_ctx.object_map_lock);
+    ceph_assert(image_ctx.image_lock.is_locked());
+
     bool sent = image_ctx.object_map->template aio_update<Context>(
       CEPH_NOSNAP, m_object_no, m_head_object_map_state, {}, m_trace, false,
       this);
@@ -79,6 +80,8 @@ public:
 
   int update_snapshot(uint64_t snap_id) {
     auto& image_ctx = this->m_image_ctx;
+    ceph_assert(image_ctx.image_lock.is_locked());
+
     uint8_t state = OBJECT_EXISTS;
     if (image_ctx.test_features(RBD_FEATURE_FAST_DIFF, image_ctx.image_lock) &&
         (m_snap_id_idx > 0 || m_first_snap_is_clean)) {
@@ -87,7 +90,6 @@ public:
       state = OBJECT_EXISTS_CLEAN;
     }
 
-    RWLock::RLocker object_map_locker(image_ctx.object_map_lock);
     bool sent = image_ctx.object_map->template aio_update<Context>(
       snap_id, m_object_no, state, {}, m_trace, true, this);
     ceph_assert(sent);
@@ -326,12 +328,10 @@ void CopyupRequest<I>::update_object_maps() {
     head_object_map_state = (*r_it)->get_pre_write_object_map_state();
   }
 
-  RWLock::WLocker object_map_locker(m_image_ctx->object_map_lock);
   if ((*m_image_ctx->object_map)[m_object_no] != head_object_map_state) {
     // (maybe) need to update the HEAD object map state
     m_snap_ids.push_back(CEPH_NOSNAP);
   }
-  object_map_locker.unlock();
   image_locker.unlock();
 
   ceph_assert(m_image_ctx->exclusive_lock->is_lock_owner());

diff --git a/src/librbd/io/ObjectRequest.cc b/src/librbd/io/ObjectRequest.cc
index b3173cd46bb58c8bbcd4eb68c74d392cad75a4a3..90016674450554312a385def48de325525b20501 100644 (file)
--- a/src/librbd/io/ObjectRequest.cc
+++ b/src/librbd/io/ObjectRequest.cc
@@ -467,18 +467,15 @@ void AbstractObjectWriteRequest<I>::pre_write_object_map_update() {
   ldout(image_ctx->cct, 20) << this->m_oid << " " << this->m_object_off
                             << "~" << this->m_object_len << dendl;
 
-  image_ctx->object_map_lock.get_write();
   if (image_ctx->object_map->template aio_update<
         AbstractObjectWriteRequest<I>,
         &AbstractObjectWriteRequest<I>::handle_pre_write_object_map_update>(
           CEPH_NOSNAP, this->m_object_no, new_state, {}, this->m_trace, false,
           this)) {
-    image_ctx->object_map_lock.put_write();
     image_ctx->image_lock.put_read();
     return;
   }
 
-  image_ctx->object_map_lock.put_write();
   image_ctx->image_lock.put_read();
   write_object();
 }
@@ -632,18 +629,15 @@ void AbstractObjectWriteRequest<I>::post_write_object_map_update() {
 
   // should have been flushed prior to releasing lock
   ceph_assert(image_ctx->exclusive_lock->is_lock_owner());
-  image_ctx->object_map_lock.get_write();
   if (image_ctx->object_map->template aio_update<
         AbstractObjectWriteRequest<I>,
         &AbstractObjectWriteRequest<I>::handle_post_write_object_map_update>(
           CEPH_NOSNAP, this->m_object_no, OBJECT_NONEXISTENT, OBJECT_PENDING,
           this->m_trace, false, this)) {
-    image_ctx->object_map_lock.put_write();
     image_ctx->image_lock.put_read();
     return;
   }
 
-  image_ctx->object_map_lock.put_write();
   image_ctx->image_lock.put_read();
   this->finish(0);
 }

diff --git a/src/librbd/operation/ObjectMapIterate.cc b/src/librbd/operation/ObjectMapIterate.cc
index 578598792700cff5212862f6aa222a63401057f2..ce8cced1339087180cbc6921a79459825a044e25 100644 (file)
--- a/src/librbd/operation/ObjectMapIterate.cc
+++ b/src/librbd/operation/ObjectMapIterate.cc
@@ -159,9 +159,7 @@ private:
     RWLock::RLocker image_locker(image_ctx.image_lock);
     ceph_assert(image_ctx.object_map != nullptr);
 
-    RWLock::WLocker l(image_ctx.object_map_lock);
     uint8_t state = (*image_ctx.object_map)[m_object_no];
-
     ldout(cct, 10) << "C_VerifyObjectCallback::object_map_action"
                   << " object " << image_ctx.get_object_name(m_object_no)
                   << " state " << (int)state

diff --git a/src/librbd/operation/SnapshotCreateRequest.cc b/src/librbd/operation/SnapshotCreateRequest.cc
index a7808f68b4d3243f5926fd6913cb7e03080c8676..6945674410b6cf9e874dab2f45b08fbfe1cef098 100644 (file)
--- a/src/librbd/operation/SnapshotCreateRequest.cc
+++ b/src/librbd/operation/SnapshotCreateRequest.cc
@@ -220,13 +220,10 @@ Context *SnapshotCreateRequest<I>::send_create_object_map() {
   CephContext *cct = image_ctx.cct;
   ldout(cct, 5) << this << " " << __func__ << dendl;
 
-  {
-    RWLock::RLocker object_map_lock(image_ctx.object_map_lock);
-    image_ctx.object_map->snapshot_add(
-      m_snap_id, create_context_callback<
-        SnapshotCreateRequest<I>,
-        &SnapshotCreateRequest<I>::handle_create_object_map>(this));
-  }
+  image_ctx.object_map->snapshot_add(
+    m_snap_id, create_context_callback<
+      SnapshotCreateRequest<I>,
+      &SnapshotCreateRequest<I>::handle_create_object_map>(this));
   image_ctx.image_lock.put_read();
   return nullptr;
 }

diff --git a/src/librbd/operation/SnapshotRemoveRequest.cc b/src/librbd/operation/SnapshotRemoveRequest.cc
index a3e7f987ce935ff187cee03c207ccc78d5d794ce..feda957a02fee218dcbc5cb0227a817789222f7c 100644 (file)
--- a/src/librbd/operation/SnapshotRemoveRequest.cc
+++ b/src/librbd/operation/SnapshotRemoveRequest.cc
@@ -39,7 +39,6 @@ void SnapshotRemoveRequest<I>::send_op() {
   ceph_assert(image_ctx.owner_lock.is_locked());
   {
     RWLock::RLocker image_locker(image_ctx.image_lock);
-    RWLock::RLocker object_map_locker(image_ctx.object_map_lock);
     if (image_ctx.snap_info.find(m_snap_id) == image_ctx.snap_info.end()) {
       lderr(cct) << "snapshot doesn't exist" << dendl;
       this->async_complete(-ENOENT);
@@ -227,7 +226,6 @@ void SnapshotRemoveRequest<I>::remove_object_map() {
   {
     RWLock::RLocker owner_lock(image_ctx.owner_lock);
     RWLock::WLocker image_locker(image_ctx.image_lock);
-    RWLock::RLocker object_map_locker(image_ctx.object_map_lock);
     if (image_ctx.object_map != nullptr) {
       ldout(cct, 5) << dendl;
 

diff --git a/src/librbd/operation/SnapshotRollbackRequest.cc b/src/librbd/operation/SnapshotRollbackRequest.cc
index a7363f08e9cd79a7163695f1c617ca544b8f5c8d..fa4725231bc8c862fe27928519a8f1723aade3c3 100644 (file)
--- a/src/librbd/operation/SnapshotRollbackRequest.cc
+++ b/src/librbd/operation/SnapshotRollbackRequest.cc
@@ -240,7 +240,6 @@ void SnapshotRollbackRequest<I>::send_rollback_object_map() {
   {
     RWLock::RLocker owner_locker(image_ctx.owner_lock);
     RWLock::RLocker image_locker(image_ctx.image_lock);
-    RWLock::WLocker object_map_lock(image_ctx.object_map_lock);
     if (image_ctx.object_map != nullptr) {
       CephContext *cct = image_ctx.cct;
       ldout(cct, 5) << this << " " << __func__ << dendl;

diff --git a/src/librbd/operation/SparsifyRequest.cc b/src/librbd/operation/SparsifyRequest.cc
index 5770a2c047d2d366385b76fc31b40964f41aa559..95f0cdc273130694d169995586a65934aa7a7506 100644 (file)
--- a/src/librbd/operation/SparsifyRequest.cc
+++ b/src/librbd/operation/SparsifyRequest.cc
@@ -231,13 +231,11 @@ public:
       C_SparsifyObject<I>,
       &C_SparsifyObject<I>::handle_pre_update_object_map>(this);
 
-    image_ctx.object_map_lock.get_write();
     bool sent = image_ctx.object_map->template aio_update<
       Context, &Context::complete>(CEPH_NOSNAP, m_object_no, OBJECT_PENDING,
                                    OBJECT_EXISTS, {}, false, ctx);
 
     // NOTE: state machine might complete before we reach here
-    image_ctx.object_map_lock.put_write();
     image_ctx.image_lock.put_read();
     image_ctx.owner_lock.put_read();
     if (!sent) {
@@ -305,8 +303,6 @@ public:
       assert(image_ctx.exclusive_lock->is_lock_owner());
       assert(image_ctx.object_map != nullptr);
 
-      RWLock::WLocker object_map_locker(image_ctx.object_map_lock);
-
       sent = image_ctx.object_map->template aio_update<
         Context, &Context::complete>(CEPH_NOSNAP, m_object_no,
                                      exists ? OBJECT_EXISTS : OBJECT_NONEXISTENT,

diff --git a/src/librbd/operation/TrimRequest.cc b/src/librbd/operation/TrimRequest.cc
index 2dd8d6c47a9640e4aa21a1c7552c36589150a552..b599872e9cce8b5b608ce81a2cf0c271017b4712 100644 (file)
--- a/src/librbd/operation/TrimRequest.cc
+++ b/src/librbd/operation/TrimRequest.cc
@@ -197,7 +197,6 @@ void TrimRequest<I>::send_pre_trim() {
 
       ceph_assert(image_ctx.exclusive_lock->is_lock_owner());
 
-      RWLock::WLocker object_map_locker(image_ctx.object_map_lock);
       if (image_ctx.object_map->template aio_update<AsyncRequest<I> >(
             CEPH_NOSNAP, m_delete_start_min, m_num_objects, OBJECT_PENDING,
             OBJECT_EXISTS, {}, false, this)) {
@@ -293,7 +292,6 @@ void TrimRequest<I>::send_post_trim() {
 
       ceph_assert(image_ctx.exclusive_lock->is_lock_owner());
 
-      RWLock::WLocker object_map_locker(image_ctx.object_map_lock);
       if (image_ctx.object_map->template aio_update<AsyncRequest<I> >(
             CEPH_NOSNAP, m_delete_start_min, m_num_objects, OBJECT_NONEXISTENT,
             OBJECT_PENDING, {}, false, this)) {

diff --git a/src/test/librbd/mock/MockImageCtx.h b/src/test/librbd/mock/MockImageCtx.h
index e4e66c7cf6f8b717c742ac35d1669133a6ec6a46..9ecb609d6c78582e969e96653ab2cf17e358981a 100644 (file)
--- a/src/test/librbd/mock/MockImageCtx.h
+++ b/src/test/librbd/mock/MockImageCtx.h
@@ -64,7 +64,6 @@ struct MockImageCtx {
       image_lock(image_ctx.image_lock),
       timestamp_lock(image_ctx.timestamp_lock),
       parent_lock(image_ctx.parent_lock),
-      object_map_lock(image_ctx.object_map_lock),
       async_ops_lock(image_ctx.async_ops_lock),
       copyup_list_lock(image_ctx.copyup_list_lock),
       order(image_ctx.order),
@@ -250,7 +249,6 @@ struct MockImageCtx {
   RWLock &image_lock;
   RWLock &timestamp_lock;
   RWLock &parent_lock;
-  RWLock &object_map_lock;
   Mutex &async_ops_lock;
   Mutex &copyup_list_lock;
 

diff --git a/src/test/librbd/test_internal.cc b/src/test/librbd/test_internal.cc
index 11db69f2e1ee76e83d01f96c7e51ba15f93734dd..763f18953711ca93c91c2f35704d3098df6c9b4f 100644 (file)
--- a/src/test/librbd/test_internal.cc
+++ b/src/test/librbd/test_internal.cc
@@ -746,7 +746,7 @@ TEST_F(TestInternal, SnapshotCopyupZeros)
       object_map.open(&ctx);
       ASSERT_EQ(0, ctx.wait());
 
-      RWLock::WLocker object_map_locker(ictx2->object_map_lock);
+      RWLock::RLocker image_locker(ictx2->image_lock);
       ASSERT_EQ(state, object_map[0]);
     }
   }
@@ -833,7 +833,7 @@ TEST_F(TestInternal, SnapshotCopyupZerosMigration)
       object_map.open(&ctx);
       ASSERT_EQ(0, ctx.wait());
 
-      RWLock::WLocker object_map_locker(ictx2->object_map_lock);
+      RWLock::RLocker image_locker(ictx2->image_lock);
       ASSERT_EQ(state, object_map[0]);
     }
   }