From 2449b3a5c365987746ada095fde30e3dc63ee0c7 Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Thu, 13 Jul 2017 14:49:48 +0800 Subject: [PATCH] common,mds,mgr,mon,osd: store event only if it's added otherwise * we will try to cancel it even it's never been added * we will keep a dangling pointer around. which is, well, scaring. * static analyzer will yell at us: Memory - illegal accesses (USE_AFTER_FREE) Signed-off-by: Kefu Chai --- src/client/Client.cc | 23 ++----- src/client/Client.h | 1 - src/common/Timer.cc | 8 +-- src/common/Timer.h | 4 +- src/journal/JournalMetadata.cc | 6 +- src/journal/ObjectPlayer.cc | 13 ++-- src/journal/ObjectPlayer.h | 6 -- src/journal/ObjectRecorder.cc | 10 +-- src/journal/ObjectRecorder.h | 10 +-- src/mds/Beacon.cc | 22 ++---- src/mds/Beacon.h | 3 +- src/mds/MDSDaemon.cc | 21 ++---- src/mds/MDSDaemon.h | 3 +- src/mgr/MgrClient.cc | 11 +-- src/mon/Elector.cc | 10 +-- src/mon/MgrMonitor.cc | 7 +- src/mon/Monitor.cc | 61 ++++++++++------- src/mon/Paxos.cc | 67 +++++++++---------- src/mon/PaxosService.cc | 6 +- src/osd/PrimaryLogPG.h | 6 +- src/osd/Watch.cc | 12 ++-- src/test/perf_local.cc | 5 +- src/test/rbd_mirror/mock/MockSafeTimer.h | 2 +- .../rbd_mirror/test_mock_ImageReplayer.cc | 8 ++- .../rbd_mirror/test_mock_InstanceReplayer.cc | 9 ++- src/test/rbd_mirror/test_mock_PoolWatcher.cc | 17 +++-- src/tools/rbd_mirror/PoolWatcher.cc | 9 +-- .../rbd_mirror/image_sync/ImageCopyRequest.cc | 5 +- 28 files changed, 171 insertions(+), 194 deletions(-) diff --git a/src/client/Client.cc b/src/client/Client.cc index e461ab4a59e..419450f1b04 100644 --- a/src/client/Client.cc +++ b/src/client/Client.cc @@ -5944,19 +5944,6 @@ void Client::unmount() ldout(cct, 2) << "unmounted." << dendl; } - - -class C_C_Tick : public Context { - Client *client; -public: - explicit C_C_Tick(Client *c) : client(c) {} - void finish(int r) override { - // Called back via Timer, which takes client_lock for us - assert(client->client_lock.is_locked_by_me()); - client->tick(); - } -}; - void Client::flush_cap_releases() { // send any cap releases @@ -5985,9 +5972,13 @@ void Client::tick() } ldout(cct, 21) << "tick" << dendl; - tick_event = new C_C_Tick(this); - timer.add_event_after(cct->_conf->client_tick_interval, tick_event); - + tick_event = timer.add_event_after( + cct->_conf->client_tick_interval, + new FunctionContext([this](int) { + // Called back via Timer, which takes client_lock for us + assert(client_lock.is_locked_by_me()); + tick(); + })); utime_t now = ceph_clock_now(); if (!mounted && !mds_requests.empty()) { diff --git a/src/client/Client.h b/src/client/Client.h index beefa1eba5c..555741125e5 100644 --- a/src/client/Client.h +++ b/src/client/Client.h @@ -498,7 +498,6 @@ protected: friend class C_Client_CacheInvalidate; // calls ino_invalidate_cb friend class C_Client_DentryInvalidate; // calls dentry_invalidate_cb friend class C_Block_Sync; // Calls block map and protected helpers - friend class C_C_Tick; // Asserts on client_lock friend class C_Client_RequestInterrupt; friend class C_Client_Remount; friend void intrusive_ptr_release(Inode *in); diff --git a/src/common/Timer.cc b/src/common/Timer.cc index f211a6f8ff8..45305f553fa 100644 --- a/src/common/Timer.cc +++ b/src/common/Timer.cc @@ -114,7 +114,7 @@ void SafeTimer::timer_thread() lock.Unlock(); } -bool SafeTimer::add_event_after(double seconds, Context *callback) +Context* SafeTimer::add_event_after(double seconds, Context *callback) { assert(lock.is_locked()); @@ -123,14 +123,14 @@ bool SafeTimer::add_event_after(double seconds, Context *callback) return add_event_at(when, callback); } -bool SafeTimer::add_event_at(utime_t when, Context *callback) +Context* SafeTimer::add_event_at(utime_t when, Context *callback) { assert(lock.is_locked()); ldout(cct,10) << __func__ << " " << when << " -> " << callback << dendl; if (stopping) { ldout(cct,5) << __func__ << " already shutdown, event not added" << dendl; delete callback; - return false; + return nullptr; } scheduled_map_t::value_type s_val(when, callback); scheduled_map_t::iterator i = schedule.insert(s_val); @@ -145,7 +145,7 @@ bool SafeTimer::add_event_at(utime_t when, Context *callback) * adjust our timeout. */ if (i == schedule.begin()) cond.Signal(); - return true; + return callback; } bool SafeTimer::cancel_event(Context *callback) diff --git a/src/common/Timer.h b/src/common/Timer.h index 861b239ca32..8fd478a9934 100644 --- a/src/common/Timer.h +++ b/src/common/Timer.h @@ -70,8 +70,8 @@ public: /* Schedule an event in the future * Call with the event_lock LOCKED */ - bool add_event_after(double seconds, Context *callback); - bool add_event_at(utime_t when, Context *callback); + Context* add_event_after(double seconds, Context *callback); + Context* add_event_at(utime_t when, Context *callback); /* Cancel an event. * Call with the event_lock LOCKED diff --git a/src/journal/JournalMetadata.cc b/src/journal/JournalMetadata.cc index 3d6fcfb2eca..4073216bcdf 100644 --- a/src/journal/JournalMetadata.cc +++ b/src/journal/JournalMetadata.cc @@ -802,9 +802,9 @@ void JournalMetadata::schedule_commit_task() { assert(m_lock.is_locked()); assert(m_commit_position_ctx != nullptr); if (m_commit_position_task_ctx == NULL) { - m_commit_position_task_ctx = new C_CommitPositionTask(this); - m_timer->add_event_after(m_settings.commit_interval, - m_commit_position_task_ctx); + m_commit_position_task_ctx = + m_timer->add_event_after(m_settings.commit_interval, + new C_CommitPositionTask(this)); } } diff --git a/src/journal/ObjectPlayer.cc b/src/journal/ObjectPlayer.cc index 92dd702615b..8292ebb1abf 100644 --- a/src/journal/ObjectPlayer.cc +++ b/src/journal/ObjectPlayer.cc @@ -234,9 +234,12 @@ void ObjectPlayer::schedule_watch() { } ldout(m_cct, 20) << __func__ << ": " << m_oid << " scheduling watch" << dendl; - assert(m_watch_task == NULL); - m_watch_task = new C_WatchTask(this); - m_timer.add_event_after(m_watch_interval, m_watch_task); + assert(m_watch_task == nullptr); + m_watch_task = m_timer.add_event_after( + m_watch_interval, + new FunctionContext([this](int) { + handle_watch_task(); + })); } bool ObjectPlayer::cancel_watch() { @@ -301,10 +304,6 @@ void ObjectPlayer::C_Fetch::finish(int r) { on_finish->complete(r); } -void ObjectPlayer::C_WatchTask::finish(int r) { - object_player->handle_watch_task(); -} - void ObjectPlayer::C_WatchFetch::finish(int r) { object_player->handle_watch_fetched(r); } diff --git a/src/journal/ObjectPlayer.h b/src/journal/ObjectPlayer.h index 3d495ba7ff7..a3cbe807332 100644 --- a/src/journal/ObjectPlayer.h +++ b/src/journal/ObjectPlayer.h @@ -90,12 +90,6 @@ private: } void finish(int r) override; }; - struct C_WatchTask : public Context { - ObjectPlayerPtr object_player; - C_WatchTask(ObjectPlayer *o) : object_player(o) { - } - void finish(int r) override; - }; struct C_WatchFetch : public Context { ObjectPlayerPtr object_player; C_WatchFetch(ObjectPlayer *o) : object_player(o) { diff --git a/src/journal/ObjectRecorder.cc b/src/journal/ObjectRecorder.cc index a2faeae8aa6..a87c31ddb29 100644 --- a/src/journal/ObjectRecorder.cc +++ b/src/journal/ObjectRecorder.cc @@ -28,7 +28,7 @@ ObjectRecorder::ObjectRecorder(librados::IoCtx &ioctx, const std::string &oid, m_timer_lock(timer_lock), m_handler(handler), m_order(order), m_soft_max_size(1 << m_order), m_flush_interval(flush_interval), m_flush_bytes(flush_bytes), m_flush_age(flush_age), m_flush_handler(this), - m_append_task(NULL), m_lock(lock), m_append_tid(0), m_pending_bytes(0), + m_lock(lock), m_append_tid(0), m_pending_bytes(0), m_size(0), m_overflowed(false), m_object_closed(false), m_in_flight_flushes(false), m_aio_scheduled(false) { m_ioctx.dup(ioctx); @@ -194,9 +194,11 @@ void ObjectRecorder::cancel_append_task() { void ObjectRecorder::schedule_append_task() { Mutex::Locker locker(m_timer_lock); - if (m_append_task == NULL && m_flush_age > 0) { - m_append_task = new C_AppendTask(this); - m_timer.add_event_after(m_flush_age, m_append_task); + if (m_append_task == nullptr && m_flush_age > 0) { + m_append_task = m_timer.add_event_after( + m_flush_age, new FunctionContext([this](int) { + handle_append_task(); + })); } } diff --git a/src/journal/ObjectRecorder.h b/src/journal/ObjectRecorder.h index aad46690134..22a46697c52 100644 --- a/src/journal/ObjectRecorder.h +++ b/src/journal/ObjectRecorder.h @@ -90,14 +90,6 @@ private: object_recorder->flush(future); } }; - struct C_AppendTask : public Context { - ObjectRecorder *object_recorder; - C_AppendTask(ObjectRecorder *o) : object_recorder(o) { - } - void finish(int r) override { - object_recorder->handle_append_task(); - } - }; struct C_AppendFlush : public Context { ObjectRecorder *object_recorder; uint64_t tid; @@ -132,7 +124,7 @@ private: FlushHandler m_flush_handler; - C_AppendTask *m_append_task; + Context *m_append_task = nullptr; mutable std::shared_ptr m_lock; AppendBuffers m_append_buffers; diff --git a/src/mds/Beacon.cc b/src/mds/Beacon.cc index 602240bfe12..aae46eab4d7 100644 --- a/src/mds/Beacon.cc +++ b/src/mds/Beacon.cc @@ -33,18 +33,6 @@ #define dout_prefix *_dout << "mds.beacon." << name << ' ' -class Beacon::C_MDS_BeaconSender : public Context { -public: - explicit C_MDS_BeaconSender(Beacon *beacon_) : beacon(beacon_) {} - void finish(int r) override { - assert(beacon->lock.is_locked_by_me()); - beacon->sender = NULL; - beacon->_send(); - } -private: - Beacon *beacon; -}; - Beacon::Beacon(CephContext *cct_, MonClient *monc_, std::string name_) : Dispatcher(cct_), lock("Beacon"), monc(monc_), timer(g_ceph_context, lock), name(name_), standby_for_rank(MDS_RANK_NONE), @@ -52,7 +40,6 @@ Beacon::Beacon(CephContext *cct_, MonClient *monc_, std::string name_) : awaiting_seq(-1) { last_seq = 0; - sender = NULL; was_laggy = false; epoch = 0; @@ -191,8 +178,13 @@ void Beacon::_send() if (sender) { timer.cancel_event(sender); } - sender = new C_MDS_BeaconSender(this); - timer.add_event_after(g_conf->mds_beacon_interval, sender); + sender = timer.add_event_after( + g_conf->mds_beacon_interval, + new FunctionContext([this](int) { + assert(lock.is_locked_by_me()); + sender = nullptr; + _send(); + })); if (!cct->get_heartbeat_map()->is_healthy()) { /* If anything isn't progressing, let avoid sending a beacon so that diff --git a/src/mds/Beacon.h b/src/mds/Beacon.h index 571f7f55995..201804def07 100644 --- a/src/mds/Beacon.h +++ b/src/mds/Beacon.h @@ -102,8 +102,7 @@ private: MDSHealth health; // Ticker - class C_MDS_BeaconSender; - C_MDS_BeaconSender *sender; + Context *sender = nullptr; version_t awaiting_seq; Cond waiting_cond; diff --git a/src/mds/MDSDaemon.cc b/src/mds/MDSDaemon.cc index 19be284834b..fabcdb059a1 100644 --- a/src/mds/MDSDaemon.cc +++ b/src/mds/MDSDaemon.cc @@ -69,18 +69,6 @@ #undef dout_prefix #define dout_prefix *_dout << "mds." << name << ' ' - -class MDSDaemon::C_MDS_Tick : public Context { - protected: - MDSDaemon *mds_daemon; -public: - explicit C_MDS_Tick(MDSDaemon *m) : mds_daemon(m) {} - void finish(int r) override { - assert(mds_daemon->mds_lock.is_locked_by_me()); - mds_daemon->tick(); - } -}; - // cons/des MDSDaemon::MDSDaemon(const std::string &n, Messenger *m, MonClient *mc) : Dispatcher(m->cct), @@ -102,7 +90,6 @@ MDSDaemon::MDSDaemon(const std::string &n, Messenger *m, MonClient *mc) : mgrc(m->cct, m), log_client(m->cct, messenger, &mc->monmap, LogClient::NO_FLAGS), mds_rank(NULL), - tick_event(0), asok_hook(NULL) { orig_argc = 0; @@ -539,8 +526,12 @@ void MDSDaemon::reset_tick() if (tick_event) timer.cancel_event(tick_event); // schedule - tick_event = new C_MDS_Tick(this); - timer.add_event_after(g_conf->mds_tick_interval, tick_event); + tick_event = timer.add_event_after( + g_conf->mds_tick_interval, + new FunctionContext([this](int) { + assert(mds_lock.is_locked_by_me()); + tick(); + })); } void MDSDaemon::tick() diff --git a/src/mds/MDSDaemon.h b/src/mds/MDSDaemon.h index 0c7a1a7378a..0e3bbaf2639 100644 --- a/src/mds/MDSDaemon.h +++ b/src/mds/MDSDaemon.h @@ -87,8 +87,7 @@ class MDSDaemon : public Dispatcher, public md_config_obs_t { const std::set &changed) override; protected: // tick and other timer fun - class C_MDS_Tick; - C_MDS_Tick *tick_event; + Context *tick_event = nullptr; void reset_tick(); void wait_for_omap_osds(); diff --git a/src/mgr/MgrClient.cc b/src/mgr/MgrClient.cc index 849590ba93c..e85aab6ebc5 100644 --- a/src/mgr/MgrClient.cc +++ b/src/mgr/MgrClient.cc @@ -114,11 +114,12 @@ void MgrClient::reconnect() when += cct->_conf->mgr_connect_retry_interval; if (now < when) { if (!connect_retry_callback) { - connect_retry_callback = new FunctionContext([this](int r){ - connect_retry_callback = nullptr; - reconnect(); - }); - timer.add_event_at(when, connect_retry_callback); + connect_retry_callback = timer.add_event_at( + when, + new FunctionContext([this](int r){ + connect_retry_callback = nullptr; + reconnect(); + })); } ldout(cct, 4) << "waiting to retry connect until " << when << dendl; return; diff --git a/src/mon/Elector.cc b/src/mon/Elector.cc index b7fde85528d..f69bcf16d5a 100644 --- a/src/mon/Elector.cc +++ b/src/mon/Elector.cc @@ -159,11 +159,11 @@ void Elector::reset_timer(double plus) * as far as we know, we may even be dead); so, just propose ourselves as the * Leader. */ - expire_event = new C_MonContext(mon, [this](int) { - expire(); - }); - mon->timer.add_event_after(g_conf->mon_election_timeout + plus, - expire_event); + expire_event = mon->timer.add_event_after( + g_conf->mon_election_timeout + plus, + new C_MonContext(mon, [this](int) { + expire(); + })); } diff --git a/src/mon/MgrMonitor.cc b/src/mon/MgrMonitor.cc index fc4f08d8d70..e35a43a6738 100644 --- a/src/mon/MgrMonitor.cc +++ b/src/mon/MgrMonitor.cc @@ -453,10 +453,11 @@ void MgrMonitor::send_digests() sub->session->con->send_message(mdigest); } - digest_event = new C_MonContext(mon, [this](int){ + digest_event = mon->timer.add_event_after( + g_conf->mon_mgr_digest_period, + new C_MonContext(mon, [this](int) { send_digests(); - }); - mon->timer.add_event_after(g_conf->mon_mgr_digest_period, digest_event); + })); } void MgrMonitor::cancel_timer() diff --git a/src/mon/Monitor.cc b/src/mon/Monitor.cc index 3f1f56036e9..74f879979b5 100755 --- a/src/mon/Monitor.cc +++ b/src/mon/Monitor.cc @@ -1240,10 +1240,11 @@ void Monitor::sync_reset_timeout() dout(10) << __func__ << dendl; if (sync_timeout_event) timer.cancel_event(sync_timeout_event); - sync_timeout_event = new C_MonContext(this, [this](int) { - sync_timeout(); - }); - timer.add_event_after(g_conf->mon_sync_timeout, sync_timeout_event); + sync_timeout_event = timer.add_event_after( + g_conf->mon_sync_timeout, + new C_MonContext(this, [this](int) { + sync_timeout(); + })); } void Monitor::sync_finish(version_t last_committed) @@ -1586,8 +1587,12 @@ void Monitor::reset_probe_timeout() 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; + if (timer.add_event_after(t, probe_timeout_event)) { + dout(10) << "reset_probe_timeout " << probe_timeout_event + << " after " << t << " seconds" << dendl; + } else { + probe_timeout_event = nullptr; + } } void Monitor::probe_timeout(int r) @@ -2279,14 +2284,14 @@ void Monitor::health_tick_start() dout(15) << __func__ << dendl; health_tick_stop(); - 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); + health_tick_event = timer.add_event_after( + cct->_conf->mon_health_to_clog_tick_interval, + new C_MonContext(this, [this](int r) { + if (r < 0) + return; + do_health_to_clog(); + health_tick_start(); + })); } void Monitor::health_tick_stop() @@ -2333,7 +2338,9 @@ void Monitor::health_interval_start() return; do_health_to_clog_interval(); }); - timer.add_event_at(next, health_interval_event); + if (!timer.add_event_at(next, health_interval_event)) { + health_interval_event = nullptr; + } } void Monitor::health_interval_stop() @@ -4532,10 +4539,11 @@ void Monitor::timecheck_reset_event() << " rounds_since_clean " << timecheck_rounds_since_clean << dendl; - timecheck_event = new C_MonContext(this, [this](int) { - timecheck_start_round(); - }); - timer.add_event_after(delay, timecheck_event); + timecheck_event = timer.add_event_after( + delay, + new C_MonContext(this, [this](int) { + timecheck_start_round(); + })); } void Monitor::timecheck_check_skews() @@ -5384,10 +5392,11 @@ void Monitor::scrub_event_start() return; } - scrub_event = new C_MonContext(this, [this](int) { + scrub_event = timer.add_event_after( + cct->_conf->mon_scrub_interval, + new C_MonContext(this, [this](int) { scrub_start(); - }); - timer.add_event_after(cct->_conf->mon_scrub_interval, scrub_event); + })); } void Monitor::scrub_event_cancel() @@ -5411,11 +5420,11 @@ void Monitor::scrub_reset_timeout() { dout(15) << __func__ << " reset timeout event" << dendl; scrub_cancel_timeout(); - - scrub_timeout_event = new C_MonContext(this, [this](int) { + scrub_timeout_event = timer.add_event_after( + g_conf->mon_scrub_timeout, + new C_MonContext(this, [this](int) { scrub_timeout(); - }); - timer.add_event_after(g_conf->mon_scrub_timeout, scrub_timeout_event); + })); } /************ TICK ***************/ diff --git a/src/mon/Paxos.cc b/src/mon/Paxos.cc index 3555d60dba5..ff309b5e85a 100644 --- a/src/mon/Paxos.cc +++ b/src/mon/Paxos.cc @@ -195,14 +195,14 @@ void Paxos::collect(version_t oldpn) } // set timeout event - collect_timeout_event = new C_MonContext(mon, [this](int r) { + collect_timeout_event = mon->timer.add_event_after( + g_conf->mon_accept_timeout_factor * + g_conf->mon_lease, + 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); + })); } @@ -687,14 +687,13 @@ void Paxos::begin(bufferlist& v) } // set timeout event - 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); + accept_timeout_event = mon->timer.add_event_after( + g_conf->mon_accept_timeout_factor * g_conf->mon_lease, + new C_MonContext(mon, [this](int r) { + if (r == -ECANCELED) + return; + accept_timeout(); + })); } // peon @@ -992,26 +991,25 @@ 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_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); + lease_ack_timeout_event = mon->timer.add_event_after( + g_conf->mon_lease_ack_timeout_factor * g_conf->mon_lease, + new C_MonContext(mon, [this](int r) { + if (r == -ECANCELED) + return; + lease_ack_timeout(); + })); } // set renew event - 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; - mon->timer.add_event_at(at, lease_renew_event); + lease_renew_event = mon->timer.add_event_at( + at, new C_MonContext(mon, [this](int r) { + if (r == -ECANCELED) + return; + lease_renew_timeout(); + })); } void Paxos::warn_on_future_time(utime_t t, entity_name_t from) @@ -1195,14 +1193,13 @@ 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_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); + lease_timeout_event = mon->timer.add_event_after( + g_conf->mon_lease_ack_timeout_factor * g_conf->mon_lease, + new C_MonContext(mon, [this](int r) { + if (r == -ECANCELED) + return; + lease_timeout(); + })); } void Paxos::lease_timeout() diff --git a/src/mon/PaxosService.cc b/src/mon/PaxosService.cc index dcd83506ceb..de732c32230 100644 --- a/src/mon/PaxosService.cc +++ b/src/mon/PaxosService.cc @@ -117,7 +117,7 @@ bool PaxosService::dispatch(MonOpRequestRef op) * Callback class used to propose the pending value once the proposal_timer * fires up. */ - proposal_timer = new C_MonContext(mon, [this](int r) { + auto do_propose = new C_MonContext(mon, [this](int r) { proposal_timer = 0; if (r >= 0) { propose_pending(); @@ -127,9 +127,9 @@ bool PaxosService::dispatch(MonOpRequestRef op) assert(0 == "bad return value for proposal_timer"); } }); - dout(10) << " setting proposal_timer " << proposal_timer + dout(10) << " setting proposal_timer " << do_propose << " with delay of " << delay << dendl; - mon->timer.add_event_after(delay, proposal_timer); + proposal_timer = mon->timer.add_event_after(delay, do_propose); } else { dout(10) << " proposal_timer already set" << dendl; } diff --git a/src/osd/PrimaryLogPG.h b/src/osd/PrimaryLogPG.h index a4d34d17141..df2a45f5877 100644 --- a/src/osd/PrimaryLogPG.h +++ b/src/osd/PrimaryLogPG.h @@ -1551,10 +1551,10 @@ private: }; auto *pg = context< SnapTrimmer >().pg; if (pg->cct->_conf->osd_snap_trim_sleep > 0) { - wakeup = new OnTimer{pg, pg->get_osdmap()->get_epoch()}; Mutex::Locker l(pg->osd->snap_sleep_lock); - pg->osd->snap_sleep_timer.add_event_after( - pg->cct->_conf->osd_snap_trim_sleep, wakeup); + wakeup = pg->osd->snap_sleep_timer.add_event_after( + pg->cct->_conf->osd_snap_trim_sleep, + new OnTimer{pg, pg->get_osdmap()->get_epoch()}); } else { post_event(SnapTrimTimerReady()); } diff --git a/src/osd/Watch.cc b/src/osd/Watch.cc index df92bb7712c..7ff9f99b2bf 100644 --- a/src/osd/Watch.cc +++ b/src/osd/Watch.cc @@ -124,9 +124,9 @@ void Notify::register_cb() { osd->watch_lock.Lock(); cb = new NotifyTimeoutCB(self.lock()); - osd->watch_timer.add_event_after( - timeout, - cb); + if (!osd->watch_timer.add_event_after(timeout, cb)) { + cb = nullptr; + } osd->watch_lock.Unlock(); } } @@ -333,9 +333,9 @@ void Watch::register_cb() dout(15) << "registering callback, timeout: " << timeout << dendl; } cb = new HandleWatchTimeout(self.lock()); - osd->watch_timer.add_event_after( - timeout, - cb); + if (!osd->watch_timer.add_event_after(timeout, cb)) { + cb = nullptr; + } } void Watch::unregister_cb() diff --git a/src/test/perf_local.cc b/src/test/perf_local.cc index 98cccd87bbd..c3b9f7cccc8 100644 --- a/src/test/perf_local.cc +++ b/src/test/perf_local.cc @@ -785,8 +785,9 @@ double perf_timer() uint64_t start = Cycles::rdtsc(); Mutex::Locker l(lock); for (int i = 0; i < count; i++) { - timer.add_event_after(12345, c[i]); - timer.cancel_event(c[i]); + if (timer.add_event_after(12345, c[i])) { + timer.cancel_event(c[i]); + } } uint64_t stop = Cycles::rdtsc(); delete[] c; diff --git a/src/test/rbd_mirror/mock/MockSafeTimer.h b/src/test/rbd_mirror/mock/MockSafeTimer.h index 3de5fbcdbf5..32d58471d4a 100644 --- a/src/test/rbd_mirror/mock/MockSafeTimer.h +++ b/src/test/rbd_mirror/mock/MockSafeTimer.h @@ -9,7 +9,7 @@ struct Context; struct MockSafeTimer { - MOCK_METHOD2(add_event_after, void(double, Context*)); + MOCK_METHOD2(add_event_after, Context*(double, Context*)); MOCK_METHOD1(cancel_event, bool(Context *)); }; diff --git a/src/test/rbd_mirror/test_mock_ImageReplayer.cc b/src/test/rbd_mirror/test_mock_ImageReplayer.cc index 7a0bb6706f6..9e2006c9d51 100644 --- a/src/test/rbd_mirror/test_mock_ImageReplayer.cc +++ b/src/test/rbd_mirror/test_mock_ImageReplayer.cc @@ -105,6 +105,7 @@ using ::testing::InSequence; using ::testing::Invoke; using ::testing::MatcherCast; using ::testing::Return; +using ::testing::ReturnArg; using ::testing::SetArgPointee; using ::testing::WithArg; @@ -356,9 +357,10 @@ public: void expect_add_event_after_repeatedly(MockThreads &mock_threads) { EXPECT_CALL(*mock_threads.timer, add_event_after(_, _)) .WillRepeatedly( - Invoke([this](double seconds, Context *ctx) { - m_threads->timer->add_event_after(seconds, ctx); - })); + DoAll(Invoke([this](double seconds, Context *ctx) { + m_threads->timer->add_event_after(seconds, ctx); + }), + ReturnArg<1>())); EXPECT_CALL(*mock_threads.timer, cancel_event(_)) .WillRepeatedly( Invoke([this](Context *ctx) { diff --git a/src/test/rbd_mirror/test_mock_InstanceReplayer.cc b/src/test/rbd_mirror/test_mock_InstanceReplayer.cc index 1903c55f2c9..02bc0886df5 100644 --- a/src/test/rbd_mirror/test_mock_InstanceReplayer.cc +++ b/src/test/rbd_mirror/test_mock_InstanceReplayer.cc @@ -121,9 +121,11 @@ namespace rbd { namespace mirror { using ::testing::_; +using ::testing::DoAll; using ::testing::InSequence; using ::testing::Invoke; using ::testing::Return; +using ::testing::ReturnArg; using ::testing::ReturnRef; using ::testing::WithArg; @@ -146,8 +148,8 @@ public: void expect_add_event_after(MockThreads &mock_threads, Context** timer_ctx = nullptr) { EXPECT_CALL(*mock_threads.timer, add_event_after(_, _)) - .WillOnce(WithArg<1>( - Invoke([this, &mock_threads, timer_ctx](Context *ctx) { + .WillOnce(DoAll( + WithArg<1>(Invoke([this, &mock_threads, timer_ctx](Context *ctx) { assert(mock_threads.timer_lock.is_locked()); if (timer_ctx != nullptr) { *timer_ctx = ctx; @@ -159,7 +161,8 @@ public: ctx->complete(0); }), 0); } - }))); + })), + ReturnArg<1>())); } void expect_cancel_event(MockThreads &mock_threads, bool canceled) { diff --git a/src/test/rbd_mirror/test_mock_PoolWatcher.cc b/src/test/rbd_mirror/test_mock_PoolWatcher.cc index 1b7877434ad..4c7463d660c 100644 --- a/src/test/rbd_mirror/test_mock_PoolWatcher.cc +++ b/src/test/rbd_mirror/test_mock_PoolWatcher.cc @@ -145,6 +145,7 @@ using ::testing::DoAll; using ::testing::InSequence; using ::testing::Invoke; using ::testing::Return; +using ::testing::ReturnArg; using ::testing::StrEq; using ::testing::WithArg; using ::testing::WithoutArgs; @@ -238,13 +239,15 @@ public: void expect_timer_add_event(MockThreads &mock_threads) { EXPECT_CALL(*mock_threads.timer, add_event_after(_, _)) - .WillOnce(WithArg<1>(Invoke([this](Context *ctx) { - auto wrapped_ctx = new FunctionContext([this, ctx](int r) { - Mutex::Locker timer_locker(m_threads->timer_lock); - ctx->complete(r); - }); - m_threads->work_queue->queue(wrapped_ctx, 0); - }))); + .WillOnce(DoAll(WithArg<1>(Invoke([this](Context *ctx) { + auto wrapped_ctx = + new FunctionContext([this, ctx](int r) { + Mutex::Locker timer_locker(m_threads->timer_lock); + ctx->complete(r); + }); + m_threads->work_queue->queue(wrapped_ctx, 0); + })), + ReturnArg<1>())); } int when_shut_down(MockPoolWatcher &mock_pool_watcher) { diff --git a/src/tools/rbd_mirror/PoolWatcher.cc b/src/tools/rbd_mirror/PoolWatcher.cc index 18c6df3840f..8d60aa4f47a 100644 --- a/src/tools/rbd_mirror/PoolWatcher.cc +++ b/src/tools/rbd_mirror/PoolWatcher.cc @@ -362,10 +362,11 @@ void PoolWatcher::schedule_refresh_images(double interval) { } m_image_ids_invalid = true; - m_timer_ctx = new FunctionContext([this](int r) { - process_refresh_images(); - }); - m_threads->timer->add_event_after(interval, m_timer_ctx); + m_timer_ctx = m_threads->timer->add_event_after( + interval, + new FunctionContext([this](int r) { + process_refresh_images(); + })); } template diff --git a/src/tools/rbd_mirror/image_sync/ImageCopyRequest.cc b/src/tools/rbd_mirror/image_sync/ImageCopyRequest.cc index 6278d010155..6768caa005b 100644 --- a/src/tools/rbd_mirror/image_sync/ImageCopyRequest.cc +++ b/src/tools/rbd_mirror/image_sync/ImageCopyRequest.cc @@ -161,8 +161,9 @@ void ImageCopyRequest::send_object_copies() { { Mutex::Locker timer_locker(*m_timer_lock); if (m_update_sync_ctx) { - m_timer->add_event_after(m_update_sync_point_interval, - m_update_sync_ctx); + m_update_sync_ctx = m_timer->add_event_after( + m_update_sync_point_interval, + m_update_sync_ctx); } } -- 2.39.5