From 73e03def9271fb5d1739b195e428c3ebfcebd59b Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Thu, 28 Jan 2016 14:38:20 -0500 Subject: [PATCH] librbd: ImageWatcher shouldn't block the notification thread Blocking the notification thread will also result in librados async callbacks becoming blocked (since they use the same thread). Signed-off-by: Jason Dillaman (cherry picked from commit 7e2019a72733dff43e55c9b22df12939d584f87d) Conflicts: src/librbd/ImageWatcher.[cc|h]: fewer RPC messages --- src/librbd/ImageWatcher.cc | 104 ++++++++++++++++++++++++++----------- src/librbd/ImageWatcher.h | 78 +++++++++++++++++----------- 2 files changed, 122 insertions(+), 60 deletions(-) diff --git a/src/librbd/ImageWatcher.cc b/src/librbd/ImageWatcher.cc index 71b4c863d0b2f..790a03665a755 100644 --- a/src/librbd/ImageWatcher.cc +++ b/src/librbd/ImageWatcher.cc @@ -11,6 +11,7 @@ #include "include/encoding.h" #include "include/stringify.h" #include "common/errno.h" +#include "common/WorkQueue.h" #include #include #include @@ -741,24 +742,25 @@ int ImageWatcher::notify_async_request(const AsyncRequestId &async_request_id, return ctx.wait(); } -void ImageWatcher::handle_payload(const HeaderUpdatePayload &payload, - bufferlist *out) { +bool ImageWatcher::handle_payload(const HeaderUpdatePayload &payload, + C_NotifyAck *ack_ctx) { ldout(m_image_ctx.cct, 10) << this << " image header updated" << dendl; Mutex::Locker lictx(m_image_ctx.refresh_lock); ++m_image_ctx.refresh_seq; m_image_ctx.perfcounter->inc(l_librbd_notify); + return true; } -void ImageWatcher::handle_payload(const AcquiredLockPayload &payload, - bufferlist *out) { +bool ImageWatcher::handle_payload(const AcquiredLockPayload &payload, + C_NotifyAck *ack_ctx) { ldout(m_image_ctx.cct, 10) << this << " image exclusively locked announcement" << dendl; if (payload.client_id.is_valid()) { Mutex::Locker l(m_owner_client_id_lock); if (payload.client_id == m_owner_client_id) { // we already know that the remote client is the owner - return; + return true; } set_owner_client_id(payload.client_id); } @@ -768,10 +770,11 @@ void ImageWatcher::handle_payload(const AcquiredLockPayload &payload, schedule_cancel_async_requests(); schedule_retry_aio_requests(false); } + return true; } -void ImageWatcher::handle_payload(const ReleasedLockPayload &payload, - bufferlist *out) { +bool ImageWatcher::handle_payload(const ReleasedLockPayload &payload, + C_NotifyAck *ack_ctx) { ldout(m_image_ctx.cct, 10) << this << " exclusive lock released" << dendl; if (payload.client_id.is_valid()) { Mutex::Locker l(m_owner_client_id_lock); @@ -779,7 +782,7 @@ void ImageWatcher::handle_payload(const ReleasedLockPayload &payload, ldout(m_image_ctx.cct, 10) << this << " unexpected owner: " << payload.client_id << " != " << m_owner_client_id << dendl; - return; + return true; } set_owner_client_id(ClientId()); } @@ -789,24 +792,25 @@ void ImageWatcher::handle_payload(const ReleasedLockPayload &payload, schedule_cancel_async_requests(); schedule_retry_aio_requests(false); } + return true; } -void ImageWatcher::handle_payload(const RequestLockPayload &payload, - bufferlist *out) { +bool ImageWatcher::handle_payload(const RequestLockPayload &payload, + C_NotifyAck *ack_ctx) { ldout(m_image_ctx.cct, 10) << this << " exclusive lock requested" << dendl; if (payload.client_id == get_client_id()) { - return; + return true; } RWLock::RLocker l(m_image_ctx.owner_lock); if (m_lock_owner_state == LOCK_OWNER_STATE_LOCKED) { // need to send something back so the client can detect a missing leader - ::encode(ResponseMessage(0), *out); + ::encode(ResponseMessage(0), ack_ctx->out); { Mutex::Locker l(m_owner_client_id_lock); if (!m_owner_client_id.is_valid()) { - return; + return true; } } @@ -816,10 +820,11 @@ void ImageWatcher::handle_payload(const RequestLockPayload &payload, boost::bind(&ImageWatcher::notify_release_lock, this)); m_task_finisher->queue(TASK_CODE_RELEASING_LOCK, ctx); } + return true; } -void ImageWatcher::handle_payload(const AsyncProgressPayload &payload, - bufferlist *out) { +bool ImageWatcher::handle_payload(const AsyncProgressPayload &payload, + C_NotifyAck *ack_ctx) { RWLock::RLocker l(m_async_request_lock); std::map::iterator req_it = m_async_requests.find(payload.async_request_id); @@ -831,10 +836,11 @@ void ImageWatcher::handle_payload(const AsyncProgressPayload &payload, schedule_async_request_timed_out(payload.async_request_id); req_it->second.second->update_progress(payload.offset, payload.total); } + return true; } -void ImageWatcher::handle_payload(const AsyncCompletePayload &payload, - bufferlist *out) { +bool ImageWatcher::handle_payload(const AsyncCompletePayload &payload, + C_NotifyAck *ack_ctx) { RWLock::RLocker l(m_async_request_lock); std::map::iterator req_it = m_async_requests.find(payload.async_request_id); @@ -844,10 +850,11 @@ void ImageWatcher::handle_payload(const AsyncCompletePayload &payload, << payload.result << dendl; req_it->second.first->complete(payload.result); } + return true; } -void ImageWatcher::handle_payload(const FlattenPayload &payload, - bufferlist *out) { +bool ImageWatcher::handle_payload(const FlattenPayload &payload, + C_NotifyAck *ack_ctx) { RWLock::RLocker l(m_image_ctx.owner_lock); if (m_lock_owner_state == LOCK_OWNER_STATE_LOCKED) { @@ -882,12 +889,13 @@ void ImageWatcher::handle_payload(const FlattenPayload &payload, } } - ::encode(ResponseMessage(r), *out); + ::encode(ResponseMessage(r), ack_ctx->out); } + return true; } -void ImageWatcher::handle_payload(const ResizePayload &payload, - bufferlist *out) { +bool ImageWatcher::handle_payload(const ResizePayload &payload, + C_NotifyAck *ack_ctx) { RWLock::RLocker l(m_image_ctx.owner_lock); if (m_lock_owner_state == LOCK_OWNER_STATE_LOCKED) { int r = 0; @@ -922,29 +930,33 @@ void ImageWatcher::handle_payload(const ResizePayload &payload, } } - ::encode(ResponseMessage(r), *out); + ::encode(ResponseMessage(r), ack_ctx->out); } + return true; } -void ImageWatcher::handle_payload(const SnapCreatePayload &payload, - bufferlist *out) { +bool ImageWatcher::handle_payload(const SnapCreatePayload &payload, + C_NotifyAck *ack_ctx) { RWLock::RLocker l(m_image_ctx.owner_lock); if (m_lock_owner_state == LOCK_OWNER_STATE_LOCKED) { ldout(m_image_ctx.cct, 10) << this << " remote snap_create request: " << payload.snap_name << dendl; - int r = librbd::snap_create_helper(&m_image_ctx, NULL, - payload.snap_name.c_str()); - ::encode(ResponseMessage(r), *out); + // execute outside of librados AIO thread + m_image_ctx.op_work_queue->queue(new C_SnapCreateResponseMessage( + this, ack_ctx, payload.snap_name), 0); + return false; } + return true; } -void ImageWatcher::handle_payload(const UnknownPayload &payload, - bufferlist *out) { +bool ImageWatcher::handle_payload(const UnknownPayload &payload, + C_NotifyAck *ack_ctx) { RWLock::RLocker l(m_image_ctx.owner_lock); if (is_lock_owner()) { - ::encode(ResponseMessage(-EOPNOTSUPP), *out); + ::encode(ResponseMessage(-EOPNOTSUPP), ack_ctx->out); } + return true; } void ImageWatcher::handle_notify(uint64_t notify_id, uint64_t handle, @@ -1060,4 +1072,34 @@ void ImageWatcher::RemoteContext::finish(int r) { m_image_watcher.schedule_async_complete(m_async_request_id, r); } +ImageWatcher::C_NotifyAck::C_NotifyAck(ImageWatcher *image_watcher, + uint64_t notify_id, uint64_t handle) + : image_watcher(image_watcher), notify_id(notify_id), handle(handle) { + CephContext *cct = image_watcher->m_image_ctx.cct; + ldout(cct, 10) << this << " C_NotifyAck start: id=" << notify_id << ", " + << "handle=" << handle << dendl; +} + +void ImageWatcher::C_NotifyAck::finish(int r) { + assert(r == 0); + CephContext *cct = image_watcher->m_image_ctx.cct; + ldout(cct, 10) << this << " C_NotifyAck finish: id=" << notify_id << ", " + << "handle=" << handle << dendl; + + image_watcher->acknowledge_notify(notify_id, handle, out); +} + +void ImageWatcher::C_SnapCreateResponseMessage::finish(int r) { + CephContext *cct = notify_ack->image_watcher->m_image_ctx.cct; + ldout(cct, 10) << this << " C_SnapCreateResponseMessage: r=" << r << dendl; + + RWLock::RLocker owner_locker(image_watcher->m_image_ctx.owner_lock); + if (image_watcher->m_lock_owner_state == LOCK_OWNER_STATE_LOCKED) { + r = librbd::snap_create_helper(&image_watcher->m_image_ctx, NULL, + snap_name.c_str()); + ::encode(ResponseMessage(r), notify_ack->out); + } + notify_ack->complete(0); } + +} // namespace librbd diff --git a/src/librbd/ImageWatcher.h b/src/librbd/ImageWatcher.h index 760a6981b3feb..6fbc48869258d 100644 --- a/src/librbd/ImageWatcher.h +++ b/src/librbd/ImageWatcher.h @@ -158,6 +158,31 @@ namespace librbd { RemoteProgressContext *m_prog_ctx; }; + struct C_NotifyAck : public Context { + ImageWatcher *image_watcher; + uint64_t notify_id; + uint64_t handle; + bufferlist out; + + C_NotifyAck(ImageWatcher *image_watcher, uint64_t notify_id, + uint64_t handle); + virtual void finish(int r); + }; + + struct C_SnapCreateResponseMessage : public Context { + ImageWatcher *image_watcher; + C_NotifyAck *notify_ack; + std::string snap_name; + + C_SnapCreateResponseMessage(ImageWatcher *image_watcher, + C_NotifyAck *notify_ack, + const std::string &snap_name) + : image_watcher(image_watcher), notify_ack(notify_ack), + snap_name(snap_name) { + } + virtual void finish(int r); + }; + struct HandlePayloadVisitor : public boost::static_visitor { ImageWatcher *image_watcher; uint64_t notify_id; @@ -169,17 +194,12 @@ namespace librbd { { } - inline void operator()(const WatchNotify::HeaderUpdatePayload &payload) const { - bufferlist out; - image_watcher->handle_payload(payload, &out); - image_watcher->acknowledge_notify(notify_id, handle, out); - } - template inline void operator()(const Payload &payload) const { - bufferlist out; - image_watcher->handle_payload(payload, &out); - image_watcher->acknowledge_notify(notify_id, handle, out); + C_NotifyAck *ctx = new C_NotifyAck(image_watcher, notify_id, handle); + if (image_watcher->handle_payload(payload, ctx)) { + ctx->complete(0); + } } }; @@ -242,26 +262,26 @@ namespace librbd { int notify_async_complete(const WatchNotify::AsyncRequestId &id, int r); - void handle_payload(const WatchNotify::HeaderUpdatePayload& payload, - bufferlist *out); - void handle_payload(const WatchNotify::AcquiredLockPayload& payload, - bufferlist *out); - void handle_payload(const WatchNotify::ReleasedLockPayload& payload, - bufferlist *out); - void handle_payload(const WatchNotify::RequestLockPayload& payload, - bufferlist *out); - void handle_payload(const WatchNotify::AsyncProgressPayload& payload, - bufferlist *out); - void handle_payload(const WatchNotify::AsyncCompletePayload& payload, - bufferlist *out); - void handle_payload(const WatchNotify::FlattenPayload& payload, - bufferlist *out); - void handle_payload(const WatchNotify::ResizePayload& payload, - bufferlist *out); - void handle_payload(const WatchNotify::SnapCreatePayload& payload, - bufferlist *out); - void handle_payload(const WatchNotify::UnknownPayload& payload, - bufferlist *out); + bool handle_payload(const WatchNotify::HeaderUpdatePayload& payload, + C_NotifyAck *ctx); + bool handle_payload(const WatchNotify::AcquiredLockPayload& payload, + C_NotifyAck *ctx); + bool handle_payload(const WatchNotify::ReleasedLockPayload& payload, + C_NotifyAck *ctx); + bool handle_payload(const WatchNotify::RequestLockPayload& payload, + C_NotifyAck *ctx); + bool handle_payload(const WatchNotify::AsyncProgressPayload& payload, + C_NotifyAck *ctx); + bool handle_payload(const WatchNotify::AsyncCompletePayload& payload, + C_NotifyAck *ctx); + bool handle_payload(const WatchNotify::FlattenPayload& payload, + C_NotifyAck *ctx); + bool handle_payload(const WatchNotify::ResizePayload& payload, + C_NotifyAck *ctx); + bool handle_payload(const WatchNotify::SnapCreatePayload& payload, + C_NotifyAck *ctx); + bool handle_payload(const WatchNotify::UnknownPayload& payload, + C_NotifyAck *ctx); void handle_notify(uint64_t notify_id, uint64_t handle, bufferlist &bl); void handle_error(uint64_t cookie, int err); -- 2.39.5