From 41e186a24f42d1256dd93d741f07d48f739dc135 Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Tue, 24 Feb 2015 14:33:44 -0500 Subject: [PATCH] librbd: cancel in-progress maint operations before releasing lock Ensure that all in-flight maintenance operations (resize, flatten) are not running when the exclusive lock is released. The lock will be released when transitioning to a snapshot, closing the image, or cooperatively when another client requests the lock. Signed-off-by: Jason Dillaman --- src/librbd/AsyncFlattenRequest.cc | 2 +- src/librbd/AsyncObjectThrottle.cc | 16 +++++++++---- src/librbd/AsyncObjectThrottle.h | 5 +++- src/librbd/AsyncRequest.cc | 14 ++++++++++++ src/librbd/AsyncRequest.h | 25 ++++++++++++++------ src/librbd/AsyncTrimRequest.cc | 2 +- src/librbd/ImageCtx.cc | 17 ++++++++++++++ src/librbd/ImageCtx.h | 5 ++++ src/librbd/ImageWatcher.cc | 31 +++++++++++++++++++++---- src/librbd/ImageWatcher.h | 5 +++- src/librbd/internal.cc | 29 +++++++++++++++++++++-- src/test/librbd/test_internal.cc | 38 +++++++++++++++++++++++++++++++ 12 files changed, 167 insertions(+), 22 deletions(-) diff --git a/src/librbd/AsyncFlattenRequest.cc b/src/librbd/AsyncFlattenRequest.cc index eacdc4bdb556..5f262430e231 100644 --- a/src/librbd/AsyncFlattenRequest.cc +++ b/src/librbd/AsyncFlattenRequest.cc @@ -119,7 +119,7 @@ void AsyncFlattenRequest::send() { boost::lambda::_1, &m_image_ctx, m_object_size, m_snapc, boost::lambda::_2)); AsyncObjectThrottle *throttle = new AsyncObjectThrottle( - context_factory, create_callback_context(), m_prog_ctx, 0, + *this, context_factory, create_callback_context(), m_prog_ctx, 0, m_overlap_objects); throttle->start_ops(cct->_conf->rbd_concurrent_management_ops); } diff --git a/src/librbd/AsyncObjectThrottle.cc b/src/librbd/AsyncObjectThrottle.cc index 28a6a348d1ea..4290eb8636c0 100644 --- a/src/librbd/AsyncObjectThrottle.cc +++ b/src/librbd/AsyncObjectThrottle.cc @@ -2,18 +2,20 @@ // vim: ts=8 sw=2 smarttab #include "librbd/AsyncObjectThrottle.h" #include "include/rbd/librbd.hpp" +#include "librbd/AsyncRequest.h" namespace librbd { -AsyncObjectThrottle::AsyncObjectThrottle(const ContextFactory& context_factory, +AsyncObjectThrottle::AsyncObjectThrottle(const AsyncRequest& async_request, + const ContextFactory& context_factory, Context *ctx, ProgressContext &prog_ctx, uint64_t object_no, uint64_t end_object_no) : m_lock("librbd::AsyncThrottle::m_lock"), - m_context_factory(context_factory), m_ctx(ctx), m_prog_ctx(prog_ctx), - m_object_no(object_no), m_end_object_no(end_object_no), m_current_ops(0), - m_ret(0) + m_async_request(async_request), m_context_factory(context_factory), + m_ctx(ctx), m_prog_ctx(prog_ctx), m_object_no(object_no), + m_end_object_no(end_object_no), m_current_ops(0), m_ret(0) { } @@ -56,7 +58,11 @@ void AsyncObjectThrottle::finish_op(int r) { void AsyncObjectThrottle::start_next_op() { bool done = false; while (!done) { - if (m_ret != 0 || m_object_no >= m_end_object_no) { + if (m_async_request.is_canceled() && m_ret == 0) { + // allow in-flight ops to complete, but don't start new ops + m_ret = -ERESTART; + return; + } else if (m_ret != 0 || m_object_no >= m_end_object_no) { return; } diff --git a/src/librbd/AsyncObjectThrottle.h b/src/librbd/AsyncObjectThrottle.h index c7b5bf69a627..83d69d8c773c 100644 --- a/src/librbd/AsyncObjectThrottle.h +++ b/src/librbd/AsyncObjectThrottle.h @@ -11,6 +11,7 @@ namespace librbd { +class AsyncRequest; class ProgressContext; class AsyncObjectThrottleFinisher { @@ -42,7 +43,8 @@ public: typedef boost::function ContextFactory; - AsyncObjectThrottle(const ContextFactory& context_factory, Context *ctx, + AsyncObjectThrottle(const AsyncRequest &async_request, + const ContextFactory& context_factory, Context *ctx, ProgressContext &prog_ctx, uint64_t object_no, uint64_t end_object_no); @@ -51,6 +53,7 @@ public: private: Mutex m_lock; + const AsyncRequest &m_async_request; ContextFactory m_context_factory; Context *m_ctx; ProgressContext &m_prog_ctx; diff --git a/src/librbd/AsyncRequest.cc b/src/librbd/AsyncRequest.cc index 332d883cbed2..825c8c4e50a5 100644 --- a/src/librbd/AsyncRequest.cc +++ b/src/librbd/AsyncRequest.cc @@ -1,12 +1,26 @@ // -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*- // vim: ts=8 sw=2 smarttab #include "librbd/AsyncRequest.h" +#include "librbd/ImageCtx.h" #include "librbd/internal.h" #include namespace librbd { +AsyncRequest::AsyncRequest(ImageCtx &image_ctx, Context *on_finish) + : m_image_ctx(image_ctx), m_on_finish(on_finish), m_canceled(false), + m_xlist_item(this) { + Mutex::Locker l(m_image_ctx.async_ops_lock); + m_image_ctx.async_requests.push_back(&m_xlist_item); +} + +AsyncRequest::~AsyncRequest() { + Mutex::Locker l(m_image_ctx.async_ops_lock); + assert(m_xlist_item.remove_myself()); + m_image_ctx.async_requests_cond.Signal(); +} + librados::AioCompletion *AsyncRequest::create_callback_completion() { return librados::Rados::aio_create_completion(create_callback_context(), NULL, rados_ctx_cb); diff --git a/src/librbd/AsyncRequest.h b/src/librbd/AsyncRequest.h index 3fdc85c031b4..25be0ed6d29d 100644 --- a/src/librbd/AsyncRequest.h +++ b/src/librbd/AsyncRequest.h @@ -6,6 +6,7 @@ #include "include/int_types.h" #include "include/Context.h" #include "include/rados/librados.hpp" +#include "include/xlist.h" namespace librbd { @@ -14,15 +15,14 @@ class ImageCtx; class AsyncRequest { public: - AsyncRequest(ImageCtx &image_ctx, Context *on_finish) - : m_image_ctx(image_ctx), m_on_finish(on_finish) - { - } - - virtual ~AsyncRequest() {} + AsyncRequest(ImageCtx &image_ctx, Context *on_finish); + virtual ~AsyncRequest(); void complete(int r) { - if (should_complete(r)) { + if (m_canceled) { + m_on_finish->complete(-ERESTART); + delete this; + } else if (should_complete(r)) { m_on_finish->complete(r); delete this; } @@ -30,6 +30,13 @@ public: virtual void send() = 0; + inline bool is_canceled() const { + return m_canceled; + } + inline void cancel() { + m_canceled = true; + } + protected: ImageCtx &m_image_ctx; Context *m_on_finish; @@ -38,6 +45,10 @@ protected: Context *create_callback_context(); virtual bool should_complete(int r) = 0; + +private: + bool m_canceled; + xlist::item m_xlist_item; }; class C_AsyncRequest : public Context diff --git a/src/librbd/AsyncTrimRequest.cc b/src/librbd/AsyncTrimRequest.cc index 3bbab5525258..14a58a8e3b94 100644 --- a/src/librbd/AsyncTrimRequest.cc +++ b/src/librbd/AsyncTrimRequest.cc @@ -154,7 +154,7 @@ void AsyncTrimRequest::send_remove_objects() { boost::lambda::bind(boost::lambda::new_ptr(), boost::lambda::_1, &m_image_ctx, boost::lambda::_2)); AsyncObjectThrottle *throttle = new AsyncObjectThrottle( - context_factory, ctx, m_prog_ctx, m_delete_start, m_num_objects); + *this, context_factory, ctx, m_prog_ctx, m_delete_start, m_num_objects); throttle->start_ops(cct->_conf->rbd_concurrent_management_ops); } diff --git a/src/librbd/ImageCtx.cc b/src/librbd/ImageCtx.cc index 2637d64acc15..1e499382b8dd 100644 --- a/src/librbd/ImageCtx.cc +++ b/src/librbd/ImageCtx.cc @@ -8,6 +8,7 @@ #include "common/perf_counters.h" #include "librbd/AsyncOperation.h" +#include "librbd/AsyncRequest.h" #include "librbd/internal.h" #include "librbd/ImageCtx.h" #include "librbd/ImageWatcher.h" @@ -710,4 +711,20 @@ namespace librbd { << "count=" << async_ops.size() << dendl; async_ops.front()->add_flush_context(on_finish); } + + void ImageCtx::cancel_async_requests() { + Mutex::Locker l(async_ops_lock); + ldout(cct, 10) << "canceling async requests: count=" + << async_requests.size() << dendl; + + for (xlist::iterator it = async_requests.begin(); + !it.end(); ++it) { + ldout(cct, 10) << "canceling async request: " << *it << dendl; + (*it)->cancel(); + } + + while (!async_requests.empty()) { + async_requests_cond.Wait(async_ops_lock); + } + } } diff --git a/src/librbd/ImageCtx.h b/src/librbd/ImageCtx.h index 781b66b9576c..c7b48a0e1d3c 100644 --- a/src/librbd/ImageCtx.h +++ b/src/librbd/ImageCtx.h @@ -36,6 +36,7 @@ class PerfCounters; namespace librbd { class AsyncOperation; + class AsyncRequest; class CopyupRequest; class ImageWatcher; class ObjectMap; @@ -112,6 +113,8 @@ namespace librbd { std::map copyup_list; xlist async_ops; + xlist async_requests; + Cond async_requests_cond; ObjectMap *object_map; @@ -187,6 +190,8 @@ namespace librbd { void flush_async_operations(); void flush_async_operations(Context *on_finish); + + void cancel_async_requests(); }; } diff --git a/src/librbd/ImageWatcher.cc b/src/librbd/ImageWatcher.cc index a1afa1eb8ef0..de30d265b3e8 100644 --- a/src/librbd/ImageWatcher.cc +++ b/src/librbd/ImageWatcher.cc @@ -73,7 +73,8 @@ bool ImageWatcher::is_lock_supported() const { bool ImageWatcher::is_lock_owner() const { assert(m_image_ctx.owner_lock.is_locked()); - return (m_lock_owner_state == LOCK_OWNER_STATE_LOCKED); + return (m_lock_owner_state == LOCK_OWNER_STATE_LOCKED || + m_lock_owner_state == LOCK_OWNER_STATE_RELEASING); } int ImageWatcher::register_watch() { @@ -328,6 +329,20 @@ int ImageWatcher::lock() { return 0; } +void ImageWatcher::prepare_unlock() { + assert(m_image_ctx.owner_lock.is_wlocked()); + if (m_lock_owner_state == LOCK_OWNER_STATE_LOCKED) { + m_lock_owner_state = LOCK_OWNER_STATE_RELEASING; + } +} + +void ImageWatcher::cancel_unlock() { + assert(m_image_ctx.owner_lock.is_wlocked()); + if (m_lock_owner_state == LOCK_OWNER_STATE_RELEASING) { + m_lock_owner_state = LOCK_OWNER_STATE_LOCKED; + } +} + int ImageWatcher::unlock() { assert(m_image_ctx.owner_lock.is_wlocked()); @@ -357,6 +372,14 @@ int ImageWatcher::unlock() void ImageWatcher::release_lock() { + ldout(m_image_ctx.cct, 10) << "releasing exclusive lock by request" << dendl; + { + RWLock::WLocker l(m_image_ctx.owner_lock); + prepare_unlock(); + } + + m_image_ctx.cancel_async_requests(); + RWLock::WLocker l(m_image_ctx.owner_lock); { RWLock::WLocker l2(m_image_ctx.md_lock); @@ -809,7 +832,7 @@ void ImageWatcher::handle_payload(const FlattenPayload &payload, bufferlist *out) { RWLock::RLocker l(m_image_ctx.owner_lock); - if (is_lock_owner()) { + if (m_lock_owner_state == LOCK_OWNER_STATE_LOCKED) { int r = 0; bool new_request = false; if (payload.async_request_id.client_id == get_client_id()) { @@ -848,7 +871,7 @@ void ImageWatcher::handle_payload(const FlattenPayload &payload, void ImageWatcher::handle_payload(const ResizePayload &payload, bufferlist *out) { RWLock::RLocker l(m_image_ctx.owner_lock); - if (is_lock_owner()) { + if (m_lock_owner_state == LOCK_OWNER_STATE_LOCKED) { int r = 0; bool new_request = false; if (payload.async_request_id.client_id == get_client_id()) { @@ -888,7 +911,7 @@ void ImageWatcher::handle_payload(const ResizePayload &payload, void ImageWatcher::handle_payload(const SnapCreatePayload &payload, bufferlist *out) { RWLock::RLocker l(m_image_ctx.owner_lock); - if (is_lock_owner()) { + if (m_lock_owner_state == LOCK_OWNER_STATE_LOCKED) { ldout(m_image_ctx.cct, 10) << "remote snap_create request: " << payload.snap_name << dendl; int r = librbd::snap_create(&m_image_ctx, payload.snap_name.c_str(), false); diff --git a/src/librbd/ImageWatcher.h b/src/librbd/ImageWatcher.h index 3a17f7f1722b..17573d74daac 100644 --- a/src/librbd/ImageWatcher.h +++ b/src/librbd/ImageWatcher.h @@ -40,6 +40,8 @@ namespace librbd { int try_lock(); int request_lock(const boost::function& restart_op, AioCompletion* c); + void prepare_unlock(); + void cancel_unlock(); int unlock(); void assert_header_locked(librados::ObjectWriteOperation *op); @@ -56,7 +58,8 @@ namespace librbd { enum LockOwnerState { LOCK_OWNER_STATE_NOT_LOCKED, - LOCK_OWNER_STATE_LOCKED + LOCK_OWNER_STATE_LOCKED, + LOCK_OWNER_STATE_RELEASING }; enum WatchState { diff --git a/src/librbd/internal.cc b/src/librbd/internal.cc index e4584acc64e0..db9199023fff 100644 --- a/src/librbd/internal.cc +++ b/src/librbd/internal.cc @@ -2345,6 +2345,19 @@ reprotect_and_return_err: // ignore return value, since we may be set to a non-existent // snapshot and the user is trying to fix that ictx_check(ictx); + + bool unlocking = false; + { + RWLock::WLocker l(ictx->owner_lock); + if (ictx->image_watcher != NULL && ictx->image_watcher->is_lock_owner() && + snap_name != NULL && strlen(snap_name) != 0) { + // stop incoming requests since we will release the lock + ictx->image_watcher->prepare_unlock(); + unlocking = true; + } + } + + ictx->cancel_async_requests(); ictx->flush_async_operations(); if (ictx->object_cacher) { // complete pending writes before we're set to a snapshot and @@ -2354,13 +2367,16 @@ reprotect_and_return_err: } int r = _snap_set(ictx, snap_name); if (r < 0) { + RWLock::WLocker l(ictx->owner_lock); + if (unlocking) { + ictx->image_watcher->cancel_unlock(); + } return r; } RWLock::WLocker l(ictx->owner_lock); if (ictx->image_watcher != NULL) { - if (!ictx->image_watcher->is_lock_supported() && - ictx->image_watcher->is_lock_owner()) { + if (unlocking) { r = ictx->image_watcher->unlock(); if (r < 0) { lderr(ictx->cct) << "error unlocking image: " << cpp_strerror(r) @@ -2411,6 +2427,15 @@ reprotect_and_return_err: { ldout(ictx->cct, 20) << "close_image " << ictx << dendl; + { + RWLock::WLocker l(ictx->owner_lock); + if (ictx->image_watcher != NULL && ictx->image_watcher->is_lock_owner()) { + // stop incoming requests + ictx->image_watcher->prepare_unlock(); + } + } + + ictx->cancel_async_requests(); ictx->readahead.wait_for_pending(); if (ictx->object_cacher) { ictx->shutdown_cache(); // implicitly flushes diff --git a/src/test/librbd/test_internal.cc b/src/test/librbd/test_internal.cc index 2831db5f86aa..ddd06afed8ec 100644 --- a/src/test/librbd/test_internal.cc +++ b/src/test/librbd/test_internal.cc @@ -281,3 +281,41 @@ TEST_F(TestInternal, AioDiscardRequestsLock) { ASSERT_FALSE(is_owner); ASSERT_FALSE(c->is_complete()); } + +TEST_F(TestInternal, CancelAsyncResize) { + REQUIRE_FEATURE(RBD_FEATURE_EXCLUSIVE_LOCK); + + librbd::ImageCtx *ictx; + ASSERT_EQ(0, open_image(m_image_name, &ictx)); + + { + RWLock::WLocker l(ictx->owner_lock); + ASSERT_EQ(0, ictx->image_watcher->try_lock()); + ASSERT_TRUE(ictx->image_watcher->is_lock_owner()); + } + + uint64_t size; + ASSERT_EQ(0, librbd::get_size(ictx, &size)); + + uint32_t attempts = 0; + while (attempts++ < 20 && size > 0) { + C_SaferCond ctx; + librbd::NoOpProgressContext prog_ctx; + + size -= MIN(size, 1<<18); + { + RWLock::RLocker l(ictx->owner_lock); + ASSERT_EQ(0, librbd::async_resize(ictx, &ctx, size, prog_ctx)); + } + + // try to interrupt the in-progress resize + ictx->cancel_async_requests(); + + int r = ctx.wait(); + if (r == -ERESTART) { + std::cout << "detected canceled async request" << std::endl; + break; + } + ASSERT_EQ(0, r); + } +} -- 2.47.3