Merge pull request #12838 from dillaman/wip-18419

librbd: possible deadlock with flush if refresh in-progress Reviewed-by: Mykola Golub <mgolub@mirantis.com>
ceph · Jan 11, 2017 · 31f76d1 · 31f76d1
2 parents e0da839 + b95f92a
commit 31f76d1
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 5 deletions.
diff --git a/src/librbd/ImageState.cc b/src/librbd/ImageState.cc
@@ -304,7 +304,7 @@ void ImageState<I>::handle_update_notification() {
 template <typename I>
 bool ImageState<I>::is_refresh_required() const {
   Mutex::Locker locker(m_lock);
-  return (m_last_refresh != m_refresh_seq);
+  return (m_last_refresh != m_refresh_seq || find_pending_refresh() != nullptr);
 }
 
 template <typename I>
@@ -336,22 +336,43 @@ int ImageState<I>::refresh_if_required() {
   C_SaferCond ctx;
   {
     m_lock.Lock();
-    if (m_last_refresh == m_refresh_seq) {
+    Action action(ACTION_TYPE_REFRESH);
+    action.refresh_seq = m_refresh_seq;
+
+    auto refresh_action = find_pending_refresh();
+    if (refresh_action != nullptr) {
+      // if a refresh is in-flight, delay until it is finished
+      action = *refresh_action;
+    } else if (m_last_refresh == m_refresh_seq) {
       m_lock.Unlock();
       return 0;
     } else if (is_closed()) {
       m_lock.Unlock();
       return -ESHUTDOWN;
     }
 
-    Action action(ACTION_TYPE_REFRESH);
-    action.refresh_seq = m_refresh_seq;
     execute_action_unlock(action, &ctx);
   }
 
   return ctx.wait();
 }
 
+template <typename I>
+const typename ImageState<I>::Action *
+ImageState<I>::find_pending_refresh() const {
+  assert(m_lock.is_locked());
+
+  auto it = std::find_if(m_actions_contexts.rbegin(),
+                         m_actions_contexts.rend(),
+                         [](const ActionContexts& action_contexts) {
+      return (action_contexts.first == ACTION_TYPE_REFRESH);
+    });
+  if (it != m_actions_contexts.rend()) {
+    return &it->first;
+  }
+  return nullptr;
+}
+
 template <typename I>
 void ImageState<I>::snap_set(const std::string &snap_name, Context *on_finish) {
   CephContext *cct = m_image_ctx->cct;

diff --git a/src/librbd/ImageState.h b/src/librbd/ImageState.h
@@ -112,6 +112,8 @@ class ImageState {
   bool is_transition_state() const;
   bool is_closed() const;
 
+  const Action *find_pending_refresh() const;
+
   void append_context(const Action &action, Context *context);
   void execute_next_action_unlock();
   void execute_action_unlock(const Action &action, Context *context);

diff --git a/src/librbd/internal.cc b/src/librbd/internal.cc
@@ -2410,10 +2410,13 @@ void filter_out_mirror_watchers(ImageCtx *ictx,
     }
 
     ictx->user_flushed();
+    C_SaferCond ctx;
     {
       RWLock::RLocker owner_locker(ictx->owner_lock);
-      r = ictx->flush();
+      ictx->flush(&ctx);
     }
+    r = ctx.wait();
+
     ictx->perfcounter->inc(l_librbd_flush);
     return r;
   }