From 2d45060d3a426e68f71f5c4d2ac824ce10c08d0e Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Tue, 3 Mar 2020 15:17:52 -0500 Subject: [PATCH] librbd: track reason why ImageCtx is read-only This will be utilized by the RefreshRequest state machine to flag the image as read-only if the new RBD_FEATURE_NON_PRIMARY feature is enabled. Also allow that flag to be masked out by rbd-mirror daemon to permit IO and operations against a non-primary image. Signed-off-by: Jason Dillaman --- src/librbd/ImageCtx.cc | 1 + src/librbd/ImageCtx.h | 2 + src/librbd/Types.h | 5 ++ src/librbd/image/DetachChildRequest.cc | 3 + src/librbd/image/OpenRequest.cc | 2 +- src/librbd/image/RefreshRequest.cc | 38 ++++++++++-- src/librbd/image/RefreshRequest.h | 2 + .../image/test_mock_DetachChildRequest.cc | 4 +- .../librbd/image/test_mock_RefreshRequest.cc | 61 +++++++++++++++++++ src/test/librbd/mock/MockImageCtx.h | 4 ++ 10 files changed, 115 insertions(+), 7 deletions(-) diff --git a/src/librbd/ImageCtx.cc b/src/librbd/ImageCtx.cc index 65669db5bb829..4026fb934595a 100644 --- a/src/librbd/ImageCtx.cc +++ b/src/librbd/ImageCtx.cc @@ -100,6 +100,7 @@ public: snap_id(CEPH_NOSNAP), snap_exists(true), read_only(ro), + read_only_flags(ro ? IMAGE_READ_ONLY_FLAG_USER : 0U), exclusive_locked(false), name(image_name), image_watcher(NULL), diff --git a/src/librbd/ImageCtx.h b/src/librbd/ImageCtx.h index ecc4cdb400c56..11f33b1d40c66 100644 --- a/src/librbd/ImageCtx.h +++ b/src/librbd/ImageCtx.h @@ -97,6 +97,8 @@ namespace librbd { bool snap_exists; // false if our snap_id was deleted // whether the image was opened read-only. cannot be changed after opening bool read_only; + uint32_t read_only_flags = 0U; + uint32_t read_only_mask = ~0U; std::map lockers; diff --git a/src/librbd/Types.h b/src/librbd/Types.h index 3f1104478ebce..2e014b8b47643 100644 --- a/src/librbd/Types.h +++ b/src/librbd/Types.h @@ -94,6 +94,11 @@ enum { OPEN_FLAG_IGNORE_MIGRATING = 1 << 2 }; +enum ImageReadOnlyFlag { + IMAGE_READ_ONLY_FLAG_USER = 1 << 0, + IMAGE_READ_ONLY_FLAG_NON_PRIMARY = 1 << 1, +}; + struct MigrationInfo { int64_t pool_id = -1; std::string pool_namespace; diff --git a/src/librbd/image/DetachChildRequest.cc b/src/librbd/image/DetachChildRequest.cc index fdaeb331a7582..d84a0edff115f 100644 --- a/src/librbd/image/DetachChildRequest.cc +++ b/src/librbd/image/DetachChildRequest.cc @@ -164,6 +164,9 @@ void DetachChildRequest::clone_v2_open_parent() { m_parent_image_ctx = I::create("", m_parent_spec.image_id, nullptr, m_parent_io_ctx, false); + // ensure non-primary images can be modified + m_parent_image_ctx->read_only_mask &= ~IMAGE_READ_ONLY_FLAG_NON_PRIMARY; + auto ctx = create_context_callback< DetachChildRequest, &DetachChildRequest::handle_clone_v2_open_parent>(this); diff --git a/src/librbd/image/OpenRequest.cc b/src/librbd/image/OpenRequest.cc index 0188230c9f4ac..88d5d62e0e5ab 100644 --- a/src/librbd/image/OpenRequest.cc +++ b/src/librbd/image/OpenRequest.cc @@ -597,7 +597,7 @@ Context *OpenRequest::send_init_cache(int *result) { template Context *OpenRequest::send_register_watch(int *result) { - if (m_image_ctx->read_only) { + if ((m_image_ctx->read_only_flags & IMAGE_READ_ONLY_FLAG_USER) != 0U) { return send_set_snap(result); } diff --git a/src/librbd/image/RefreshRequest.cc b/src/librbd/image/RefreshRequest.cc index 602221bfdd642..8679e179af68d 100644 --- a/src/librbd/image/RefreshRequest.cc +++ b/src/librbd/image/RefreshRequest.cc @@ -113,7 +113,7 @@ Context *RefreshRequest::handle_get_migration_header(int *result) { switch(m_migration_spec.header_type) { case cls::rbd::MIGRATION_HEADER_TYPE_SRC: - if (!m_image_ctx.read_only) { + if (!m_read_only) { lderr(cct) << "image being migrated" << dendl; *result = -EROFS; return m_on_finish; @@ -193,6 +193,12 @@ Context *RefreshRequest::handle_v1_read_header(int *result) { } } + { + std::shared_lock image_locker{m_image_ctx.image_lock}; + m_read_only = m_image_ctx.read_only; + m_read_only_flags = m_image_ctx.read_only_flags; + } + memcpy(&v1_header, m_out_bl.c_str(), sizeof(v1_header)); m_order = v1_header.options.order; m_size = v1_header.image_size; @@ -332,9 +338,14 @@ void RefreshRequest::send_v2_get_mutable_metadata() { { std::shared_lock image_locker{m_image_ctx.image_lock}; snap_id = m_image_ctx.snap_id; + m_read_only = m_image_ctx.read_only; + m_read_only_flags = m_image_ctx.read_only_flags; } - bool read_only = m_image_ctx.read_only || snap_id != CEPH_NOSNAP; + // mask out the non-primary read-only flag since its state can change + bool read_only = ( + ((m_read_only_flags & ~IMAGE_READ_ONLY_FLAG_NON_PRIMARY) != 0) || + (snap_id != CEPH_NOSNAP)); librados::ObjectReadOperation op; cls_client::get_size_start(&op, CEPH_NOSNAP); cls_client::get_features_start(&op, read_only); @@ -411,6 +422,21 @@ Context *RefreshRequest::handle_v2_get_mutable_metadata(int *result) { m_incomplete_update = true; } + if (((m_incompatible_features & RBD_FEATURE_NON_PRIMARY) != 0U) && + ((m_read_only_flags & IMAGE_READ_ONLY_FLAG_NON_PRIMARY) == 0U) && + ((m_image_ctx.read_only_mask & IMAGE_READ_ONLY_FLAG_NON_PRIMARY) != 0U)) { + // implies we opened a non-primary image in R/W mode + ldout(cct, 5) << "adding non-primary read-only image flag" << dendl; + m_read_only_flags |= IMAGE_READ_ONLY_FLAG_NON_PRIMARY; + } else if ((((m_incompatible_features & RBD_FEATURE_NON_PRIMARY) == 0U) || + ((m_image_ctx.read_only_mask & + IMAGE_READ_ONLY_FLAG_NON_PRIMARY) == 0U)) && + ((m_read_only_flags & IMAGE_READ_ONLY_FLAG_NON_PRIMARY) != 0U)) { + ldout(cct, 5) << "removing non-primary read-only image flag" << dendl; + m_read_only_flags &= ~IMAGE_READ_ONLY_FLAG_NON_PRIMARY; + } + m_read_only = (m_read_only_flags != 0U); + send_v2_get_parent(); return nullptr; } @@ -804,7 +830,7 @@ Context *RefreshRequest::handle_v2_refresh_parent(int *result) { template void RefreshRequest::send_v2_init_exclusive_lock() { if ((m_features & RBD_FEATURE_EXCLUSIVE_LOCK) == 0 || - m_image_ctx.read_only || !m_image_ctx.snap_name.empty() || + m_read_only || !m_image_ctx.snap_name.empty() || m_image_ctx.exclusive_lock != nullptr) { send_v2_open_object_map(); return; @@ -846,7 +872,7 @@ template void RefreshRequest::send_v2_open_journal() { bool journal_disabled = ( (m_features & RBD_FEATURE_JOURNALING) == 0 || - m_image_ctx.read_only || + m_read_only || !m_image_ctx.snap_name.empty() || m_image_ctx.journal != nullptr || m_image_ctx.exclusive_lock == nullptr || @@ -948,7 +974,7 @@ void RefreshRequest::send_v2_open_object_map() { if ((m_features & RBD_FEATURE_OBJECT_MAP) == 0 || m_image_ctx.object_map != nullptr || (m_image_ctx.snap_name.empty() && - (m_image_ctx.read_only || + (m_read_only || m_image_ctx.exclusive_lock == nullptr || !m_image_ctx.exclusive_lock->is_lock_owner()))) { send_v2_open_journal(); @@ -1234,6 +1260,8 @@ void RefreshRequest::apply() { std::scoped_lock locker{m_image_ctx.owner_lock, m_image_ctx.image_lock}; + m_image_ctx.read_only_flags = m_read_only_flags; + m_image_ctx.read_only = m_read_only; m_image_ctx.size = m_size; m_image_ctx.lockers = m_lockers; m_image_ctx.lock_tag = m_lock_tag; diff --git a/src/librbd/image/RefreshRequest.h b/src/librbd/image/RefreshRequest.h index bd3fda40fc56c..74e80cf98b159 100644 --- a/src/librbd/image/RefreshRequest.h +++ b/src/librbd/image/RefreshRequest.h @@ -144,6 +144,8 @@ private: uint64_t m_incompatible_features = 0; uint64_t m_flags = 0; uint64_t m_op_features = 0; + uint32_t m_read_only_flags = 0U; + bool m_read_only = false; librados::IoCtx m_pool_metadata_io_ctx; std::map m_metadata; diff --git a/src/test/librbd/image/test_mock_DetachChildRequest.cc b/src/test/librbd/image/test_mock_DetachChildRequest.cc index 5fecd6a23e3f7..cd62099c00ff9 100644 --- a/src/test/librbd/image/test_mock_DetachChildRequest.cc +++ b/src/test/librbd/image/test_mock_DetachChildRequest.cc @@ -158,7 +158,9 @@ public: void expect_open(MockImageCtx &mock_image_ctx, int r) { EXPECT_CALL(*mock_image_ctx.state, open(true, _)) - .WillOnce(WithArg<1>(Invoke([this, r](Context* ctx) { + .WillOnce(WithArg<1>(Invoke([this, &mock_image_ctx, r](Context* ctx) { + EXPECT_EQ(0U, mock_image_ctx.read_only_mask & + IMAGE_READ_ONLY_FLAG_NON_PRIMARY); image_ctx->op_work_queue->queue(ctx, r); }))); if (r == 0) { diff --git a/src/test/librbd/image/test_mock_RefreshRequest.cc b/src/test/librbd/image/test_mock_RefreshRequest.cc index 0b35e6c0529b8..b871561e9887f 100644 --- a/src/test/librbd/image/test_mock_RefreshRequest.cc +++ b/src/test/librbd/image/test_mock_RefreshRequest.cc @@ -1453,5 +1453,66 @@ TEST_F(TestMockImageRefreshRequest, ApplyMetadataError) { ASSERT_EQ(0, ctx.wait()); } +TEST_F(TestMockImageRefreshRequest, NonPrimaryFeature) { + REQUIRE_FORMAT_V2(); + + librbd::ImageCtx *ictx; + ASSERT_EQ(0, open_image(m_image_name, &ictx)); + + MockRefreshImageCtx mock_image_ctx(*ictx); + MockRefreshParentRequest mock_refresh_parent_request; + MockExclusiveLock mock_exclusive_lock; + expect_op_work_queue(mock_image_ctx); + expect_test_features(mock_image_ctx); + + InSequence seq; + + // ensure the image is put into read-only mode + expect_get_mutable_metadata(mock_image_ctx, + ictx->features | RBD_FEATURE_NON_PRIMARY, 0); + expect_get_parent(mock_image_ctx, 0); + MockGetMetadataRequest mock_get_metadata_request; + expect_get_metadata(mock_image_ctx, mock_get_metadata_request, + mock_image_ctx.header_oid, {}, 0); + expect_get_metadata(mock_image_ctx, mock_get_metadata_request, RBD_INFO, {}, + 0); + expect_apply_metadata(mock_image_ctx, 0); + expect_get_group(mock_image_ctx, 0); + expect_refresh_parent_is_required(mock_refresh_parent_request, false); + + C_SaferCond ctx1; + auto req = new MockRefreshRequest(mock_image_ctx, false, false, &ctx1); + req->send(); + + ASSERT_EQ(0, ctx1.wait()); + ASSERT_TRUE(mock_image_ctx.read_only); + ASSERT_EQ(IMAGE_READ_ONLY_FLAG_NON_PRIMARY, mock_image_ctx.read_only_flags); + + // try again but permit R/W against non-primary image + mock_image_ctx.read_only_mask = ~IMAGE_READ_ONLY_FLAG_NON_PRIMARY; + + expect_get_mutable_metadata(mock_image_ctx, + ictx->features | RBD_FEATURE_NON_PRIMARY, 0); + expect_get_parent(mock_image_ctx, 0); + expect_get_metadata(mock_image_ctx, mock_get_metadata_request, + mock_image_ctx.header_oid, {}, 0); + expect_get_metadata(mock_image_ctx, mock_get_metadata_request, RBD_INFO, {}, + 0); + expect_apply_metadata(mock_image_ctx, 0); + expect_get_group(mock_image_ctx, 0); + expect_refresh_parent_is_required(mock_refresh_parent_request, false); + if (ictx->test_features(RBD_FEATURE_EXCLUSIVE_LOCK)) { + expect_init_exclusive_lock(mock_image_ctx, mock_exclusive_lock, 0); + } + + C_SaferCond ctx2; + req = new MockRefreshRequest(mock_image_ctx, false, false, &ctx2); + req->send(); + + ASSERT_EQ(0, ctx2.wait()); + ASSERT_FALSE(mock_image_ctx.read_only); + ASSERT_EQ(0U, mock_image_ctx.read_only_flags); +} + } // namespace image } // namespace librbd diff --git a/src/test/librbd/mock/MockImageCtx.h b/src/test/librbd/mock/MockImageCtx.h index 32097393ea334..dcd620abd76c7 100644 --- a/src/test/librbd/mock/MockImageCtx.h +++ b/src/test/librbd/mock/MockImageCtx.h @@ -56,6 +56,8 @@ struct MockImageCtx { snap_ids(image_ctx.snap_ids), old_format(image_ctx.old_format), read_only(image_ctx.read_only), + read_only_flags(image_ctx.read_only_flags), + read_only_mask(image_ctx.read_only_mask), clone_copy_on_read(image_ctx.clone_copy_on_read), lockers(image_ctx.lockers), exclusive_locked(image_ctx.exclusive_locked), @@ -235,6 +237,8 @@ struct MockImageCtx { bool old_format; bool read_only; + uint32_t read_only_flags; + uint32_t read_only_mask; bool clone_copy_on_read; -- 2.39.5