From 20b2ef931aaecb455b6eb292cb5c00b08f70d20e Mon Sep 17 00:00:00 2001 From: Mykola Golub Date: Wed, 28 Oct 2020 13:13:02 +0000 Subject: [PATCH 1/2] librbd: relax requirements on finisher canceled callback The finisher timer is started with safe_callbacks = false, and cancel_event may fail. When canceling a task it is safe to just ignore the cancel_event result and proceed, because the returned false value means the callback is in TaskFinisher::complete already but before acquiring the lock, so when it eventually acquires the lock it will just find out the task is already deleted and return. Signed-off-by: Mykola Golub --- src/librbd/TaskFinisher.h | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/librbd/TaskFinisher.h b/src/librbd/TaskFinisher.h index 268c7e7a917..97f6e529f6e 100644 --- a/src/librbd/TaskFinisher.h +++ b/src/librbd/TaskFinisher.h @@ -65,8 +65,7 @@ public: return false; } it->second.first->complete(-ECANCELED); - bool canceled = m_safe_timer->cancel_event(it->second.second); - ceph_assert(canceled); + m_safe_timer->cancel_event(it->second.second); m_task_contexts.erase(it); return true; } @@ -75,8 +74,7 @@ public: std::lock_guard l{*m_lock}; for (auto &[task, pair] : m_task_contexts) { pair.first->complete(-ECANCELED); - bool canceled = m_safe_timer->cancel_event(pair.second); - ceph_assert(canceled); + m_safe_timer->cancel_event(pair.second); } m_task_contexts.clear(); } @@ -102,7 +100,9 @@ public: return false; } bool canceled = m_safe_timer->cancel_event(it->second.second); - ceph_assert(canceled); + if (!canceled) { + return false; + } auto timer_ctx = new C_Task(this, task); it->second.second = timer_ctx; m_safe_timer->add_event_after(seconds, timer_ctx); @@ -117,8 +117,8 @@ public: std::lock_guard l{*m_lock}; typename TaskContexts::iterator it = m_task_contexts.find(task); if (it != m_task_contexts.end()) { - if (it->second.second != NULL) { - ceph_assert(m_safe_timer->cancel_event(it->second.second)); + if (it->second.second != NULL && + m_safe_timer->cancel_event(it->second.second)) { delete it->second.first; } else { // task already scheduled on the finisher From 0fe9cdefdcc3c38ded49c70842468a953ede53d0 Mon Sep 17 00:00:00 2001 From: Mykola Golub Date: Wed, 28 Oct 2020 13:36:09 +0000 Subject: [PATCH 2/2] librbd: complete with -ECANCELED queued task requests instead of deleting This is cleanup just to make it consistent with other cases. Signed-off-by: Mykola Golub --- src/librbd/ImageWatcher.cc | 22 +++++++++++++++------- src/librbd/TaskFinisher.h | 4 ++-- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/src/librbd/ImageWatcher.cc b/src/librbd/ImageWatcher.cc index 44e748fb974..13ab9d4541e 100644 --- a/src/librbd/ImageWatcher.cc +++ b/src/librbd/ImageWatcher.cc @@ -112,9 +112,11 @@ void ImageWatcher::block_notifies(Context *on_finish) { template void ImageWatcher::schedule_async_progress(const AsyncRequestId &request, uint64_t offset, uint64_t total) { - auto ctx = new LambdaContext( - boost::bind(&ImageWatcher::notify_async_progress, this, request, offset, - total)); + auto ctx = new LambdaContext([this, request, offset, total](int r) { + if (r != -ECANCELED) { + notify_async_progress(request, offset, total); + } + }); m_task_finisher->queue(Task(TASK_CODE_ASYNC_PROGRESS, request), ctx); } @@ -133,8 +135,11 @@ template void ImageWatcher::schedule_async_complete(const AsyncRequestId &request, int r) { m_async_op_tracker.start_op(); - auto ctx = new LambdaContext( - boost::bind(&ImageWatcher::notify_async_complete, this, request, r)); + auto ctx = new LambdaContext([this, request, ret_val=r](int r) { + if (r != -ECANCELED) { + notify_async_complete(request, ret_val); + } + }); m_task_finisher->queue(ctx); } @@ -463,8 +468,11 @@ void ImageWatcher::notify_metadata_remove(const std::string &key, template void ImageWatcher::schedule_cancel_async_requests() { - auto ctx = new LambdaContext( - boost::bind(&ImageWatcher::cancel_async_requests, this)); + auto ctx = new LambdaContext([this](int r) { + if (r != -ECANCELED) { + cancel_async_requests(); + } + }); m_task_finisher->queue(TASK_CODE_CANCEL_ASYNC_REQUESTS, ctx); } diff --git a/src/librbd/TaskFinisher.h b/src/librbd/TaskFinisher.h index 97f6e529f6e..65e7da4a6ba 100644 --- a/src/librbd/TaskFinisher.h +++ b/src/librbd/TaskFinisher.h @@ -119,10 +119,10 @@ public: if (it != m_task_contexts.end()) { if (it->second.second != NULL && m_safe_timer->cancel_event(it->second.second)) { - delete it->second.first; + it->second.first->complete(-ECANCELED); } else { // task already scheduled on the finisher - delete ctx; + ctx->complete(-ECANCELED); return false; } }