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 <dillaman@redhat.com>
(cherry picked from commit 7e2019a72733dff43e55c9b22df12939d584f87d)

 Conflicts:
	src/librbd/ImageWatcher.[cc|h]: fewer RPC messages

diff --git a/src/librbd/ImageWatcher.cc b/src/librbd/ImageWatcher.cc
index 71b4c863d0b2f2a9411134a99f5875491dcdf497..790a03665a7559255e3ca925aff0033b33d0b91e 100644 (file)
--- 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 <sstream>
 #include <boost/bind.hpp>
 #include <boost/function.hpp>
@@ -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<AsyncRequestId, AsyncRequest>::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<AsyncRequestId, AsyncRequest>::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 760a6981b3feb3d8e44fe6c6a16a8aee0c49745a..6fbc48869258d0b8ddb2af8fdf32c95d96614995 100644 (file)
--- 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<void> {
       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 <typename Payload>
       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);