From 561cbded0c7e28231b1c7ce18663b8d7d40aad6d Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Fri, 5 May 2017 12:02:05 +0800 Subject: [PATCH] mon: check is_shutdown() in timer callbacks introduce a helper class: C_MonContext, and initialize all timer events using it, to ensure that they do check is_shutdown() before doing their work. Fixes: http://tracker.ceph.com/issues/19825 Signed-off-by: Kefu Chai --- src/mon/Elector.cc | 16 +++----- src/mon/Elector.h | 3 +- src/mon/MgrMonitor.cc | 2 +- src/mon/Monitor.cc | 75 +++++++++++++++++------------------- src/mon/Monitor.h | 60 ++++++----------------------- src/mon/Paxos.cc | 85 ++++++++++++----------------------------- src/mon/PaxosService.cc | 17 +++------ src/mon/PaxosService.h | 2 - src/mon/QuorumService.h | 19 +++------ 9 files changed, 89 insertions(+), 190 deletions(-) diff --git a/src/mon/Elector.cc b/src/mon/Elector.cc index c2da8bbb40d..303510530b5 100644 --- a/src/mon/Elector.cc +++ b/src/mon/Elector.cc @@ -126,6 +126,8 @@ void Elector::defer(int who) void Elector::reset_timer(double plus) { + // set the timer + cancel_timer(); /** * This class is used as the callback when the expire_event timer fires up. * @@ -139,17 +141,9 @@ void Elector::reset_timer(double plus) * as far as we know, we may even be dead); so, just propose ourselves as the * Leader. */ - class C_ElectionExpire : public Context { - Elector *elector; - public: - explicit C_ElectionExpire(Elector *e) : elector(e) { } - void finish(int r) override { - elector->expire(); - } - }; - // set the timer - cancel_timer(); - expire_event = new C_ElectionExpire(this); + expire_event = new C_MonContext(mon, [this](int) { + expire(); + }); mon->timer.add_event_after(g_conf->mon_election_timeout + plus, expire_event); } diff --git a/src/mon/Elector.h b/src/mon/Elector.h index 02097510f81..2e407d29058 100644 --- a/src/mon/Elector.h +++ b/src/mon/Elector.h @@ -65,7 +65,7 @@ class Elector { * Event callback responsible for dealing with an expired election once a * timer runs out and fires up. */ - Context *expire_event; + Context *expire_event = nullptr; /** * Resets the expire_event timer, by cancelling any existing one and @@ -335,7 +335,6 @@ class Elector { * @param m A Monitor instance */ explicit Elector(Monitor *m) : mon(m), - expire_event(0), epoch(0), participating(true), electing_me(false), diff --git a/src/mon/MgrMonitor.cc b/src/mon/MgrMonitor.cc index 7079db17b5b..b36da2fce57 100644 --- a/src/mon/MgrMonitor.cc +++ b/src/mon/MgrMonitor.cc @@ -298,7 +298,7 @@ void MgrMonitor::send_digests() sub->session->con->send_message(mdigest); } - digest_callback = new FunctionContext([this](int r){ + digest_callback = new C_MonContext(mon, [this](int){ send_digests(); }); mon->timer.add_event_after(g_conf->mon_mgr_digest_period, digest_callback); diff --git a/src/mon/Monitor.cc b/src/mon/Monitor.cc index af44df8e392..2a1e6ad111e 100644 --- a/src/mon/Monitor.cc +++ b/src/mon/Monitor.cc @@ -147,6 +147,12 @@ long parse_pos_long(const char *s, ostream *pss) return r; } +void C_MonContext::finish(int r) { + if (mon->is_shutdown()) + return; + FunctionContext::finish(r); +} + Monitor::Monitor(CephContext* cct_, string nm, MonitorDBStore *s, Messenger *m, Messenger *mgr_m, MonMap *map) : Dispatcher(cct_), @@ -199,12 +205,8 @@ Monitor::Monitor(CephContext* cct_, string nm, MonitorDBStore *s, timecheck_rounds_since_clean(0), timecheck_event(NULL), - probe_timeout_event(NULL), - paxos_service(PAXOS_NUM), admin_hook(NULL), - health_tick_event(NULL), - health_interval_event(NULL), routed_request_tid(0), op_tracker(cct, true, 1) { @@ -1260,7 +1262,9 @@ void Monitor::sync_reset_timeout() dout(10) << __func__ << dendl; if (sync_timeout_event) timer.cancel_event(sync_timeout_event); - sync_timeout_event = new C_SyncTimeout(this); + sync_timeout_event = new C_MonContext(this, [this](int) { + sync_timeout(); + }); timer.add_event_after(g_conf->mon_sync_timeout, sync_timeout_event); } @@ -1598,7 +1602,9 @@ void Monitor::cancel_probe_timeout() void Monitor::reset_probe_timeout() { cancel_probe_timeout(); - probe_timeout_event = new C_ProbeTimeout(this); + probe_timeout_event = new C_MonContext(this, [this](int r) { + probe_timeout(r); + }); double t = g_conf->mon_probe_timeout; timer.add_event_after(t, probe_timeout_event); dout(10) << "reset_probe_timeout " << probe_timeout_event << " after " << t << " seconds" << dendl; @@ -2257,8 +2263,12 @@ void Monitor::health_tick_start() dout(15) << __func__ << dendl; health_tick_stop(); - health_tick_event = new C_HealthToClogTick(this); - + health_tick_event = new C_MonContext(this, [this](int r) { + if (r < 0) + return; + do_health_to_clog(); + health_tick_start(); + }); timer.add_event_after(cct->_conf->mon_health_to_clog_tick_interval, health_tick_event); } @@ -2302,7 +2312,11 @@ void Monitor::health_interval_start() health_interval_stop(); utime_t next = health_interval_calc_next_update(); - health_interval_event = new C_HealthToClogInterval(this); + health_interval_event = new C_MonContext(this, [this](int r) { + if (r < 0) + return; + do_health_to_clog_interval(); + }); timer.add_event_at(next, health_interval_event); } @@ -4112,7 +4126,9 @@ void Monitor::timecheck_reset_event() << " rounds_since_clean " << timecheck_rounds_since_clean << dendl; - timecheck_event = new C_TimeCheck(this); + timecheck_event = new C_MonContext(this, [this](int) { + timecheck_start_round(); + }); timer.add_event_after(delay, timecheck_event); } @@ -4952,15 +4968,9 @@ void Monitor::scrub_event_start() return; } - struct C_Scrub : public Context { - Monitor *mon; - explicit C_Scrub(Monitor *m) : mon(m) { } - void finish(int r) override { - mon->scrub_start(); - } - }; - - scrub_event = new C_Scrub(this); + scrub_event = new C_MonContext(this, [this](int) { + scrub_start(); + }); timer.add_event_after(cct->_conf->mon_scrub_interval, scrub_event); } @@ -4986,33 +4996,18 @@ void Monitor::scrub_reset_timeout() dout(15) << __func__ << " reset timeout event" << dendl; scrub_cancel_timeout(); - struct C_ScrubTimeout : public Context { - Monitor *mon; - explicit C_ScrubTimeout(Monitor *m) : mon(m) { } - void finish(int r) override { - mon->scrub_timeout(); - } - }; - - scrub_timeout_event = new C_ScrubTimeout(this); + scrub_timeout_event = new C_MonContext(this, [this](int) { + scrub_timeout(); + }); timer.add_event_after(g_conf->mon_scrub_timeout, scrub_timeout_event); } /************ TICK ***************/ - -class C_Mon_Tick : public Context { - Monitor *mon; -public: - explicit C_Mon_Tick(Monitor *m) : mon(m) {} - void finish(int r) override { - mon->tick(); - } -}; - void Monitor::new_tick() { - C_Mon_Tick *ctx = new C_Mon_Tick(this); - timer.add_event_after(g_conf->mon_tick_interval, ctx); + timer.add_event_after(g_conf->mon_tick_interval, new C_MonContext(this, [this](int) { + tick(); + })); } void Monitor::tick() diff --git a/src/mon/Monitor.h b/src/mon/Monitor.h index 285997e18aa..3f8b5950888 100644 --- a/src/mon/Monitor.h +++ b/src/mon/Monitor.h @@ -114,6 +114,14 @@ struct MonCommand; #define COMPAT_SET_LOC "feature_set" +class C_MonContext final : public FunctionContext { + const Monitor *mon; +public: + explicit C_MonContext(Monitor *m, boost::function&& callback) + : FunctionContext(std::move(callback)), mon(m) {} + void finish(int r) override; +}; + class Monitor : public Dispatcher, public md_config_obs_t { public: @@ -163,7 +171,6 @@ public: private: void new_tick(); - friend class C_Mon_Tick; // -- local storage -- public: @@ -336,14 +343,6 @@ private: */ version_t sync_last_committed_floor; - struct C_SyncTimeout : public Context { - Monitor *mon; - explicit C_SyncTimeout(Monitor *m) : mon(m) {} - void finish(int r) override { - mon->sync_timeout(); - } - }; - /** * Obtain the synchronization target prefixes in set form. * @@ -503,14 +502,6 @@ private: */ Context *timecheck_event; - struct C_TimeCheck : public Context { - Monitor *mon; - explicit C_TimeCheck(Monitor *m) : mon(m) { } - void finish(int r) override { - mon->timecheck_start_round(); - } - }; - void timecheck_start(); void timecheck_finish(); void timecheck_start_round(); @@ -546,15 +537,7 @@ private: */ void handle_ping(MonOpRequestRef op); - Context *probe_timeout_event; // for probing - - struct C_ProbeTimeout : public Context { - Monitor *mon; - explicit C_ProbeTimeout(Monitor *m) : mon(m) {} - void finish(int r) override { - mon->probe_timeout(r); - } - }; + Context *probe_timeout_event = nullptr; // for probing void reset_probe_timeout(); void cancel_probe_timeout(); @@ -713,29 +696,8 @@ public: } } health_status_cache; - struct C_HealthToClogTick : public Context { - Monitor *mon; - explicit C_HealthToClogTick(Monitor *m) : mon(m) { } - void finish(int r) override { - if (r < 0) - return; - mon->do_health_to_clog(); - mon->health_tick_start(); - } - }; - - struct C_HealthToClogInterval : public Context { - Monitor *mon; - explicit C_HealthToClogInterval(Monitor *m) : mon(m) { } - void finish(int r) override { - if (r < 0) - return; - mon->do_health_to_clog_interval(); - } - }; - - Context *health_tick_event; - Context *health_interval_event; + Context *health_tick_event = nullptr; + Context *health_interval_event = nullptr; void health_tick_start(); void health_tick_stop(); diff --git a/src/mon/Paxos.cc b/src/mon/Paxos.cc index 2e905ac8fb7..8ba73118e0f 100644 --- a/src/mon/Paxos.cc +++ b/src/mon/Paxos.cc @@ -37,61 +37,6 @@ static ostream& _prefix(std::ostream *_dout, Monitor *mon, const string& name, << ") "; } -class Paxos::C_CollectTimeout : public Context { - Paxos *paxos; -public: - explicit C_CollectTimeout(Paxos *p) : paxos(p) {} - void finish(int r) override { - if (r == -ECANCELED) - return; - paxos->collect_timeout(); - } -}; - -class Paxos::C_AcceptTimeout : public Context { - Paxos *paxos; -public: - explicit C_AcceptTimeout(Paxos *p) : paxos(p) {} - void finish(int r) override { - if (r == -ECANCELED) - return; - paxos->accept_timeout(); - } -}; - -class Paxos::C_LeaseAckTimeout : public Context { - Paxos *paxos; -public: - explicit C_LeaseAckTimeout(Paxos *p) : paxos(p) {} - void finish(int r) override { - if (r == -ECANCELED) - return; - paxos->lease_ack_timeout(); - } -}; - -class Paxos::C_LeaseTimeout : public Context { - Paxos *paxos; -public: - explicit C_LeaseTimeout(Paxos *p) : paxos(p) {} - void finish(int r) override { - if (r == -ECANCELED) - return; - paxos->lease_timeout(); - } -}; - -class Paxos::C_LeaseRenew : public Context { - Paxos *paxos; -public: - explicit C_LeaseRenew(Paxos *p) : paxos(p) {} - void finish(int r) override { - if (r == -ECANCELED) - return; - paxos->lease_renew_timeout(); - } -}; - class Paxos::C_Trimmed : public Context { Paxos *paxos; public: @@ -249,7 +194,11 @@ void Paxos::collect(version_t oldpn) } // set timeout event - collect_timeout_event = new C_CollectTimeout(this); + collect_timeout_event = new C_MonContext(mon, [this](int r) { + if (r == -ECANCELED) + return; + collect_timeout(); + }); mon->timer.add_event_after(g_conf->mon_accept_timeout_factor * g_conf->mon_lease, collect_timeout_event); @@ -737,7 +686,11 @@ void Paxos::begin(bufferlist& v) } // set timeout event - accept_timeout_event = new C_AcceptTimeout(this); + accept_timeout_event = new C_MonContext(mon, [this](int r) { + if (r == -ECANCELED) + return; + accept_timeout(); + }); mon->timer.add_event_after(g_conf->mon_accept_timeout_factor * g_conf->mon_lease, accept_timeout_event); @@ -1023,14 +976,22 @@ void Paxos::extend_lease() // set timeout event. // if old timeout is still in place, leave it. if (!lease_ack_timeout_event) { - lease_ack_timeout_event = new C_LeaseAckTimeout(this); + lease_ack_timeout_event = new C_MonContext(mon, [this](int r) { + if (r == -ECANCELED) + return; + lease_ack_timeout(); + }); mon->timer.add_event_after(g_conf->mon_lease_ack_timeout_factor * g_conf->mon_lease, lease_ack_timeout_event); } // set renew event - lease_renew_event = new C_LeaseRenew(this); + lease_renew_event = new C_MonContext(mon, [this](int r) { + if (r == -ECANCELED) + return; + lease_renew_timeout(); + }); utime_t at = lease_expire; at -= g_conf->mon_lease; at += g_conf->mon_lease_renew_interval_factor * g_conf->mon_lease; @@ -1213,7 +1174,11 @@ void Paxos::reset_lease_timeout() dout(20) << "reset_lease_timeout - setting timeout event" << dendl; if (lease_timeout_event) mon->timer.cancel_event(lease_timeout_event); - lease_timeout_event = new C_LeaseTimeout(this); + lease_timeout_event = new C_MonContext(mon, [this](int r) { + if (r == -ECANCELED) + return; + lease_timeout(); + }); mon->timer.add_event_after(g_conf->mon_lease_ack_timeout_factor * g_conf->mon_lease, lease_timeout_event); diff --git a/src/mon/PaxosService.cc b/src/mon/PaxosService.cc index 203be1c817c..a1c9df895a8 100644 --- a/src/mon/PaxosService.cc +++ b/src/mon/PaxosService.cc @@ -100,22 +100,15 @@ bool PaxosService::dispatch(MonOpRequestRef op) * Callback class used to propose the pending value once the proposal_timer * fires up. */ - class C_Propose : public Context { - PaxosService *ps; - public: - explicit C_Propose(PaxosService *p) : ps(p) { } - void finish(int r) override { - ps->proposal_timer = 0; + proposal_timer = new C_MonContext(mon, [this](int r) { + proposal_timer = 0; if (r >= 0) - ps->propose_pending(); + propose_pending(); else if (r == -ECANCELED || r == -EAGAIN) return; else - assert(0 == "bad return value for C_Propose"); - } - }; - - proposal_timer = new C_Propose(this); + assert(0 == "bad return value for proposal_timer"); + }); dout(10) << " setting proposal_timer " << proposal_timer << " with delay of " << delay << dendl; mon->timer.add_event_after(delay, proposal_timer); } else { diff --git a/src/mon/PaxosService.h b/src/mon/PaxosService.h index ae2a3892e9c..5c6b2872b09 100644 --- a/src/mon/PaxosService.h +++ b/src/mon/PaxosService.h @@ -117,8 +117,6 @@ protected: /** * @} */ - friend class C_Propose; - public: /** diff --git a/src/mon/QuorumService.h b/src/mon/QuorumService.h index f1100520a0d..b354c40a77f 100644 --- a/src/mon/QuorumService.h +++ b/src/mon/QuorumService.h @@ -25,19 +25,9 @@ class QuorumService { - Context *tick_event; + Context *tick_event = nullptr; double tick_period; - struct C_Tick : public Context { - QuorumService *s; - C_Tick(QuorumService *qs) : s(qs) { } - void finish(int r) override { - if (r < 0) - return; - s->tick(); - } - }; - public: enum { SERVICE_HEALTH = 0x01, @@ -50,7 +40,6 @@ protected: epoch_t epoch; QuorumService(Monitor *m) : - tick_event(NULL), tick_period(g_conf->mon_tick_interval), mon(m), epoch(0) @@ -70,7 +59,11 @@ protected: if (tick_period <= 0) return; - tick_event = new C_Tick(this); + tick_event = new C_MonContext(mon, [this](int r) { + if (r < 0) + return; + tick(); + }); mon->timer.add_event_after(tick_period, tick_event); } -- 2.39.5