diff --git a/src/common/admin_socket.cc b/src/common/admin_socket.cc index aa0146fe1c3..2c86949d1f0 100644 --- a/src/common/admin_socket.cc +++ b/src/common/admin_socket.cc @@ -109,6 +109,7 @@ AdminSocket::AdminSocket(CephContext *cct) m_sock_fd(-1), m_shutdown_rd_fd(-1), m_shutdown_wr_fd(-1), + in_hook(false), m_lock("AdminSocket::m_lock"), m_version_hook(NULL), m_help_hook(NULL), @@ -384,9 +385,22 @@ bool AdminSocket::do_accept() lderr(m_cct) << "AdminSocket: request '" << c << "' not defined" << dendl; } else { string args; - if (match != c) + if (match != c) { args = c.substr(match.length() + 1); - bool success = p->second->call(match, cmdmap, format, out); + } + + // Drop lock to avoid cycles in cases where the hook takes + // the same lock that was held during calls to register/unregister, + // and set in_hook to allow unregister to wait for us before + // removing this hook. + in_hook = true; + auto match_hook = p->second; + m_lock.Unlock(); + bool success = match_hook->call(match, cmdmap, format, out); + m_lock.Lock(); + in_hook = false; + in_hook_cond.Signal(); + if (!success) { ldout(m_cct, 0) << "AdminSocket: request '" << match << "' args '" << args << "' to " << p->second << " failed" << dendl; @@ -439,6 +453,14 @@ int AdminSocket::unregister_command(std::string command) m_hooks.erase(command); m_descs.erase(command); m_help.erase(command); + + // If we are currently processing a command, wait for it to + // complete in case it referenced the hook that we are + // unregistering. + if (in_hook) { + in_hook_cond.Wait(m_lock); + } + ret = 0; } else { ldout(m_cct, 5) << "unregister_command " << command << " ENOENT" << dendl; diff --git a/src/common/admin_socket.h b/src/common/admin_socket.h index bad235a277f..3a6351da6fd 100644 --- a/src/common/admin_socket.h +++ b/src/common/admin_socket.h @@ -22,6 +22,7 @@ #include #include "include/buffer_fwd.h" #include "common/cmdparse.h" +#include "common/Cond.h" class AdminSocket; class CephContext; @@ -63,7 +64,11 @@ public: int register_command(std::string command, std::string cmddesc, AdminSocketHook *hook, std::string help); /** - * unregister an admin socket command + * unregister an admin socket command. + * + * If a command is currently in progress, this will block until it + * is done. For that reason, you must not hold any locks required + * by your hook while you call this. * * @param command command string * @return 0 on succest, -ENOENT if command dne. @@ -91,6 +96,8 @@ private: int m_shutdown_rd_fd; int m_shutdown_wr_fd; + bool in_hook; + Cond in_hook_cond; Mutex m_lock; // protects m_hooks, m_descs, m_help AdminSocketHook *m_version_hook, *m_help_hook, *m_getdescs_hook; diff --git a/src/osdc/Objecter.cc b/src/osdc/Objecter.cc index 9ace054ca0c..8a6f1e659e0 100644 --- a/src/osdc/Objecter.cc +++ b/src/osdc/Objecter.cc @@ -466,13 +466,6 @@ void Objecter::shutdown() tick_event = 0; } - if (m_request_state_hook) { - AdminSocket* admin_socket = cct->get_admin_socket(); - admin_socket->unregister_command("objecter_requests"); - delete m_request_state_hook; - m_request_state_hook = NULL; - } - if (logger) { cct->get_perfcounters_collection()->remove(logger); delete logger; @@ -481,6 +474,16 @@ void Objecter::shutdown() // Let go of Objecter write lock so timer thread can shutdown wl.unlock(); + + // Outside of lock to avoid cycle WRT calls to RequestStateHook + // This is safe because we guarantee no concurrent calls to + // shutdown() with the ::initialized check at start. + if (m_request_state_hook) { + AdminSocket* admin_socket = cct->get_admin_socket(); + admin_socket->unregister_command("objecter_requests"); + delete m_request_state_hook; + m_request_state_hook = NULL; + } } void Objecter::_send_linger(LingerOp *info,