librbd: test_flags helper should require snap id parameter

The HEAD and snapshots have potentially different flag states since object maps get invalidated per revision. Signed-off-by: Jason Dillaman <dillaman@redhat.com>
ceph · Sep 14, 2018 · 8620827 · 8620827
1 parent b035df6
commit 8620827
Show file tree

Hide file tree

Showing 11 changed files with 53 additions and 29 deletions.
diff --git a/src/librbd/ImageCtx.cc b/src/librbd/ImageCtx.cc
@@ -573,18 +573,20 @@ class SafeTimerSingleton : public SafeTimer {
     return -ENOENT;
   }
 
-  int ImageCtx::test_flags(uint64_t flags, bool *flags_set) const
+  int ImageCtx::test_flags(librados::snap_t in_snap_id,
+                           uint64_t flags, bool *flags_set) const
   {
     RWLock::RLocker l(snap_lock);
-    return test_flags(flags, snap_lock, flags_set);
+    return test_flags(in_snap_id, flags, snap_lock, flags_set);
   }
 
-  int ImageCtx::test_flags(uint64_t flags, const RWLock &in_snap_lock,
+  int ImageCtx::test_flags(librados::snap_t in_snap_id,
+                           uint64_t flags, const RWLock &in_snap_lock,
                            bool *flags_set) const
   {
     ceph_assert(snap_lock.is_locked());
     uint64_t snap_flags;
-    int r = get_flags(snap_id, &snap_flags);
+    int r = get_flags(in_snap_id, &snap_flags);
     if (r < 0) {
       return r;
     }

diff --git a/src/librbd/ImageCtx.h b/src/librbd/ImageCtx.h
@@ -303,8 +303,10 @@ namespace librbd {
     bool test_op_features(uint64_t op_features,
                           const RWLock &in_snap_lock) const;
     int get_flags(librados::snap_t in_snap_id, uint64_t *flags) const;
-    int test_flags(uint64_t test_flags, bool *flags_set) const;
-    int test_flags(uint64_t test_flags, const RWLock &in_snap_lock,
+    int test_flags(librados::snap_t in_snap_id,
+                   uint64_t test_flags, bool *flags_set) const;
+    int test_flags(librados::snap_t in_snap_id,
+                   uint64_t test_flags, const RWLock &in_snap_lock,
                    bool *flags_set) const;
     int update_flags(librados::snap_t in_snap_id, uint64_t flag, bool enabled);
 

diff --git a/src/librbd/ObjectMap.cc b/src/librbd/ObjectMap.cc
@@ -96,7 +96,8 @@ bool ObjectMap<I>::object_may_exist(uint64_t object_no) const
   }
 
   bool flags_set;
-  int r = m_image_ctx.test_flags(RBD_FLAG_OBJECT_MAP_INVALID,
+  int r = m_image_ctx.test_flags(m_image_ctx.snap_id,
+                                 RBD_FLAG_OBJECT_MAP_INVALID,
                                  m_image_ctx.snap_lock, &flags_set);
   if (r < 0 || flags_set) {
     return true;
@@ -122,7 +123,8 @@ bool ObjectMap<I>::object_may_not_exist(uint64_t object_no) const
   }
 
   bool flags_set;
-  int r = m_image_ctx.test_flags(RBD_FLAG_OBJECT_MAP_INVALID,
+  int r = m_image_ctx.test_flags(m_image_ctx.snap_id,
+                                 RBD_FLAG_OBJECT_MAP_INVALID,
                                  m_image_ctx.snap_lock, &flags_set);
   if (r < 0 || flags_set) {
     return true;

diff --git a/src/librbd/object_map/Request.cc b/src/librbd/object_map/Request.cc
@@ -49,7 +49,8 @@ bool Request::should_complete(int r) {
 
 bool Request::invalidate() {
   bool flags_set;
-  int r = m_image_ctx.test_flags(RBD_FLAG_OBJECT_MAP_INVALID, &flags_set);
+  int r = m_image_ctx.test_flags(m_snap_id, RBD_FLAG_OBJECT_MAP_INVALID,
+                                 &flags_set);
   if (r == 0 && flags_set) {
     return true;
   }

diff --git a/src/librbd/operation/SnapshotRemoveRequest.cc b/src/librbd/operation/SnapshotRemoveRequest.cc
@@ -267,7 +267,6 @@ void SnapshotRemoveRequest<I>::release_snap_id() {
   ldout(cct, 5) << "snap_name=" << m_snap_name << ", "
                 << "snap_id=" << m_snap_id << dendl;
 
-
   auto aio_comp = create_rados_callback<
     SnapshotRemoveRequest<I>,
     &SnapshotRemoveRequest<I>::handle_release_snap_id>(this);

diff --git a/src/test/librbd/object_map/test_mock_InvalidateRequest.cc b/src/test/librbd/object_map/test_mock_InvalidateRequest.cc
@@ -28,7 +28,8 @@ TEST_F(TestMockObjectMapInvalidateRequest, UpdatesInMemoryFlag) {
   librbd::ImageCtx *ictx;
   ASSERT_EQ(0, open_image(m_image_name, &ictx));
   bool flags_set;
-  ASSERT_EQ(0, ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID, &flags_set));
+  ASSERT_EQ(0, ictx->test_flags(CEPH_NOSNAP,
+                                RBD_FLAG_OBJECT_MAP_INVALID, &flags_set));
   ASSERT_FALSE(flags_set);
 
   C_SaferCond cond_ctx;
@@ -45,7 +46,8 @@ TEST_F(TestMockObjectMapInvalidateRequest, UpdatesInMemoryFlag) {
   }
   ASSERT_EQ(0, cond_ctx.wait());
 
-  ASSERT_EQ(0, ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID, &flags_set));
+  ASSERT_EQ(0, ictx->test_flags(CEPH_NOSNAP,
+                                RBD_FLAG_OBJECT_MAP_INVALID, &flags_set));
   ASSERT_TRUE(flags_set);
 }
 

diff --git a/src/test/librbd/object_map/test_mock_SnapshotRollbackRequest.cc b/src/test/librbd/object_map/test_mock_SnapshotRollbackRequest.cc
@@ -105,7 +105,8 @@ TEST_F(TestMockObjectMapSnapshotRollbackRequest, ReadMapError) {
     ASSERT_NE(0U, flags & RBD_FLAG_OBJECT_MAP_INVALID);
   }
   bool flags_set;
-  ASSERT_EQ(0, ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID, &flags_set));
+  ASSERT_EQ(0, ictx->test_flags(CEPH_NOSNAP,
+                                RBD_FLAG_OBJECT_MAP_INVALID, &flags_set));
   ASSERT_TRUE(flags_set);
   expect_unlock_exclusive_lock(*ictx);
 }
@@ -136,7 +137,8 @@ TEST_F(TestMockObjectMapSnapshotRollbackRequest, WriteMapError) {
     ASSERT_EQ(0U, flags & RBD_FLAG_OBJECT_MAP_INVALID);
   }
   bool flags_set;
-  ASSERT_EQ(0, ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID, &flags_set));
+  ASSERT_EQ(0, ictx->test_flags(CEPH_NOSNAP,
+                                RBD_FLAG_OBJECT_MAP_INVALID, &flags_set));
   ASSERT_TRUE(flags_set);
   expect_unlock_exclusive_lock(*ictx);
 }

diff --git a/src/test/librbd/test_DeepCopy.cc b/src/test/librbd/test_DeepCopy.cc
@@ -87,8 +87,10 @@ struct TestDeepCopy : public TestFixture {
 
       if (m_dst_ictx->test_features(RBD_FEATURE_LAYERING)) {
         bool flags_set;
-        EXPECT_EQ(0, m_dst_ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID,
-                                            &flags_set));
+        RWLock::RLocker dst_locker(m_dst_ictx->snap_lock);
+        EXPECT_EQ(0, m_dst_ictx->test_flags(m_dst_ictx->snap_id,
+                                            RBD_FLAG_OBJECT_MAP_INVALID,
+                                            m_dst_ictx->snap_lock, &flags_set));
         EXPECT_FALSE(flags_set);
       }
 

diff --git a/src/test/librbd/test_Migration.cc b/src/test/librbd/test_Migration.cc
@@ -104,8 +104,10 @@ struct TestMigration : public TestFixture {
 
     if (dst_ictx->test_features(RBD_FEATURE_LAYERING)) {
       bool flags_set;
-      EXPECT_EQ(0, dst_ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID,
-                                        &flags_set));
+      RWLock::RLocker dst_locker(dst_ictx->snap_lock);
+      EXPECT_EQ(0, dst_ictx->test_flags(dst_ictx->snap_id,
+                                        RBD_FLAG_OBJECT_MAP_INVALID,
+                                        dst_ictx->snap_lock, &flags_set));
       EXPECT_FALSE(flags_set);
     }
 

diff --git a/src/test/librbd/test_ObjectMap.cc b/src/test/librbd/test_ObjectMap.cc
@@ -31,7 +31,8 @@ TEST_F(TestObjectMap, RefreshInvalidatesWhenCorrupt) {
   librbd::ImageCtx *ictx;
   ASSERT_EQ(0, open_image(m_image_name, &ictx));
   bool flags_set;
-  ASSERT_EQ(0, ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID, &flags_set));
+  ASSERT_EQ(0, ictx->test_flags(CEPH_NOSNAP, RBD_FLAG_OBJECT_MAP_INVALID,
+                                &flags_set));
   ASSERT_FALSE(flags_set);
 
   C_SaferCond lock_ctx;
@@ -47,7 +48,8 @@ TEST_F(TestObjectMap, RefreshInvalidatesWhenCorrupt) {
   ASSERT_EQ(0, ictx->md_ctx.write_full(oid, bl));
 
   ASSERT_EQ(0, when_open_object_map(ictx));
-  ASSERT_EQ(0, ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID, &flags_set));
+  ASSERT_EQ(0, ictx->test_flags(CEPH_NOSNAP, RBD_FLAG_OBJECT_MAP_INVALID,
+                                &flags_set));
   ASSERT_TRUE(flags_set);
 }
 
@@ -57,7 +59,8 @@ TEST_F(TestObjectMap, RefreshInvalidatesWhenTooSmall) {
   librbd::ImageCtx *ictx;
   ASSERT_EQ(0, open_image(m_image_name, &ictx));
   bool flags_set;
-  ASSERT_EQ(0, ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID, &flags_set));
+  ASSERT_EQ(0, ictx->test_flags(CEPH_NOSNAP, RBD_FLAG_OBJECT_MAP_INVALID,
+                                &flags_set));
   ASSERT_FALSE(flags_set);
 
   C_SaferCond lock_ctx;
@@ -74,7 +77,8 @@ TEST_F(TestObjectMap, RefreshInvalidatesWhenTooSmall) {
   ASSERT_EQ(0, ictx->md_ctx.operate(oid, &op));
 
   ASSERT_EQ(0, when_open_object_map(ictx));
-  ASSERT_EQ(0, ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID, &flags_set));
+  ASSERT_EQ(0, ictx->test_flags(CEPH_NOSNAP, RBD_FLAG_OBJECT_MAP_INVALID,
+                                &flags_set));
   ASSERT_TRUE(flags_set);
 }
 
@@ -84,7 +88,8 @@ TEST_F(TestObjectMap, InvalidateFlagOnDisk) {
   librbd::ImageCtx *ictx;
   ASSERT_EQ(0, open_image(m_image_name, &ictx));
   bool flags_set;
-  ASSERT_EQ(0, ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID, &flags_set));
+  ASSERT_EQ(0, ictx->test_flags(CEPH_NOSNAP, RBD_FLAG_OBJECT_MAP_INVALID,
+                                &flags_set));
   ASSERT_FALSE(flags_set);
 
   C_SaferCond lock_ctx;
@@ -100,11 +105,13 @@ TEST_F(TestObjectMap, InvalidateFlagOnDisk) {
   ASSERT_EQ(0, ictx->md_ctx.write_full(oid, bl));
 
   ASSERT_EQ(0, when_open_object_map(ictx));
-  ASSERT_EQ(0, ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID, &flags_set));
+  ASSERT_EQ(0, ictx->test_flags(CEPH_NOSNAP, RBD_FLAG_OBJECT_MAP_INVALID,
+                                &flags_set));
   ASSERT_TRUE(flags_set);
 
   ASSERT_EQ(0, open_image(m_image_name, &ictx));
-  ASSERT_EQ(0, ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID, &flags_set));
+  ASSERT_EQ(0, ictx->test_flags(CEPH_NOSNAP, RBD_FLAG_OBJECT_MAP_INVALID,
+                                &flags_set));
   ASSERT_TRUE(flags_set);
 }
 
@@ -114,7 +121,8 @@ TEST_F(TestObjectMap, AcquireLockInvalidatesWhenTooSmall) {
   librbd::ImageCtx *ictx;
   ASSERT_EQ(0, open_image(m_image_name, &ictx));
   bool flags_set;
-  ASSERT_EQ(0, ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID, &flags_set));
+  ASSERT_EQ(0, ictx->test_flags(CEPH_NOSNAP, RBD_FLAG_OBJECT_MAP_INVALID,
+                                &flags_set));
   ASSERT_FALSE(flags_set);
 
   librados::ObjectWriteOperation op;
@@ -130,12 +138,13 @@ TEST_F(TestObjectMap, AcquireLockInvalidatesWhenTooSmall) {
   }
   ASSERT_EQ(0, lock_ctx.wait());
 
-  ASSERT_EQ(0, ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID, &flags_set));
+  ASSERT_EQ(0, ictx->test_flags(CEPH_NOSNAP, RBD_FLAG_OBJECT_MAP_INVALID,
+                                &flags_set));
   ASSERT_TRUE(flags_set);
 
   // Test the flag is stored on disk
   ASSERT_EQ(0, ictx->state->refresh());
-  ASSERT_EQ(0, ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID,
+  ASSERT_EQ(0, ictx->test_flags(CEPH_NOSNAP, RBD_FLAG_OBJECT_MAP_INVALID,
                                 &flags_set));
   ASSERT_TRUE(flags_set);
 }
diff --git a/src/test/rbd_mirror/test_ImageSync.cc b/src/test/rbd_mirror/test_ImageSync.cc
@@ -319,7 +319,8 @@ TEST_F(TestImageSync, SnapshotStress) {
       local_size = m_local_image_ctx->get_image_size(
         m_local_image_ctx->snap_id);
       bool flags_set;
-      ASSERT_EQ(0, m_local_image_ctx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID,
+      ASSERT_EQ(0, m_local_image_ctx->test_flags(m_local_image_ctx->snap_id,
+                                                 RBD_FLAG_OBJECT_MAP_INVALID,
                                                  m_local_image_ctx->snap_lock,
                                                  &flags_set));
       ASSERT_FALSE(flags_set);