From: Jason Dillaman Date: Fri, 24 Jul 2020 16:13:10 +0000 (-0400) Subject: librbd: use task finisher thread for image open/close callbacks X-Git-Tag: v16.1.0~1606^2 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=refs%2Fpull%2F36287%2Fhead;p=ceph.git 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 --- diff --git a/src/librbd/ImageCtx.h b/src/librbd/ImageCtx.h index de783e0fb3c..34677bdf314 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 96f57f87910..b528cc260fc 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 a791e786ff1..bf76f633f7b 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 2beca5adb0b..b33b3f4998f 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 739e55c33cc..859b13adb6c 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 8db5ee0efc7..1b11b3a5fae 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 95460268251..ecd1af7852b 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 f786c2d3a65..ab39dbcd72d 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 8cc0cf949a0..6689b137b05 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 3142b7e3928..42af593b1b5 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 cad10336424..f518694e603 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 2e10188acea..34c47ea885e 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 d4627c2c130..c7f7d8746f0 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 bff30df6cec..1149d1d8011 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 f20867cd4ba..815401c97fc 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 1a0524e5689..25de66dabb7 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 f160305a007..6812125cb20 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 9a6e8abdb00..9700202d64f 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 a06ce09e6a5..70ba2d8a0ca 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 7c5fb546209..fb7d263ba25 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 2e75b3ac5b8..51d6123de02 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 ef0d0afb1c0..2f14854def5 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 c85fb150014..c3702a9cc33 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 642149c31ae..19a98804c71 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 243651bba0d..7f718cb9c66 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 10ab661e277..872c8baa996 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 0827a1dcaa7..e6ab382bea0 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 8040cd214ba..7f8d9608eb4 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;