From 391936a30cc6022f067cb11c2e39bd47b9e58d61 Mon Sep 17 00:00:00 2001 From: Lu Shi Date: Wed, 15 Jun 2016 09:24:43 +0800 Subject: [PATCH] librbd: enable/disable of features is not allowed when already enabled/disabled Fixes: http://tracker.ceph.com/issues/16079 Signed-off-by: Lu Shi (cherry picked from commit a8a633396a4105991c9643c2b39391621934c26d) --- src/librbd/internal.cc | 12 ++- .../librbd/image/test_mock_RefreshRequest.cc | 86 ++++++++++++++----- src/test/librbd/test_librbd.cc | 12 ++- src/test/librbd/test_mirroring.cc | 6 +- 4 files changed, 82 insertions(+), 34 deletions(-) diff --git a/src/librbd/internal.cc b/src/librbd/internal.cc index a999f6f79b0d1..07bc843537a18 100644 --- a/src/librbd/internal.cc +++ b/src/librbd/internal.cc @@ -1719,17 +1719,21 @@ int mirror_image_disable_internal(ImageCtx *ictx, bool force, RWLock::WLocker snap_locker(ictx->snap_lock); uint64_t new_features; if (enabled) { + if ((features & ictx->features) != 0) { + lderr(cct) << "one or more requested features are already enabled" << dendl; + return -EINVAL; + } features &= ~ictx->features; new_features = ictx->features | features; } else { + if ((features & ~ictx->features) != 0) { + lderr(cct) << "one or more requested features are already disabled" << dendl; + return -EINVAL; + } features &= ictx->features; new_features = ictx->features & ~features; } - if (features == 0) { - return 0; - } - bool enable_mirroring = false; uint64_t features_mask = features; uint64_t disable_flags = 0; diff --git a/src/test/librbd/image/test_mock_RefreshRequest.cc b/src/test/librbd/image/test_mock_RefreshRequest.cc index 05034919b149f..eff4b9dadad41 100644 --- a/src/test/librbd/image/test_mock_RefreshRequest.cc +++ b/src/test/librbd/image/test_mock_RefreshRequest.cc @@ -503,12 +503,22 @@ TEST_F(TestMockImageRefreshRequest, DisableExclusiveLock) { mock_image_ctx.journal = &mock_journal; } - ASSERT_EQ(0, update_features(ictx, - RBD_FEATURE_EXCLUSIVE_LOCK | - RBD_FEATURE_OBJECT_MAP | - RBD_FEATURE_FAST_DIFF | - RBD_FEATURE_JOURNALING, false)); + if (ictx->test_features(RBD_FEATURE_JOURNALING)) { + ASSERT_EQ(0, update_features(ictx, RBD_FEATURE_JOURNALING, false)); + } + + if (ictx->test_features(RBD_FEATURE_FAST_DIFF)) { + ASSERT_EQ(0, update_features(ictx, RBD_FEATURE_FAST_DIFF , false)); + } + if (ictx->test_features(RBD_FEATURE_OBJECT_MAP)) { + ASSERT_EQ(0, update_features(ictx, RBD_FEATURE_OBJECT_MAP , false)); + } + + if (ictx->test_features(RBD_FEATURE_EXCLUSIVE_LOCK)) { + ASSERT_EQ(0, update_features(ictx, RBD_FEATURE_EXCLUSIVE_LOCK , false)); + } + expect_op_work_queue(mock_image_ctx); expect_test_features(mock_image_ctx); @@ -539,11 +549,21 @@ TEST_F(TestMockImageRefreshRequest, DisableExclusiveLockWhileAcquiringLock) { MockExclusiveLock mock_exclusive_lock; mock_image_ctx.exclusive_lock = &mock_exclusive_lock; - ASSERT_EQ(0, update_features(ictx, - RBD_FEATURE_EXCLUSIVE_LOCK | - RBD_FEATURE_OBJECT_MAP | - RBD_FEATURE_FAST_DIFF | - RBD_FEATURE_JOURNALING, false)); + if (ictx->test_features(RBD_FEATURE_JOURNALING)) { + ASSERT_EQ(0, update_features(ictx, RBD_FEATURE_JOURNALING, false)); + } + + if (ictx->test_features(RBD_FEATURE_FAST_DIFF)) { + ASSERT_EQ(0, update_features(ictx, RBD_FEATURE_FAST_DIFF , false)); + } + + if (ictx->test_features(RBD_FEATURE_OBJECT_MAP)) { + ASSERT_EQ(0, update_features(ictx, RBD_FEATURE_OBJECT_MAP , false)); + } + + if (ictx->test_features(RBD_FEATURE_EXCLUSIVE_LOCK)) { + ASSERT_EQ(0, update_features(ictx, RBD_FEATURE_EXCLUSIVE_LOCK , false)); + } expect_op_work_queue(mock_image_ctx); expect_test_features(mock_image_ctx); @@ -567,9 +587,13 @@ TEST_F(TestMockImageRefreshRequest, EnableJournalWithExclusiveLock) { librbd::ImageCtx *ictx; ASSERT_EQ(0, open_image(m_image_name, &ictx)); - ASSERT_EQ(0, update_features(ictx, - RBD_FEATURE_OBJECT_MAP | - RBD_FEATURE_FAST_DIFF, false)); + if (ictx->test_features(RBD_FEATURE_FAST_DIFF)) { + ASSERT_EQ(0, update_features(ictx, RBD_FEATURE_FAST_DIFF , false)); + } + + if (ictx->test_features(RBD_FEATURE_OBJECT_MAP)) { + ASSERT_EQ(0, update_features(ictx, RBD_FEATURE_OBJECT_MAP , false)); + } MockRefreshImageCtx mock_image_ctx(*ictx); MockRefreshParentRequest mock_refresh_parent_request; @@ -603,9 +627,13 @@ TEST_F(TestMockImageRefreshRequest, EnableJournalWithoutExclusiveLock) { librbd::ImageCtx *ictx; ASSERT_EQ(0, open_image(m_image_name, &ictx)); - ASSERT_EQ(0, update_features(ictx, - RBD_FEATURE_OBJECT_MAP | - RBD_FEATURE_FAST_DIFF, false)); + if (ictx->test_features(RBD_FEATURE_FAST_DIFF)) { + ASSERT_EQ(0, update_features(ictx, RBD_FEATURE_FAST_DIFF , false)); + } + + if (ictx->test_features(RBD_FEATURE_OBJECT_MAP)) { + ASSERT_EQ(0, update_features(ictx, RBD_FEATURE_OBJECT_MAP , false)); + } MockRefreshImageCtx mock_image_ctx(*ictx); MockRefreshParentRequest mock_refresh_parent_request; @@ -651,7 +679,9 @@ TEST_F(TestMockImageRefreshRequest, DisableJournal) { MockJournal *mock_journal = new MockJournal(); mock_image_ctx.journal = mock_journal; - ASSERT_EQ(0, update_features(ictx, RBD_FEATURE_JOURNALING, false)); + if (ictx->test_features(RBD_FEATURE_JOURNALING)) { + ASSERT_EQ(0, update_features(ictx, RBD_FEATURE_JOURNALING , false)); + } expect_op_work_queue(mock_image_ctx); expect_test_features(mock_image_ctx); @@ -679,7 +709,9 @@ TEST_F(TestMockImageRefreshRequest, EnableObjectMapWithExclusiveLock) { librbd::ImageCtx *ictx; ASSERT_EQ(0, open_image(m_image_name, &ictx)); - ASSERT_EQ(0, update_features(ictx, RBD_FEATURE_JOURNALING, false)); + if (ictx->test_features(RBD_FEATURE_JOURNALING)) { + ASSERT_EQ(0, update_features(ictx, RBD_FEATURE_JOURNALING , false)); + } MockRefreshImageCtx mock_image_ctx(*ictx); MockRefreshParentRequest mock_refresh_parent_request; @@ -713,7 +745,9 @@ TEST_F(TestMockImageRefreshRequest, EnableObjectMapWithoutExclusiveLock) { librbd::ImageCtx *ictx; ASSERT_EQ(0, open_image(m_image_name, &ictx)); - ASSERT_EQ(0, update_features(ictx, RBD_FEATURE_JOURNALING, false)); + if (ictx->test_features(RBD_FEATURE_JOURNALING)) { + ASSERT_EQ(0, update_features(ictx, RBD_FEATURE_JOURNALING , false)); + } MockRefreshImageCtx mock_image_ctx(*ictx); MockRefreshParentRequest mock_refresh_parent_request; @@ -758,9 +792,13 @@ TEST_F(TestMockImageRefreshRequest, DisableObjectMap) { mock_image_ctx.journal = &mock_journal; } - ASSERT_EQ(0, update_features(ictx, - RBD_FEATURE_OBJECT_MAP | - RBD_FEATURE_FAST_DIFF, false)); + if (ictx->test_features(RBD_FEATURE_FAST_DIFF)) { + ASSERT_EQ(0, update_features(ictx, RBD_FEATURE_FAST_DIFF , false)); + } + + if (ictx->test_features(RBD_FEATURE_OBJECT_MAP)) { + ASSERT_EQ(0, update_features(ictx, RBD_FEATURE_OBJECT_MAP , false)); + } expect_op_work_queue(mock_image_ctx); expect_test_features(mock_image_ctx); @@ -785,7 +823,9 @@ TEST_F(TestMockImageRefreshRequest, OpenObjectMapError) { librbd::ImageCtx *ictx; ASSERT_EQ(0, open_image(m_image_name, &ictx)); - ASSERT_EQ(0, update_features(ictx, RBD_FEATURE_JOURNALING, false)); + if (ictx->test_features(RBD_FEATURE_JOURNALING)) { + ASSERT_EQ(0, update_features(ictx, RBD_FEATURE_JOURNALING , false)); + } MockRefreshImageCtx mock_image_ctx(*ictx); MockRefreshParentRequest mock_refresh_parent_request; diff --git a/src/test/librbd/test_librbd.cc b/src/test/librbd/test_librbd.cc index c03ce647154e3..5101a7e4b2aa6 100644 --- a/src/test/librbd/test_librbd.cc +++ b/src/test/librbd/test_librbd.cc @@ -3652,10 +3652,14 @@ TEST_F(TestLibRBD, UpdateFeatures) // must provide a single feature ASSERT_EQ(-EINVAL, image.update_features(0, true)); - ASSERT_EQ(0, image.update_features(RBD_FEATURE_EXCLUSIVE_LOCK | - RBD_FEATURE_OBJECT_MAP | - RBD_FEATURE_FAST_DIFF | - RBD_FEATURE_JOURNALING, false)); + uint64_t disable_features; + disable_features = features & (RBD_FEATURE_EXCLUSIVE_LOCK | + RBD_FEATURE_OBJECT_MAP | + RBD_FEATURE_FAST_DIFF | + RBD_FEATURE_JOURNALING); + if (disable_features != 0) { + ASSERT_EQ(0, image.update_features(disable_features, false)); + } // cannot enable object map nor journaling w/o exclusive lock ASSERT_EQ(-EINVAL, image.update_features(RBD_FEATURE_OBJECT_MAP, true)); diff --git a/src/test/librbd/test_mirroring.cc b/src/test/librbd/test_mirroring.cc index cd59dd711fbdc..fcd98ec767f0b 100644 --- a/src/test/librbd/test_mirroring.cc +++ b/src/test/librbd/test_mirroring.cc @@ -464,7 +464,7 @@ TEST_F(TestMirroring, EnableJournaling_In_MirrorModeDisabled) { uint64_t init_features = 0; init_features |= RBD_FEATURE_OBJECT_MAP; init_features |= RBD_FEATURE_EXCLUSIVE_LOCK; - uint64_t features = init_features | RBD_FEATURE_JOURNALING; + uint64_t features = RBD_FEATURE_JOURNALING; check_mirroring_on_update_features(init_features, true, false, features, 0, RBD_MIRROR_MODE_DISABLED, RBD_MIRROR_IMAGE_DISABLED); } @@ -473,7 +473,7 @@ TEST_F(TestMirroring, EnableJournaling_In_MirrorModeImage) { uint64_t init_features = 0; init_features |= RBD_FEATURE_OBJECT_MAP; init_features |= RBD_FEATURE_EXCLUSIVE_LOCK; - uint64_t features = init_features | RBD_FEATURE_JOURNALING; + uint64_t features = RBD_FEATURE_JOURNALING; check_mirroring_on_update_features(init_features, true, false, features, 0, RBD_MIRROR_MODE_IMAGE, RBD_MIRROR_IMAGE_DISABLED); } @@ -482,7 +482,7 @@ TEST_F(TestMirroring, EnableJournaling_In_MirrorModePool) { uint64_t init_features = 0; init_features |= RBD_FEATURE_OBJECT_MAP; init_features |= RBD_FEATURE_EXCLUSIVE_LOCK; - uint64_t features = init_features | RBD_FEATURE_JOURNALING; + uint64_t features = RBD_FEATURE_JOURNALING; check_mirroring_on_update_features(init_features, true, false, features, 0, RBD_MIRROR_MODE_POOL, RBD_MIRROR_IMAGE_ENABLED); } -- 2.39.5