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 de783e0fb3c68..34677bdf31454 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 96f57f87910e5..b528cc260fc1b 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 a791e786ff157..bf76f633f7b4c 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 2beca5adb0b9e..b33b3f4998f87 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 739e55c33ccc2..859b13adb6ce9 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 8db5ee0efc7f3..1b11b3a5fae03 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 95460268251e1..ecd1af7852b10 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 f786c2d3a651e..ab39dbcd72ddc 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 8cc0cf949a0f2..6689b137b05f6 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 3142b7e3928ce..42af593b1b5e7 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 cad103364244c..f518694e60350 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 2e10188acea9f..34c47ea885e3f 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 d4627c2c13047..c7f7d8746f0d7 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 bff30df6cec86..1149d1d8011db 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 f20867cd4ba30..815401c97fc72 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 1a0524e568982..25de66dabb75f 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 f160305a0071d..6812125cb20a4 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 9a6e8abdb00de..9700202d64fe4 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 a06ce09e6a5dc..70ba2d8a0caf6 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 7c5fb54620905..fb7d263ba2593 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 2e75b3ac5b8f1..51d6123de02cf 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 ef0d0afb1c08c..2f14854def581 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 c85fb150014e0..c3702a9cc33bf 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 642149c31aebf..19a98804c71a9 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 243651bba0d46..7f718cb9c66e2 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 10ab661e27711..872c8baa996eb 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 0827a1dcaa7f6..e6ab382bea039 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 8040cd214ba11..7f8d9608eb4d8 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.39.5