From e023bf4a5462b07066183a5e6c13575199b780ef Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Thu, 1 Jul 2021 14:32:31 +0800 Subject: [PATCH] crimson/admin: let AdminSocket::register_command() return void instead of returning a seastar::future<>, it can just return void, as it does not involve asynchronous operations. also drop servers_tbl_rwlock, as we don't register hooks after OSD starts, there is no need to protect hooks with a rw lock. Signed-off-by: Kefu Chai --- src/crimson/admin/admin_socket.cc | 126 ++++++++++++------------------ src/crimson/admin/admin_socket.h | 16 +--- src/crimson/osd/osd.cc | 27 +++---- 3 files changed, 67 insertions(+), 102 deletions(-) diff --git a/src/crimson/admin/admin_socket.cc b/src/crimson/admin/admin_socket.cc index 136adb41a72..76fe5e15e2f 100644 --- a/src/crimson/admin/admin_socket.cc +++ b/src/crimson/admin/admin_socket.cc @@ -44,25 +44,14 @@ tell_result_t::tell_result_t(std::unique_ptr formatter) formatter->flush(out); } -seastar::future<> -AdminSocket::register_command(std::unique_ptr&& hook) +void AdminSocket::register_command(std::unique_ptr&& hook) { - return seastar::with_lock(servers_tbl_rwlock, - [this, hook = std::move(hook)]() mutable { - auto prefix = hook->prefix; - auto [it, added] = hooks.emplace(prefix, std::move(hook)); - // was this server tag already registered? - assert(added); - if (added) { - logger().info("register_command(): {})", it->first); - } - return seastar::now(); - }); + auto prefix = hook->prefix; + auto [it, added] = hooks.emplace(prefix, std::move(hook)); + assert(added); + logger().info("register_command(): {})", it->first); } -/* - * Note: parse_cmd() is executed with servers_tbl_rwlock held as shared - */ auto AdminSocket::parse_cmd(const std::vector& cmd) -> std::variant { @@ -155,26 +144,23 @@ auto AdminSocket::execute_command(const std::vector& cmd, ceph::bufferlist&& buf) -> seastar::future { - return seastar::with_shared(servers_tbl_rwlock, - [cmd, buf=std::move(buf), this]() mutable { - auto maybe_parsed = parse_cmd(cmd); - if (auto* parsed = std::get_if(&maybe_parsed); parsed) { - stringstream os; - string desc{parsed->hook.desc}; - if (!validate_cmd(nullptr, desc, parsed->params, os)) { - logger().error("AdminSocket::execute_command: " - "failed to validate '{}': {}", cmd, os.str()); - ceph::bufferlist out; - out.append(os); - return seastar::make_ready_future( - tell_result_t{-EINVAL, "invalid command json", std::move(out)}); - } - return parsed->hook.call(parsed->params, parsed->format, std::move(buf)); - } else { - auto& result = std::get(maybe_parsed); - return seastar::make_ready_future(std::move(result)); + auto maybe_parsed = parse_cmd(cmd); + if (auto* parsed = std::get_if(&maybe_parsed); parsed) { + stringstream os; + string desc{parsed->hook.desc}; + if (!validate_cmd(nullptr, desc, parsed->params, os)) { + logger().error("AdminSocket::execute_command: " + "failed to validate '{}': {}", cmd, os.str()); + ceph::bufferlist out; + out.append(os); + return seastar::make_ready_future( + tell_result_t{-EINVAL, "invalid command json", std::move(out)}); } - }); + return parsed->hook.call(parsed->params, parsed->format, std::move(buf)); + } else { + auto& result = std::get(maybe_parsed); + return seastar::make_ready_future(std::move(result)); + } } // an input_stream consumer that reads buffer into a std::string up to the first @@ -342,18 +328,16 @@ class HelpHook final : public AdminSocketHook { std::string_view format, ceph::bufferlist&&) const final { - return seastar::with_shared(m_as.servers_tbl_rwlock, - [format, this] { - unique_ptr f{Formatter::create(format, "json-pretty", "json-pretty")}; - f->open_object_section("help"); - for (const auto& [prefix, hook] : m_as) { - if (!hook->help.empty()) { - f->dump_string(prefix.data(), hook->help); - } + unique_ptr f{Formatter::create(format, + "json-pretty", "json-pretty")}; + f->open_object_section("help"); + for (const auto& [prefix, hook] : m_as) { + if (!hook->help.empty()) { + f->dump_string(prefix.data(), hook->help); } - f->close_section(); - return seastar::make_ready_future(std::move(f)); - }); + } + f->close_section(); + return seastar::make_ready_future(std::move(f)); } }; @@ -371,20 +355,18 @@ class GetdescsHook final : public AdminSocketHook { std::string_view format, ceph::bufferlist&&) const final { - return seastar::with_shared(m_as.servers_tbl_rwlock, [format, this] { - unique_ptr f{Formatter::create(format, "json-pretty", "json-pretty")}; - int cmdnum = 0; - f->open_object_section("command_descriptions"); - for (const auto& [prefix, hook] : m_as) { - auto secname = fmt::format("cmd {:>03}", cmdnum); - auto cmd = fmt::format("{} {}", hook->prefix, hook->desc); - dump_cmd_and_help_to_json(f.get(), CEPH_FEATURES_ALL, secname, - cmd, std::string{hook->help}); - cmdnum++; - } - f->close_section(); - return seastar::make_ready_future(std::move(f)); - }); + unique_ptr f{Formatter::create(format, "json-pretty", "json-pretty")}; + int cmdnum = 0; + f->open_object_section("command_descriptions"); + for (const auto& [prefix, hook] : m_as) { + auto secname = fmt::format("cmd {:>03}", cmdnum); + auto cmd = fmt::format("{} {}", hook->prefix, hook->desc); + dump_cmd_and_help_to_json(f.get(), CEPH_FEATURES_ALL, secname, + cmd, std::string{hook->help}); + cmdnum++; + } + f->close_section(); + return seastar::make_ready_future(std::move(f)); } }; @@ -529,21 +511,17 @@ public: }; /// the hooks that are served directly by the admin_socket server -seastar::future<> AdminSocket::register_admin_commands() +void AdminSocket::register_admin_commands() { - return seastar::when_all_succeed( - register_command(std::make_unique()), - register_command(std::make_unique()), - register_command(std::make_unique(*this)), - register_command(std::make_unique(*this)), - register_command(std::make_unique()), - register_command(std::make_unique()), - register_command(std::make_unique()), - register_command(std::make_unique()), - register_command(std::make_unique()) - ).then_unpack([] { - return seastar::now(); - }); + register_command(std::make_unique()); + register_command(std::make_unique()); + register_command(std::make_unique(*this)); + register_command(std::make_unique(*this)); + register_command(std::make_unique()); + register_command(std::make_unique()); + register_command(std::make_unique()); + register_command(std::make_unique()); + register_command(std::make_unique()); } } // namespace crimson::admin diff --git a/src/crimson/admin/admin_socket.h b/src/crimson/admin/admin_socket.h index 81017dd4717..a7540a6071f 100644 --- a/src/crimson/admin/admin_socket.h +++ b/src/crimson/admin/admin_socket.h @@ -112,12 +112,12 @@ class AdminSocket : public seastar::enable_lw_shared_from_this { * A note regarding the help text: if empty, command will not be * included in 'help' output. */ - seastar::future<> register_command(std::unique_ptr&& hook); + void register_command(std::unique_ptr&& hook); /** * Registering the APIs that are served directly by the admin_socket server. */ - seastar::future<> register_admin_commands(); + void register_admin_commands(); /** * handle a command message by replying an MCommandReply with the same tid * @@ -171,16 +171,10 @@ private: std::variant parse_cmd(const std::vector& cmd); - /** - * The servers table is protected by a rw-lock, to be acquired exclusively - * only when registering or removing a server. - * The lock is locked-shared when executing any hook. - */ - mutable seastar::shared_mutex servers_tbl_rwlock; using hooks_t = std::map>; hooks_t hooks; - public: +public: /** * iterator support */ @@ -190,10 +184,6 @@ private: hooks_t::const_iterator end() const { return hooks.cend(); } - - friend class AdminSocketTest; - friend class HelpHook; - friend class GetdescsHook; }; } // namespace crimson::admin diff --git a/src/crimson/osd/osd.cc b/src/crimson/osd/osd.cc index d1673acce65..17924a94283 100644 --- a/src/crimson/osd/osd.cc +++ b/src/crimson/osd/osd.cc @@ -459,21 +459,18 @@ seastar::future<> OSD::start_asok_admin() auto asok_path = local_conf().get_val("admin_socket"); using namespace crimson::admin; return asok->start(asok_path).then([this] { - return seastar::when_all_succeed( - asok->register_admin_commands(), - asok->register_command(make_asok_hook(std::as_const(*this))), - asok->register_command(make_asok_hook(*this)), - asok->register_command(make_asok_hook(*this)), - asok->register_command(make_asok_hook(std::as_const(*this))), - asok->register_command(make_asok_hook()), - asok->register_command(make_asok_hook()), - asok->register_command(make_asok_hook(get_shard_services())), - asok->register_command(make_asok_hook(get_shard_services())), - // PG commands - asok->register_command(make_asok_hook(*this)), - asok->register_command(make_asok_hook(*this))); - }).then_unpack([] { - return seastar::now(); + asok->register_admin_commands(); + asok->register_command(make_asok_hook(std::as_const(*this))); + asok->register_command(make_asok_hook(*this)); + asok->register_command(make_asok_hook(*this)); + asok->register_command(make_asok_hook(std::as_const(*this))); + asok->register_command(make_asok_hook()); + asok->register_command(make_asok_hook()); + asok->register_command(make_asok_hook(get_shard_services())); + asok->register_command(make_asok_hook(get_shard_services())); + // PG commands + asok->register_command(make_asok_hook(*this)); + asok->register_command(make_asok_hook(*this)); }); } -- 2.47.3