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>

diff --git a/src/librbd/object_map/SnapshotRemoveRequest.cc b/src/librbd/object_map/SnapshotRemoveRequest.cc
index ca28ba5a0a261daa5f382479dd561d7c614384b4..932b7095bf879dfed52497f3bdee1b965934ab18 100644 (file)
--- 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 ede490045579af55dcfc8d47cbe2ec40ac7ba699..bd020d1acbd00c064cda4a0c634db7a76d2c2abd 100644 (file)
--- 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 33c93cf94fd6278cec55b74d322974f4ef786005..b9dd81689b47225121a9d110c51cccd009d9b18d 100644 (file)
--- 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);
 }