From 0a31c55ea83d85da88c7586c9a8fa8d6ec6618a7 Mon Sep 17 00:00:00 2001
From: Jason Dillaman <dillaman@redhat.com>
Date: Fri, 14 Sep 2018 11:46:13 -0400
Subject: [PATCH] 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>
---
 .../object_map/SnapshotRemoveRequest.cc       | 23 +++++++++++--------
 src/librbd/object_map/SnapshotRemoveRequest.h |  2 ++
 .../test_mock_SnapshotRemoveRequest.cc        | 13 +++++++++++
 3 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/src/librbd/object_map/SnapshotRemoveRequest.cc b/src/librbd/object_map/SnapshotRemoveRequest.cc
index ca28ba5a0a261..932b7095bf879 100644
--- a/src/librbd/object_map/SnapshotRemoveRequest.cc
+++ b/src/librbd/object_map/SnapshotRemoveRequest.cc
@@ -22,17 +22,11 @@ void SnapshotRemoveRequest::send() {
   ceph_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);
     ceph_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 ede490045579a..bd020d1acbd00 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 33c93cf94fd62..b9dd81689b472 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);
 }
 
-- 
2.39.5