From: Colin Patrick McCabe Date: Thu, 4 Aug 2011 23:42:58 +0000 (-0700) Subject: config: add proper locking, fix comments X-Git-Tag: v0.33~15^2~9 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=2c9949e0caae61965178602d6f63df28f33e9ee3;p=ceph.git config: add proper locking, fix comments Signed-off-by: Colin McCabe --- diff --git a/src/common/config.cc b/src/common/config.cc index c1a5d1eb2889..711614f40042 100644 --- a/src/common/config.cc +++ b/src/common/config.cc @@ -126,6 +126,8 @@ const void *config_option::conf_ptr(const md_config_t *conf) const return v; } +/* All options must appear here, or else they won't be initialized + * when md_config_t is created. */ struct config_option config_optionsp[] = { OPTION(host, OPT_STR, "localhost"), OPTION(public_addr, OPT_ADDR, NULL), @@ -460,15 +462,8 @@ bool ceph_resolve_file_search(const std::string& filename_list, md_config_t:: md_config_t() + : lock("md_config_t", true) { - // - // Note: because our md_config_t structure is a global, the memory used to - // store it will start out zeroed. So there is no need to manually initialize - // everything to 0 here. - // - // However, it's good practice to add your new config option to config_optionsp - // so that its default value is explicit rather than implicit. - // for (int i = 0; i < NUM_CONFIG_OPTIONS; i++) { config_option *opt = config_optionsp + i; set_val_from_default(opt); @@ -484,6 +479,7 @@ md_config_t:: void md_config_t:: add_observer(md_config_obs_t* observer_) { + Mutex::Locker l(lock); const char **keys = observer_->get_tracked_conf_keys(); for (const char ** k = keys; *k; ++k) { obs_map_t::value_type val(*k, observer_); @@ -494,6 +490,7 @@ add_observer(md_config_obs_t* observer_) void md_config_t:: remove_observer(md_config_obs_t* observer_) { + Mutex::Locker l(lock); bool found_obs = false; for (obs_map_t::iterator o = observers.begin(); o != observers.end(); ) { if (o->second == observer_) { @@ -511,6 +508,7 @@ int md_config_t:: parse_config_files(const char *conf_files, std::deque *parse_errors, int flags) { + Mutex::Locker l(lock); if (!conf_files) { const char *c = getenv("CEPH_CONF"); if (c) { @@ -531,6 +529,7 @@ int md_config_t:: parse_config_files_impl(const std::list &conf_files, std::deque *parse_errors) { + Mutex::Locker l(lock); // open new conf list::const_iterator c; for (c = conf_files.begin(); c != conf_files.end(); ++c) { @@ -583,6 +582,7 @@ parse_config_files_impl(const std::list &conf_files, void md_config_t:: parse_env() { + Mutex::Locker l(lock); if (getenv("CEPH_KEYRING")) keyring = getenv("CEPH_KEYRING"); } @@ -590,6 +590,7 @@ parse_env() void md_config_t:: parse_argv(std::vector& args) { + Mutex::Locker l(lock); // In this function, don't change any parts of the configuration directly. // Instead, use set_val to set them. This will allow us to send the proper // observer notifications later. @@ -662,6 +663,7 @@ parse_argv(std::vector& args) void md_config_t:: apply_changes(std::ostringstream *oss) { + Mutex::Locker l(lock); /* Maps observers to the configuration options that they care about which * have changed. */ typedef std::map < md_config_obs_t*, std::set > rev_obs_map_t; @@ -710,6 +712,7 @@ apply_changes(std::ostringstream *oss) void md_config_t:: injectargs(const std::string& s, std::ostringstream *oss) { + Mutex::Locker l(lock); char b[s.length()+1]; strcpy(b, s.c_str()); std::vector nargs; @@ -746,6 +749,7 @@ set_val_or_die(const char *key, const char *val) int md_config_t:: set_val(const char *key, const char *val) { + Mutex::Locker l(lock); if (!key) return -EINVAL; if (!val) @@ -770,6 +774,7 @@ set_val(const char *key, const char *val) int md_config_t:: get_val(const char *key, char **buf, int len) const { + Mutex::Locker l(lock); if (!key) return -EINVAL; @@ -834,6 +839,7 @@ get_val(const char *key, char **buf, int len) const void md_config_t:: get_my_sections(std::vector §ions) const { + Mutex::Locker l(lock); sections.push_back(name.to_str()); sections.push_back(name.get_type_name()); @@ -845,6 +851,7 @@ get_my_sections(std::vector §ions) const int md_config_t:: get_all_sections(std::vector §ions) const { + Mutex::Locker l(lock); for (ConfFile::const_section_iter_t s = cf.sections_begin(); s != cf.sections_end(); ++s) { sections.push_back(s->first); @@ -856,6 +863,7 @@ int md_config_t:: get_val_from_conf_file(const std::vector §ions, const char *key, std::string &out, bool emeta) const { + Mutex::Locker l(lock); std::vector ::const_iterator s = sections.begin(); std::vector ::const_iterator s_end = sections.end(); for (; s != s_end; ++s) { @@ -874,6 +882,7 @@ get_val_from_conf_file(const std::vector §ions, void md_config_t:: set_val_from_default(const config_option *opt) { + Mutex::Locker l(lock); // set_val_from_default can't fail! Unless the programmer screwed up, and // in that case we'll abort. // Anyway, we know that this function changed something. diff --git a/src/common/config.h b/src/common/config.h index d60a0027103a..a69d54782197 100644 --- a/src/common/config.h +++ b/src/common/config.h @@ -24,7 +24,7 @@ extern struct ceph_file_layout g_default_file_layout; #include "common/ConfUtils.h" #include "common/entity_name.h" -#include "common/Mutex.h" // TODO: remove +#include "common/Mutex.h" #include "include/assert.h" // TODO: remove #include "common/config_obs.h" #include "msg/msg_types.h" @@ -42,6 +42,28 @@ extern const char *CEPH_CONF_FILE_DEFAULT; #define LOG_TO_STDERR_SOME 1 #define LOG_TO_STDERR_ALL 2 +/** This class represents the current Ceph configuration. + * + * For Ceph daemons, this is the daemon configuration. Log levels, caching + * settings, btrfs settings, and so forth can all be found here. For libceph + * and librados users, this is the configuration associated with their context. + * + * For information about how this class is loaded from a configuration file, + * see common/ConfUtils. + * + * ACCESS + * + * There are two ways to read the ceph context-- the old way and the new way. + * In the old way, code would simply read the public variables of the + * configuration, without taking a lock. In the new way, code registers a + * configuration obserever which receives callbacks when a value changes. These + * callbacks take place under the md_config_t lock. + * + * Clearly, the old way is not compatible with altering the value of the + * configuration after multiple threads have been started. So, to avoid + * thread-safety problems, we disallow changing configuration values unless + * there is an observer registered to handle that change. + */ class md_config_t { public: /* Maps configuration options to the observer listening for them. */ @@ -128,6 +150,12 @@ private: // The configuration file we read, or NULL if we haven't read one. ConfFile cf; + /** 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; + obs_map_t observers; changed_set_t changed; diff --git a/src/mds/MDS.cc b/src/mds/MDS.cc index 1b25ec98475a..519a66af03ec 100644 --- a/src/mds/MDS.cc +++ b/src/mds/MDS.cc @@ -716,7 +716,9 @@ void MDS::handle_command(MMonCommand *m) dout(10) << "handle_command args: " << m->cmd << dendl; if (m->cmd[0] == "injectargs") { std::ostringstream oss; + mds_lock.Unlock(); g_conf->injectargs(m->cmd[1], &oss); + mds_lock.Lock(); derr << "injectargs:" << dendl; derr << oss.str() << dendl; } diff --git a/src/osd/OSD.cc b/src/osd/OSD.cc index de9fec2a97e5..f261a81223a2 100644 --- a/src/osd/OSD.cc +++ b/src/osd/OSD.cc @@ -2139,7 +2139,9 @@ void OSD::handle_command(MMonCommand *m) dout(20) << "handle_command args: " << m->cmd << dendl; if (m->cmd[0] == "injectargs") { ostringstream oss; + osd_lock.Unlock(); g_conf->injectargs(m->cmd[1], &oss); + osd_lock.Lock(); derr << "injectargs:" << dendl; derr << oss.str() << dendl; }