librbd: prevent assertion failure when journal IO is blacklisted

Fixes: http://tracker.ceph.com/issues/18429
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit c720f6e3704ed7e8cf41dffdb931dbb05d59a003)

diff --git a/src/librbd/ImageCtx.cc b/src/librbd/ImageCtx.cc
index d972edf757b5d3c41f20a29f5aaa1862309170f4..2e867ab65cb4a253c5051869740193cb7b8dfde3 100644 (file)
--- a/src/librbd/ImageCtx.cc
+++ b/src/librbd/ImageCtx.cc
@@ -138,7 +138,9 @@ struct C_InvalidateCache : public Context {
     } else {
       lderr(cct) << "could not release all objects from cache: "
                  << unclean << " bytes remain" << dendl;
-      r = -EBUSY;
+      if (r == 0) {
+        r = -EBUSY;
+      }
     }
 
     if (reentrant_safe) {
@@ -789,7 +791,7 @@ struct C_InvalidateCache : public Context {
     return result;
   }
 
-  void ImageCtx::invalidate_cache(Context *on_finish) {
+  void ImageCtx::invalidate_cache(bool purge_on_error, Context *on_finish) {
     if (object_cacher == NULL) {
       op_work_queue->queue(on_finish, 0);
       return;
@@ -799,7 +801,7 @@ struct C_InvalidateCache : public Context {
     object_cacher->release_set(object_set);
     cache_lock.Unlock();
 
-    flush_cache(new C_InvalidateCache(this, false, false, on_finish));
+    flush_cache(new C_InvalidateCache(this, purge_on_error, false, on_finish));
   }
 
   void ImageCtx::clear_nonexistence_cache() {
@@ -809,6 +811,11 @@ struct C_InvalidateCache : public Context {
     object_cacher->clear_nonexistence(object_set);
   }
 
+  bool ImageCtx::is_cache_empty() {
+    Mutex::Locker locker(cache_lock);
+    return object_cacher->set_is_empty(object_set);
+  }
+
   void ImageCtx::register_watch(Context *on_finish) {
     assert(image_watcher == NULL);
     image_watcher = new ImageWatcher<>(*this);

diff --git a/src/librbd/ImageCtx.h b/src/librbd/ImageCtx.h
index 52cf2847a5691ef7c3a8a130d56c9ac4d9c3a029..de8232ddc25f6f03aeae4fdf480505dcc617ab57 100644 (file)
--- a/src/librbd/ImageCtx.h
+++ b/src/librbd/ImageCtx.h
@@ -277,9 +277,10 @@ namespace librbd {
     void user_flushed();
     void flush_cache(Context *onfinish);
     void shut_down_cache(Context *on_finish);
-    int invalidate_cache(bool purge_on_error=false);
-    void invalidate_cache(Context *on_finish);
+    int invalidate_cache(bool purge_on_error);
+    void invalidate_cache(bool purge_on_error, Context *on_finish);
     void clear_nonexistence_cache();
+    bool is_cache_empty();
     void register_watch(Context *on_finish);
     uint64_t prune_parent_extents(vector<pair<uint64_t,uint64_t> >& objectx,
                                  uint64_t overlap);

diff --git a/src/librbd/LibrbdWriteback.cc b/src/librbd/LibrbdWriteback.cc
index 3b56ed282424e78c9dd55af8222ca1f40931d0e6..8a6244d79f545c406c0ac02472f7258c453251f4 100644 (file)
--- a/src/librbd/LibrbdWriteback.cc
+++ b/src/librbd/LibrbdWriteback.cc
@@ -118,7 +118,12 @@ namespace librbd {
 
     virtual void complete(int r) {
       if (request_sent || r < 0) {
-        commit_io_event_extent(r);
+        if (request_sent && r == 0) {
+          // only commit IO events that are safely recorded to the backing image
+          // since the cache will retry all IOs that fail
+          commit_io_event_extent(0);
+        }
+
         req_comp->complete(r);
         delete this;
       } else {

diff --git a/src/librbd/exclusive_lock/ReleaseRequest.cc b/src/librbd/exclusive_lock/ReleaseRequest.cc
index dc496536cd782af3fa381a42a5643a1ba1ff0b18..c37cbc86456c8ba71db555a60508d101c98ae885 100644 (file)
--- a/src/librbd/exclusive_lock/ReleaseRequest.cc
+++ b/src/librbd/exclusive_lock/ReleaseRequest.cc
@@ -129,9 +129,52 @@ Context *ReleaseRequest<I>::handle_block_writes(int *ret_val) {
 
   if (*ret_val == -EBLACKLISTED) {
     // allow clean shut down if blacklisted
-    lderr(cct) << "failed to block writes: " << cpp_strerror(*ret_val) << dendl;
-    *ret_val = 0;
+    lderr(cct) << "failed to block writes because client is blacklisted"
+               << dendl;
   } else if (*ret_val < 0) {
+    lderr(cct) << "failed to block writes: " << cpp_strerror(*ret_val) << dendl;
+    m_image_ctx.aio_work_queue->unblock_writes();
+    return m_on_finish;
+  }
+
+  send_invalidate_cache(false);
+  return nullptr;
+}
+
+template <typename I>
+void ReleaseRequest<I>::send_invalidate_cache(bool purge_on_error) {
+  if (m_image_ctx.object_cacher == nullptr) {
+    send_flush_notifies();
+    return;
+  }
+
+  CephContext *cct = m_image_ctx.cct;
+  ldout(cct, 10) << __func__ << ": purge_on_error=" << purge_on_error << dendl;
+
+  RWLock::RLocker owner_lock(m_image_ctx.owner_lock);
+  Context *ctx = create_async_context_callback(
+    m_image_ctx, create_context_callback<
+      ReleaseRequest<I>,
+      &ReleaseRequest<I>::handle_invalidate_cache>(this));
+  m_image_ctx.invalidate_cache(purge_on_error, ctx);
+}
+
+template <typename I>
+Context *ReleaseRequest<I>::handle_invalidate_cache(int *ret_val) {
+  CephContext *cct = m_image_ctx.cct;
+  ldout(cct, 10) << __func__ << ": r=" << *ret_val << dendl;
+
+  if (*ret_val == -EBLACKLISTED) {
+    lderr(cct) << "failed to invalidate cache because client is blacklisted"
+               << dendl;
+    if (!m_image_ctx.is_cache_empty()) {
+      // force purge the cache after after being blacklisted
+      send_invalidate_cache(true);
+      return nullptr;
+    }
+  } else if (*ret_val < 0 && *ret_val != -EBUSY) {
+    lderr(cct) << "failed to invalidate cache: " << cpp_strerror(*ret_val)
+               << dendl;
     m_image_ctx.aio_work_queue->unblock_writes();
     return m_on_finish;
   }

diff --git a/src/librbd/exclusive_lock/ReleaseRequest.h b/src/librbd/exclusive_lock/ReleaseRequest.h
index 7c070ef69035202f90fc83cc4b8f7e6b3068139a..17d5b93dfa471417e904835e7200adc3fd3c56a8 100644 (file)
--- a/src/librbd/exclusive_lock/ReleaseRequest.h
+++ b/src/librbd/exclusive_lock/ReleaseRequest.h
@@ -42,6 +42,9 @@ private:
    * BLOCK_WRITES
    *    |
    *    v
+   * INVALIDATE_CACHE
+   *    |
+   *    v
    * FLUSH_NOTIFIES . . . . . . . . . . . . . .
    *    |                                     .
    *    v                                     .
@@ -81,6 +84,9 @@ private:
   void send_block_writes();
   Context *handle_block_writes(int *ret_val);
 
+  void send_invalidate_cache(bool purge_on_error);
+  Context *handle_invalidate_cache(int *ret_val);
+
   void send_flush_notifies();
   Context *handle_flush_notifies(int *ret_val);
 

diff --git a/src/librbd/internal.cc b/src/librbd/internal.cc
index eb9f3f2cf9de2de58d086f1b0a6b0230dd88e7e7..8512e6f4999cc74f479ebab77147bfa2368a3063 100644 (file)
--- a/src/librbd/internal.cc
+++ b/src/librbd/internal.cc
@@ -2381,7 +2381,7 @@ int mirror_image_disable_internal(ImageCtx *ictx, bool force,
 
     RWLock::RLocker owner_locker(ictx->owner_lock);
     RWLock::WLocker md_locker(ictx->md_lock);
-    r = ictx->invalidate_cache();
+    r = ictx->invalidate_cache(false);
     ictx->perfcounter->inc(l_librbd_invalidate_cache);
     return r;
   }

diff --git a/src/librbd/operation/ResizeRequest.cc b/src/librbd/operation/ResizeRequest.cc
index edd1ca36f08c42553f7b6aba12ba35f57f858b09..078c268defaa3a8ea978e5bd77f162717a54a071 100644 (file)
--- a/src/librbd/operation/ResizeRequest.cc
+++ b/src/librbd/operation/ResizeRequest.cc
@@ -220,7 +220,7 @@ void ResizeRequest<I>::send_invalidate_cache() {
   // need to invalidate since we're deleting objects, and
   // ObjectCacher doesn't track non-existent objects
   RWLock::RLocker owner_locker(image_ctx.owner_lock);
-  image_ctx.invalidate_cache(create_async_context_callback(
+  image_ctx.invalidate_cache(false, create_async_context_callback(
     image_ctx, create_context_callback<
       ResizeRequest<I>, &ResizeRequest<I>::handle_invalidate_cache>(this)));
 }

diff --git a/src/librbd/operation/SnapshotRollbackRequest.cc b/src/librbd/operation/SnapshotRollbackRequest.cc
index 8edf5b763164c34fb34075e1dc07ff8b4f26a574..4129544103dc74f77ef3a54a682bda5617993011 100644 (file)
--- a/src/librbd/operation/SnapshotRollbackRequest.cc
+++ b/src/librbd/operation/SnapshotRollbackRequest.cc
@@ -286,7 +286,7 @@ Context *SnapshotRollbackRequest<I>::send_invalidate_cache() {
   Context *ctx = create_context_callback<
     SnapshotRollbackRequest<I>,
     &SnapshotRollbackRequest<I>::handle_invalidate_cache>(this);
-  image_ctx.invalidate_cache(ctx);
+  image_ctx.invalidate_cache(true, ctx);
   return nullptr;
 }
 

diff --git a/src/test/librbd/exclusive_lock/test_mock_ReleaseRequest.cc b/src/test/librbd/exclusive_lock/test_mock_ReleaseRequest.cc
index 801099872f6e8b0188e0d6e04d3a7aef9294f7b6..f15c2ff29b138b09b882a37ce356ffe0ab0d5e6a 100644 (file)
--- a/src/test/librbd/exclusive_lock/test_mock_ReleaseRequest.cc
+++ b/src/test/librbd/exclusive_lock/test_mock_ReleaseRequest.cc
@@ -33,6 +33,7 @@ using ::testing::InSequence;
 using ::testing::Invoke;
 using ::testing::Return;
 using ::testing::StrEq;
+using ::testing::WithArg;
 
 static const std::string TEST_COOKIE("auto 123");
 
@@ -91,6 +92,21 @@ public:
                   .WillOnce(CompleteContext(0, mock_image_ctx.image_ctx->op_work_queue));
   }
 
+  void expect_invalidate_cache(MockImageCtx &mock_image_ctx, bool purge,
+                               int r) {
+    if (mock_image_ctx.object_cacher != nullptr) {
+      EXPECT_CALL(mock_image_ctx, invalidate_cache(purge, _))
+                    .WillOnce(WithArg<1>(CompleteContext(r, NULL)));
+    }
+  }
+
+  void expect_is_cache_empty(MockImageCtx &mock_image_ctx, bool empty) {
+    if (mock_image_ctx.object_cacher != nullptr) {
+      EXPECT_CALL(mock_image_ctx, is_cache_empty())
+        .WillOnce(Return(empty));
+    }
+  }
+
   void expect_flush_notifies(MockImageCtx &mock_image_ctx) {
     EXPECT_CALL(*mock_image_ctx.image_watcher, flush(_))
                   .WillOnce(CompleteContext(0, mock_image_ctx.image_ctx->op_work_queue));
@@ -122,6 +138,7 @@ TEST_F(TestMockExclusiveLockReleaseRequest, Success) {
   expect_prepare_lock(mock_image_ctx);
   expect_cancel_op_requests(mock_image_ctx, 0);
   expect_block_writes(mock_image_ctx, 0);
+  expect_invalidate_cache(mock_image_ctx, false, 0);
   expect_flush_notifies(mock_image_ctx);
 
   MockJournal *mock_journal = new MockJournal();
@@ -159,6 +176,7 @@ TEST_F(TestMockExclusiveLockReleaseRequest, SuccessJournalDisabled) {
   InSequence seq;
   expect_prepare_lock(mock_image_ctx);
   expect_cancel_op_requests(mock_image_ctx, 0);
+  expect_invalidate_cache(mock_image_ctx, false, 0);
   expect_flush_notifies(mock_image_ctx);
 
   MockObjectMap *mock_object_map = new MockObjectMap();
@@ -191,6 +209,7 @@ TEST_F(TestMockExclusiveLockReleaseRequest, SuccessObjectMapDisabled) {
 
   InSequence seq;
   expect_cancel_op_requests(mock_image_ctx, 0);
+  expect_invalidate_cache(mock_image_ctx, false, 0);
   expect_flush_notifies(mock_image_ctx);
 
   expect_unlock(mock_image_ctx, 0);
@@ -219,6 +238,10 @@ TEST_F(TestMockExclusiveLockReleaseRequest, Blacklisted) {
   expect_prepare_lock(mock_image_ctx);
   expect_cancel_op_requests(mock_image_ctx, 0);
   expect_block_writes(mock_image_ctx, -EBLACKLISTED);
+  expect_invalidate_cache(mock_image_ctx, false, -EBLACKLISTED);
+  expect_is_cache_empty(mock_image_ctx, false);
+  expect_invalidate_cache(mock_image_ctx, true, -EBLACKLISTED);
+  expect_is_cache_empty(mock_image_ctx, true);
   expect_flush_notifies(mock_image_ctx);
 
   MockJournal *mock_journal = new MockJournal();
@@ -278,6 +301,7 @@ TEST_F(TestMockExclusiveLockReleaseRequest, UnlockError) {
   InSequence seq;
   expect_cancel_op_requests(mock_image_ctx, 0);
   expect_block_writes(mock_image_ctx, 0);
+  expect_invalidate_cache(mock_image_ctx, false, 0);
   expect_flush_notifies(mock_image_ctx);
 
   expect_unlock(mock_image_ctx, -EINVAL);

diff --git a/src/test/librbd/mock/MockImageCtx.h b/src/test/librbd/mock/MockImageCtx.h
index 8fb4c6489bcf455693bc4cbbbdef2d8e5b05957e..943f0e874882a36d66e93911be099fe0671c955b 100644 (file)
--- a/src/test/librbd/mock/MockImageCtx.h
+++ b/src/test/librbd/mock/MockImageCtx.h
@@ -163,8 +163,9 @@ struct MockImageCtx {
   MOCK_METHOD1(flush_copyup, void(Context *));
 
   MOCK_METHOD1(flush_cache, void(Context *));
-  MOCK_METHOD1(invalidate_cache, void(Context *));
+  MOCK_METHOD2(invalidate_cache, void(bool, Context *));
   MOCK_METHOD1(shut_down_cache, void(Context *));
+  MOCK_METHOD0(is_cache_empty, bool());
 
   MOCK_CONST_METHOD1(test_features, bool(uint64_t test_features));
   MOCK_CONST_METHOD2(test_features, bool(uint64_t test_features,

diff --git a/src/test/librbd/operation/test_mock_ResizeRequest.cc b/src/test/librbd/operation/test_mock_ResizeRequest.cc
index 5a996599262fadf2bc55f0b48420520f8b00367c..a2145fcedc237a92e351c5144ff3786b9cebea0a 100644 (file)
--- a/src/test/librbd/operation/test_mock_ResizeRequest.cc
+++ b/src/test/librbd/operation/test_mock_ResizeRequest.cc
@@ -119,8 +119,8 @@ public:
   }
 
   void expect_invalidate_cache(MockImageCtx &mock_image_ctx, int r) {
-    EXPECT_CALL(mock_image_ctx, invalidate_cache(_))
-                  .WillOnce(CompleteContext(r, NULL));
+    EXPECT_CALL(mock_image_ctx, invalidate_cache(false, _))
+                  .WillOnce(WithArg<1>(CompleteContext(r, NULL)));
     expect_op_work_queue(mock_image_ctx);
   }
 

diff --git a/src/test/librbd/operation/test_mock_SnapshotRollbackRequest.cc b/src/test/librbd/operation/test_mock_SnapshotRollbackRequest.cc
index addb32b279b2633861611ccfe2a5a769293c2825..4000eb9b052037c691958fcef6b603245babc3be 100644 (file)
--- a/src/test/librbd/operation/test_mock_SnapshotRollbackRequest.cc
+++ b/src/test/librbd/operation/test_mock_SnapshotRollbackRequest.cc
@@ -165,8 +165,8 @@ public:
 
   void expect_invalidate_cache(MockOperationImageCtx &mock_image_ctx, int r) {
     if (mock_image_ctx.object_cacher != nullptr) {
-      EXPECT_CALL(mock_image_ctx, invalidate_cache(_))
-                    .WillOnce(CompleteContext(r, NULL));
+      EXPECT_CALL(mock_image_ctx, invalidate_cache(true, _))
+                    .WillOnce(WithArg<1>(CompleteContext(r, NULL)));
     }
   }