librbd: ImageWatcher shouldn't block the notification thread

Now that all RPC operations are asynchronous, 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>

diff --git a/src/librbd/ImageWatcher.cc b/src/librbd/ImageWatcher.cc
index 62cf77f263074a4aa063fb7181f06c4caf6c4e8b..30dca71acafc2fa6b693f3b73a3c7339b102b105 100644 (file)
--- a/src/librbd/ImageWatcher.cc
+++ b/src/librbd/ImageWatcher.cc
@@ -872,17 +872,18 @@ int ImageWatcher::prepare_async_request(const AsyncRequestId& async_request_id,
   return 0;
 }
 
-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;
 
@@ -902,10 +903,11 @@ void ImageWatcher::handle_payload(const AcquiredLockPayload &payload,
     }
     notify_listeners_updated_lock(LOCK_UPDATE_STATE_NOTIFICATION);
   }
+  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;
 
   bool cancel_async_requests = true;
@@ -928,24 +930,25 @@ void ImageWatcher::handle_payload(const ReleasedLockPayload &payload,
     }
     notify_listeners_updated_lock(LOCK_UPDATE_STATE_NOTIFICATION);
   }
+  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;
       }
     }
 
@@ -969,10 +972,11 @@ void ImageWatcher::handle_payload(const RequestLockPayload &payload,
       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);
@@ -984,10 +988,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);
@@ -997,10 +1002,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) {
@@ -1015,12 +1021,13 @@ void ImageWatcher::handle_payload(const FlattenPayload &payload,
       librbd::async_flatten(&m_image_ctx, ctx, *prog_ctx);
     }
 
-    ::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) {
     bool new_request;
@@ -1035,88 +1042,84 @@ void ImageWatcher::handle_payload(const ResizePayload &payload,
       librbd::async_resize(&m_image_ctx, ctx, payload.size, *prog_ctx);
     }
 
-    ::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;
-    C_SaferCond cond_ctx;
-    librbd::snap_create_helper(&m_image_ctx, &cond_ctx,
-                               payload.snap_name.c_str());
 
-    int r = cond_ctx.wait();
-    ::encode(ResponseMessage(r), *out);
+    librbd::snap_create_helper(&m_image_ctx, new C_ResponseMessage(ack_ctx),
+                               payload.snap_name.c_str());
+    return false;
   }
+  return true;
 }
 
-void ImageWatcher::handle_payload(const SnapRenamePayload &payload,
-                                 bufferlist *out) {
+bool ImageWatcher::handle_payload(const SnapRenamePayload &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_rename request: "
-                              << payload.snap_id << " to " 
+                              << payload.snap_id << " to "
                               << payload.snap_name << dendl;
-    C_SaferCond cond_ctx;
-    librbd::snap_rename_helper(&m_image_ctx, &cond_ctx,
-                               payload.snap_id,
-                               payload.snap_name.c_str());
 
-    int r = cond_ctx.wait();
-    ::encode(ResponseMessage(r), *out);
+    librbd::snap_rename_helper(&m_image_ctx, new C_ResponseMessage(ack_ctx),
+                               payload.snap_id, payload.snap_name.c_str());
+    return false;
   }
+  return true;
 }
-void ImageWatcher::handle_payload(const SnapRemovePayload &payload,
-                                 bufferlist *out) {
+
+bool ImageWatcher::handle_payload(const SnapRemovePayload &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_remove request: "
                               << payload.snap_name << dendl;
-    C_SaferCond cond_ctx;
-    librbd::snap_remove_helper(&m_image_ctx, &cond_ctx,
-                               payload.snap_name.c_str());
 
-    int r = cond_ctx.wait();
-    ::encode(ResponseMessage(r), *out);
+    librbd::snap_remove_helper(&m_image_ctx, new C_ResponseMessage(ack_ctx),
+                               payload.snap_name.c_str());
+    return false;
   }
+  return true;
 }
 
-void ImageWatcher::handle_payload(const SnapProtectPayload& payload,
-                                  bufferlist *out) {
+bool ImageWatcher::handle_payload(const SnapProtectPayload& payload,
+                                  C_NotifyAck *ack_ctx) {
   RWLock::RLocker owner_locker(m_image_ctx.owner_lock);
   if (m_lock_owner_state == LOCK_OWNER_STATE_LOCKED) {
     ldout(m_image_ctx.cct, 10) << this << " remote snap_protect request: "
                                << payload.snap_name << dendl;
-    C_SaferCond cond_ctx;
-    librbd::snap_protect_helper(&m_image_ctx, &cond_ctx,
-                                payload.snap_name.c_str());
 
-    int r = cond_ctx.wait();
-    ::encode(ResponseMessage(r), *out);
+    librbd::snap_protect_helper(&m_image_ctx, new C_ResponseMessage(ack_ctx),
+                                payload.snap_name.c_str());
+    return false;
   }
+  return true;
 }
 
-void ImageWatcher::handle_payload(const SnapUnprotectPayload& payload,
-                                  bufferlist *out) {
+bool ImageWatcher::handle_payload(const SnapUnprotectPayload& payload,
+                                  C_NotifyAck *ack_ctx) {
   RWLock::RLocker owner_locker(m_image_ctx.owner_lock);
   if (m_lock_owner_state == LOCK_OWNER_STATE_LOCKED) {
     ldout(m_image_ctx.cct, 10) << this << " remote snap_unprotect request: "
                                << payload.snap_name << dendl;
-    C_SaferCond cond_ctx;
-    librbd::snap_unprotect_helper(&m_image_ctx, &cond_ctx,
-                                  payload.snap_name.c_str());
 
-    int r = cond_ctx.wait();
-    ::encode(ResponseMessage(r), *out);
+    librbd::snap_unprotect_helper(&m_image_ctx, new C_ResponseMessage(ack_ctx),
+                                  payload.snap_name.c_str());
+    return false;
   }
+  return true;
 }
 
-void ImageWatcher::handle_payload(const RebuildObjectMapPayload& payload,
-                                  bufferlist *out) {
+bool ImageWatcher::handle_payload(const RebuildObjectMapPayload& payload,
+                                  C_NotifyAck *ack_ctx) {
   RWLock::RLocker l(m_image_ctx.owner_lock);
   if (m_lock_owner_state == LOCK_OWNER_STATE_LOCKED) {
     bool new_request;
@@ -1131,32 +1134,32 @@ void ImageWatcher::handle_payload(const RebuildObjectMapPayload& payload,
       librbd::async_rebuild_object_map(&m_image_ctx, ctx, *prog_ctx);
     }
 
-    ::encode(ResponseMessage(r), *out);
+    ::encode(ResponseMessage(r), ack_ctx->out);
   }
+  return true;
 }
 
-void ImageWatcher::handle_payload(const RenamePayload& payload,
-                                  bufferlist *out) {
+bool ImageWatcher::handle_payload(const RenamePayload& payload,
+                                  C_NotifyAck *ack_ctx) {
   RWLock::RLocker owner_locker(m_image_ctx.owner_lock);
   if (m_lock_owner_state == LOCK_OWNER_STATE_LOCKED) {
     ldout(m_image_ctx.cct, 10) << this << " remote rename request: "
                                << payload.image_name << dendl;
 
-    C_SaferCond cond_ctx;
-    librbd::rename_helper(&m_image_ctx, &cond_ctx,
+    librbd::rename_helper(&m_image_ctx, new C_ResponseMessage(ack_ctx),
                           payload.image_name.c_str());
-
-    int r = cond_ctx.wait();
-    ::encode(ResponseMessage(r), *out);
+    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,
@@ -1289,4 +1292,30 @@ void ImageWatcher::notify_listeners_updated_lock(
   m_listeners_cond.Signal();
 }
 
+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_ResponseMessage::finish(int r) {
+  CephContext *cct = notify_ack->image_watcher->m_image_ctx.cct;
+  ldout(cct, 10) << this << " C_ResponseMessage: r=" << r << dendl;
+
+  ::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 57227653d770e0ddf5aa8381d43db31bdb21912b..b54f48a926dd79cc8075dc10653d065962819ed8 100644 (file)
--- a/src/librbd/ImageWatcher.h
+++ b/src/librbd/ImageWatcher.h
@@ -181,6 +181,25 @@ private:
     ProgressContext *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_ResponseMessage : public Context {
+    C_NotifyAck *notify_ack;
+
+    C_ResponseMessage(C_NotifyAck *notify_ack) : notify_ack(notify_ack) {
+    }
+    virtual void finish(int r);
+  };
+
   struct HandlePayloadVisitor : public boost::static_visitor<void> {
     ImageWatcher *image_watcher;
     uint64_t notify_id;
@@ -192,17 +211,13 @@ private:
     {
     }
 
-    inline void operator()(const watch_notify::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);
+      }
     }
   };
 
@@ -272,38 +287,38 @@ private:
                             bool* new_request, Context** ctx,
                             ProgressContext** prog_ctx);
 
-  void handle_payload(const watch_notify::HeaderUpdatePayload& payload,
-                      bufferlist *out);
-  void handle_payload(const watch_notify::AcquiredLockPayload& payload,
-                      bufferlist *out);
-  void handle_payload(const watch_notify::ReleasedLockPayload& payload,
-                      bufferlist *out);
-  void handle_payload(const watch_notify::RequestLockPayload& payload,
-                      bufferlist *out);
-  void handle_payload(const watch_notify::AsyncProgressPayload& payload,
-                      bufferlist *out);
-  void handle_payload(const watch_notify::AsyncCompletePayload& payload,
-                      bufferlist *out);
-  void handle_payload(const watch_notify::FlattenPayload& payload,
-                      bufferlist *out);
-  void handle_payload(const watch_notify::ResizePayload& payload,
-                      bufferlist *out);
-  void handle_payload(const watch_notify::SnapCreatePayload& payload,
-                      bufferlist *out);
-  void handle_payload(const watch_notify::SnapRenamePayload& payload,
-                      bufferlist *out);
-  void handle_payload(const watch_notify::SnapRemovePayload& payload,
-                      bufferlist *out);
-  void handle_payload(const watch_notify::SnapProtectPayload& payload,
-                      bufferlist *out);
-  void handle_payload(const watch_notify::SnapUnprotectPayload& payload,
-                      bufferlist *out);
-  void handle_payload(const watch_notify::RebuildObjectMapPayload& payload,
-                      bufferlist *out);
-  void handle_payload(const watch_notify::RenamePayload& payload,
-                      bufferlist *out);
-  void handle_payload(const watch_notify::UnknownPayload& payload,
-                      bufferlist *out);
+  bool handle_payload(const watch_notify::HeaderUpdatePayload& payload,
+                      C_NotifyAck *ctx);
+  bool handle_payload(const watch_notify::AcquiredLockPayload& payload,
+                      C_NotifyAck *ctx);
+  bool handle_payload(const watch_notify::ReleasedLockPayload& payload,
+                      C_NotifyAck *ctx);
+  bool handle_payload(const watch_notify::RequestLockPayload& payload,
+                      C_NotifyAck *ctx);
+  bool handle_payload(const watch_notify::AsyncProgressPayload& payload,
+                      C_NotifyAck *ctx);
+  bool handle_payload(const watch_notify::AsyncCompletePayload& payload,
+                      C_NotifyAck *ctx);
+  bool handle_payload(const watch_notify::FlattenPayload& payload,
+                      C_NotifyAck *ctx);
+  bool handle_payload(const watch_notify::ResizePayload& payload,
+                      C_NotifyAck *ctx);
+  bool handle_payload(const watch_notify::SnapCreatePayload& payload,
+                      C_NotifyAck *ctx);
+  bool handle_payload(const watch_notify::SnapRenamePayload& payload,
+                      C_NotifyAck *ctx);
+  bool handle_payload(const watch_notify::SnapRemovePayload& payload,
+                      C_NotifyAck *ctx);
+  bool handle_payload(const watch_notify::SnapProtectPayload& payload,
+                      C_NotifyAck *ctx);
+  bool handle_payload(const watch_notify::SnapUnprotectPayload& payload,
+                      C_NotifyAck *ctx);
+  bool handle_payload(const watch_notify::RebuildObjectMapPayload& payload,
+                      C_NotifyAck *ctx);
+  bool handle_payload(const watch_notify::RenamePayload& payload,
+                      C_NotifyAck *ctx);
+  bool handle_payload(const watch_notify::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);