librbd: ensure that thread pool lock is held when processing throttled IOs

There previously was a potential race for throttled IOs to complete prior
to the main worker thread finishing the processing of the blocked IO.

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

diff --git a/src/librbd/io/ImageRequestWQ.cc b/src/librbd/io/ImageRequestWQ.cc
index 94dd982180f27489f887560edc35547db250b9eb..1ff272fc2817dbb4b78010d57ef86c4af3d98583 100644 (file)
--- a/src/librbd/io/ImageRequestWQ.cc
+++ b/src/librbd/io/ImageRequestWQ.cc
@@ -871,21 +871,23 @@ void ImageRequestWQ<I>::apply_qos_limit(const uint64_t flag,
 }
 
 template <typename I>
-void ImageRequestWQ<I>::handle_throttle_ready(int r, ImageDispatchSpec<I> *item, uint64_t flag) {
+void ImageRequestWQ<I>::handle_throttle_ready(int r, ImageDispatchSpec<I> *item,
+                                              uint64_t flag) {
   CephContext *cct = m_image_ctx.cct;
   ldout(cct, 15) << "r=" << r << ", " << "req=" << item << dendl;
 
+  std::lock_guard pool_locker{this->get_pool_lock()};
   ceph_assert(m_io_throttled.load() > 0);
   item->set_throttled(flag);
   if (item->were_all_throttled()) {
-    this->requeue_back(item);
+    this->requeue_back(pool_locker, item);
     --m_io_throttled;
-    this->signal();
+    this->signal(pool_locker);
   }
 }
 
 template <typename I>
-bool ImageRequestWQ<I>::needs_throttle(ImageDispatchSpec<I> *item) { 
+bool ImageRequestWQ<I>::needs_throttle(ImageDispatchSpec<I> *item) {
   uint64_t tokens = 0;
   uint64_t flag = 0;
   bool blocked = false;

diff --git a/src/test/librbd/io/test_mock_ImageRequestWQ.cc b/src/test/librbd/io/test_mock_ImageRequestWQ.cc
index 8ebea73cf85d1247b775eae7784ef7de9c49d4af..39a5e5d11c6b2c7e37ecd9a810628c0bf1066949 100644 (file)
--- a/src/test/librbd/io/test_mock_ImageRequestWQ.cc
+++ b/src/test/librbd/io/test_mock_ImageRequestWQ.cc
@@ -105,11 +105,13 @@ struct ThreadPool::PointerWQ<librbd::io::ImageDispatchSpec<librbd::MockTestImage
   MOCK_METHOD0(empty, bool());
   MOCK_METHOD0(mock_empty, bool());
   MOCK_METHOD0(signal, void());
+  MOCK_METHOD1(signal, void(const std::lock_guard<ceph::mutex>&));
   MOCK_METHOD0(process_finish, void());
 
   MOCK_METHOD0(front, ImageDispatchSpec*());
   MOCK_METHOD1(requeue_front, void(ImageDispatchSpec*));
-  MOCK_METHOD1(requeue_back, void(ImageDispatchSpec*));
+  MOCK_METHOD2(requeue_back, void(const std::lock_guard<ceph::mutex>&,
+                                  ImageDispatchSpec*));
 
   MOCK_METHOD0(dequeue, void*());
   MOCK_METHOD1(queue, void(ImageDispatchSpec*));
@@ -171,12 +173,16 @@ struct TestMockIoImageRequestWQ : public TestMockFixture {
     EXPECT_CALL(image_request_wq, signal());
   }
 
+  void expect_signal_locked(MockImageRequestWQ &image_request_wq) {
+    EXPECT_CALL(image_request_wq, signal(_));
+  }
+
   void expect_queue(MockImageRequestWQ &image_request_wq) {
     EXPECT_CALL(image_request_wq, queue(_));
   }
 
   void expect_requeue_back(MockImageRequestWQ &image_request_wq) {
-    EXPECT_CALL(image_request_wq, requeue_back(_));
+    EXPECT_CALL(image_request_wq, requeue_back(_, _));
   }
 
   void expect_front(MockImageRequestWQ &image_request_wq,
@@ -445,7 +451,7 @@ TEST_F(TestMockIoImageRequestWQ, BPSQosNoBurst) {
   expect_dequeue(mock_image_request_wq, &mock_queued_image_request);
   expect_all_throttled(mock_queued_image_request, true);
   expect_requeue_back(mock_image_request_wq);
-  expect_signal(mock_image_request_wq);
+  expect_signal_locked(mock_image_request_wq);
   ASSERT_TRUE(mock_image_request_wq.invoke_dequeue() == nullptr);
 }
 
@@ -469,7 +475,7 @@ TEST_F(TestMockIoImageRequestWQ, BPSQosWithBurst) {
   expect_dequeue(mock_image_request_wq, &mock_queued_image_request);
   expect_all_throttled(mock_queued_image_request, true);
   expect_requeue_back(mock_image_request_wq);
-  expect_signal(mock_image_request_wq);
+  expect_signal_locked(mock_image_request_wq);
   ASSERT_TRUE(mock_image_request_wq.invoke_dequeue() == nullptr);
 }