From 90bd1d7a857c0f3c57bda60975f58f9859940185 Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Fri, 24 Jul 2020 12:13:10 -0400 Subject: [PATCH] librbd: use task finisher thread for image open/close callbacks There was a potential race condition with utilizing the AsioEngine to deliver asynchronous image open and close callbacks. This left the potential for the io_context thread to attempt to destroy itself. This commit changes the behavior of the image open and close callbacks to always delete the ImageCtx (now matches the synchronous API behavior) and it always invokes the callback in Finisher thread whose lifetime is tied to the CephContext. Signed-off-by: Jason Dillaman --- src/librbd/ImageCtx.h | 3 - src/librbd/ImageState.cc | 25 ++++-- src/librbd/TaskFinisher.h | 13 ++- src/librbd/api/Group.cc | 3 - src/librbd/deep_copy/ImageCopyRequest.cc | 2 - src/librbd/image/CloneRequest.cc | 15 +--- src/librbd/image/CloseRequest.cc | 1 - src/librbd/image/DetachChildRequest.cc | 2 - src/librbd/image/RefreshParentRequest.cc | 23 +++--- src/librbd/image/RemoveRequest.cc | 2 - src/librbd/io/AioCompletion.cc | 79 +++++++++++-------- src/librbd/librbd.cc | 5 +- src/librbd/mirror/EnableRequest.cc | 2 - src/librbd/trash/RemoveRequest.cc | 1 - .../deep_copy/test_mock_ImageCopyRequest.cc | 45 ----------- .../librbd/image/test_mock_CloneRequest.cc | 4 - .../image/test_mock_DetachChildRequest.cc | 2 - .../librbd/image/test_mock_RemoveRequest.cc | 4 - src/test/librbd/mock/MockImageCtx.h | 1 - .../operation/test_mock_ResizeRequest.cc | 8 +- .../librbd/trash/test_mock_RemoveRequest.cc | 1 - .../test_mock_SnapshotPurgeRequest.cc | 12 --- .../test_mock_TrashMoveRequest.cc | 13 --- .../image_deleter/SnapshotPurgeRequest.cc | 2 - .../image_deleter/TrashMoveRequest.cc | 2 - .../image_replayer/CloseImageRequest.cc | 1 - .../image_replayer/OpenImageRequest.cc | 1 - .../image_replayer/OpenLocalImageRequest.cc | 1 - 28 files changed, 93 insertions(+), 180 deletions(-) diff --git a/src/librbd/ImageCtx.h b/src/librbd/ImageCtx.h index de783e0fb3c6..34677bdf3145 100644 --- a/src/librbd/ImageCtx.h +++ b/src/librbd/ImageCtx.h @@ -246,9 +246,6 @@ namespace librbd { bool read_only) { return new ImageCtx(image_name, image_id, snap_id, p, read_only); } - void destroy() { - delete this; - } /** * Either image_name or image_id must be set. diff --git a/src/librbd/ImageState.cc b/src/librbd/ImageState.cc index 96f57f87910e..b528cc260fc1 100644 --- a/src/librbd/ImageState.cc +++ b/src/librbd/ImageState.cc @@ -9,6 +9,7 @@ #include "common/WorkQueue.h" #include "librbd/AsioEngine.h" #include "librbd/ImageCtx.h" +#include "librbd/TaskFinisher.h" #include "librbd/Utils.h" #include "librbd/asio/ContextWQ.h" #include "librbd/image/CloseRequest.h" @@ -441,9 +442,6 @@ int ImageState::open(uint64_t flags) { open(flags, &ctx); int r = ctx.wait(); - if (r < 0) { - delete m_image_ctx; - } return r; } @@ -468,7 +466,6 @@ int ImageState::close() { close(&ctx); int r = ctx.wait(); - delete m_image_ctx; return r; } @@ -763,11 +760,23 @@ void ImageState::complete_action_unlock(State next_state, int r) { m_state = next_state; m_lock.unlock(); - for (auto ctx : action_contexts.second) { - ctx->complete(r); - } + if (next_state == STATE_CLOSED || + (next_state == STATE_UNINITIALIZED && r < 0)) { + // the ImageCtx must be deleted outside the scope of its callback threads + auto ctx = new LambdaContext( + [image_ctx=m_image_ctx, contexts=std::move(action_contexts.second)] + (int r) { + delete image_ctx; + for (auto ctx : contexts) { + ctx->complete(r); + } + }); + TaskFinisherSingleton::get_singleton(m_image_ctx->cct).queue(ctx, r); + } else { + for (auto ctx : action_contexts.second) { + ctx->complete(r); + } - if (next_state != STATE_UNINITIALIZED && next_state != STATE_CLOSED) { m_lock.lock(); if (!is_transition_state() && !m_actions_contexts.empty()) { execute_next_action_unlock(); diff --git a/src/librbd/TaskFinisher.h b/src/librbd/TaskFinisher.h index a791e786ff15..bf76f633f7b4 100644 --- a/src/librbd/TaskFinisher.h +++ b/src/librbd/TaskFinisher.h @@ -20,6 +20,11 @@ struct TaskFinisherSingleton { SafeTimer *m_safe_timer; Finisher *m_finisher; + static TaskFinisherSingleton& get_singleton(CephContext* cct) { + return cct->lookup_or_create_singleton_object< + TaskFinisherSingleton>("librbd::TaskFinisherSingleton", false, cct); + } + explicit TaskFinisherSingleton(CephContext *cct) { m_safe_timer = new SafeTimer(cct, m_lock, false); m_safe_timer->init(); @@ -36,6 +41,10 @@ struct TaskFinisherSingleton { m_finisher->stop(); delete m_finisher; } + + void queue(Context* ctx, int r) { + m_finisher->queue(ctx, r); + } }; @@ -43,9 +52,7 @@ template class TaskFinisher { public: TaskFinisher(CephContext &cct) : m_cct(cct) { - auto& singleton = - cct.lookup_or_create_singleton_object( - "librbd::TaskFinisher::m_safe_timer", false, &cct); + auto& singleton = TaskFinisherSingleton::get_singleton(&cct); m_lock = &singleton.m_lock; m_safe_timer = singleton.m_safe_timer; m_finisher = singleton.m_finisher; diff --git a/src/librbd/api/Group.cc b/src/librbd/api/Group.cc index 2beca5adb0b9..b33b3f4998f8 100644 --- a/src/librbd/api/Group.cc +++ b/src/librbd/api/Group.cc @@ -251,7 +251,6 @@ int group_snap_remove_by_record(librados::IoCtx& group_ioctx, r = on_finishes[i]->wait(); delete on_finishes[i]; if (r < 0) { - delete ictxs[i]; ictxs[i] = nullptr; ret_code = r; } @@ -359,7 +358,6 @@ int group_snap_rollback_by_record(librados::IoCtx& group_ioctx, r = on_finishes[i]->wait(); delete on_finishes[i]; if (r < 0) { - delete ictxs[i]; ictxs[i] = nullptr; ret_code = r; } @@ -973,7 +971,6 @@ int Group::snap_create(librados::IoCtx& group_ioctx, r = on_finishes[i]->wait(); delete on_finishes[i]; if (r < 0) { - delete ictxs[i]; ictxs[i] = nullptr; ret_code = r; } diff --git a/src/librbd/deep_copy/ImageCopyRequest.cc b/src/librbd/deep_copy/ImageCopyRequest.cc index 739e55c33ccc..859b13adb6ce 100644 --- a/src/librbd/deep_copy/ImageCopyRequest.cc +++ b/src/librbd/deep_copy/ImageCopyRequest.cc @@ -7,8 +7,6 @@ #include "librbd/Utils.h" #include "librbd/deep_copy/Handler.h" #include "librbd/deep_copy/Utils.h" -#include "librbd/image/CloseRequest.h" -#include "librbd/image/OpenRequest.h" #include "librbd/object_map/DiffRequest.h" #include "osdc/Striper.h" diff --git a/src/librbd/image/CloneRequest.cc b/src/librbd/image/CloneRequest.cc index 8db5ee0efc7f..1b11b3a5fae0 100644 --- a/src/librbd/image/CloneRequest.cc +++ b/src/librbd/image/CloneRequest.cc @@ -160,7 +160,6 @@ void CloneRequest::handle_open_parent(int r) { ldout(m_cct, 20) << "r=" << r << dendl; if (r < 0) { - m_parent_image_ctx->destroy(); m_parent_image_ctx = nullptr; lderr(m_cct) << "failed to open parent image: " << cpp_strerror(r) << dendl; @@ -345,7 +344,6 @@ void CloneRequest::handle_open_child(int r) { ldout(m_cct, 15) << "r=" << r << dendl; if (r < 0) { - m_imctx->destroy(); m_imctx = nullptr; lderr(m_cct) << "Error opening new image: " << cpp_strerror(r) << dendl; @@ -517,10 +515,8 @@ void CloneRequest::close_child() { ceph_assert(m_imctx != nullptr); - using klass = CloneRequest; - Context *ctx = create_async_context_callback( - *m_imctx, create_context_callback< - klass, &klass::handle_close_child>(this)); + auto ctx = create_context_callback< + CloneRequest, &CloneRequest::handle_close_child>(this); m_imctx->state->close(ctx); } @@ -528,7 +524,6 @@ template void CloneRequest::handle_close_child(int r) { ldout(m_cct, 15) << dendl; - m_imctx->destroy(); m_imctx = nullptr; if (r < 0) { @@ -576,9 +571,8 @@ void CloneRequest::close_parent() { ldout(m_cct, 20) << dendl; ceph_assert(m_parent_image_ctx != nullptr); - Context *ctx = create_async_context_callback( - *m_parent_image_ctx, create_context_callback< - CloneRequest, &CloneRequest::handle_close_parent>(this)); + auto ctx = create_context_callback< + CloneRequest, &CloneRequest::handle_close_parent>(this); m_parent_image_ctx->state->close(ctx); } @@ -586,7 +580,6 @@ template void CloneRequest::handle_close_parent(int r) { ldout(m_cct, 20) << "r=" << r << dendl; - m_parent_image_ctx->destroy(); m_parent_image_ctx = nullptr; if (r < 0) { diff --git a/src/librbd/image/CloseRequest.cc b/src/librbd/image/CloseRequest.cc index 95460268251e..ecd1af7852b1 100644 --- a/src/librbd/image/CloseRequest.cc +++ b/src/librbd/image/CloseRequest.cc @@ -297,7 +297,6 @@ void CloseRequest::handle_close_parent(int r) { CephContext *cct = m_image_ctx->cct; ldout(cct, 10) << this << " " << __func__ << ": r=" << r << dendl; - delete m_image_ctx->parent; m_image_ctx->parent = nullptr; save_result(r); if (r < 0) { diff --git a/src/librbd/image/DetachChildRequest.cc b/src/librbd/image/DetachChildRequest.cc index f786c2d3a651..ab39dbcd72dd 100644 --- a/src/librbd/image/DetachChildRequest.cc +++ b/src/librbd/image/DetachChildRequest.cc @@ -184,7 +184,6 @@ void DetachChildRequest::handle_clone_v2_open_parent(int r) { if (r < 0) { ldout(cct, 5) << "failed to open parent for read/write: " << cpp_strerror(r) << dendl; - m_parent_image_ctx->destroy(); m_parent_image_ctx = nullptr; finish(0); return; @@ -339,7 +338,6 @@ void DetachChildRequest::handle_clone_v2_close_parent(int r) { << dendl; } - m_parent_image_ctx->destroy(); m_parent_image_ctx = nullptr; finish(0); } diff --git a/src/librbd/image/RefreshParentRequest.cc b/src/librbd/image/RefreshParentRequest.cc index 8cc0cf949a0f..6689b137b05f 100644 --- a/src/librbd/image/RefreshParentRequest.cc +++ b/src/librbd/image/RefreshParentRequest.cc @@ -6,10 +6,9 @@ #include "common/dout.h" #include "common/errno.h" #include "librbd/ImageCtx.h" +#include "librbd/ImageState.h" #include "librbd/Utils.h" #include "librbd/asio/ContextWQ.h" -#include "librbd/image/CloseRequest.h" -#include "librbd/image/OpenRequest.h" #include "librbd/io/ObjectDispatcherInterface.h" #define dout_subsys ceph_subsys_rbd @@ -140,12 +139,11 @@ void RefreshParentRequest::send_open_parent() { m_parent_image_ctx->set_read_flag(librados::OPERATION_LOCALIZE_READS); } - using klass = RefreshParentRequest; - Context *ctx = create_async_context_callback( + auto ctx = create_async_context_callback( m_child_image_ctx, create_context_callback< - klass, &klass::handle_open_parent, false>(this)); - OpenRequest *req = OpenRequest::create(m_parent_image_ctx, flags, ctx); - req->send(); + RefreshParentRequest, + &RefreshParentRequest::handle_open_parent, false>(this)); + m_parent_image_ctx->state->open(flags, ctx); } template @@ -159,7 +157,6 @@ Context *RefreshParentRequest::handle_open_parent(int *result) { << dendl; // image already closed by open state machine - delete m_parent_image_ctx; m_parent_image_ctx = nullptr; } @@ -173,12 +170,11 @@ void RefreshParentRequest::send_close_parent() { CephContext *cct = m_child_image_ctx.cct; ldout(cct, 10) << this << " " << __func__ << dendl; - using klass = RefreshParentRequest; - Context *ctx = create_async_context_callback( + auto ctx = create_async_context_callback( m_child_image_ctx, create_context_callback< - klass, &klass::handle_close_parent, false>(this)); - CloseRequest *req = CloseRequest::create(m_parent_image_ctx, ctx); - req->send(); + RefreshParentRequest, + &RefreshParentRequest::handle_close_parent, false>(this)); + m_parent_image_ctx->state->close(ctx); } template @@ -186,7 +182,6 @@ Context *RefreshParentRequest::handle_close_parent(int *result) { CephContext *cct = m_child_image_ctx.cct; ldout(cct, 10) << this << " " << __func__ << " r=" << *result << dendl; - delete m_parent_image_ctx; m_parent_image_ctx = nullptr; if (*result < 0) { diff --git a/src/librbd/image/RemoveRequest.cc b/src/librbd/image/RemoveRequest.cc index 3142b7e3928c..42af593b1b5e 100644 --- a/src/librbd/image/RemoveRequest.cc +++ b/src/librbd/image/RemoveRequest.cc @@ -85,7 +85,6 @@ void RemoveRequest::handle_open_image(int r) { ldout(m_cct, 20) << "r=" << r << dendl; if (r < 0) { - m_image_ctx->destroy(); m_image_ctx = nullptr; if (r != -ENOENT) { @@ -251,7 +250,6 @@ void RemoveRequest::handle_send_close_image(int r) { << cpp_strerror(r) << dendl; } - m_image_ctx->destroy(); m_image_ctx = nullptr; if (m_ret_val < 0) { r = m_ret_val; diff --git a/src/librbd/io/AioCompletion.cc b/src/librbd/io/AioCompletion.cc index cad103364244..f518694e6035 100644 --- a/src/librbd/io/AioCompletion.cc +++ b/src/librbd/io/AioCompletion.cc @@ -63,41 +63,38 @@ void AioCompletion::finalize() { void AioCompletion::complete() { ceph_assert(ictx != nullptr); - CephContext *cct = ictx->cct; ssize_t r = rval; - tracepoint(librbd, aio_complete_enter, this, r); - if (ictx->perfcounter != nullptr) { - ceph::timespan elapsed = coarse_mono_clock::now() - start_time; - switch (aio_type) { - case AIO_TYPE_GENERIC: - case AIO_TYPE_OPEN: - case AIO_TYPE_CLOSE: - break; - case AIO_TYPE_READ: - ictx->perfcounter->tinc(l_librbd_rd_latency, elapsed); break; - case AIO_TYPE_WRITE: - ictx->perfcounter->tinc(l_librbd_wr_latency, elapsed); break; - case AIO_TYPE_DISCARD: - ictx->perfcounter->tinc(l_librbd_discard_latency, elapsed); break; - case AIO_TYPE_FLUSH: - ictx->perfcounter->tinc(l_librbd_flush_latency, elapsed); break; - case AIO_TYPE_WRITESAME: - ictx->perfcounter->tinc(l_librbd_ws_latency, elapsed); break; - case AIO_TYPE_COMPARE_AND_WRITE: - ictx->perfcounter->tinc(l_librbd_cmp_latency, elapsed); break; - default: - lderr(cct) << "completed invalid aio_type: " << aio_type << dendl; - break; - } - } - - if ((aio_type == AIO_TYPE_CLOSE) || - (aio_type == AIO_TYPE_OPEN && r < 0)) { - // must destroy ImageCtx prior to invoking callback - delete ictx; + if ((aio_type == AIO_TYPE_CLOSE) || (aio_type == AIO_TYPE_OPEN && r < 0)) { ictx = nullptr; external_callback = false; + } else { + CephContext *cct = ictx->cct; + + tracepoint(librbd, aio_complete_enter, this, r); + if (ictx->perfcounter != nullptr) { + ceph::timespan elapsed = coarse_mono_clock::now() - start_time; + switch (aio_type) { + case AIO_TYPE_GENERIC: + case AIO_TYPE_OPEN: + break; + case AIO_TYPE_READ: + ictx->perfcounter->tinc(l_librbd_rd_latency, elapsed); break; + case AIO_TYPE_WRITE: + ictx->perfcounter->tinc(l_librbd_wr_latency, elapsed); break; + case AIO_TYPE_DISCARD: + ictx->perfcounter->tinc(l_librbd_discard_latency, elapsed); break; + case AIO_TYPE_FLUSH: + ictx->perfcounter->tinc(l_librbd_flush_latency, elapsed); break; + case AIO_TYPE_WRITESAME: + ictx->perfcounter->tinc(l_librbd_ws_latency, elapsed); break; + case AIO_TYPE_COMPARE_AND_WRITE: + ictx->perfcounter->tinc(l_librbd_cmp_latency, elapsed); break; + default: + lderr(cct) << "completed invalid aio_type: " << aio_type << dendl; + break; + } + } } state = AIO_STATE_CALLBACK; @@ -182,17 +179,29 @@ void AioCompletion::unblock(CephContext* cct) { void AioCompletion::fail(int r) { ceph_assert(ictx != nullptr); - CephContext *cct = ictx->cct; - lderr(cct) << cpp_strerror(r) << dendl; + ceph_assert(r < 0); + + bool queue_required = true; + if (aio_type == AIO_TYPE_CLOSE || aio_type == AIO_TYPE_OPEN) { + // executing from a safe context and the ImageCtx has been destructed + queue_required = false; + } else { + CephContext *cct = ictx->cct; + lderr(cct) << cpp_strerror(r) << dendl; + } ceph_assert(!was_armed); was_armed = true; - error_rval = r; + rval = r; uint32_t previous_pending_count = pending_count.load(); if (previous_pending_count == 0) { - queue_complete(); + if (queue_required) { + queue_complete(); + } else { + complete(); + } } } diff --git a/src/librbd/librbd.cc b/src/librbd/librbd.cc index 2e10188acea9..34c47ea885e3 100644 --- a/src/librbd/librbd.cc +++ b/src/librbd/librbd.cc @@ -127,7 +127,7 @@ struct C_AioCompletion : public Context { } void finish(int r) override { - ldout(cct, 20) << "C_AioComplete::finish: r=" << r << dendl; + ldout(cct, 20) << "C_AioCompletion::finish: r=" << r << dendl; if (r < 0) { aio_comp->fail(r); } else { @@ -145,7 +145,7 @@ struct C_OpenComplete : public C_AioCompletion { ictx(ictx), ictxp(ictxp) { } void finish(int r) override { - ldout(ictx->cct, 20) << "C_OpenComplete::finish: r=" << r << dendl; + ldout(cct, 20) << "C_OpenComplete::finish: r=" << r << dendl; if (r < 0) { *ictxp = nullptr; } else { @@ -168,7 +168,6 @@ struct C_OpenAfterCloseComplete : public Context { void finish(int r) override { ldout(ictx->cct, 20) << "C_OpenAfterCloseComplete::finish: r=" << r << dendl; - delete reinterpret_cast(*ictxp); *ictxp = nullptr; ictx->state->open(0, new C_OpenComplete(ictx, comp, ictxp)); diff --git a/src/librbd/mirror/EnableRequest.cc b/src/librbd/mirror/EnableRequest.cc index d4627c2c1304..c7f7d8746f0d 100644 --- a/src/librbd/mirror/EnableRequest.cc +++ b/src/librbd/mirror/EnableRequest.cc @@ -173,7 +173,6 @@ void EnableRequest::handle_open_image(int r) { if (r < 0) { lderr(m_cct) << "failed to open image: " << cpp_strerror(r) << dendl; - m_image_ctx->destroy(); m_image_ctx = nullptr; finish(r); return; @@ -237,7 +236,6 @@ template void EnableRequest::handle_close_image(int r) { ldout(m_cct, 10) << "r=" << r << dendl; - m_image_ctx->destroy(); m_image_ctx = nullptr; if (r < 0) { diff --git a/src/librbd/trash/RemoveRequest.cc b/src/librbd/trash/RemoveRequest.cc index bff30df6cec8..1149d1d8011d 100644 --- a/src/librbd/trash/RemoveRequest.cc +++ b/src/librbd/trash/RemoveRequest.cc @@ -91,7 +91,6 @@ void RemoveRequest::handle_close_image(int r) { ldout(m_cct, 5) << "failed to close image:" << cpp_strerror(r) << dendl; } - m_image_ctx->destroy(); m_image_ctx = nullptr; finish(m_ret_val); } diff --git a/src/test/librbd/deep_copy/test_mock_ImageCopyRequest.cc b/src/test/librbd/deep_copy/test_mock_ImageCopyRequest.cc index f20867cd4ba3..815401c97fc7 100644 --- a/src/test/librbd/deep_copy/test_mock_ImageCopyRequest.cc +++ b/src/test/librbd/deep_copy/test_mock_ImageCopyRequest.cc @@ -11,8 +11,6 @@ #include "librbd/deep_copy/Handler.h" #include "librbd/deep_copy/ImageCopyRequest.h" #include "librbd/deep_copy/ObjectCopyRequest.h" -#include "librbd/image/CloseRequest.h" -#include "librbd/image/OpenRequest.h" #include "librbd/object_map/DiffRequest.h" #include "test/librados_test_stub/MockTestMemIoCtxImpl.h" #include "test/librbd/mock/MockImageCtx.h" @@ -83,49 +81,6 @@ ObjectCopyRequest* ObjectCopyRequest -struct CloseRequest { - Context* on_finish = nullptr; - static CloseRequest* s_instance; - static CloseRequest* create(MockTestImageCtx *image_ctx, Context *on_finish) { - ceph_assert(s_instance != nullptr); - s_instance->on_finish = on_finish; - return s_instance; - } - - MOCK_METHOD0(send, void()); - - CloseRequest() { - s_instance = this; - } -}; - -CloseRequest* CloseRequest::s_instance = nullptr; - -template <> -struct OpenRequest { - Context* on_finish = nullptr; - static OpenRequest* s_instance; - static OpenRequest* create(MockTestImageCtx *image_ctx, - bool skip_open_parent, Context *on_finish) { - ceph_assert(s_instance != nullptr); - s_instance->on_finish = on_finish; - return s_instance; - } - - MOCK_METHOD0(send, void()); - - OpenRequest() { - s_instance = this; - } -}; - -OpenRequest* OpenRequest::s_instance = nullptr; - -} // namespace image - namespace object_map { template <> diff --git a/src/test/librbd/image/test_mock_CloneRequest.cc b/src/test/librbd/image/test_mock_CloneRequest.cc index 1a0524e56898..25de66dabb75 100644 --- a/src/test/librbd/image/test_mock_CloneRequest.cc +++ b/src/test/librbd/image/test_mock_CloneRequest.cc @@ -291,9 +291,6 @@ public: .WillOnce(WithArg<1>(Invoke([this, r](Context* ctx) { image_ctx->op_work_queue->queue(ctx, r); }))); - if (r < 0) { - EXPECT_CALL(mock_image_ctx, destroy()); - } } void expect_attach_parent(MockAttachParentRequest& mock_request, int r) { @@ -352,7 +349,6 @@ public: .WillOnce(Invoke([this, r](Context* ctx) { image_ctx->op_work_queue->queue(ctx, r); })); - EXPECT_CALL(mock_image_ctx, destroy()); } void expect_remove(MockRemoveRequest& mock_remove_request, int r) { diff --git a/src/test/librbd/image/test_mock_DetachChildRequest.cc b/src/test/librbd/image/test_mock_DetachChildRequest.cc index f160305a0071..6812125cb20a 100644 --- a/src/test/librbd/image/test_mock_DetachChildRequest.cc +++ b/src/test/librbd/image/test_mock_DetachChildRequest.cc @@ -174,7 +174,6 @@ public: .WillOnce(Invoke([this, r](Context* ctx) { image_ctx->op_work_queue->queue(ctx, r); })); - EXPECT_CALL(mock_image_ctx, destroy()); } void expect_snap_remove(MockImageCtx &mock_image_ctx, @@ -373,7 +372,6 @@ TEST_F(TestMockImageDetachChildRequest, TrashedSnapshotOpenParentError) { {234, {cls::rbd::TrashSnapshotNamespace{}}, "snap1", 123, {}, 0}, 0); expect_open(mock_image_ctx, -EPERM); - EXPECT_CALL(mock_image_ctx, destroy()); C_SaferCond ctx; auto req = MockDetachChildRequest::create(mock_image_ctx, &ctx); diff --git a/src/test/librbd/image/test_mock_RemoveRequest.cc b/src/test/librbd/image/test_mock_RemoveRequest.cc index 9a6e8abdb00d..9700202d64fe 100644 --- a/src/test/librbd/image/test_mock_RemoveRequest.cc +++ b/src/test/librbd/image/test_mock_RemoveRequest.cc @@ -245,9 +245,6 @@ public: .WillOnce(Invoke([r](bool open_parent, Context *on_ready) { on_ready->complete(r); })); - if (r < 0) { - EXPECT_CALL(mock_image_ctx, destroy()); - } } void expect_state_close(MockTestImageCtx &mock_image_ctx) { @@ -255,7 +252,6 @@ public: .WillOnce(Invoke([](Context *on_ready) { on_ready->complete(0); })); - EXPECT_CALL(mock_image_ctx, destroy()); } void expect_wq_queue(ContextWQ &wq, int r) { diff --git a/src/test/librbd/mock/MockImageCtx.h b/src/test/librbd/mock/MockImageCtx.h index a06ce09e6a5d..70ba2d8a0caf 100644 --- a/src/test/librbd/mock/MockImageCtx.h +++ b/src/test/librbd/mock/MockImageCtx.h @@ -40,7 +40,6 @@ struct MockImageCtx { ceph_assert(s_instance != nullptr); return s_instance; } - MOCK_METHOD0(destroy, void()); MockImageCtx(librbd::ImageCtx &image_ctx) : image_ctx(&image_ctx), diff --git a/src/test/librbd/operation/test_mock_ResizeRequest.cc b/src/test/librbd/operation/test_mock_ResizeRequest.cc index 7c5fb5462090..fb7d263ba259 100644 --- a/src/test/librbd/operation/test_mock_ResizeRequest.cc +++ b/src/test/librbd/operation/test_mock_ResizeRequest.cc @@ -160,7 +160,13 @@ public: .WillOnce(Invoke([&mock_image_ctx, &mock_io_image_dispatch_spec, r]() { auto aio_comp = mock_io_image_dispatch_spec.s_instance->aio_comp; auto ctx = new LambdaContext([aio_comp](int r) { - aio_comp->fail(r); + if (r < 0) { + aio_comp->fail(r); + } else { + aio_comp->set_request_count(1); + aio_comp->add_request(); + aio_comp->complete_request(r); + } }); mock_image_ctx.image_ctx->op_work_queue->queue(ctx, r); })); diff --git a/src/test/librbd/trash/test_mock_RemoveRequest.cc b/src/test/librbd/trash/test_mock_RemoveRequest.cc index 2e75b3ac5b8f..51d6123de02c 100644 --- a/src/test/librbd/trash/test_mock_RemoveRequest.cc +++ b/src/test/librbd/trash/test_mock_RemoveRequest.cc @@ -122,7 +122,6 @@ struct TestMockTrashRemoveRequest : public TestMockFixture { .WillOnce(Invoke([r](Context *on_finish) { on_finish->complete(r); })); - EXPECT_CALL(mock_image_ctx, destroy()); } void expect_remove_image(MockImageRemoveRequest& mock_image_remove_request, diff --git a/src/test/rbd_mirror/image_deleter/test_mock_SnapshotPurgeRequest.cc b/src/test/rbd_mirror/image_deleter/test_mock_SnapshotPurgeRequest.cc index ef0d0afb1c08..2f14854def58 100644 --- a/src/test/rbd_mirror/image_deleter/test_mock_SnapshotPurgeRequest.cc +++ b/src/test/rbd_mirror/image_deleter/test_mock_SnapshotPurgeRequest.cc @@ -152,10 +152,6 @@ public: })); } - void expect_destroy(librbd::MockTestImageCtx& mock_image_ctx) { - EXPECT_CALL(mock_image_ctx, destroy()); - } - librbd::ImageCtx *m_local_image_ctx; }; @@ -198,7 +194,6 @@ TEST_F(TestMockImageDeleterSnapshotPurgeRequest, SuccessJournal) { 0); expect_close(mock_image_ctx, 0); - expect_destroy(mock_image_ctx); C_SaferCond ctx; auto req = MockSnapshotPurgeRequest::create(m_local_io_ctx, mock_image_ctx.id, @@ -240,7 +235,6 @@ TEST_F(TestMockImageDeleterSnapshotPurgeRequest, SuccessSnapshot) { 0); expect_close(mock_image_ctx, 0); - expect_destroy(mock_image_ctx); C_SaferCond ctx; auto req = MockSnapshotPurgeRequest::create(m_local_io_ctx, mock_image_ctx.id, @@ -264,7 +258,6 @@ TEST_F(TestMockImageDeleterSnapshotPurgeRequest, OpenError) { InSequence seq; expect_set_journal_policy(mock_image_ctx); expect_open(mock_image_ctx, -EPERM); - expect_destroy(mock_image_ctx); C_SaferCond ctx; auto req = MockSnapshotPurgeRequest::create(m_local_io_ctx, mock_image_ctx.id, @@ -290,7 +283,6 @@ TEST_F(TestMockImageDeleterSnapshotPurgeRequest, AcquireLockError) { expect_open(mock_image_ctx, 0); expect_acquire_lock(mock_image_ctx, -EPERM); expect_close(mock_image_ctx, -EINVAL); - expect_destroy(mock_image_ctx); C_SaferCond ctx; auto req = MockSnapshotPurgeRequest::create(m_local_io_ctx, mock_image_ctx.id, @@ -324,7 +316,6 @@ TEST_F(TestMockImageDeleterSnapshotPurgeRequest, SnapUnprotectBusy) { "snap1", -EBUSY); expect_close(mock_image_ctx, -EINVAL); - expect_destroy(mock_image_ctx); C_SaferCond ctx; auto req = MockSnapshotPurgeRequest::create(m_local_io_ctx, mock_image_ctx.id, @@ -358,7 +349,6 @@ TEST_F(TestMockImageDeleterSnapshotPurgeRequest, SnapUnprotectError) { "snap1", -EPERM); expect_close(mock_image_ctx, -EINVAL); - expect_destroy(mock_image_ctx); C_SaferCond ctx; auto req = MockSnapshotPurgeRequest::create(m_local_io_ctx, mock_image_ctx.id, @@ -393,7 +383,6 @@ TEST_F(TestMockImageDeleterSnapshotPurgeRequest, SnapRemoveError) { -EINVAL); expect_close(mock_image_ctx, -EPERM); - expect_destroy(mock_image_ctx); C_SaferCond ctx; auto req = MockSnapshotPurgeRequest::create(m_local_io_ctx, mock_image_ctx.id, @@ -428,7 +417,6 @@ TEST_F(TestMockImageDeleterSnapshotPurgeRequest, CloseError) { 0); expect_close(mock_image_ctx, -EINVAL); - expect_destroy(mock_image_ctx); C_SaferCond ctx; auto req = MockSnapshotPurgeRequest::create(m_local_io_ctx, mock_image_ctx.id, diff --git a/src/test/rbd_mirror/image_deleter/test_mock_TrashMoveRequest.cc b/src/test/rbd_mirror/image_deleter/test_mock_TrashMoveRequest.cc index c85fb150014e..c3702a9cc33b 100644 --- a/src/test/rbd_mirror/image_deleter/test_mock_TrashMoveRequest.cc +++ b/src/test/rbd_mirror/image_deleter/test_mock_TrashMoveRequest.cc @@ -260,10 +260,6 @@ public: })); } - void expect_destroy(librbd::MockTestImageCtx& mock_image_ctx) { - EXPECT_CALL(mock_image_ctx, destroy()); - } - void expect_mirror_image_set(const std::string& image_id, const cls::rbd::MirrorImage& mirror_image, int r) { @@ -362,7 +358,6 @@ TEST_F(TestMockImageDeleterTrashMoveRequest, SuccessJournal) { expect_mirror_image_remove(m_local_io_ctx, 0); expect_close(mock_image_ctx, 0); - expect_destroy(mock_image_ctx); MockTrashWatcher mock_trash_watcher; expect_notify_image_added(mock_trash_watcher, "image id"); @@ -404,7 +399,6 @@ TEST_F(TestMockImageDeleterTrashMoveRequest, SuccessSnapshot) { expect_mirror_image_remove(m_local_io_ctx, 0); expect_close(mock_image_ctx, 0); - expect_destroy(mock_image_ctx); MockTrashWatcher mock_trash_watcher; expect_notify_image_added(mock_trash_watcher, "image id"); @@ -581,7 +575,6 @@ TEST_F(TestMockImageDeleterTrashMoveRequest, OpenImageError) { expect_set_journal_policy(mock_image_ctx); expect_open(mock_image_ctx, -EINVAL); - expect_destroy(mock_image_ctx); C_SaferCond ctx; auto req = MockTrashMoveRequest::create(m_local_io_ctx, "global image id", @@ -620,7 +613,6 @@ TEST_F(TestMockImageDeleterTrashMoveRequest, ResetJournalError) { expect_journal_reset(mock_journal_reset_request, -EINVAL); expect_close(mock_image_ctx, 0); - expect_destroy(mock_image_ctx); C_SaferCond ctx; auto req = MockTrashMoveRequest::create(m_local_io_ctx, "global image id", @@ -662,7 +654,6 @@ TEST_F(TestMockImageDeleterTrashMoveRequest, AcquireLockError) { expect_acquire_lock(mock_image_ctx, -EINVAL); expect_close(mock_image_ctx, 0); - expect_destroy(mock_image_ctx); C_SaferCond ctx; auto req = MockTrashMoveRequest::create(m_local_io_ctx, "global image id", @@ -708,7 +699,6 @@ TEST_F(TestMockImageDeleterTrashMoveRequest, TrashMoveError) { {}, -EINVAL); expect_close(mock_image_ctx, 0); - expect_destroy(mock_image_ctx); C_SaferCond ctx; auto req = MockTrashMoveRequest::create(m_local_io_ctx, "global image id", @@ -755,7 +745,6 @@ TEST_F(TestMockImageDeleterTrashMoveRequest, RemoveMirrorImageError) { expect_mirror_image_remove(m_local_io_ctx, -EINVAL); expect_close(mock_image_ctx, 0); - expect_destroy(mock_image_ctx); MockTrashWatcher mock_trash_watcher; expect_notify_image_added(mock_trash_watcher, "image id"); @@ -805,7 +794,6 @@ TEST_F(TestMockImageDeleterTrashMoveRequest, CloseImageError) { expect_mirror_image_remove(m_local_io_ctx, 0); expect_close(mock_image_ctx, -EINVAL); - expect_destroy(mock_image_ctx); MockTrashWatcher mock_trash_watcher; expect_notify_image_added(mock_trash_watcher, "image id"); @@ -856,7 +844,6 @@ TEST_F(TestMockImageDeleterTrashMoveRequest, DelayedDelation) { expect_mirror_image_remove(m_local_io_ctx, 0); expect_close(mock_image_ctx, 0); - expect_destroy(mock_image_ctx); MockTrashWatcher mock_trash_watcher; expect_notify_image_added(mock_trash_watcher, "image id"); diff --git a/src/tools/rbd_mirror/image_deleter/SnapshotPurgeRequest.cc b/src/tools/rbd_mirror/image_deleter/SnapshotPurgeRequest.cc index 642149c31aeb..19a98804c71a 100644 --- a/src/tools/rbd_mirror/image_deleter/SnapshotPurgeRequest.cc +++ b/src/tools/rbd_mirror/image_deleter/SnapshotPurgeRequest.cc @@ -55,7 +55,6 @@ void SnapshotPurgeRequest::handle_open_image(int r) { if (r < 0) { derr << "failed to open image '" << m_image_id << "': " << cpp_strerror(r) << dendl; - m_image_ctx->destroy(); m_image_ctx = nullptr; finish(r); @@ -264,7 +263,6 @@ template void SnapshotPurgeRequest::handle_close_image(int r) { dout(10) << "r=" << r << dendl; - m_image_ctx->destroy(); m_image_ctx = nullptr; if (r < 0) { diff --git a/src/tools/rbd_mirror/image_deleter/TrashMoveRequest.cc b/src/tools/rbd_mirror/image_deleter/TrashMoveRequest.cc index 243651bba0d4..7f718cb9c66e 100644 --- a/src/tools/rbd_mirror/image_deleter/TrashMoveRequest.cc +++ b/src/tools/rbd_mirror/image_deleter/TrashMoveRequest.cc @@ -183,7 +183,6 @@ void TrashMoveRequest::handle_open_image(int r) { if (r < 0) { derr << "failed to open image: " << cpp_strerror(r) << dendl; - m_image_ctx->destroy(); m_image_ctx = nullptr; finish(r); return; @@ -355,7 +354,6 @@ template void TrashMoveRequest::handle_close_image(int r) { dout(10) << "r=" << r << dendl; - m_image_ctx->destroy(); m_image_ctx = nullptr; if (r < 0) { diff --git a/src/tools/rbd_mirror/image_replayer/CloseImageRequest.cc b/src/tools/rbd_mirror/image_replayer/CloseImageRequest.cc index 10ab661e2771..872c8baa996e 100644 --- a/src/tools/rbd_mirror/image_replayer/CloseImageRequest.cc +++ b/src/tools/rbd_mirror/image_replayer/CloseImageRequest.cc @@ -48,7 +48,6 @@ void CloseImageRequest::handle_close_image(int r) { << dendl; } - delete *m_image_ctx; *m_image_ctx = nullptr; m_on_finish->complete(0); diff --git a/src/tools/rbd_mirror/image_replayer/OpenImageRequest.cc b/src/tools/rbd_mirror/image_replayer/OpenImageRequest.cc index 0827a1dcaa7f..e6ab382bea03 100644 --- a/src/tools/rbd_mirror/image_replayer/OpenImageRequest.cc +++ b/src/tools/rbd_mirror/image_replayer/OpenImageRequest.cc @@ -58,7 +58,6 @@ void OpenImageRequest::handle_open_image(int r) { if (r < 0) { derr << ": failed to open image '" << m_image_id << "': " << cpp_strerror(r) << dendl; - (*m_image_ctx)->destroy(); *m_image_ctx = nullptr; } diff --git a/src/tools/rbd_mirror/image_replayer/OpenLocalImageRequest.cc b/src/tools/rbd_mirror/image_replayer/OpenLocalImageRequest.cc index 8040cd214ba1..7f8d9608eb4d 100644 --- a/src/tools/rbd_mirror/image_replayer/OpenLocalImageRequest.cc +++ b/src/tools/rbd_mirror/image_replayer/OpenLocalImageRequest.cc @@ -149,7 +149,6 @@ void OpenLocalImageRequest::handle_open_image(int r) { derr << ": failed to open image '" << m_local_image_id << "': " << cpp_strerror(r) << dendl; } - (*m_local_image_ctx)->destroy(); *m_local_image_ctx = nullptr; finish(r); return; -- 2.47.3