librbd: delay invalidation of exclusive lock pointer

Certain IO and maintenance ops code paths have an expectation
that the exclusive lock pointer will be valid while in-flight.
Let the exclusive lock state machine clean up the pointer after
it has flushed all IO and canceled all ops.

Fixes: http://tracker.ceph.com/issues/15471
Signed-off-by: Jason Dillaman <dillaman@redhat.com>

diff --git a/src/librbd/ExclusiveLock.cc b/src/librbd/ExclusiveLock.cc
index 82d00425e0bd89699a7ab0047496d6330f309439..5c065fe4428a6702d0c55cacd9fcc848c0c354e3 100644 (file)
--- a/src/librbd/ExclusiveLock.cc
+++ b/src/librbd/ExclusiveLock.cc
@@ -111,28 +111,31 @@ void ExclusiveLock<I>::shut_down(Context *on_shut_down) {
 
 template <typename I>
 void ExclusiveLock<I>::try_lock(Context *on_tried_lock) {
+  int r = 0;
   {
     Mutex::Locker locker(m_lock);
     assert(m_image_ctx.owner_lock.is_locked());
-    assert(!is_shutdown());
-
-    if (m_state != STATE_LOCKED || !m_actions_contexts.empty()) {
+    if (is_shutdown()) {
+      r = -ESHUTDOWN;
+    } else if (m_state != STATE_LOCKED || !m_actions_contexts.empty()) {
       ldout(m_image_ctx.cct, 10) << this << " " << __func__ << dendl;
       execute_action(ACTION_TRY_LOCK, on_tried_lock);
       return;
     }
   }
 
-  on_tried_lock->complete(0);
+  on_tried_lock->complete(r);
 }
 
 template <typename I>
 void ExclusiveLock<I>::request_lock(Context *on_locked) {
+  int r = 0;
   {
     Mutex::Locker locker(m_lock);
     assert(m_image_ctx.owner_lock.is_locked());
-    assert(!is_shutdown());
-    if (m_state != STATE_LOCKED || !m_actions_contexts.empty()) {
+    if (is_shutdown()) {
+      r = -ESHUTDOWN;
+    } else if (m_state != STATE_LOCKED || !m_actions_contexts.empty()) {
       ldout(m_image_ctx.cct, 10) << this << " " << __func__ << dendl;
       execute_action(ACTION_REQUEST_LOCK, on_locked);
       return;
@@ -140,25 +143,26 @@ void ExclusiveLock<I>::request_lock(Context *on_locked) {
   }
 
   if (on_locked != nullptr) {
-    on_locked->complete(0);
+    on_locked->complete(r);
   }
 }
 
 template <typename I>
 void ExclusiveLock<I>::release_lock(Context *on_released) {
+  int r = 0;
   {
     Mutex::Locker locker(m_lock);
     assert(m_image_ctx.owner_lock.is_locked());
-    assert(!is_shutdown());
-
-    if (m_state != STATE_UNLOCKED || !m_actions_contexts.empty()) {
+    if (is_shutdown()) {
+      r = -ESHUTDOWN;
+    } else if (m_state != STATE_UNLOCKED || !m_actions_contexts.empty()) {
       ldout(m_image_ctx.cct, 10) << this << " " << __func__ << dendl;
       execute_action(ACTION_RELEASE_LOCK, on_released);
       return;
     }
   }
 
-  on_released->complete(0);
+  on_released->complete(r);
 }
 
 template <typename I>
@@ -466,10 +470,8 @@ void ExclusiveLock<I>::send_shutdown() {
   assert(m_lock.is_locked());
   if (m_state == STATE_UNLOCKED) {
     m_state = STATE_SHUTTING_DOWN;
-    m_image_ctx.aio_work_queue->clear_require_lock_on_read();
-    m_image_ctx.aio_work_queue->unblock_writes();
-    m_image_ctx.image_watcher->flush(util::create_context_callback<
-      ExclusiveLock<I>, &ExclusiveLock<I>::complete_shutdown>(this));
+    m_image_ctx.op_work_queue->queue(util::create_context_callback<
+      ExclusiveLock<I>, &ExclusiveLock<I>::handle_unlocked_shutdown>(this), 0);
     return;
   }
 
@@ -493,15 +495,20 @@ void ExclusiveLock<I>::send_shutdown_release() {
   using el = ExclusiveLock<I>;
   ReleaseRequest<I>* req = ReleaseRequest<I>::create(
     m_image_ctx, cookie, nullptr,
-    util::create_context_callback<el, &el::handle_shutdown>(this));
+    util::create_context_callback<el, &el::handle_locked_shutdown>(this));
   req->send();
 }
 
 template <typename I>
-void ExclusiveLock<I>::handle_shutdown(int r) {
+void ExclusiveLock<I>::handle_locked_shutdown(int r) {
   CephContext *cct = m_image_ctx.cct;
   ldout(cct, 10) << this << " " << __func__ << ": r=" << r << dendl;
 
+  {
+    RWLock::WLocker owner_locker(m_image_ctx.owner_lock);
+    m_image_ctx.exclusive_lock = nullptr;
+  }
+
   if (r < 0) {
     lderr(cct) << "failed to shut down exclusive lock: " << cpp_strerror(r)
                << dendl;
@@ -514,6 +521,22 @@ void ExclusiveLock<I>::handle_shutdown(int r) {
   complete_shutdown(r);
 }
 
+template <typename I>
+void ExclusiveLock<I>::handle_unlocked_shutdown(int r) {
+  CephContext *cct = m_image_ctx.cct;
+  ldout(cct, 10) << this << " " << __func__ << ": r=" << r << dendl;
+
+  {
+    RWLock::WLocker owner_locker(m_image_ctx.owner_lock);
+    m_image_ctx.exclusive_lock = nullptr;
+  }
+
+  m_image_ctx.aio_work_queue->clear_require_lock_on_read();
+  m_image_ctx.aio_work_queue->unblock_writes();
+  m_image_ctx.image_watcher->flush(util::create_context_callback<
+    ExclusiveLock<I>, &ExclusiveLock<I>::complete_shutdown>(this));
+}
+
 template <typename I>
 void ExclusiveLock<I>::complete_shutdown(int r) {
   ActionContexts action_contexts;

diff --git a/src/librbd/ExclusiveLock.h b/src/librbd/ExclusiveLock.h
index 910c67f2df59f6e3634b2e98306831a09256d175..c547372dd1af316787f5034c3ab9c56888457230 100644 (file)
--- a/src/librbd/ExclusiveLock.h
+++ b/src/librbd/ExclusiveLock.h
@@ -151,7 +151,8 @@ private:
 
   void send_shutdown();
   void send_shutdown_release();
-  void handle_shutdown(int r);
+  void handle_locked_shutdown(int r);
+  void handle_unlocked_shutdown(int r);
   void complete_shutdown(int r);
 };
 

diff --git a/src/librbd/image/CloseRequest.cc b/src/librbd/image/CloseRequest.cc
index 55e25ab37648d222207ca110462392895b1e853f..b953860f5122fb8d9bda313eade916ebe0259893 100644 (file)
--- a/src/librbd/image/CloseRequest.cc
+++ b/src/librbd/image/CloseRequest.cc
@@ -87,9 +87,10 @@ template <typename I>
 void CloseRequest<I>::send_shut_down_exclusive_lock() {
   {
     RWLock::WLocker owner_locker(m_image_ctx->owner_lock);
-    RWLock::WLocker snap_locker(m_image_ctx->snap_lock);
-    std::swap(m_exclusive_lock, m_image_ctx->exclusive_lock);
+    m_exclusive_lock = m_image_ctx->exclusive_lock;
 
+    // if reading a snapshot -- possible object map is open
+    RWLock::WLocker snap_locker(m_image_ctx->snap_lock);
     if (m_exclusive_lock == nullptr) {
       delete m_image_ctx->object_map;
       m_image_ctx->object_map = nullptr;
@@ -104,6 +105,8 @@ void CloseRequest<I>::send_shut_down_exclusive_lock() {
   CephContext *cct = m_image_ctx->cct;
   ldout(cct, 10) << this << " " << __func__ << dendl;
 
+  // in-flight IO will be flushed and in-flight requests will be canceled
+  // before releasing lock
   m_exclusive_lock->shut_down(create_context_callback<
     CloseRequest<I>, &CloseRequest<I>::handle_shut_down_exclusive_lock>(this));
 }
@@ -113,10 +116,18 @@ void CloseRequest<I>::handle_shut_down_exclusive_lock(int r) {
   CephContext *cct = m_image_ctx->cct;
   ldout(cct, 10) << this << " " << __func__ << ": r=" << r << dendl;
 
-  // object map and journal closed during exclusive lock shutdown
-  assert(m_image_ctx->journal == nullptr);
-  assert(m_image_ctx->object_map == nullptr);
+  {
+    RWLock::RLocker owner_locker(m_image_ctx->owner_lock);
+    assert(m_image_ctx->exclusive_lock == nullptr);
+
+    // object map and journal closed during exclusive lock shutdown
+    RWLock::RLocker snap_locker(m_image_ctx->snap_lock);
+    assert(m_image_ctx->journal == nullptr);
+    assert(m_image_ctx->object_map == nullptr);
+  }
+
   delete m_exclusive_lock;
+  m_exclusive_lock = nullptr;
 
   save_result(r);
   if (r < 0) {

diff --git a/src/librbd/image/RefreshRequest.cc b/src/librbd/image/RefreshRequest.cc
index 12bee84493d025ba56d5bf4aa96d5d321accf5f0..5894f5032f22cd46888d180394aecb49fae6d853 100644 (file)
--- a/src/librbd/image/RefreshRequest.cc
+++ b/src/librbd/image/RefreshRequest.cc
@@ -581,7 +581,8 @@ Context *RefreshRequest<I>::send_v2_shut_down_exclusive_lock() {
   CephContext *cct = m_image_ctx.cct;
   ldout(cct, 10) << this << " " << __func__ << dendl;
 
-  // exclusive lock feature was dynamically disabled
+  // exclusive lock feature was dynamically disabled. in-flight IO will be
+  // flushed and in-flight requests will be canceled before releasing lock
   using klass = RefreshRequest<I>;
   Context *ctx = create_context_callback<
     klass, &klass::handle_v2_shut_down_exclusive_lock>(this);
@@ -600,6 +601,11 @@ Context *RefreshRequest<I>::handle_v2_shut_down_exclusive_lock(int *result) {
     save_result(result);
   }
 
+  {
+    RWLock::WLocker owner_locker(m_image_ctx.owner_lock);
+    assert(m_image_ctx.exclusive_lock == nullptr);
+  }
+
   assert(m_exclusive_lock != nullptr);
   delete m_exclusive_lock;
   m_exclusive_lock = nullptr;
@@ -784,9 +790,11 @@ void RefreshRequest<I>::apply() {
                                    m_image_ctx.snap_lock)) {
       // disabling exclusive lock will automatically handle closing
       // object map and journaling
-      std::swap(m_exclusive_lock, m_image_ctx.exclusive_lock);
+      assert(m_exclusive_lock == nullptr);
+      m_exclusive_lock = m_image_ctx.exclusive_lock;
     } else {
       if (m_exclusive_lock != nullptr) {
+        assert(m_image_ctx.exclusive_lock == nullptr);
         std::swap(m_exclusive_lock, m_image_ctx.exclusive_lock);
       }
       if (!m_image_ctx.test_features(RBD_FEATURE_JOURNALING,

diff --git a/src/test/librbd/image/test_mock_RefreshRequest.cc b/src/test/librbd/image/test_mock_RefreshRequest.cc
index e362986ac632cefd77f15aaf91bf118f2adc4d9e..e6c5912834e72f58c6622080d52f959c489dafc6 100644 (file)
--- a/src/test/librbd/image/test_mock_RefreshRequest.cc
+++ b/src/test/librbd/image/test_mock_RefreshRequest.cc
@@ -75,6 +75,7 @@ ACTION_P(TestFeatures, image_ctx) {
 
 ACTION_P(ShutDownExclusiveLock, image_ctx) {
   // shutting down exclusive lock will close object map and journal
+  image_ctx->exclusive_lock = nullptr;
   image_ctx->object_map = nullptr;
   image_ctx->journal = nullptr;
 }