From 1a4e1e09b1e562bf97cfe96f5cb9f937b6987165 Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Tue, 13 Dec 2016 14:10:58 -0500 Subject: [PATCH] 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 (cherry picked from commit ce4f2a52ec0a794d89e7576b59c9b9aefe3db288) Conflicts: src/librbd/operation/SnapshotCreateRequest.cc: rbd class does not support the snapshot namespaces in Jewel, skip the corresponding argument --- src/librbd/ExclusiveLock.cc | 7 ------- src/librbd/ExclusiveLock.h | 2 -- src/librbd/object_map/InvalidateRequest.cc | 4 ---- src/librbd/operation/FlattenRequest.cc | 3 --- src/librbd/operation/RebuildObjectMapRequest.cc | 3 --- src/librbd/operation/ResizeRequest.cc | 3 --- src/librbd/operation/SnapshotCreateRequest.cc | 3 --- src/librbd/operation/SnapshotRemoveRequest.cc | 4 ---- src/librbd/operation/SnapshotRenameRequest.cc | 4 ---- src/test/librbd/mock/MockExclusiveLock.h | 2 -- .../librbd/object_map/test_mock_InvalidateRequest.cc | 9 --------- src/test/librbd/object_map/test_mock_ResizeRequest.cc | 2 -- .../librbd/object_map/test_mock_SnapshotRemoveRequest.cc | 3 --- .../object_map/test_mock_SnapshotRollbackRequest.cc | 3 --- src/test/librbd/object_map/test_mock_UpdateRequest.cc | 3 --- src/test/librbd/operation/test_mock_ResizeRequest.cc | 3 --- .../librbd/operation/test_mock_SnapshotCreateRequest.cc | 6 ------ 17 files changed, 64 deletions(-) diff --git a/src/librbd/ExclusiveLock.cc b/src/librbd/ExclusiveLock.cc index 3fedf8ddee347..6530813ebbfd7 100644 --- a/src/librbd/ExclusiveLock.cc +++ b/src/librbd/ExclusiveLock.cc @@ -249,13 +249,6 @@ void ExclusiveLock::handle_peer_notification(int r) { handle_acquire_lock(r); } -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 531567a52aed1..58eaa9376893b 100644 --- a/src/librbd/ExclusiveLock.h +++ b/src/librbd/ExclusiveLock.h @@ -45,8 +45,6 @@ public: void handle_peer_notification(int r); - 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 e60d04fe099a7..e744add68db85 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 8cfddbeac74dc..911534dea2ca8 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 352a439a83bd8..6d5ec4ab7de83 100644 --- a/src/librbd/operation/RebuildObjectMapRequest.cc +++ b/src/librbd/operation/RebuildObjectMapRequest.cc @@ -352,9 +352,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 f890537570dda..2c0046ca6b99a 100644 --- a/src/librbd/operation/ResizeRequest.cc +++ b/src/librbd/operation/ResizeRequest.cc @@ -381,9 +381,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 4f20ec4f977f1..46f4a6d90440e 100644 --- a/src/librbd/operation/SnapshotCreateRequest.cc +++ b/src/librbd/operation/SnapshotCreateRequest.cc @@ -207,9 +207,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); } diff --git a/src/librbd/operation/SnapshotRemoveRequest.cc b/src/librbd/operation/SnapshotRemoveRequest.cc index eadbfc2a106bf..1e68a01bcd661 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 5b836cd061c9c..236d82815b3ea 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 83d3058d1d8bd..7d8629c349f50 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 4f3831cf05953..3c801e6e57733 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 44fd714353ba1..5d0544c1b8491 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 270b267c53fed..c1fadcfb39043 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 ff3e06a8d70c5..3a21db640c565 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 f82ffb82ac21d..20314308fef3b 100644 --- a/src/test/librbd/object_map/test_mock_UpdateRequest.cc +++ b/src/test/librbd/object_map/test_mock_UpdateRequest.cc @@ -43,9 +43,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 f1276ebd4cad0..80d2981462fed 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 1b94b7e1f9f1b..4ff2cbcaa043a 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" : -- 2.39.5