librbd: exclusive lock failures should bubble up to IO

Signed-off-by: Jason Dillaman <dillaman@redhat.com>

diff --git a/src/librbd/ExclusiveLock.cc b/src/librbd/ExclusiveLock.cc
index f126ccbb482ab655eb406618f24faaa31c9e6a45..8436bad87695ea389c26bb3d07db3e6d5cbdb717 100644 (file)
--- a/src/librbd/ExclusiveLock.cc
+++ b/src/librbd/ExclusiveLock.cc
@@ -308,11 +308,6 @@ void ExclusiveLock<I>::post_release_lock_handler(bool shutting_down, int r,
 
     if (r >= 0) {
       m_image_ctx.image_watcher->notify_released_lock();
-      if (m_image_ctx.io_work_queue->is_lock_request_needed()) {
-        // if we have blocked IO -- re-request the lock
-        RWLock::RLocker owner_locker(m_image_ctx.owner_lock);
-        ML<I>::acquire_lock(nullptr);
-      }
     }
   } else {
     {

diff --git a/src/librbd/exclusive_lock/PreReleaseRequest.cc b/src/librbd/exclusive_lock/PreReleaseRequest.cc
index 1fd6e2bfd22a8acc65ad2abc1254f750696b8b09..504de0f0b93f9ae39bf1ec58c652d2ca80faf59e 100644 (file)
--- a/src/librbd/exclusive_lock/PreReleaseRequest.cc
+++ b/src/librbd/exclusive_lock/PreReleaseRequest.cc
@@ -109,6 +109,8 @@ void PreReleaseRequest<I>::send_block_writes() {
 
   {
     RWLock::RLocker owner_locker(m_image_ctx.owner_lock);
+    // setting the lock as required will automatically cause the IO
+    // queue to re-request the lock if any IO is queued
     if (m_image_ctx.clone_copy_on_read ||
         m_image_ctx.test_features(RBD_FEATURE_JOURNALING)) {
       m_image_ctx.io_work_queue->set_require_lock(io::DIRECTION_BOTH, true);

diff --git a/src/librbd/io/ImageRequestWQ.cc b/src/librbd/io/ImageRequestWQ.cc
index 577aa3801fee337ad14b02a34e7874af4c5960d2..8b59964e94c7652317e00bab9618f1cd2f5abff9 100644 (file)
--- a/src/librbd/io/ImageRequestWQ.cc
+++ b/src/librbd/io/ImageRequestWQ.cc
@@ -21,6 +21,46 @@
 namespace librbd {
 namespace io {
 
+template <typename I>
+struct ImageRequestWQ<I>::C_AcquireLock : public Context {
+  ImageRequestWQ *work_queue;
+  ImageRequest<I> *image_request;
+
+  C_AcquireLock(ImageRequestWQ *work_queue, ImageRequest<I> *image_request)
+    : work_queue(work_queue), image_request(image_request) {
+  }
+
+  void finish(int r) override {
+    work_queue->handle_acquire_lock(r, image_request);
+  }
+};
+
+template <typename I>
+struct ImageRequestWQ<I>::C_BlockedWrites : public Context {
+  ImageRequestWQ *work_queue;
+  C_BlockedWrites(ImageRequestWQ *_work_queue)
+    : work_queue(_work_queue) {
+  }
+
+  void finish(int r) override {
+    work_queue->handle_blocked_writes(r);
+  }
+};
+
+template <typename I>
+struct ImageRequestWQ<I>::C_RefreshFinish : public Context {
+  ImageRequestWQ *work_queue;
+  ImageRequest<I> *image_request;
+
+  C_RefreshFinish(ImageRequestWQ *work_queue,
+                  ImageRequest<I> *image_request)
+    : work_queue(work_queue), image_request(image_request) {
+  }
+  void finish(int r) override {
+    work_queue->handle_refreshed(r, image_request);
+  }
+};
+
 template <typename I>
 ImageRequestWQ<I>::ImageRequestWQ(I *image_ctx, const string &name,
                                  time_t ti, ThreadPool *tp)
@@ -147,18 +187,11 @@ void ImageRequestWQ<I>::aio_read(AioCompletion *c, uint64_t off, uint64_t len,
     return;
   }
 
-  RWLock::RLocker owner_locker(m_image_ctx.owner_lock);
-
   // if journaling is enabled -- we need to replay the journal because
   // it might contain an uncommitted write
-  bool lock_required;
-  {
-    RWLock::RLocker locker(m_lock);
-    lock_required = m_require_lock_on_read;
-  }
-
+  RWLock::RLocker owner_locker(m_image_ctx.owner_lock);
   if (m_image_ctx.non_blocking_aio || writes_blocked() || !writes_empty() ||
-      lock_required) {
+      require_lock_on_read()) {
     queue(new ImageReadRequest<I>(m_image_ctx, c, {{off, len}},
                                  std::move(read_result), op_flags, trace));
   } else {
@@ -335,13 +368,6 @@ void ImageRequestWQ<I>::shut_down(Context *on_shutdown) {
   m_image_ctx.flush(on_shutdown);
 }
 
-template <typename I>
-bool ImageRequestWQ<I>::is_lock_request_needed() const {
-  RWLock::RLocker locker(m_lock);
-  return (m_queued_writes > 0 ||
-          (m_require_lock_on_read && m_queued_reads > 0));
-}
-
 template <typename I>
 int ImageRequestWQ<I>::block_writes() {
   C_SaferCond cond_ctx;
@@ -426,27 +452,23 @@ void ImageRequestWQ<I>::set_require_lock(Direction direction, bool enabled) {
 
 template <typename I>
 void *ImageRequestWQ<I>::_void_dequeue() {
+  CephContext *cct = m_image_ctx.cct;
   ImageRequest<I> *peek_item = this->front();
 
-  // no IO ops available or refresh in-progress (IO stalled)
-  if (peek_item == nullptr || m_refresh_in_progress) {
+  // no queued IO requests or all IO is blocked/stalled
+  if (peek_item == nullptr || m_io_blockers.load() > 0) {
     return nullptr;
   }
 
+  bool lock_required;
   bool refresh_required = m_image_ctx.state->is_refresh_required();
   {
     RWLock::RLocker locker(m_lock);
-    if (peek_item->is_write_op()) {
-      if (m_write_blockers > 0) {
-        return nullptr;
-      }
-
-      // refresh will requeue the op -- don't count it as in-progress
-      if (!refresh_required) {
-        m_in_flight_writes++;
-      }
-    } else if (m_require_lock_on_read) {
-      return nullptr;
+    bool write_op = peek_item->is_write_op();
+    lock_required = is_lock_required(write_op);
+    if (write_op && !lock_required && !refresh_required) {
+      // completed ops will requeue the IO -- don't count it as in-progress
+      m_in_flight_writes++;
     }
   }
 
@@ -454,12 +476,39 @@ void *ImageRequestWQ<I>::_void_dequeue() {
     ThreadPool::PointerWQ<ImageRequest<I> >::_void_dequeue());
   assert(peek_item == item);
 
+  if (lock_required) {
+    this->get_pool_lock().Unlock();
+    m_image_ctx.owner_lock.get_read();
+    if (m_image_ctx.exclusive_lock != nullptr) {
+      ldout(cct, 5) << "exclusive lock required: delaying IO " << item << dendl;
+      if (!m_image_ctx.get_exclusive_lock_policy()->may_auto_request_lock()) {
+        lderr(cct) << "op requires exclusive lock" << dendl;
+        fail_in_flight_io(-EROFS, item);
+
+        // wake up the IO since we won't be returning a request to process
+        this->signal();
+      } else {
+        // stall IO until the acquire completes
+        ++m_io_blockers;
+        m_image_ctx.exclusive_lock->acquire_lock(new C_AcquireLock(this, item));
+      }
+    } else {
+      // raced with the exclusive lock being disabled
+      lock_required = false;
+    }
+    m_image_ctx.owner_lock.put_read();
+    this->get_pool_lock().Lock();
+
+    if (lock_required) {
+      return nullptr;
+    }
+  }
+
   if (refresh_required) {
-    ldout(m_image_ctx.cct, 15) << "image refresh required: delaying IO " << item
-                               << dendl;
+    ldout(cct, 5) << "image refresh required: delaying IO " << item << dendl;
 
     // stall IO until the refresh completes
-    m_refresh_in_progress = true;
+    ++m_io_blockers;
 
     this->get_pool_lock().Unlock();
     m_image_ctx.state->refresh(new C_RefreshFinish(this, item));
@@ -552,74 +601,72 @@ void ImageRequestWQ<I>::finish_in_flight_io() {
 }
 
 template <typename I>
-bool ImageRequestWQ<I>::is_lock_required() const {
-  assert(m_image_ctx.owner_lock.is_locked());
-  if (m_image_ctx.exclusive_lock == NULL) {
-    return false;
-  }
+void ImageRequestWQ<I>::fail_in_flight_io(int r, ImageRequest<I> *req) {
+  this->process_finish();
+  req->fail(r);
+  finish_queued_io(req);
+  delete req;
+  finish_in_flight_io();
+}
 
-  return (!m_image_ctx.exclusive_lock->is_lock_owner());
+template <typename I>
+bool ImageRequestWQ<I>::is_lock_required(bool write_op) const {
+  assert(m_lock.is_locked());
+  return ((write_op && m_require_lock_on_write) ||
+          (!write_op && m_require_lock_on_read));
 }
 
 template <typename I>
 void ImageRequestWQ<I>::queue(ImageRequest<I> *req) {
+  assert(m_image_ctx.owner_lock.is_locked());
+
   CephContext *cct = m_image_ctx.cct;
   ldout(cct, 20) << "ictx=" << &m_image_ctx << ", "
                  << "req=" << req << dendl;
 
-  assert(m_image_ctx.owner_lock.is_locked());
-  bool write_op = req->is_write_op();
-  bool lock_required = (m_image_ctx.exclusive_lock != nullptr &&
-                        ((write_op && is_lock_required()) ||
-                          (!write_op && m_require_lock_on_read)));
-
-  if (lock_required && !m_image_ctx.get_exclusive_lock_policy()->may_auto_request_lock()) {
-    lderr(cct) << "op requires exclusive lock" << dendl;
-    req->fail(-EROFS);
-    delete req;
-    finish_in_flight_io();
-    return;
-  }
-
-  if (write_op) {
+  if (req->is_write_op()) {
     m_queued_writes++;
   } else {
     m_queued_reads++;
   }
 
   ThreadPool::PointerWQ<ImageRequest<I> >::queue(req);
+}
 
-  if (lock_required) {
-    m_image_ctx.exclusive_lock->acquire_lock(nullptr);
+template <typename I>
+void ImageRequestWQ<I>::handle_acquire_lock(int r, ImageRequest<I> *req) {
+  CephContext *cct = m_image_ctx.cct;
+  ldout(cct, 5) << "r=" << r << ", " << "req=" << req << dendl;
+
+  if (r < 0) {
+    fail_in_flight_io(r, req);
+  } else {
+    // since IO was stalled for acquire -- original IO order is preserved
+    // if we requeue this op for work queue processing
+    this->requeue(req);
   }
+
+  assert(m_io_blockers.load() > 0);
+  --m_io_blockers;
+  this->signal();
 }
 
 template <typename I>
 void ImageRequestWQ<I>::handle_refreshed(int r, ImageRequest<I> *req) {
   CephContext *cct = m_image_ctx.cct;
-  ldout(cct, 15) << "resuming IO after image refresh: r=" << r << ", "
-                 << "req=" << req << dendl;
+  ldout(cct, 5) << "resuming IO after image refresh: r=" << r << ", "
+                << "req=" << req << dendl;
   if (r < 0) {
-    this->process_finish();
-    req->fail(r);
-    finish_queued_io(req);
-    delete req;
-    finish_in_flight_io();
+    fail_in_flight_io(r, req);
   } else {
     // since IO was stalled for refresh -- original IO order is preserved
     // if we requeue this op for work queue processing
     this->requeue(req);
   }
 
-  m_refresh_in_progress = false;
+  assert(m_io_blockers.load() > 0);
+  --m_io_blockers;
   this->signal();
-
-  // refresh might have enabled exclusive lock -- IO stalled until
-  // we acquire the lock
-  RWLock::RLocker owner_locker(m_image_ctx.owner_lock);
-  if (is_lock_required() && is_lock_request_needed()) {
-    m_image_ctx.exclusive_lock->acquire_lock(nullptr);
-  }
 }
 
 template <typename I>

diff --git a/src/librbd/io/ImageRequestWQ.h b/src/librbd/io/ImageRequestWQ.h
index b3a91c3010bf19102d5d867bc49d973ff4c36145..4f390b48e1fc203b9f57f62b4869fee57534c51f 100644 (file)
--- a/src/librbd/io/ImageRequestWQ.h
+++ b/src/librbd/io/ImageRequestWQ.h
@@ -51,9 +51,6 @@ public:
 
   void shut_down(Context *on_shutdown);
 
-  bool is_lock_required() const;
-  bool is_lock_request_needed() const;
-
   inline bool writes_blocked() const {
     RWLock::RLocker locker(m_lock);
     return (m_write_blockers > 0);
@@ -72,29 +69,9 @@ protected:
 private:
   typedef std::list<Context *> Contexts;
 
-  struct C_RefreshFinish : public Context {
-    ImageRequestWQ *aio_work_queue;
-    ImageRequest<ImageCtxT> *aio_image_request;
-
-    C_RefreshFinish(ImageRequestWQ *aio_work_queue,
-                    ImageRequest<ImageCtxT> *aio_image_request)
-      : aio_work_queue(aio_work_queue), aio_image_request(aio_image_request) {
-    }
-    void finish(int r) override {
-      aio_work_queue->handle_refreshed(r, aio_image_request);
-    }
-  };
-
-  struct C_BlockedWrites : public Context {
-    ImageRequestWQ *aio_work_queue;
-    C_BlockedWrites(ImageRequestWQ *_aio_work_queue)
-      : aio_work_queue(_aio_work_queue) {
-    }
-
-    void finish(int r) override {
-      aio_work_queue->handle_blocked_writes(r);
-    }
-  };
+  struct C_AcquireLock;
+  struct C_BlockedWrites;
+  struct C_RefreshFinish;
 
   ImageCtxT &m_image_ctx;
   mutable RWLock m_lock;
@@ -106,12 +83,17 @@ private:
   std::atomic<unsigned> m_queued_writes { 0 };
   std::atomic<unsigned> m_in_flight_ios { 0 };
   std::atomic<unsigned> m_in_flight_writes { 0 };
-
-  bool m_refresh_in_progress = false;
+  std::atomic<unsigned> m_io_blockers { 0 };
 
   bool m_shutdown = false;
   Context *m_on_shutdown = nullptr;
 
+  bool is_lock_required(bool write_op) const;
+
+  inline bool require_lock_on_read() const {
+    RWLock::RLocker locker(m_lock);
+    return m_require_lock_on_read;
+  }
   inline bool writes_empty() const {
     RWLock::RLocker locker(m_lock);
     return (m_queued_writes == 0);
@@ -122,9 +104,11 @@ private:
 
   int start_in_flight_io(AioCompletion *c);
   void finish_in_flight_io();
+  void fail_in_flight_io(int r, ImageRequest<ImageCtxT> *req);
 
   void queue(ImageRequest<ImageCtxT> *req);
 
+  void handle_acquire_lock(int r, ImageRequest<ImageCtxT> *req);
   void handle_refreshed(int r, ImageRequest<ImageCtxT> *req);
   void handle_blocked_writes(int r);
 };

diff --git a/src/test/librbd/mock/io/MockImageRequestWQ.h b/src/test/librbd/mock/io/MockImageRequestWQ.h
index c3187e086cf42f1d958a9228b1e42ecd25ace957..ab0804523c12a4e529f02c16cf2cbcef3e190011 100644 (file)
--- a/src/test/librbd/mock/io/MockImageRequestWQ.h
+++ b/src/test/librbd/mock/io/MockImageRequestWQ.h
@@ -17,8 +17,6 @@ struct MockImageRequestWQ {
   MOCK_METHOD0(unblock_writes, void());
 
   MOCK_METHOD2(set_require_lock, void(Direction, bool));
-
-  MOCK_CONST_METHOD0(is_lock_request_needed, bool());
 };
 
 } // namespace io

diff --git a/src/test/librbd/test_mock_ExclusiveLock.cc b/src/test/librbd/test_mock_ExclusiveLock.cc
index 6496af5089be498080a3ff1891903a1bfd202a42..2d5d0c62ba327b94b15f298a76a1172b625d7a3c 100644 (file)
--- a/src/test/librbd/test_mock_ExclusiveLock.cc
+++ b/src/test/librbd/test_mock_ExclusiveLock.cc
@@ -273,11 +273,6 @@ public:
       .Times(1);
   }
 
-  void expect_is_lock_request_needed(MockExclusiveLockImageCtx &mock_image_ctx, bool ret) {
-    EXPECT_CALL(*mock_image_ctx.io_work_queue, is_lock_request_needed())
-                  .WillRepeatedly(Return(ret));
-  }
-
   void expect_flush_notifies(MockExclusiveLockImageCtx &mock_image_ctx) {
     EXPECT_CALL(*mock_image_ctx.image_watcher, flush(_))
                   .WillOnce(CompleteContext(0, mock_image_ctx.image_ctx->op_work_queue));
@@ -383,7 +378,6 @@ TEST_F(TestMockExclusiveLock, StateTransitions) {
   expect_is_state_pre_releasing(exclusive_lock, false);
   expect_is_state_releasing(exclusive_lock, true);
   expect_notify_released_lock(mock_image_ctx);
-  expect_is_lock_request_needed(mock_image_ctx, false);
   ASSERT_EQ(0, when_post_release_lock_handler(exclusive_lock, false, 0));
 
   // (try) acquire lock