From 70adbbe58f79c2ec72c6cd13a9482c30bfe6ecf1 Mon Sep 17 00:00:00 2001
From: Ilya Dryomov <idryomov@gmail.com>
Date: Mon, 17 Apr 2023 23:31:37 +0200
Subject: [PATCH] librbd: always refresh after creating snapshot in
 CreatePrimaryRequest

Up until now this was conditioned on whether the caller expressed
interest in the ID of the created snapshot and happened to work only
because CreatePrimaryRequest wasn't actually consulting any mirror
snapshot metadata.  This has just changed with unlink_peer() needing to
see an up-to-date complete flag which is set in SetImageStateRequest
following the write out of image state object(s).

Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
(cherry picked from commit df2bb13d0308d2d48d846d11e19e9b93e1a050a2)
---
 src/librbd/mirror/snapshot/CreatePrimaryRequest.cc    | 11 +++--------
 .../mirror/snapshot/test_mock_CreatePrimaryRequest.cc | 10 ++++++++++
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc b/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc
index eac01571d7c22..69157dccab997 100644
--- a/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc
+++ b/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc
@@ -143,13 +143,8 @@ void CreatePrimaryRequest<I>::handle_create_snapshot(int r) {
 
 template <typename I>
 void CreatePrimaryRequest<I>::refresh_image() {
-  // if snapshot created via remote RPC, refresh is required to retrieve
-  // the snapshot id
-  if (m_snap_id == nullptr) {
-    unlink_peer();
-    return;
-  }
-
+  // refresh is required to retrieve the snapshot id (if snapshot
+  // created via remote RPC) and complete flag (regardless)
   CephContext *cct = m_image_ctx->cct;
   ldout(cct, 15) << dendl;
 
@@ -170,7 +165,7 @@ void CreatePrimaryRequest<I>::handle_refresh_image(int r) {
     return;
   }
 
-  {
+  if (m_snap_id != nullptr) {
     std::shared_lock image_locker{m_image_ctx->image_lock};
     *m_snap_id = m_image_ctx->get_snap_id(
       cls::rbd::MirrorSnapshotNamespace{}, m_snap_name);
diff --git a/src/test/librbd/mirror/snapshot/test_mock_CreatePrimaryRequest.cc b/src/test/librbd/mirror/snapshot/test_mock_CreatePrimaryRequest.cc
index 1f777a1cf9ad8..538c94e1f629a 100644
--- a/src/test/librbd/mirror/snapshot/test_mock_CreatePrimaryRequest.cc
+++ b/src/test/librbd/mirror/snapshot/test_mock_CreatePrimaryRequest.cc
@@ -167,6 +167,11 @@ public:
                   ));
   }
 
+  void expect_refresh_image(MockTestImageCtx &mock_image_ctx, int r) {
+    EXPECT_CALL(*mock_image_ctx.state, refresh(_))
+      .WillOnce(CompleteContext(r, mock_image_ctx.image_ctx->op_work_queue));
+  }
+
   void expect_unlink_peer(MockTestImageCtx &mock_image_ctx,
                           MockUnlinkPeerRequest &mock_unlink_peer_request,
                           uint64_t snap_id, const std::string &peer_uuid,
@@ -214,6 +219,7 @@ TEST_F(TestMockMirrorSnapshotCreatePrimaryRequest, Success) {
                           {{"uuid", cls::rbd::MIRROR_PEER_DIRECTION_TX, "ceph",
                             "mirror", "mirror uuid"}}, 0);
   expect_create_snapshot(mock_image_ctx, 0);
+  expect_refresh_image(mock_image_ctx, 0);
 
   C_SaferCond ctx;
   auto req = new MockCreatePrimaryRequest(&mock_image_ctx, "gid", CEPH_NOSNAP,
@@ -314,6 +320,7 @@ TEST_F(TestMockMirrorSnapshotCreatePrimaryRequest, SuccessUnlinkIncomplete) {
                           {{"uuid", cls::rbd::MIRROR_PEER_DIRECTION_TX, "ceph",
                             "mirror", "mirror uuid"}}, 0);
   expect_create_snapshot(mock_image_ctx, 0);
+  expect_refresh_image(mock_image_ctx, 0);
   MockUnlinkPeerRequest mock_unlink_peer_request;
   auto it = mock_image_ctx.snap_info.rbegin();
   auto snap_id = it->first;
@@ -350,6 +357,7 @@ TEST_F(TestMockMirrorSnapshotCreatePrimaryRequest, SuccessUnlinkPeer) {
                           {{"uuid", cls::rbd::MIRROR_PEER_DIRECTION_TX, "ceph",
                             "mirror", "mirror uuid"}}, 0);
   expect_create_snapshot(mock_image_ctx, 0);
+  expect_refresh_image(mock_image_ctx, 0);
   MockUnlinkPeerRequest mock_unlink_peer_request;
   auto it = mock_image_ctx.snap_info.rbegin();
   auto snap_id = it->first;
@@ -384,6 +392,7 @@ TEST_F(TestMockMirrorSnapshotCreatePrimaryRequest, SuccessUnlinkNoPeer) {
                           {{"uuid", cls::rbd::MIRROR_PEER_DIRECTION_TX, "ceph",
                             "mirror", "mirror uuid"}}, 0);
   expect_create_snapshot(mock_image_ctx, 0);
+  expect_refresh_image(mock_image_ctx, 0);
   MockUnlinkPeerRequest mock_unlink_peer_request;
   auto it = mock_image_ctx.snap_info.rbegin();
   auto snap_id = it->first;
@@ -424,6 +433,7 @@ TEST_F(TestMockMirrorSnapshotCreatePrimaryRequest, SuccessUnlinkMultiplePeers) {
                            {"uuid2", cls::rbd::MIRROR_PEER_DIRECTION_TX, "ceph",
                             "mirror", "mirror uuid"}}, 0);
   expect_create_snapshot(mock_image_ctx, 0);
+  expect_refresh_image(mock_image_ctx, 0);
   MockUnlinkPeerRequest mock_unlink_peer_request;
   auto it = mock_image_ctx.snap_info.rbegin();
   auto snap_id = it->first;
-- 
2.39.5