From: Mykola Golub <mgolub@suse.com>
Date: Tue, 9 Jun 2020 15:25:03 +0000 (+0100)
Subject: librbd: fix potential race when notify quiesce fails
X-Git-Tag: v16.1.0~2015^2~4
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=60e084a927dfc15bb65af03005fa4e35e08dc55d;p=ceph.git

librbd: fix potential race when notify quiesce fails

In SnapshotCreateRequest, handle_notify_quiesce could return
earlier than notify_quiesce. As the result m_request_id was not
properly set at the moment when notify_unquiesce was called.

Signed-off-by: Mykola Golub <mgolub@suse.com>
---

diff --git a/src/librbd/ImageWatcher.cc b/src/librbd/ImageWatcher.cc
index 33bf2e7a0d83..15421cb487a3 100644
--- a/src/librbd/ImageWatcher.cc
+++ b/src/librbd/ImageWatcher.cc
@@ -341,21 +341,20 @@ void ImageWatcher<I>::notify_header_update(librados::IoCtx &io_ctx,
 }
 
 template <typename I>
-uint64_t ImageWatcher<I>::notify_quiesce(ProgressContext &prog_ctx,
-                                         Context *on_finish) {
-  uint64_t request_id = m_image_ctx.operations->reserve_async_request_id();
+void ImageWatcher<I>::notify_quiesce(uint64_t *request_id,
+                                     ProgressContext &prog_ctx,
+                                     Context *on_finish) {
+  *request_id = m_image_ctx.operations->reserve_async_request_id();
 
   ldout(m_image_ctx.cct, 10) << this << " " << __func__ << ": request_id="
                              << request_id << dendl;
 
-  AsyncRequestId async_request_id(get_client_id(), request_id);
+  AsyncRequestId async_request_id(get_client_id(), *request_id);
 
   auto attempts = m_image_ctx.config.template get_val<uint64_t>(
     "rbd_quiesce_notification_attempts");
 
   notify_quiesce(async_request_id, attempts, prog_ctx, on_finish);
-
-  return request_id;
 }
 
 template <typename I>
diff --git a/src/librbd/ImageWatcher.h b/src/librbd/ImageWatcher.h
index ed3db723a007..0c9a634f4b81 100644
--- a/src/librbd/ImageWatcher.h
+++ b/src/librbd/ImageWatcher.h
@@ -73,7 +73,8 @@ public:
   static void notify_header_update(librados::IoCtx &io_ctx,
                                    const std::string &oid);
 
-  uint64_t notify_quiesce(ProgressContext &prog_ctx, Context *on_finish);
+  void notify_quiesce(uint64_t *request_id, ProgressContext &prog_ctx,
+                      Context *on_finish);
   void notify_unquiesce(uint64_t request_id, Context *on_finish);
 
 private:
diff --git a/src/librbd/api/Group.cc b/src/librbd/api/Group.cc
index 6af1ed1d54f3..2beca5adb0b9 100644
--- a/src/librbd/api/Group.cc
+++ b/src/librbd/api/Group.cc
@@ -467,8 +467,8 @@ int notify_quiesce(std::vector<I*> &ictxs, ProgressContext &prog_ctx,
   for (int i = 0; i < image_count; ++i) {
     auto ictx = ictxs[i];
 
-    (*requests)[i] = ictx->image_watcher->notify_quiesce(prog_ctx,
-                                                         &on_finishes[i]);
+    ictx->image_watcher->notify_quiesce(&(*requests)[i], prog_ctx,
+                                        &on_finishes[i]);
   }
 
   int ret_code = 0;
diff --git a/src/librbd/operation/SnapshotCreateRequest.cc b/src/librbd/operation/SnapshotCreateRequest.cc
index bf3b37431845..8dc7f706e3a2 100644
--- a/src/librbd/operation/SnapshotCreateRequest.cc
+++ b/src/librbd/operation/SnapshotCreateRequest.cc
@@ -65,8 +65,8 @@ void SnapshotCreateRequest<I>::send_notify_quiesce() {
   CephContext *cct = image_ctx.cct;
   ldout(cct, 5) << this << " " << __func__ << dendl;
 
-  m_request_id = image_ctx.image_watcher->notify_quiesce(
-    m_prog_ctx, create_async_context_callback(
+  image_ctx.image_watcher->notify_quiesce(
+      &m_request_id, m_prog_ctx, create_async_context_callback(
       image_ctx, create_context_callback<SnapshotCreateRequest<I>,
       &SnapshotCreateRequest<I>::handle_notify_quiesce>(this)));
 }
diff --git a/src/test/librbd/mock/MockImageWatcher.h b/src/test/librbd/mock/MockImageWatcher.h
index f58bb2ba9622..0f010564115d 100644
--- a/src/test/librbd/mock/MockImageWatcher.h
+++ b/src/test/librbd/mock/MockImageWatcher.h
@@ -25,7 +25,7 @@ struct MockImageWatcher {
   MOCK_METHOD0(notify_released_lock, void());
   MOCK_METHOD0(notify_request_lock, void());
 
-  MOCK_METHOD2(notify_quiesce, uint64_t(ProgressContext &, Context *));
+  MOCK_METHOD3(notify_quiesce, void(uint64_t *, ProgressContext &, Context *));
   MOCK_METHOD2(notify_unquiesce, void(uint64_t, Context *));
 };
 
diff --git a/src/test/librbd/operation/test_mock_SnapshotCreateRequest.cc b/src/test/librbd/operation/test_mock_SnapshotCreateRequest.cc
index efc9001a8857..cf56f9e29804 100644
--- a/src/test/librbd/operation/test_mock_SnapshotCreateRequest.cc
+++ b/src/test/librbd/operation/test_mock_SnapshotCreateRequest.cc
@@ -63,10 +63,9 @@ public:
   typedef mirror::snapshot::SetImageStateRequest<MockImageCtx> MockSetImageStateRequest;
 
   void expect_notify_quiesce(MockImageCtx &mock_image_ctx, int r) {
-    EXPECT_CALL(*mock_image_ctx.image_watcher, notify_quiesce(_, _))
-      .WillOnce(DoAll(WithArg<1>(CompleteContext(
-                                   r, mock_image_ctx.image_ctx->op_work_queue)),
-                      Return(0)));
+    EXPECT_CALL(*mock_image_ctx.image_watcher, notify_quiesce(_, _, _))
+      .WillOnce(WithArg<2>(CompleteContext(
+                             r, mock_image_ctx.image_ctx->op_work_queue)));
   }
 
   void expect_block_writes(MockImageCtx &mock_image_ctx) {