librbd: exclusive lock failures should bubble up to IO

(derived from commit 048d475127b600b6a40bd5e0c3a0daf8133294f4)

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

diff --git a/src/librbd/AioImageRequestWQ.cc b/src/librbd/AioImageRequestWQ.cc
index 93bbe6ba37dc4078435e003b57861b298490b1a6..f9942ce20596ac512d2dd0c4afd3169fcdec8c37 100644 (file)
--- a/src/librbd/AioImageRequestWQ.cc
+++ b/src/librbd/AioImageRequestWQ.cc
@@ -18,6 +18,46 @@
 
 namespace librbd {
 
+template <typename I>
+struct AioImageRequestWQ<I>::C_AcquireLock : public Context {
+  AioImageRequestWQ *work_queue;
+  AioImageRequest<I> *image_request;
+
+  C_AcquireLock(AioImageRequestWQ *work_queue, AioImageRequest<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 AioImageRequestWQ<I>::C_BlockedWrites : public Context {
+  AioImageRequestWQ *work_queue;
+  C_BlockedWrites(AioImageRequestWQ *_work_queue)
+    : work_queue(_work_queue) {
+  }
+
+  void finish(int r) override {
+    work_queue->handle_blocked_writes(r);
+  }
+};
+
+template <typename I>
+struct AioImageRequestWQ<I>::C_RefreshFinish : public Context {
+  AioImageRequestWQ *work_queue;
+  AioImageRequest<I> *image_request;
+
+  C_RefreshFinish(AioImageRequestWQ *work_queue,
+                  AioImageRequest<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>
 AioImageRequestWQ<I>::AioImageRequestWQ(I *image_ctx, const string &name,
                                        time_t ti, ThreadPool *tp)
@@ -114,18 +154,11 @@ void AioImageRequestWQ<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 AioImageRead<>(m_image_ctx, c, off, len, buf, pbl, op_flags));
   } else {
     c->start_op();
@@ -235,13 +268,6 @@ void AioImageRequestWQ<I>::shut_down(Context *on_shutdown) {
   m_image_ctx.flush(on_shutdown);
 }
 
-template <typename I>
-bool AioImageRequestWQ<I>::is_lock_request_needed() const {
-  RWLock::RLocker locker(m_lock);
-  return (m_queued_writes.read() > 0 ||
-          (m_require_lock_on_read && m_queued_reads.read() > 0));
-}
-
 template <typename I>
 int AioImageRequestWQ<I>::block_writes() {
   C_SaferCond cond_ctx;
@@ -326,27 +352,23 @@ void AioImageRequestWQ<I>::set_require_lock(AioDirection aio_direction,
 
 template <typename I>
 void *AioImageRequestWQ<I>::_void_dequeue() {
+  CephContext *cct = m_image_ctx.cct;
   AioImageRequest<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.read() > 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.inc();
-      }
-    } 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.inc();
     }
   }
 
@@ -354,12 +376,39 @@ void *AioImageRequestWQ<I>::_void_dequeue() {
     ThreadPool::PointerWQ<AioImageRequest<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.inc();
+        m_image_ctx.exclusive_lock->request_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.inc();
 
     this->get_pool_lock().Unlock();
     m_image_ctx.state->refresh(new C_RefreshFinish(this, item));
@@ -455,73 +504,72 @@ void AioImageRequestWQ<I>::finish_in_flight_io() {
 }
 
 template <typename I>
-bool AioImageRequestWQ<I>::is_lock_required() const {
-  assert(m_image_ctx.owner_lock.is_locked());
-  if (m_image_ctx.exclusive_lock == NULL) {
-    return false;
-  }
+void AioImageRequestWQ<I>::fail_in_flight_io(int r, AioImageRequest<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 AioImageRequestWQ<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 AioImageRequestWQ<I>::queue(AioImageRequest<I> *req) {
+  assert(m_image_ctx.owner_lock.is_locked());
+
   CephContext *cct = m_image_ctx.cct;
   ldout(cct, 20) << __func__ << ": 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 = (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.inc();
   } else {
     m_queued_reads.inc();
   }
 
   ThreadPool::PointerWQ<AioImageRequest<I> >::queue(req);
+}
 
-  if (lock_required) {
-    m_image_ctx.exclusive_lock->request_lock(nullptr);
+template <typename I>
+void AioImageRequestWQ<I>::handle_acquire_lock(int r, AioImageRequest<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.read() > 0);
+  m_io_blockers.dec();
+  this->signal();
 }
 
 template <typename I>
 void AioImageRequestWQ<I>::handle_refreshed(int r, AioImageRequest<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.read() > 0);
+  m_io_blockers.dec();
   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->request_lock(nullptr);
-  }
 }
 
 template <typename I>

diff --git a/src/librbd/AioImageRequestWQ.h b/src/librbd/AioImageRequestWQ.h
index c21a23de52bf67364e6809a5c46719d94f31c759..ba535d699cf5092e705d54908eb439ecac33a794 100644 (file)
--- a/src/librbd/AioImageRequestWQ.h
+++ b/src/librbd/AioImageRequestWQ.h
@@ -46,9 +46,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);
@@ -67,29 +64,9 @@ protected:
 private:
   typedef std::list<Context *> Contexts;
 
-  struct C_RefreshFinish : public Context {
-    AioImageRequestWQ *aio_work_queue;
-    AioImageRequest<ImageCtxT> *aio_image_request;
-
-    C_RefreshFinish(AioImageRequestWQ *aio_work_queue,
-                    AioImageRequest<ImageCtxT> *aio_image_request)
-      : aio_work_queue(aio_work_queue), aio_image_request(aio_image_request) {
-    }
-    virtual void finish(int r) override {
-      aio_work_queue->handle_refreshed(r, aio_image_request);
-    }
-  };
-
-  struct C_BlockedWrites : public Context {
-    AioImageRequestWQ *aio_work_queue;
-    C_BlockedWrites(AioImageRequestWQ *_aio_work_queue)
-      : aio_work_queue(_aio_work_queue) {
-    }
-
-    virtual void finish(int r) {
-      aio_work_queue->handle_blocked_writes(r);
-    }
-  };
+  struct C_AcquireLock;
+  struct C_BlockedWrites;
+  struct C_RefreshFinish;
 
   ImageCtxT &m_image_ctx;
   mutable RWLock m_lock;
@@ -101,12 +78,18 @@ private:
   atomic_t m_queued_reads {0};
   atomic_t m_queued_writes {0};
   atomic_t m_in_flight_ios {0};
-
-  bool m_refresh_in_progress = false;
+  atomic_t 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.read() == 0);
@@ -117,9 +100,11 @@ private:
 
   int start_in_flight_io(AioCompletion *c);
   void finish_in_flight_io();
+  void fail_in_flight_io(int r, AioImageRequest<ImageCtxT> *req);
 
   void queue(AioImageRequest<ImageCtxT> *req);
 
+  void handle_acquire_lock(int r, AioImageRequest<ImageCtxT> *req);
   void handle_refreshed(int r, AioImageRequest<ImageCtxT> *req);
   void handle_blocked_writes(int r);
 };

diff --git a/src/librbd/ExclusiveLock.cc b/src/librbd/ExclusiveLock.cc
index 50cd61eb6d7082023b0a3dc284f9d30d50b89cfc..f32794052622ac3127743ed907c68392245831f3 100644 (file)
--- a/src/librbd/ExclusiveLock.cc
+++ b/src/librbd/ExclusiveLock.cc
@@ -646,31 +646,21 @@ void ExclusiveLock<I>::handle_releasing_lock(int r) {
 
 template <typename I>
 void ExclusiveLock<I>::handle_release_lock(int r) {
-  bool lock_request_needed = false;
-  {
-    Mutex::Locker locker(m_lock);
-    ldout(m_image_ctx.cct, 10) << this << " " << __func__ << ": r=" << r
-                               << dendl;
-
-    assert(m_state == STATE_PRE_RELEASING ||
-           m_state == STATE_RELEASING);
-    if (r >= 0) {
-      m_lock.Unlock();
-      m_image_ctx.image_watcher->notify_released_lock();
-      lock_request_needed = m_image_ctx.aio_work_queue->is_lock_request_needed();
-      m_lock.Lock();
+  Mutex::Locker locker(m_lock);
+  ldout(m_image_ctx.cct, 10) << this << " " << __func__ << ": r=" << r
+                             << dendl;
 
-      m_cookie = "";
-      m_watch_handle = 0;
-    }
-    complete_active_action(r < 0 ? STATE_LOCKED : STATE_UNLOCKED, r);
-  }
+  assert(m_state == STATE_PRE_RELEASING ||
+         m_state == STATE_RELEASING);
+  if (r >= 0) {
+    m_lock.Unlock();
+    m_image_ctx.image_watcher->notify_released_lock();
+    m_lock.Lock();
 
-  if (r >= 0 && lock_request_needed) {
-    // if we have blocked IO -- re-request the lock
-    RWLock::RLocker owner_locker(m_image_ctx.owner_lock);
-    request_lock(nullptr);
+    m_cookie = "";
+    m_watch_handle = 0;
   }
+  complete_active_action(r < 0 ? STATE_LOCKED : STATE_UNLOCKED, r);
 }
 
 template <typename I>

diff --git a/src/librbd/exclusive_lock/ReleaseRequest.cc b/src/librbd/exclusive_lock/ReleaseRequest.cc
index 552ca065816d74e13734bc46de0a037bf400d648..58df357fc738d449ef5f5fb89f08584cbcc6dc5d 100644 (file)
--- a/src/librbd/exclusive_lock/ReleaseRequest.cc
+++ b/src/librbd/exclusive_lock/ReleaseRequest.cc
@@ -116,6 +116,8 @@ void ReleaseRequest<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.aio_work_queue->set_require_lock(AIO_DIRECTION_BOTH, true);

diff --git a/src/test/librbd/mock/MockAioImageRequestWQ.h b/src/test/librbd/mock/MockAioImageRequestWQ.h
index 0f3efac64c7fa1b9eef4e1f080c94d12594040a4..99b283c989c69a1b540569bbc5e321a784d6f192 100644 (file)
--- a/src/test/librbd/mock/MockAioImageRequestWQ.h
+++ b/src/test/librbd/mock/MockAioImageRequestWQ.h
@@ -16,8 +16,6 @@ struct MockAioImageRequestWQ {
   MOCK_METHOD0(unblock_writes, void());
 
   MOCK_METHOD2(set_require_lock, void(AioDirection, bool));
-
-  MOCK_CONST_METHOD0(is_lock_request_needed, bool());
 };
 
 } // namespace librbd

diff --git a/src/test/librbd/test_mock_ExclusiveLock.cc b/src/test/librbd/test_mock_ExclusiveLock.cc
index df74828c97788d0f616be451a85d5f95844f7b51..8d6a1bb0f4e85c84d981f7baf2750c4091759353 100644 (file)
--- a/src/test/librbd/test_mock_ExclusiveLock.cc
+++ b/src/test/librbd/test_mock_ExclusiveLock.cc
@@ -152,7 +152,6 @@ public:
         expect_unblock_writes(mock_image_ctx);
       }
       expect_notify_released_lock(mock_image_ctx);
-      expect_is_lock_request_needed(mock_image_ctx, false);
     }
   }
 
@@ -182,11 +181,6 @@ public:
                   .Times(1);
   }
 
-  void expect_is_lock_request_needed(MockExclusiveLockImageCtx &mock_image_ctx, bool ret) {
-    EXPECT_CALL(*mock_image_ctx.aio_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));
@@ -566,7 +560,6 @@ TEST_F(TestMockExclusiveLock, ConcurrentRequests) {
   EXPECT_CALL(release, send())
                 .WillOnce(Notify(&wait_for_send_ctx2));
   expect_notify_released_lock(mock_image_ctx);
-  expect_is_lock_request_needed(mock_image_ctx, false);
 
   C_SaferCond try_request_ctx1;
   {