From: Kefu Chai Date: Sun, 15 Jul 2018 08:49:59 +0000 (+0800) Subject: common/config: promote lock from md_config_t to ConfigProxy X-Git-Tag: v14.0.1~777^2~15 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=e406d8eb9e1deb801ecb346169eaaf96adbb4b53;p=ceph.git common/config: promote lock from md_config_t to ConfigProxy seastar's ConfigProxy and alien's ConfigProxy follow different threading models and expose different methods. the former updates a setting with 3 steps: 1. create a local copy of current setting, and apply the proposed change to the copy 2. populate the updated change with a foreign_ptr<> to all shards (including itself) 3. on each shards, call apply_changes() to get the interested observers updated, please note, apply_changes() should only update the local observers on current shard. while the alien's ConfigProxy do all the job in a single synchronized call, but we can split it into a finer-grained steps: 1. apply the proposed change in-place 2. apply_changes() to get the interested observers updated. so, to reuse the code across these two implementations, for instance, set_mon_vals() will be implemented in ConfigProxy instead, so we can have different behavior in different ConfigProxy classes. if we keep using the existing single-piece md_config_t::set_mon_vals(), we have no chance to differentiate the apply_changes() for seastar port. but the alien implementation requires a grand lock protecting set_val() and apply_changes(), so we have to move the lock from md_config_t up to ConfigProxy. it's also simpler this way, as we don't need an extra layer to have a dummy Mutex for seastar's ConfigProxy. as only the alien's ConfigProxy requires the lock. Signed-off-by: Kefu Chai --- diff --git a/src/common/config.cc b/src/common/config.cc index 1c8d47592ba5..2c5bafbe0304 100644 --- a/src/common/config.cc +++ b/src/common/config.cc @@ -102,8 +102,7 @@ namespace ceph::internal { template md_config_impl::md_config_impl(ConfigValues& values, bool is_daemon) - : is_daemon(is_daemon), - lock("md_config_t", true, false) + : is_daemon(is_daemon) { // Load the compile-time list of Option into // a map so that we can resolve keys quickly. @@ -261,7 +260,6 @@ template void md_config_impl::set_val_default(ConfigValues& values, const string& name, const std::string& val) { - auto locker = lock(); const Option *o = find_option(name); assert(o); string err; @@ -276,7 +274,6 @@ int md_config_impl::set_mon_vals(CephContext *cct, const map& kv, config_callback config_cb) { - auto locker = lock(); ignored_mon_values.clear(); if (!config_cb) { @@ -336,7 +333,6 @@ int md_config_impl::set_mon_vals(CephContext *cct, template void md_config_impl::add_observer(md_config_obs_impl* observer_) { - auto locker = lock(); const char **keys = observer_->get_tracked_conf_keys(); for (const char ** k = keys; *k; ++k) { observers.emplace(*k, observer_); @@ -346,7 +342,6 @@ void md_config_impl::add_observer(md_config_obs_impl* observer_) template void md_config_impl::remove_observer(md_config_obs_impl* observer_) { - auto locker = lock(); bool found_obs = false; for (auto o = observers.begin(); o != observers.end(); ) { if (o->second == observer_) { @@ -366,7 +361,6 @@ int md_config_impl::parse_config_files(ConfigValues& values, std::ostream *warnings, int flags) { - auto locker = lock(); if (safe_to_start_threads) return -ENOSYS; @@ -497,12 +491,10 @@ void md_config_impl::parse_env(ConfigValues& values, const char *args_var) args_var = "CEPH_ARGS"; } if (getenv("CEPH_KEYRING")) { - auto locker = lock(); _set_val(values, getenv("CEPH_KEYRING"), *find_option("keyring"), CONF_ENV, nullptr); } if (const char *dir = getenv("CEPH_LIB")) { - auto locker = lock(); for (auto name : { "erasure_code_dir", "plugin_dir", "osd_class_dir" }) { std::string err; const Option *o = find_option(name); @@ -520,21 +512,18 @@ void md_config_impl::parse_env(ConfigValues& values, const char *args_var) template void md_config_impl::show_config(const ConfigValues& values, std::ostream& out) { - auto locker = lock(); _show_config(values, &out, nullptr); } template void md_config_impl::show_config(const ConfigValues& values, Formatter *f) { - auto locker = lock(); _show_config(values, nullptr, f); } template void md_config_impl::config_options(Formatter *f) { - auto locker = lock(); f->open_array_section("options"); for (const auto& i: schema) { f->dump_object("option", i.second); @@ -571,7 +560,6 @@ template int md_config_impl::parse_argv(ConfigValues& values, std::vector& args, int level) { - auto locker = lock(); if (safe_to_start_threads) { return -ENOSYS; } @@ -658,7 +646,6 @@ int md_config_impl::parse_argv(ConfigValues& values, template void md_config_impl::do_argv_commands(const ConfigValues& values) { - auto locker = lock(); if (do_show_config) { _show_config(values, &cout, NULL); @@ -781,7 +768,6 @@ void md_config_impl::apply_changes(ConfigValues& values, const ConfigProxy& proxy, std::ostream *oss) { - auto locker = lock(); /* * apply changes until the cluster name is assigned */ @@ -845,7 +831,6 @@ void md_config_impl::call_all_observers(const ConfigProxy& proxy) // An alternative might be to pass a std::unique_lock to // handle_conf_change and have a version of get_var that can take it // by reference and lock as appropriate. - auto locker = lock(); { for (auto r = observers.begin(); r != observers.end(); ++r) { obs[r->second].insert(r->first); @@ -876,7 +861,6 @@ int md_config_impl::injectargs(ConfigValues& values, const std::string& s, std::ostream *oss) { int ret; - auto locker = lock(); char b[s.length()+1]; strcpy(b, s.c_str()); std::vector nargs; @@ -923,7 +907,6 @@ int md_config_impl::set_val(ConfigValues& values, const std::string &key, const char *val, std::stringstream *err_ss) { - auto locker = lock(); if (key.empty()) { if (err_ss) *err_ss << "No key specified"; return -EINVAL; @@ -957,7 +940,6 @@ int md_config_impl::set_val(ConfigValues& values, template int md_config_impl::rm_val(ConfigValues& values, const std::string& key) { - auto locker = lock(); return _rm_val(values, key, CONF_OVERRIDE); } @@ -965,7 +947,6 @@ template void md_config_impl::get_defaults_bl(const ConfigValues& values, bufferlist *bl) { - auto locker = lock(); if (defaults_bl.length() == 0) { uint32_t n = 0; bufferlist bl; @@ -994,7 +975,6 @@ void md_config_impl::get_config_bl( bufferlist *bl, uint64_t *got_version) { - auto locker = lock(); if (values_bl.length() == 0) { uint32_t n = 0; bufferlist bl; @@ -1047,7 +1027,6 @@ template int md_config_impl::get_val(const ConfigValues& values, const std::string &key, char **buf, int len) const { - auto locker = lock(); string k(ConfFile::normalize_key_name(key)); return _get_val_cstr(values, k, buf, len); } @@ -1066,7 +1045,6 @@ Option::value_t md_config_impl::get_val_generic( const ConfigValues& values, const std::string &key) const { - auto locker = lock(); string k(ConfFile::normalize_key_name(key)); return _get_val(values, k); } @@ -1078,7 +1056,6 @@ Option::value_t md_config_impl::_get_val( expand_stack_t *stack, std::ostream *err) const { - assert(lock.is_locked()); if (key.empty()) { return Option::value_t(boost::blank()); } @@ -1139,7 +1116,6 @@ void md_config_impl::early_expand_meta( std::string &val, std::ostream *err) const { - auto locker = lock(); expand_stack_t stack; Option::value_t v = _expand_meta(values, Option::value_t(val), @@ -1299,8 +1275,6 @@ int md_config_impl::_get_val_cstr( const ConfigValues& values, const std::string &key, char **buf, int len) const { - assert(lock.is_locked()); - if (key.empty()) return -EINVAL; @@ -1348,7 +1322,6 @@ template void md_config_impl::get_my_sections(const ConfigValues& values, std::vector §ions) const { - auto locker = lock(); _get_my_sections(values, sections); } @@ -1356,7 +1329,6 @@ template void md_config_impl::_get_my_sections(const ConfigValues& values, std::vector §ions) const { - assert(lock.is_locked()); sections.push_back(values.name.to_str()); sections.push_back(values.name.get_type_name()); @@ -1368,7 +1340,6 @@ void md_config_impl::_get_my_sections(const ConfigValues& values, template int md_config_impl::get_all_sections(std::vector §ions) const { - auto locker = lock(); for (ConfFile::const_section_iter_t s = cf.sections_begin(); s != cf.sections_end(); ++s) { sections.push_back(s->first); @@ -1384,7 +1355,6 @@ int md_config_impl::get_val_from_conf_file( std::string &out, bool emeta) const { - auto locker = lock(); int r = _get_val_from_conf_file(sections, key, out); if (r < 0) { return r; @@ -1403,7 +1373,6 @@ int md_config_impl::_get_val_from_conf_file( const std::string &key, std::string &out) const { - assert(lock.is_locked()); std::vector ::const_iterator s = sections.begin(); std::vector ::const_iterator s_end = sections.end(); for (; s != s_end; ++s) { @@ -1425,8 +1394,6 @@ int md_config_impl::_set_val( int level, std::string *error_message) { - assert(lock.is_locked()); - Option::value_t new_value; int r = opt.parse_value(raw_val, &new_value, error_message); if (r < 0) { @@ -1593,7 +1560,6 @@ void md_config_impl::diff( Formatter *f, string name) const { - auto locker = lock(); values.for_each([this, f, &values] (auto& name, auto& configs) { if (configs.size() == 1 && configs.begin()->first == CONF_DEFAULT) { diff --git a/src/common/config.h b/src/common/config.h index 94500ce1ecdf..938fae210850 100644 --- a/src/common/config.h +++ b/src/common/config.h @@ -25,7 +25,6 @@ #include "common/subsys_types.h" #include "common/config_fwd.h" #include "common/config_values.h" -#include "common/lock_mutex.h" class CephContext; @@ -372,12 +371,6 @@ public: return min_size ? std::min(min_size, size) : (size - size / 2); } - /** A lock that protects the md_config_t internals. It is - * recursive, for simplicity. - * It is best if this lock comes first in the lock hierarchy. We will - * hold this lock when calling configuration observers. */ - LockMutex lock; - friend class test_md_config_t; }; diff --git a/src/common/config_proxy.h b/src/common/config_proxy.h index dbfb1845e9e7..2b34b9a2711f 100644 --- a/src/common/config_proxy.h +++ b/src/common/config_proxy.h @@ -5,6 +5,7 @@ #include #include "common/config.h" #include "common/config_fwd.h" +#include "common/Mutex.h" class ConfigProxy { /** @@ -12,10 +13,16 @@ class ConfigProxy { */ ConfigValues values; md_config_t config; + /** A lock that protects the md_config_t internals. It is + * recursive, for simplicity. + * It is best if this lock comes first in the lock hierarchy. We will + * hold this lock when calling configuration observers. */ + mutable Mutex lock; public: explicit ConfigProxy(bool is_daemon) - : config{values, is_daemon} + : config{values, is_daemon}, + lock{"ConfigProxy", true, false} {} const ConfigValues* operator->() const noexcept { return &values; @@ -24,17 +31,21 @@ public: return &values; } int get_val(const std::string& key, char** buf, int len) const { + Mutex::Locker l{lock}; return config.get_val(values, key, buf, len); } int get_val(const std::string &key, std::string *val) const { + Mutex::Locker l{lock}; return config.get_val(values, key, val); } template const T get_val(const std::string& key) const { + Mutex::Locker l{lock}; return config.template get_val(values, key); } template auto with_val(const string& key, Callback&& cb, Args&&... args) const { + Mutex::Locker l{lock}; return config.template with_val(values, key, std::forward(cb), std::forward(args)...); @@ -55,16 +66,20 @@ public: } void diff(Formatter *f, const std::string& name=string{}) const { return config.diff(values, f, name); + Mutex::Locker l{lock}; } void get_my_sections(std::vector §ions) const { + Mutex::Locker l{lock}; config.get_my_sections(values, sections); } int get_all_sections(std::vector& sections) const { + Mutex::Locker l{lock}; return config.get_all_sections(sections); } int get_val_from_conf_file(const std::vector& sections, const std::string& key, std::string& out, bool emeta) const { + Mutex::Locker l{lock}; return config.get_val_from_conf_file(values, sections, key, out, emeta); } @@ -73,68 +88,114 @@ public: } void early_expand_meta(std::string &val, std::ostream *oss) const { + Mutex::Locker l{lock}; return config.early_expand_meta(values, val, oss); } // change `values` in-place void finalize_reexpand_meta() { +<<<<<<< HEAD + Mutex::Locker l(lock); config.finalize_reexpand_meta(values, *this); } void add_observer(md_config_obs_t* obs) { + Mutex::Locker l(lock); config.add_observer(obs); } void remove_observer(md_config_obs_t* obs) { + Mutex::Locker l(lock); config.remove_observer(obs); } + void call_all_observers() { + Mutex::Locker l(lock); + config.call_all_observers(*this); +======= + Mutex::Locker l{lock}; + if (config.finalize_reexpand_meta(values, obs_mgr)) { + obs_mgr.apply_changes(values.changed, *this, nullptr); + values.changed.clear(); + } + } + void add_observer(md_config_obs_t* obs) { + Mutex::Locker l{lock}; + obs_mgr.add_observer(obs); + } + void remove_observer(md_config_obs_t* obs) { + Mutex::Locker l{lock}; + obs_mgr.remove_observer(obs); + } + void call_all_observers() { + Mutex::Locker l{lock}; + // Have the scope of the lock extend to the scope of + // handle_conf_change since that function expects to be called with + // the lock held. (And the comment in config.h says that is the + // expected behavior.) + // + // An alternative might be to pass a std::unique_lock to + // handle_conf_change and have a version of get_var that can take it + // by reference and lock as appropriate. + obs_mgr.call_all_observers(*this); +>>>>>>> 9a2bc3c2eb... wip + } void set_safe_to_start_threads() { config.set_safe_to_start_threads(); } void _clear_safe_to_start_threads() { config._clear_safe_to_start_threads(); } - void call_all_observers() { - config.call_all_observers(*this); - } void show_config(std::ostream& out) { + Mutex::Locker l{lock}; config.show_config(values, out); } void show_config(Formatter *f) { + Mutex::Locker l{lock}; config.show_config(values, f); } void config_options(Formatter *f) { + Mutex::Locker l{lock}; config.config_options(f); } int rm_val(const std::string& key) { + Mutex::Locker l{lock}; return config.rm_val(values, key); } void apply_changes(std::ostream* oss) { + Mutex::Locker l{lock}; config.apply_changes(values, *this, oss); } int set_val(const std::string& key, const std::string& s, std::stringstream* err_ss=nullptr) { + Mutex::Locker l{lock}; return config.set_val(values, key, s); } void set_val_default(const std::string& key, const std::string& val) { + Mutex::Locker l{lock}; config.set_val_default(values, key, val); } void set_val_or_die(const std::string& key, const std::string& val) { + Mutex::Locker l{lock}; config.set_val_or_die(values, key, val); } int set_mon_vals(CephContext *cct, const map& kv, md_config_t::config_callback config_cb) { + Mutex::Locker l{lock}; config.set_mon_vals(cct, values, *this, kv, config_cb); } int injectargs(const std::string &s, std::ostream *oss) { + Mutex::Locker l{lock}; config.injectargs(values, *this, s, oss); } void parse_env(const char *env_var = "CEPH_ARGS") { + Mutex::Locker l{lock}; config.parse_env(values, env_var); } int parse_argv(std::vector& args, int level=CONF_CMDLINE) { + Mutex::Locker l{lock}; return config.parse_argv(values, args, level); } int parse_config_files(const char *conf_files, std::ostream *warnings, int flags) { + Mutex::Locker l{lock}; return config.parse_config_files(values, conf_files, warnings, flags); } size_t num_parse_errors() const { @@ -144,14 +205,17 @@ public: return config.complain_about_parse_errors(cct); } void do_argv_commands() { + Mutex::Locker l{lock}; config.do_argv_commands(values); } void get_config_bl(uint64_t have_version, bufferlist *bl, uint64_t *got_version) { + Mutex::Locker l{lock}; config.get_config_bl(values, have_version, bl, got_version); } void get_defaults_bl(bufferlist *bl) { + Mutex::Locker l{lock}; config.get_defaults_bl(values, bl); } };