librbd: fix stuck with disable request

Problem:
-------
Trying to disable any feature on an rbd image mapped with nbd leads to stuck
in rbd-nbd.

The rbd-nbd registers a watcher callback to detect image resize in
NBDWatchCtx::handle_notify(). The handle_notify calls image info method, which
calls refresh_if_required and it got stuck there.

It is getting stuck in ImageState::refresh_if_required() because
DisableFeaturesRequest issues update notifications while still holding onto
the exclusive lock with everything that has to do with it blocked.

Solution:
--------
Change DisableFeaturesRequest to call ImageCtx::notify_update() after
unblocking the exclusive lock.

Fixes: https://tracker.ceph.com/issues/58740
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>

src/librbd/operation/DisableFeaturesRequest.cc

@@ -580,12 +580,40 @@ Context *DisableFeaturesRequest<I>::handle_update_flags(int *result) {
   return nullptr;
 }
 
+template <typename I>
+void DisableFeaturesRequest<I>::send_unblock_writes() {
+  I &image_ctx = this->m_image_ctx;
+  CephContext *cct = image_ctx.cct;
+  ldout(cct, 20) << this << " " << __func__ << dendl;
+
+  {
+    std::unique_lock locker{image_ctx.owner_lock};
+
+    if (image_ctx.exclusive_lock != nullptr && m_requests_blocked) {
+      image_ctx.exclusive_lock->unblock_requests();
+      m_requests_blocked = false;
+    }
+
+    if (m_writes_blocked) {
+      image_ctx.io_image_dispatcher->unblock_writes();
+      m_writes_blocked = false;
+    }
+  }
+
+  if (!m_prepare_lock_completed) {
+    image_ctx.state->handle_prepare_lock_complete();
+    m_prepare_lock_completed = true;
+  }
+}
+
 template <typename I>
 void DisableFeaturesRequest<I>::send_notify_update() {
   I &image_ctx = this->m_image_ctx;
   CephContext *cct = image_ctx.cct;
   ldout(cct, 20) << this << " " << __func__ << dendl;
 
+  send_unblock_writes();
+
   Context *ctx = create_context_callback<
     DisableFeaturesRequest<I>,
     &DisableFeaturesRequest<I>::handle_notify_update>(this);
@@ -636,15 +664,7 @@ Context *DisableFeaturesRequest<I>::handle_finish(int r) {
   CephContext *cct = image_ctx.cct;
   ldout(cct, 20) << this << " " << __func__ << ": r=" << r << dendl;
 
-  {
-    std::unique_lock locker{image_ctx.owner_lock};
-    if (image_ctx.exclusive_lock != nullptr && m_requests_blocked) {
-      image_ctx.exclusive_lock->unblock_requests();
-    }
-
-    image_ctx.io_image_dispatcher->unblock_writes();
-  }
-  image_ctx.state->handle_prepare_lock_complete();
+  send_unblock_writes();
 
   return this->create_context_finisher(r);
 }

src/librbd/operation/DisableFeaturesRequest.h

@@ -88,13 +88,13 @@ class DisableFeaturesRequest : public Request<ImageCtxT> {
    *    v
    * STATE_UPDATE_FLAGS
    *    |
+   *    | (unblock writes)
    *    v
    * STATE_NOTIFY_UPDATE
    *    |
    *    v
    * STATE_RELEASE_EXCLUSIVE_LOCK (skip if not
    *    |                          required)
-   *    | (unblock writes)
    *    v
    * <finish>
    *
@@ -109,6 +109,7 @@ class DisableFeaturesRequest : public Request<ImageCtxT> {
   bool m_writes_blocked = false;
   bool m_image_lock_acquired = false;
   bool m_requests_blocked = false;
+  bool m_prepare_lock_completed = false;
 
   uint64_t m_new_features = 0;
   uint64_t m_disable_flags = 0;
@@ -154,6 +155,8 @@ class DisableFeaturesRequest : public Request<ImageCtxT> {
   void send_update_flags();
   Context *handle_update_flags(int *result);
 
+  void send_unblock_writes();
+
   void send_notify_update();
   Context *handle_notify_update(int *result);
 

src/librbd/operation/EnableFeaturesRequest.cc

@@ -445,12 +445,40 @@ Context *EnableFeaturesRequest<I>::handle_enable_mirror_image(int *result) {
   return nullptr;
 }
 
+template <typename I>
+void EnableFeaturesRequest<I>::send_unblock_writes() {
+  I &image_ctx = this->m_image_ctx;
+  CephContext *cct = image_ctx.cct;
+  ldout(cct, 20) << this << " " << __func__ << dendl;
+
+  {
+    std::unique_lock locker{image_ctx.owner_lock};
+
+    if (image_ctx.exclusive_lock != nullptr && m_requests_blocked) {
+      image_ctx.exclusive_lock->unblock_requests();
+      m_requests_blocked = false;
+    }
+
+    if (m_writes_blocked) {
+      image_ctx.io_image_dispatcher->unblock_writes();
+      m_writes_blocked = false;
+    }
+  }
+
+  if (!m_prepare_lock_completed) {
+    image_ctx.state->handle_prepare_lock_complete();
+    m_prepare_lock_completed = true;
+  }
+}
+
 template <typename I>
 void EnableFeaturesRequest<I>::send_notify_update() {
   I &image_ctx = this->m_image_ctx;
   CephContext *cct = image_ctx.cct;
   ldout(cct, 20) << this << " " << __func__ << dendl;
 
+  send_unblock_writes();
+
   Context *ctx = create_context_callback<
     EnableFeaturesRequest<I>,
     &EnableFeaturesRequest<I>::handle_notify_update>(this);
@@ -473,17 +501,7 @@ Context *EnableFeaturesRequest<I>::handle_finish(int r) {
   CephContext *cct = image_ctx.cct;
   ldout(cct, 20) << this << " " << __func__ << ": r=" << r << dendl;
 
-  {
-    std::unique_lock locker{image_ctx.owner_lock};
-
-    if (image_ctx.exclusive_lock != nullptr && m_requests_blocked) {
-      image_ctx.exclusive_lock->unblock_requests();
-    }
-    if (m_writes_blocked) {
-      image_ctx.io_image_dispatcher->unblock_writes();
-    }
-  }
-  image_ctx.state->handle_prepare_lock_complete();
+  send_unblock_writes();
 
   return this->create_context_finisher(r);
 }

src/librbd/operation/EnableFeaturesRequest.h

@@ -72,10 +72,10 @@ class EnableFeaturesRequest : public Request<ImageCtxT> {
    *    v
    * STATE_ENABLE_MIRROR_IMAGE
    *    |
+   *    | (unblock writes)
    *    V
    * STATE_NOTIFY_UPDATE
    *    |
-   *    | (unblock writes)
    *    v
    * <finish>
    * @endverbatim
@@ -87,6 +87,7 @@ class EnableFeaturesRequest : public Request<ImageCtxT> {
   bool m_enable_mirroring = false;
   bool m_requests_blocked = false;
   bool m_writes_blocked = false;
+  bool m_prepare_lock_completed = false;
 
   uint64_t m_new_features = 0;
   uint64_t m_enable_flags = 0;
@@ -121,6 +122,8 @@ class EnableFeaturesRequest : public Request<ImageCtxT> {
   void send_enable_mirror_image();
   Context *handle_enable_mirror_image(int *result);
 
+  void send_unblock_writes();
+
   void send_notify_update();
   Context *handle_notify_update(int *result);
 

src/test/librbd/operation/test_mock_DisableFeaturesRequest.cc

@@ -340,10 +340,10 @@ TEST_F(TestMockOperationDisableFeaturesRequest, All) {
     expect_set_flags_request_send(mock_image_ctx,
 				  mock_set_flags_request, 0);
   }
-  expect_notify_update(mock_image_ctx);
   expect_unblock_requests(mock_image_ctx);
   expect_unblock_writes(mock_image_ctx);
   expect_handle_prepare_lock_complete(mock_image_ctx);
+  expect_notify_update(mock_image_ctx);
 
   C_SaferCond cond_ctx;
   MockDisableFeaturesRequest *req = new MockDisableFeaturesRequest(
@@ -385,10 +385,10 @@ TEST_F(TestMockOperationDisableFeaturesRequest, ObjectMap) {
                                         mock_remove_object_map_request, 0);
   expect_set_flags_request_send(mock_image_ctx,
                                 mock_set_flags_request, 0);
-  expect_notify_update(mock_image_ctx);
   expect_unblock_requests(mock_image_ctx);
   expect_unblock_writes(mock_image_ctx);
   expect_handle_prepare_lock_complete(mock_image_ctx);
+  expect_notify_update(mock_image_ctx);
   expect_commit_op_event(mock_image_ctx, 0);
 
   C_SaferCond cond_ctx;
@@ -476,10 +476,10 @@ TEST_F(TestMockOperationDisableFeaturesRequest, Mirroring) {
   expect_close_journal(mock_image_ctx, 0);
   expect_remove_journal_request_send(mock_image_ctx,
                                      mock_remove_journal_request, 0);
-  expect_notify_update(mock_image_ctx);
   expect_unblock_requests(mock_image_ctx);
   expect_unblock_writes(mock_image_ctx);
   expect_handle_prepare_lock_complete(mock_image_ctx);
+  expect_notify_update(mock_image_ctx);
 
   C_SaferCond cond_ctx;
   MockDisableFeaturesRequest *req = new MockDisableFeaturesRequest(
@@ -519,10 +519,10 @@ TEST_F(TestMockOperationDisableFeaturesRequest, MirroringError) {
   expect_close_journal(mock_image_ctx, 0);
   expect_remove_journal_request_send(mock_image_ctx,
                                      mock_remove_journal_request, 0);
-  expect_notify_update(mock_image_ctx);
   expect_unblock_requests(mock_image_ctx);
   expect_unblock_writes(mock_image_ctx);
   expect_handle_prepare_lock_complete(mock_image_ctx);
+  expect_notify_update(mock_image_ctx);
 
   C_SaferCond cond_ctx;
   MockDisableFeaturesRequest *req = new MockDisableFeaturesRequest(

src/test/librbd/operation/test_mock_EnableFeaturesRequest.cc

@@ -332,10 +332,10 @@ TEST_F(TestMockOperationEnableFeaturesRequest, All) {
     expect_create_object_map_request_send(mock_image_ctx,
                                           mock_create_object_map_request, 0);
   }
-  expect_notify_update(mock_image_ctx);
   expect_unblock_requests(mock_image_ctx);
   expect_unblock_writes(mock_image_ctx);
   expect_handle_prepare_lock_complete(mock_image_ctx);
+  expect_notify_update(mock_image_ctx);
 
   C_SaferCond cond_ctx;
   MockEnableFeaturesRequest *req = new MockEnableFeaturesRequest(
@@ -383,10 +383,10 @@ TEST_F(TestMockOperationEnableFeaturesRequest, ObjectMap) {
                                 mock_set_flags_request, 0);
   expect_create_object_map_request_send(mock_image_ctx,
                                         mock_create_object_map_request, 0);
-  expect_notify_update(mock_image_ctx);
   expect_unblock_requests(mock_image_ctx);
   expect_unblock_writes(mock_image_ctx);
   expect_handle_prepare_lock_complete(mock_image_ctx);
+  expect_notify_update(mock_image_ctx);
   expect_commit_op_event(mock_image_ctx, 0);
 
   C_SaferCond cond_ctx;
@@ -532,10 +532,10 @@ TEST_F(TestMockOperationEnableFeaturesRequest, Mirroring) {
                                      mock_create_journal_request, 0);
   expect_enable_mirror_request_send(mock_image_ctx,
                                     mock_enable_mirror_request, 0);
-  expect_notify_update(mock_image_ctx);
   expect_unblock_requests(mock_image_ctx);
   expect_unblock_writes(mock_image_ctx);
   expect_handle_prepare_lock_complete(mock_image_ctx);
+  expect_notify_update(mock_image_ctx);
 
   C_SaferCond cond_ctx;
   MockEnableFeaturesRequest *req = new MockEnableFeaturesRequest(
@@ -625,10 +625,10 @@ TEST_F(TestMockOperationEnableFeaturesRequest, MirroringError) {
                                      mock_create_journal_request, 0);
   expect_enable_mirror_request_send(mock_image_ctx,
                                     mock_enable_mirror_request, -EINVAL);
-  expect_notify_update(mock_image_ctx);
   expect_unblock_requests(mock_image_ctx);
   expect_unblock_writes(mock_image_ctx);
   expect_handle_prepare_lock_complete(mock_image_ctx);
+  expect_notify_update(mock_image_ctx);
 
   C_SaferCond cond_ctx;
   MockEnableFeaturesRequest *req = new MockEnableFeaturesRequest(