From: Leonid Usov Date: Thu, 6 Jun 2024 11:48:56 +0000 (+0300) Subject: mds: QuiesceDbRequest: update the internal encoding of ops X-Git-Tag: v20.0.0~1794^2 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=dad52497817c372fd7c61a88a210b5a3613cb807;p=ceph.git mds: QuiesceDbRequest: update the internal encoding of ops Excluding the last root from a set will automatically mark it as QS_CANCELED. Hence, it makes more sense if `exclude` and `cancel` share the same op code, rather than `exclude` and `release`. Signed-off-by: Leonid Usov Fixes: https://tracker.ceph.com/issues/66383 --- diff --git a/src/mds/MDSRankQuiesce.cc b/src/mds/MDSRankQuiesce.cc index 0ad49b396d6a..0071cd8fa599 100644 --- a/src/mds/MDSRankQuiesce.cc +++ b/src/mds/MDSRankQuiesce.cc @@ -197,9 +197,9 @@ void MDSRank::command_quiesce_db(const cmdmap_t& cmdmap, asok_finisher on_finish } else if (op_reset) { r.reset_roots(roots); } else if (op_release) { - r.release_roots(); + r.release(); } else if (op_cancel) { - r.cancel_roots(); + r.cancel(); } double timeout; @@ -319,7 +319,7 @@ void MDSRank::quiesce_cluster_update() { struct CancelAll: public QuiesceDbManager::RequestContext { mds_rank_t whoami; CancelAll(mds_rank_t whoami) : whoami(whoami) { - request.cancel_roots(); + request.cancel(); } void finish(int rc) override { dout(rc == 0 ? 15 : 3) << "injected cancel all completed with rc: " << rc << dendl; diff --git a/src/mds/QuiesceDb.h b/src/mds/QuiesceDb.h index 244f342cc73e..8f0ed9729e44 100644 --- a/src/mds/QuiesceDb.h +++ b/src/mds/QuiesceDb.h @@ -340,8 +340,8 @@ struct QuiesceDbRequest { /// for when `roots` is empty enum RootsOp: uint8_t { INCLUDE_OR_QUERY, - EXCLUDE_OR_RELEASE, - RESET_OR_CANCEL, + EXCLUDE_OR_CANCEL, + RESET_OR_RELEASE, __INVALID }; @@ -427,15 +427,15 @@ struct QuiesceDbRequest { bool is_mutating() const { return (control.roots_op != INCLUDE_OR_QUERY) || !roots.empty() || timeout || expiration; } bool is_cancel_all() const { return !set_id && is_cancel(); } - bool excludes_roots() const { return control.roots_op == RESET_OR_CANCEL || (control.roots_op == EXCLUDE_OR_RELEASE && !roots.empty()); } - bool includes_roots() const { return (control.roots_op == RESET_OR_CANCEL || control.roots_op == INCLUDE_OR_QUERY) && !roots.empty(); } + bool excludes_roots() const { return is_exclude() || is_reset(); } + bool includes_roots() const { return is_include() || is_reset(); } bool is_include() const { return control.roots_op == INCLUDE_OR_QUERY && !roots.empty(); } bool is_query() const { return control.roots_op == INCLUDE_OR_QUERY && roots.empty(); } - bool is_exclude() const { return control.roots_op == EXCLUDE_OR_RELEASE && !roots.empty(); } - bool is_release() const { return control.roots_op == EXCLUDE_OR_RELEASE && roots.empty(); } - bool is_reset() const { return control.roots_op == RESET_OR_CANCEL && !roots.empty(); } - bool is_cancel() const { return control.roots_op == RESET_OR_CANCEL && roots.empty(); } + bool is_exclude() const { return control.roots_op == EXCLUDE_OR_CANCEL && !roots.empty(); } + bool is_release() const { return control.roots_op == RESET_OR_RELEASE && roots.empty(); } + bool is_reset() const { return control.roots_op == RESET_OR_RELEASE && !roots.empty(); } + bool is_cancel() const { return control.roots_op == EXCLUDE_OR_CANCEL && roots.empty(); } bool is_verbose() const { return control.flags & Flags::VERBOSE; } bool is_exclusive() const { return control.flags & Flags::EXCLUSIVE; } @@ -444,11 +444,11 @@ struct QuiesceDbRequest { switch (control.roots_op) { case INCLUDE_OR_QUERY: return false; - case EXCLUDE_OR_RELEASE: - return roots.contains(root); - case RESET_OR_CANCEL: - return !roots.contains(root); - default: ceph_abort("unknown roots_op"); return false; + case EXCLUDE_OR_CANCEL: + return roots.empty() || roots.contains(root); + case RESET_OR_RELEASE: + return !roots.empty() && !roots.contains(root); + default: ceph_abort("unknown roots_op"); return false; } } @@ -493,22 +493,22 @@ struct QuiesceDbRequest { template void exclude_roots(R&& roots) { - set_roots(EXCLUDE_OR_RELEASE, std::forward(roots)); + set_roots(EXCLUDE_OR_CANCEL, std::forward(roots)); } - void release_roots() { - set_roots(EXCLUDE_OR_RELEASE, {}); + void release() { + set_roots(RESET_OR_RELEASE, {}); } template void reset_roots(R&& roots) { - set_roots(RESET_OR_CANCEL, std::forward(roots)); + set_roots(RESET_OR_RELEASE, std::forward(roots)); } - void cancel_roots() + void cancel() { - set_roots(RESET_OR_CANCEL, {}); + set_roots(EXCLUDE_OR_CANCEL, {}); } template @@ -522,10 +522,10 @@ struct QuiesceDbRequest { switch (control.roots_op) { case INCLUDE_OR_QUERY: return roots.empty() ? "query" : "include"; - case EXCLUDE_OR_RELEASE: - return roots.empty() ? "release" : "exclude"; - case RESET_OR_CANCEL: - return roots.empty() ? "cancel" : "reset"; + case EXCLUDE_OR_CANCEL: + return roots.empty() ? "cancel" : "exclude"; + case RESET_OR_RELEASE: + return roots.empty() ? "release" : "reset"; default: return ""; } @@ -547,11 +547,11 @@ operator<<(std::basic_ostream& os, const QuiesceDbRequest& req) os << "q-req[" << req.op_string(); if (req.set_id) { - os << " \"" << req.set_id << "\""; + os << " \"" << *req.set_id << "\""; } if (req.if_version) { - os << " ?v:" << req.if_version; + os << " ?v:" << *req.if_version; } if (req.await) { diff --git a/src/mds/QuiesceDbManager.cc b/src/mds/QuiesceDbManager.cc index 88844f09c817..696386bea8e2 100644 --- a/src/mds/QuiesceDbManager.cc +++ b/src/mds/QuiesceDbManager.cc @@ -447,7 +447,7 @@ void QuiesceDbManager::complete_requests() { } // non-zero result codes are all errors - dout(10) << "completing request '" << req->request << " with rc: " << -res << dendl; + dout(10) << "completing " << req->request << " with rc: " << -res << dendl; req->complete(-res); } done_requests.clear(); @@ -589,6 +589,8 @@ int QuiesceDbManager::leader_process_request(RequestContext* req_ctx) return EINVAL; } + dout(20) << request << dendl; + const auto db_age = db.get_age(); if (request.is_cancel_all()) { diff --git a/src/test/mds/TestQuiesceDb.cc b/src/test/mds/TestQuiesceDb.cc index 19c8d9b6163d..4a3f8ac597a5 100644 --- a/src/test/mds/TestQuiesceDb.cc +++ b/src/test/mds/TestQuiesceDb.cc @@ -562,7 +562,7 @@ TEST_F(QuiesceDbTest, QuiesceRequestValidation) return !testing::Test::HasFailure(); }; - const auto ops = std::array { QuiesceDbRequest::RootsOp::INCLUDE_OR_QUERY, QuiesceDbRequest::RootsOp::EXCLUDE_OR_RELEASE, QuiesceDbRequest::RootsOp::RESET_OR_CANCEL, QuiesceDbRequest::RootsOp::__INVALID }; + const auto ops = std::array { QuiesceDbRequest::RootsOp::INCLUDE_OR_QUERY, QuiesceDbRequest::RootsOp::EXCLUDE_OR_CANCEL, QuiesceDbRequest::RootsOp::RESET_OR_RELEASE, QuiesceDbRequest::RootsOp::__INVALID }; const auto strings = nullopt_and_default(); const auto versions = nullopt_and_default(); const auto intervals = nullopt_and_default(); @@ -766,7 +766,7 @@ TEST_F(QuiesceDbTest, SetModification) // cancel with no set_id should cancel all active sets ASSERT_EQ(OK(), run_request([](auto& r) { - r.control.roots_op = QuiesceDbRequest::RootsOp::RESET_OR_CANCEL; + r.cancel(); })); ASSERT_TRUE(db(mds_gid_t(1)).sets.at("set1").members.at("file:/root4").excluded); @@ -829,7 +829,7 @@ TEST_F(QuiesceDbTest, Timeouts) { ASSERT_EQ(OK(), run_request([](auto& r) { r.set_id = "set2"; - r.release_roots(); + r.release(); })); ASSERT_EQ(QuiesceState::QS_RELEASING, last_request->response.sets.at("set2").rstate.state); @@ -939,7 +939,7 @@ TEST_F(QuiesceDbTest, InterruptedQuiesceAwait) ASSERT_EQ(OK(), run_request([](auto& r) { r.set_id = "set1"; r.expiration = sec(100); - r.timeout = sec(10); + r.timeout = sec(100); r.roots.emplace("root1"); })); @@ -1006,7 +1006,7 @@ TEST_F(QuiesceDbTest, InterruptedQuiesceAwait) ASSERT_EQ(OK(), run_request([](auto& r) { r.set_id = "set1"; - r.reset_roots({}); + r.cancel(); })); EXPECT_EQ(ERR(ECANCELED), await3.wait_result()); @@ -1069,7 +1069,7 @@ TEST_F(QuiesceDbTest, RepeatedQuiesceAwait) { for (int i = 0; i < 2; i++) { ASSERT_EQ(ERR(EINPROGRESS), run_request([=](auto& r) { r.set_id = "set1"; - r.release_roots(); + r.release(); r.await = (expiration*2)/5; })); } @@ -1077,7 +1077,7 @@ TEST_F(QuiesceDbTest, RepeatedQuiesceAwait) { // NB: the ETIMEDOUT is the await result, while the set itself should be EXPIRED EXPECT_EQ(ERR(ETIMEDOUT), run_request([=](auto& r) { r.set_id = "set1"; - r.release_roots(); + r.release(); r.await = expiration; })); @@ -1090,7 +1090,7 @@ TEST_F(QuiesceDbTest, RepeatedQuiesceAwait) { EXPECT_EQ(ERR(EPERM), run_request([](auto& r) { r.set_id = "set1"; - r.release_roots(); + r.release(); })); } @@ -1115,7 +1115,7 @@ TEST_F(QuiesceDbTest, ReleaseAwait) for (auto&& set_id : { "set1", "set2" }) { ASSERT_EQ(ERR(EPERM), run_request([set_id](auto& r) { r.set_id = set_id; - r.release_roots(); + r.release(); r.await = sec(1); })) << "bad release-await " << set_id; } @@ -1134,13 +1134,13 @@ TEST_F(QuiesceDbTest, ReleaseAwait) auto & release_await1 = start_request([](auto &r) { r.set_id = "set1"; - r.release_roots(); + r.release(); r.await = sec(100); }); auto& release_await2 = start_request([](auto& r) { r.set_id = "set2"; - r.release_roots(); + r.release(); r.await = sec(100); }); @@ -1155,7 +1155,7 @@ TEST_F(QuiesceDbTest, ReleaseAwait) // we can request release again without any version bump EXPECT_EQ(OK(), run_request([](auto& r) { r.set_id = "set1"; - r.release_roots(); + r.release(); })); EXPECT_EQ(releasing_v1, last_request->response.sets.at("set1").version ); @@ -1163,7 +1163,7 @@ TEST_F(QuiesceDbTest, ReleaseAwait) // we can release-await with a short await timeout EXPECT_EQ(ERR(EINPROGRESS), run_request([](auto& r) { r.set_id = "set1"; - r.release_roots(); + r.release(); r.await = sec(0.1); })); @@ -1197,7 +1197,7 @@ TEST_F(QuiesceDbTest, ReleaseAwait) // await again auto& release_await22 = start_request([](auto& r) { r.set_id = "set2"; - r.release_roots(); + r.release(); r.await = sec(100); }); @@ -1235,18 +1235,18 @@ TEST_F(QuiesceDbTest, ReleaseAwait) // it should be OK to request release or release-await on a RELEASED set EXPECT_EQ(OK(), run_request([](auto& r) { r.set_id = "set1"; - r.release_roots(); + r.release(); })); EXPECT_EQ(OK(), run_request([](auto& r) { r.set_id = "set1"; - r.release_roots(); + r.release(); r.await = sec(0.1); })); // it's invalid to send a release without a set id EXPECT_EQ(ERR(EINVAL), run_request([](auto& r) { - r.release_roots(); + r.release(); })); } @@ -1453,7 +1453,7 @@ TEST_F(QuiesceDbTest, MultiRankRelease) // release roots ASSERT_EQ(OK(), run_request([](auto& r) { r.set_id = "set1"; - r.release_roots(); + r.release(); })); EXPECT_EQ(QS_RELEASING, last_request->response.sets.at("set1").rstate.state); @@ -1463,7 +1463,7 @@ TEST_F(QuiesceDbTest, MultiRankRelease) auto &async_release = start_request([](auto& r) { r.set_id = "set2"; r.await = sec(100); - r.release_roots(); + r.release(); }); EXPECT_EQ(NA(), async_release.check_result()); @@ -1471,7 +1471,7 @@ TEST_F(QuiesceDbTest, MultiRankRelease) // shouldn't hurt to run release twice for set 1 ASSERT_EQ(OK(), run_request([](auto& r) { r.set_id = "set1"; - r.release_roots(); + r.release(); })); EXPECT_EQ(releasing_v, last_request->response.sets.at("set1").version); @@ -1522,7 +1522,7 @@ TEST_F(QuiesceDbTest, MultiRankRelease) ASSERT_EQ(OK(), run_request([set_id](auto& r) { r.set_id = set_id; r.await = sec(100); - r.release_roots(); + r.release(); })); ASSERT_EQ(ERR(EPERM), run_request([set_id](auto& r) { r.set_id = set_id;