From e1fe6d5a6a69559433c3fed792261d183e3bd094 Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Tue, 23 Apr 2019 10:22:30 -0400 Subject: [PATCH] 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 --- src/librbd/DeepCopyRequest.cc | 2 -- src/librbd/ImageCtx.cc | 1 - src/librbd/ImageCtx.h | 4 ++-- src/librbd/deep_copy/ObjectCopyRequest.cc | 2 -- src/librbd/io/CopyupRequest.cc | 8 ++++---- src/librbd/io/ObjectRequest.cc | 6 ------ src/librbd/operation/ObjectMapIterate.cc | 2 -- src/librbd/operation/SnapshotCreateRequest.cc | 11 ++++------- src/librbd/operation/SnapshotRemoveRequest.cc | 2 -- src/librbd/operation/SnapshotRollbackRequest.cc | 1 - src/librbd/operation/SparsifyRequest.cc | 4 ---- src/librbd/operation/TrimRequest.cc | 2 -- src/test/librbd/mock/MockImageCtx.h | 2 -- src/test/librbd/test_internal.cc | 4 ++-- 14 files changed, 12 insertions(+), 39 deletions(-) diff --git a/src/librbd/DeepCopyRequest.cc b/src/librbd/DeepCopyRequest.cc index 4939bcaac99..07f4e79044e 100644 --- a/src/librbd/DeepCopyRequest.cc +++ b/src/librbd/DeepCopyRequest.cc @@ -214,7 +214,6 @@ void DeepCopyRequest::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::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 0f7ea92017c..b1c81ed8d48 100644 --- 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 60f143578a5..0adf860d71b 100644 --- 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 2fe039b23c6..03517fb5a9a 100644 --- a/src/librbd/deep_copy/ObjectCopyRequest.cc +++ b/src/librbd/deep_copy/ObjectCopyRequest.cc @@ -475,13 +475,11 @@ void ObjectCopyRequest::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 4c8f19945a0..cf29df82acc 100644 --- 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( 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( snap_id, m_object_no, state, {}, m_trace, true, this); ceph_assert(sent); @@ -326,12 +328,10 @@ void CopyupRequest::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 b3173cd46bb..90016674450 100644 --- a/src/librbd/io/ObjectRequest.cc +++ b/src/librbd/io/ObjectRequest.cc @@ -467,18 +467,15 @@ void AbstractObjectWriteRequest::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, &AbstractObjectWriteRequest::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::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, &AbstractObjectWriteRequest::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 57859879270..ce8cced1339 100644 --- 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 a7808f68b4d..6945674410b 100644 --- a/src/librbd/operation/SnapshotCreateRequest.cc +++ b/src/librbd/operation/SnapshotCreateRequest.cc @@ -220,13 +220,10 @@ Context *SnapshotCreateRequest::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, - &SnapshotCreateRequest::handle_create_object_map>(this)); - } + image_ctx.object_map->snapshot_add( + m_snap_id, create_context_callback< + SnapshotCreateRequest, + &SnapshotCreateRequest::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 a3e7f987ce9..feda957a02f 100644 --- a/src/librbd/operation/SnapshotRemoveRequest.cc +++ b/src/librbd/operation/SnapshotRemoveRequest.cc @@ -39,7 +39,6 @@ void SnapshotRemoveRequest::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::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 a7363f08e9c..fa4725231bc 100644 --- a/src/librbd/operation/SnapshotRollbackRequest.cc +++ b/src/librbd/operation/SnapshotRollbackRequest.cc @@ -240,7 +240,6 @@ void SnapshotRollbackRequest::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 5770a2c047d..95f0cdc2731 100644 --- a/src/librbd/operation/SparsifyRequest.cc +++ b/src/librbd/operation/SparsifyRequest.cc @@ -231,13 +231,11 @@ public: C_SparsifyObject, &C_SparsifyObject::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 2dd8d6c47a9..b599872e9cc 100644 --- a/src/librbd/operation/TrimRequest.cc +++ b/src/librbd/operation/TrimRequest.cc @@ -197,7 +197,6 @@ void TrimRequest::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 >( CEPH_NOSNAP, m_delete_start_min, m_num_objects, OBJECT_PENDING, OBJECT_EXISTS, {}, false, this)) { @@ -293,7 +292,6 @@ void TrimRequest::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 >( 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 e4e66c7cf6f..9ecb609d6c7 100644 --- 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 ×tamp_lock; RWLock &parent_lock; - RWLock &object_map_lock; Mutex &async_ops_lock; Mutex ©up_list_lock; diff --git a/src/test/librbd/test_internal.cc b/src/test/librbd/test_internal.cc index 11db69f2e1e..763f1895371 100644 --- 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]); } } -- 2.39.5