From: Jason Dillaman Date: Tue, 13 Dec 2016 19:10:58 +0000 (-0500) Subject: librbd: remove image header lock assertions X-Git-Tag: v11.1.1~12^2 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=ce4f2a52ec0a794d89e7576b59c9b9aefe3db288;p=ceph-ci.git librbd: remove image header lock assertions This assertions can sporadically fail if the watch is lost and recovered in the background. Upon a true loss of the lock, the client would either be blacklisted or it would have completed all in-flight ops before releasing. Fixes: http://tracker.ceph.com/issues/18244 Signed-off-by: Jason Dillaman --- diff --git a/src/librbd/ExclusiveLock.cc b/src/librbd/ExclusiveLock.cc index 48ecbf850f5..58fc21e78a0 100644 --- a/src/librbd/ExclusiveLock.cc +++ b/src/librbd/ExclusiveLock.cc @@ -244,13 +244,6 @@ void ExclusiveLock::handle_peer_notification() { execute_next_action(); } -template -void ExclusiveLock::assert_header_locked(librados::ObjectWriteOperation *op) { - Mutex::Locker locker(m_lock); - rados::cls::lock::assert_locked(op, RBD_LOCK_NAME, LOCK_EXCLUSIVE, - m_cookie, WATCHER_LOCK_TAG); -} - template std::string ExclusiveLock::encode_lock_cookie() const { assert(m_lock.is_locked()); diff --git a/src/librbd/ExclusiveLock.h b/src/librbd/ExclusiveLock.h index 93ddd903e10..c313a895981 100644 --- a/src/librbd/ExclusiveLock.h +++ b/src/librbd/ExclusiveLock.h @@ -45,8 +45,6 @@ public: void handle_peer_notification(); - void assert_header_locked(librados::ObjectWriteOperation *op); - static bool decode_lock_cookie(const std::string &cookie, uint64_t *handle); private: diff --git a/src/librbd/object_map/InvalidateRequest.cc b/src/librbd/object_map/InvalidateRequest.cc index e60d04fe099..e744add68db 100644 --- a/src/librbd/object_map/InvalidateRequest.cc +++ b/src/librbd/object_map/InvalidateRequest.cc @@ -59,10 +59,6 @@ void InvalidateRequest::send() { lderr(cct) << this << " invalidating object map on-disk" << dendl; librados::ObjectWriteOperation op; - if (image_ctx.exclusive_lock != nullptr && - m_snap_id == CEPH_NOSNAP && !m_force) { - image_ctx.exclusive_lock->assert_header_locked(&op); - } cls_client::set_flags(&op, m_snap_id, flags, flags); librados::AioCompletion *rados_completion = diff --git a/src/librbd/operation/FlattenRequest.cc b/src/librbd/operation/FlattenRequest.cc index 8cfddbeac74..911534dea2c 100644 --- a/src/librbd/operation/FlattenRequest.cc +++ b/src/librbd/operation/FlattenRequest.cc @@ -140,9 +140,6 @@ bool FlattenRequest::send_update_header() { // remove parent from this (base) image librados::ObjectWriteOperation op; - if (image_ctx.exclusive_lock != nullptr) { - image_ctx.exclusive_lock->assert_header_locked(&op); - } cls_client::remove_parent(&op); librados::AioCompletion *rados_completion = this->create_callback_completion(); diff --git a/src/librbd/operation/RebuildObjectMapRequest.cc b/src/librbd/operation/RebuildObjectMapRequest.cc index 281c24c598e..ce8ee858b85 100644 --- a/src/librbd/operation/RebuildObjectMapRequest.cc +++ b/src/librbd/operation/RebuildObjectMapRequest.cc @@ -215,9 +215,6 @@ void RebuildObjectMapRequest::send_update_header() { m_state = STATE_UPDATE_HEADER; librados::ObjectWriteOperation op; - if (m_image_ctx.exclusive_lock != nullptr) { - m_image_ctx.exclusive_lock->assert_header_locked(&op); - } uint64_t flags = RBD_FLAG_OBJECT_MAP_INVALID | RBD_FLAG_FAST_DIFF_INVALID; cls_client::set_flags(&op, m_image_ctx.snap_id, 0, flags); diff --git a/src/librbd/operation/ResizeRequest.cc b/src/librbd/operation/ResizeRequest.cc index 67207d95d8f..edd1ca36f08 100644 --- a/src/librbd/operation/ResizeRequest.cc +++ b/src/librbd/operation/ResizeRequest.cc @@ -388,9 +388,6 @@ void ResizeRequest::send_update_header() { bl.append(reinterpret_cast(&m_new_size), sizeof(m_new_size)); op.write(offsetof(rbd_obj_header_ondisk, image_size), bl); } else { - if (image_ctx.exclusive_lock != nullptr) { - image_ctx.exclusive_lock->assert_header_locked(&op); - } cls_client::set_size(&op, m_new_size); } diff --git a/src/librbd/operation/SnapshotCreateRequest.cc b/src/librbd/operation/SnapshotCreateRequest.cc index 45d1ba619f1..12707b8caf9 100644 --- a/src/librbd/operation/SnapshotCreateRequest.cc +++ b/src/librbd/operation/SnapshotCreateRequest.cc @@ -210,9 +210,6 @@ void SnapshotCreateRequest::send_create_snap() { if (image_ctx.old_format) { cls_client::old_snapshot_add(&op, m_snap_id, m_snap_name); } else { - if (image_ctx.exclusive_lock != nullptr) { - image_ctx.exclusive_lock->assert_header_locked(&op); - } cls_client::snapshot_add(&op, m_snap_id, m_snap_name, m_snap_namespace); } diff --git a/src/librbd/operation/SnapshotRemoveRequest.cc b/src/librbd/operation/SnapshotRemoveRequest.cc index eadbfc2a106..1e68a01bcd6 100644 --- a/src/librbd/operation/SnapshotRemoveRequest.cc +++ b/src/librbd/operation/SnapshotRemoveRequest.cc @@ -180,10 +180,6 @@ void SnapshotRemoveRequest::send_remove_snap() { if (image_ctx.old_format) { cls_client::old_snapshot_remove(&op, m_snap_name); } else { - if (image_ctx.exclusive_lock != nullptr && - image_ctx.exclusive_lock->is_lock_owner()) { - image_ctx.exclusive_lock->assert_header_locked(&op); - } cls_client::snapshot_remove(&op, m_snap_id); } diff --git a/src/librbd/operation/SnapshotRenameRequest.cc b/src/librbd/operation/SnapshotRenameRequest.cc index 5b836cd061c..236d82815b3 100644 --- a/src/librbd/operation/SnapshotRenameRequest.cc +++ b/src/librbd/operation/SnapshotRenameRequest.cc @@ -89,10 +89,6 @@ void SnapshotRenameRequest::send_rename_snap() { if (image_ctx.old_format) { cls_client::old_snapshot_rename(&op, m_snap_id, m_snap_name); } else { - if (image_ctx.exclusive_lock != nullptr && - image_ctx.exclusive_lock->is_lock_owner()) { - image_ctx.exclusive_lock->assert_header_locked(&op); - } cls_client::snapshot_rename(&op, m_snap_id, m_snap_name); } diff --git a/src/test/librbd/mock/MockExclusiveLock.h b/src/test/librbd/mock/MockExclusiveLock.h index d15c8f53287..6e3621ad82b 100644 --- a/src/test/librbd/mock/MockExclusiveLock.h +++ b/src/test/librbd/mock/MockExclusiveLock.h @@ -15,8 +15,6 @@ namespace librbd { struct MockExclusiveLock { MOCK_CONST_METHOD0(is_lock_owner, bool()); - MOCK_METHOD1(assert_header_locked, void(librados::ObjectWriteOperation *)); - MOCK_METHOD2(init, void(uint64_t features, Context*)); MOCK_METHOD1(shut_down, void(Context*)); diff --git a/src/test/librbd/object_map/test_mock_InvalidateRequest.cc b/src/test/librbd/object_map/test_mock_InvalidateRequest.cc index 4f3831cf059..3c801e6e577 100644 --- a/src/test/librbd/object_map/test_mock_InvalidateRequest.cc +++ b/src/test/librbd/object_map/test_mock_InvalidateRequest.cc @@ -55,9 +55,6 @@ TEST_F(TestMockObjectMapInvalidateRequest, UpdatesHeadOnDiskFlag) { C_SaferCond cond_ctx; AsyncRequest<> *request = new InvalidateRequest<>(*ictx, CEPH_NOSNAP, false, &cond_ctx); - EXPECT_CALL(get_mock_io_ctx(ictx->md_ctx), - exec(ictx->header_oid, _, StrEq("lock"), StrEq("assert_locked"), _, _, _)) - .WillOnce(DoDefault()); EXPECT_CALL(get_mock_io_ctx(ictx->md_ctx), exec(ictx->header_oid, _, StrEq("rbd"), StrEq("set_flags"), _, _, _)) .WillOnce(DoDefault()); @@ -85,9 +82,6 @@ TEST_F(TestMockObjectMapInvalidateRequest, UpdatesSnapOnDiskFlag) { AsyncRequest<> *request = new InvalidateRequest<>(*ictx, ictx->snap_id, false, &cond_ctx); - EXPECT_CALL(get_mock_io_ctx(ictx->md_ctx), - exec(ictx->header_oid, _, StrEq("lock"), StrEq("assert_locked"), _, _, _)) - .Times(0); EXPECT_CALL(get_mock_io_ctx(ictx->md_ctx), exec(ictx->header_oid, _, StrEq("rbd"), StrEq("set_flags"), _, _, _)) .WillOnce(DoDefault()); @@ -133,9 +127,6 @@ TEST_F(TestMockObjectMapInvalidateRequest, IgnoresOnDiskUpdateFailure) { C_SaferCond cond_ctx; AsyncRequest<> *request = new InvalidateRequest<>(*ictx, CEPH_NOSNAP, false, &cond_ctx); - EXPECT_CALL(get_mock_io_ctx(ictx->md_ctx), - exec(ictx->header_oid, _, StrEq("lock"), StrEq("assert_locked"), _, _, _)) - .WillOnce(DoDefault()); EXPECT_CALL(get_mock_io_ctx(ictx->md_ctx), exec(ictx->header_oid, _, StrEq("rbd"), StrEq("set_flags"), _, _, _)) .WillOnce(Return(-EINVAL)); diff --git a/src/test/librbd/object_map/test_mock_ResizeRequest.cc b/src/test/librbd/object_map/test_mock_ResizeRequest.cc index 42007d34487..d2e93a9ebb3 100644 --- a/src/test/librbd/object_map/test_mock_ResizeRequest.cc +++ b/src/test/librbd/object_map/test_mock_ResizeRequest.cc @@ -41,8 +41,6 @@ public: } void expect_invalidate(librbd::ImageCtx *ictx) { - EXPECT_CALL(get_mock_io_ctx(ictx->md_ctx), - exec(ictx->header_oid, _, StrEq("lock"), StrEq("assert_locked"), _, _, _)).Times(0); EXPECT_CALL(get_mock_io_ctx(ictx->md_ctx), exec(ictx->header_oid, _, StrEq("rbd"), StrEq("set_flags"), _, _, _)) .WillOnce(DoDefault()); diff --git a/src/test/librbd/object_map/test_mock_SnapshotRemoveRequest.cc b/src/test/librbd/object_map/test_mock_SnapshotRemoveRequest.cc index 215c214eb8b..059237a1808 100644 --- a/src/test/librbd/object_map/test_mock_SnapshotRemoveRequest.cc +++ b/src/test/librbd/object_map/test_mock_SnapshotRemoveRequest.cc @@ -63,9 +63,6 @@ public: } void expect_invalidate(librbd::ImageCtx *ictx) { - EXPECT_CALL(get_mock_io_ctx(ictx->md_ctx), - exec(ictx->header_oid, _, StrEq("lock"), StrEq("assert_locked"), _, _, _)) - .Times(0); EXPECT_CALL(get_mock_io_ctx(ictx->md_ctx), exec(ictx->header_oid, _, StrEq("rbd"), StrEq("set_flags"), _, _, _)) .WillOnce(DoDefault()); diff --git a/src/test/librbd/object_map/test_mock_SnapshotRollbackRequest.cc b/src/test/librbd/object_map/test_mock_SnapshotRollbackRequest.cc index 51fc932c0b0..3cff4b4f26d 100644 --- a/src/test/librbd/object_map/test_mock_SnapshotRollbackRequest.cc +++ b/src/test/librbd/object_map/test_mock_SnapshotRollbackRequest.cc @@ -52,9 +52,6 @@ public: } void expect_invalidate(librbd::ImageCtx *ictx, uint32_t times) { - EXPECT_CALL(get_mock_io_ctx(ictx->md_ctx), - exec(ictx->header_oid, _, StrEq("lock"), StrEq("assert_locked"), _, _, _)) - .Times(0); EXPECT_CALL(get_mock_io_ctx(ictx->md_ctx), exec(ictx->header_oid, _, StrEq("rbd"), StrEq("set_flags"), _, _, _)) .Times(times) diff --git a/src/test/librbd/object_map/test_mock_UpdateRequest.cc b/src/test/librbd/object_map/test_mock_UpdateRequest.cc index 45879f2f83e..7964ffc283a 100644 --- a/src/test/librbd/object_map/test_mock_UpdateRequest.cc +++ b/src/test/librbd/object_map/test_mock_UpdateRequest.cc @@ -42,9 +42,6 @@ public: } void expect_invalidate(librbd::ImageCtx *ictx) { - EXPECT_CALL(get_mock_io_ctx(ictx->md_ctx), - exec(ictx->header_oid, _, StrEq("lock"), StrEq("assert_locked"), _, _, _)) - .Times(0); EXPECT_CALL(get_mock_io_ctx(ictx->md_ctx), exec(ictx->header_oid, _, StrEq("rbd"), StrEq("set_flags"), _, _, _)) .WillOnce(DoDefault()); diff --git a/src/test/librbd/operation/test_mock_ResizeRequest.cc b/src/test/librbd/operation/test_mock_ResizeRequest.cc index 17879ecddf3..5a996599262 100644 --- a/src/test/librbd/operation/test_mock_ResizeRequest.cc +++ b/src/test/librbd/operation/test_mock_ResizeRequest.cc @@ -101,9 +101,6 @@ public: .WillOnce(Return(r)); } else { expect_is_lock_owner(mock_image_ctx); - if (mock_image_ctx.exclusive_lock != nullptr) { - EXPECT_CALL(*mock_image_ctx.exclusive_lock, assert_header_locked(_)); - } EXPECT_CALL(get_mock_io_ctx(mock_image_ctx.md_ctx), exec(mock_image_ctx.header_oid, _, StrEq("rbd"), StrEq("set_size"), _, _, _)) .WillOnce(Return(r)); diff --git a/src/test/librbd/operation/test_mock_SnapshotCreateRequest.cc b/src/test/librbd/operation/test_mock_SnapshotCreateRequest.cc index 23398cd197f..6c845bf2e05 100644 --- a/src/test/librbd/operation/test_mock_SnapshotCreateRequest.cc +++ b/src/test/librbd/operation/test_mock_SnapshotCreateRequest.cc @@ -63,12 +63,6 @@ public: } void expect_snap_create(MockImageCtx &mock_image_ctx, int r) { - if (!mock_image_ctx.old_format && - mock_image_ctx.exclusive_lock != nullptr) { - EXPECT_CALL(*mock_image_ctx.exclusive_lock, assert_header_locked(_)) - .Times(r == -ESTALE ? 2 : 1); - } - auto &expect = EXPECT_CALL(get_mock_io_ctx(mock_image_ctx.md_ctx), exec(mock_image_ctx.header_oid, _, StrEq("rbd"), StrEq(mock_image_ctx.old_format ? "snap_add" :