From: Jason Dillaman <dillaman@redhat.com>
Date: Fri, 14 Sep 2018 15:46:13 +0000 (-0400)
Subject: librbd: do not invalidate object map when attempting to delete non-existent snapshot
X-Git-Tag: v12.2.9~15^2~2
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=f63078017b3785d88dc139bb92b935d9f27c24d5;p=ceph.git

librbd: do not invalidate object map when attempting to delete non-existent snapshot

If duplicate snapshot remove requests are received by the lock owner from a peer
client, the first request will remove the object map. If the second request
arrives while the first is in-progress, it will again attempt to remove the
object map but fail to load it since it's already been deleted. This incorrectly
results in the next object map being flagged as invalid.

Fixes: http://tracker.ceph.com/issues/24516
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit 0a31c55ea83d85da88c7586c9a8fa8d6ec6618a7)

Conflicts:
	src/librbd/object_map/SnapshotRemoveRequest.cc: trivial resolution
---

diff --git a/src/librbd/object_map/SnapshotRemoveRequest.cc b/src/librbd/object_map/SnapshotRemoveRequest.cc
index 4dc7d95a1f1b..3efea66b3d14 100644
--- a/src/librbd/object_map/SnapshotRemoveRequest.cc
+++ b/src/librbd/object_map/SnapshotRemoveRequest.cc
@@ -22,17 +22,11 @@ void SnapshotRemoveRequest::send() {
   assert(m_image_ctx.snap_lock.is_wlocked());
 
   if ((m_image_ctx.features & RBD_FEATURE_FAST_DIFF) != 0) {
-    compute_next_snap_id();
-
-    uint64_t flags;
-    int r = m_image_ctx.get_flags(m_snap_id, &flags);
+    int r = m_image_ctx.get_flags(m_snap_id, &m_flags);
     assert(r == 0);
 
-    if ((flags & RBD_FLAG_OBJECT_MAP_INVALID) != 0) {
-      invalidate_next_map();
-    } else {
-      load_map();
-    }
+    compute_next_snap_id();
+    load_map();
   } else {
     remove_map();
   }
@@ -63,6 +57,8 @@ void SnapshotRemoveRequest::handle_load_map(int r) {
     r = cls_client::object_map_load_finish(&it, &m_snap_object_map);
   }
   if (r == -ENOENT) {
+    // implies we have already deleted this snapshot and handled the
+    // necessary fast-diff cleanup
     complete(0);
     return;
   } else if (r < 0) {
@@ -80,6 +76,15 @@ void SnapshotRemoveRequest::handle_load_map(int r) {
 }
 
 void SnapshotRemoveRequest::remove_snapshot() {
+  if ((m_flags & RBD_FLAG_OBJECT_MAP_INVALID) != 0) {
+    // snapshot object map exists on disk but is invalid. cannot clean fast-diff
+    // on next snapshot if current snapshot was invalid.
+    RWLock::RLocker owner_locker(m_image_ctx.owner_lock);
+    RWLock::WLocker snap_locker(m_image_ctx.snap_lock);
+    invalidate_next_map();
+    return;
+  }
+
   CephContext *cct = m_image_ctx.cct;
   std::string oid(ObjectMap<>::object_map_name(m_image_ctx.id, m_next_snap_id));
   ldout(cct, 5) << "oid=" << oid << dendl;
diff --git a/src/librbd/object_map/SnapshotRemoveRequest.h b/src/librbd/object_map/SnapshotRemoveRequest.h
index ede490045579..bd020d1acbd0 100644
--- a/src/librbd/object_map/SnapshotRemoveRequest.h
+++ b/src/librbd/object_map/SnapshotRemoveRequest.h
@@ -58,6 +58,8 @@ private:
   uint64_t m_snap_id;
   uint64_t m_next_snap_id;
 
+  uint64_t m_flags = 0;
+
   ceph::BitVector<2> m_snap_object_map;
   bufferlist m_out_bl;
 
diff --git a/src/test/librbd/object_map/test_mock_SnapshotRemoveRequest.cc b/src/test/librbd/object_map/test_mock_SnapshotRemoveRequest.cc
index 38c9fe49e000..70959681d702 100644
--- a/src/test/librbd/object_map/test_mock_SnapshotRemoveRequest.cc
+++ b/src/test/librbd/object_map/test_mock_SnapshotRemoveRequest.cc
@@ -107,6 +107,10 @@ TEST_F(TestMockObjectMapSnapshotRemoveRequest, LoadMapMissing) {
   ASSERT_EQ(0, ictx->state->refresh_if_required());
 
   uint64_t snap_id = ictx->snap_info.rbegin()->first;
+  auto snap_it = ictx->snap_info.find(snap_id);
+  ASSERT_NE(ictx->snap_info.end(), snap_it);
+  snap_it->second.flags |= RBD_FLAG_OBJECT_MAP_INVALID;
+
   expect_load_map(ictx, snap_id, -ENOENT);
 
   ceph::BitVector<2> object_map;
@@ -120,6 +124,15 @@ TEST_F(TestMockObjectMapSnapshotRemoveRequest, LoadMapMissing) {
   }
   ASSERT_EQ(0, cond_ctx.wait());
 
+  {
+    // shouldn't invalidate the HEAD revision when we fail to load
+    // the already deleted snapshot
+    RWLock::RLocker snap_locker(ictx->snap_lock);
+    uint64_t flags;
+    ASSERT_EQ(0, ictx->get_flags(CEPH_NOSNAP, &flags));
+    ASSERT_EQ(0U, flags & RBD_FLAG_OBJECT_MAP_INVALID);
+  }
+
   expect_unlock_exclusive_lock(*ictx);
 }