From 156359c9d09423bdeabab0514b9e0e0b7eae195b Mon Sep 17 00:00:00 2001 From: Venky Shankar Date: Tue, 21 Nov 2017 11:27:40 -0500 Subject: [PATCH] rbd-mirror: ImageMap memory leak fixes Signed-off-by: Venky Shankar --- src/test/rbd_mirror/test_mock_ImageMap.cc | 23 +++++++++++++++++++ src/tools/rbd_mirror/ImageMap.cc | 9 +++----- src/tools/rbd_mirror/ImageMap.h | 2 ++ src/tools/rbd_mirror/image_map/Action.cc | 28 +++++++++++++++++++---- src/tools/rbd_mirror/image_map/Action.h | 1 + src/tools/rbd_mirror/image_map/Policy.cc | 24 +++++++++++-------- src/tools/rbd_mirror/image_map/Policy.h | 4 ++-- 7 files changed, 70 insertions(+), 21 deletions(-) diff --git a/src/test/rbd_mirror/test_mock_ImageMap.cc b/src/test/rbd_mirror/test_mock_ImageMap.cc index 0e10e0bd66e..2514c686b4a 100644 --- a/src/test/rbd_mirror/test_mock_ImageMap.cc +++ b/src/test/rbd_mirror/test_mock_ImageMap.cc @@ -395,6 +395,8 @@ TEST_F(TestMockImageMap, SetLocalImages) { // remote peer ACKs image acquire request remote_peer_ack_nowait(mock_image_map.get(), global_image_ids_ack, 0); + + wait_for_scheduled_task(); ASSERT_EQ(0, when_shut_down(mock_image_map.get())); } @@ -452,6 +454,8 @@ TEST_F(TestMockImageMap, AddRemoveLocalImage) { ASSERT_TRUE(wait_for_listener_notify(remove_global_image_ids_ack.size())); remote_peer_ack_wait(mock_image_map.get(), remove_global_image_ids_ack, 0); + + wait_for_scheduled_task(); ASSERT_EQ(0, when_shut_down(mock_image_map.get())); } @@ -510,6 +514,8 @@ TEST_F(TestMockImageMap, AddRemoveRemoteImage) { ASSERT_TRUE(wait_for_listener_notify(remove_global_image_ids_ack.size() * 2)); remote_peer_ack_wait(mock_image_map.get(), remove_global_image_ids_ack, 0); + + wait_for_scheduled_task(); ASSERT_EQ(0, when_shut_down(mock_image_map.get())); } @@ -578,6 +584,7 @@ TEST_F(TestMockImageMap, AddRemoveRemoteImageDuplicateNotification) { // trigger duplicate "remove" notification mock_image_map->update_images("uuid1", {}, std::move(remove_global_image_ids_dup)); + wait_for_scheduled_task(); ASSERT_EQ(0, when_shut_down(mock_image_map.get())); } @@ -623,6 +630,8 @@ TEST_F(TestMockImageMap, AcquireImageErrorRetry) { // remote peer ACKs image acquire request remote_peer_ack_nowait(mock_image_map.get(), initial_global_image_ids_ack, 0); + + wait_for_scheduled_task(); ASSERT_EQ(0, when_shut_down(mock_image_map.get())); } @@ -702,6 +711,8 @@ TEST_F(TestMockImageMap, RemoveRemoteAndLocalImage) { ASSERT_TRUE(wait_for_listener_notify(local_remove_global_image_ids_ack.size())); remote_peer_ack_wait(mock_image_map.get(), local_remove_global_image_ids_ack, 0); + + wait_for_scheduled_task(); ASSERT_EQ(0, when_shut_down(mock_image_map.get())); } @@ -761,6 +772,8 @@ TEST_F(TestMockImageMap, AddInstance) { // completion shuffle action for now (re)mapped images remote_peer_ack_nowait(mock_image_map.get(), shuffled_global_image_ids, 0); + + wait_for_scheduled_task(); ASSERT_EQ(0, when_shut_down(mock_image_map.get())); } @@ -839,6 +852,8 @@ TEST_F(TestMockImageMap, RemoveInstance) { // completion shuffle action for now (re)mapped images remote_peer_ack_nowait(mock_image_map.get(), shuffled_global_image_ids, 0); + + wait_for_scheduled_task(); ASSERT_EQ(0, when_shut_down(mock_image_map.get())); } @@ -929,6 +944,8 @@ TEST_F(TestMockImageMap, AddInstancePingPongImageTest) { shuffled_global_image_ids.begin(), shuffled_global_image_ids.end(), std::inserter(reshuffled, reshuffled.begin())); ASSERT_TRUE(reshuffled.empty()); + + wait_for_scheduled_task(); ASSERT_EQ(0, when_shut_down(mock_image_map.get())); } @@ -1007,6 +1024,8 @@ TEST_F(TestMockImageMap, RemoveInstanceWithRemoveImage) { mock_image_map->update_instances_removed({"9876"}); remote_peer_ack_nowait(mock_image_map.get(), shuffled_global_image_ids, -EBLACKLISTED); + + wait_for_scheduled_task(); ASSERT_EQ(0, when_shut_down(mock_image_map.get())); } @@ -1085,6 +1104,8 @@ TEST_F(TestMockImageMap, AddErrorWithRetryAndResume) { // remote peer ACKs image acquire request remote_peer_ack_nowait(mock_image_map.get(), shuffled_global_image_ids, 0); + + wait_for_scheduled_task(); ASSERT_EQ(0, when_shut_down(mock_image_map.get())); } @@ -1163,6 +1184,8 @@ TEST_F(TestMockImageMap, MirrorUUIDUpdated) { // remote peer ACKs image acquire request remote_peer_ack_nowait(mock_image_map.get(), remote_added_global_image_ids_ack, 0); + + wait_for_scheduled_task(); ASSERT_EQ(0, when_shut_down(mock_image_map.get())); } diff --git a/src/tools/rbd_mirror/ImageMap.cc b/src/tools/rbd_mirror/ImageMap.cc index 8c1ea8ecd48..9a825ba8deb 100644 --- a/src/tools/rbd_mirror/ImageMap.cc +++ b/src/tools/rbd_mirror/ImageMap.cc @@ -327,9 +327,8 @@ void ImageMap::schedule_add_action(const std::string &global_image_id) { Context *on_acquire = new FunctionContext([this, global_image_id](int r) { queue_acquire_image(global_image_id); }); - Context *on_finish = new FunctionContext([this, global_image_id, on_update](int r) { + Context *on_finish = new FunctionContext([this, global_image_id](int r) { handle_add_action(global_image_id, r); - delete on_update; }); if (m_policy->add_image(global_image_id, on_update, on_acquire, on_finish)) { @@ -346,9 +345,8 @@ void ImageMap::schedule_remove_action(const std::string &global_image_id) { queue_release_image(global_image_id); }); Context *on_remove = new C_RemoveMap(this, global_image_id); - Context *on_finish = new FunctionContext([this, global_image_id, on_remove](int r) { + Context *on_finish = new FunctionContext([this, global_image_id](int r) { handle_remove_action(global_image_id, r); - delete on_remove; }); if (m_policy->remove_image(global_image_id, on_release, on_remove, on_finish)) { @@ -370,9 +368,8 @@ void ImageMap::schedule_shuffle_action(const std::string &global_image_id) { Context *on_acquire = new FunctionContext([this, global_image_id](int r) { queue_acquire_image(global_image_id); }); - Context *on_finish = new FunctionContext([this, global_image_id, on_update](int r) { + Context *on_finish = new FunctionContext([this, global_image_id](int r) { handle_shuffle_action(global_image_id, r); - delete on_update; }); if (m_policy->shuffle_image(global_image_id, on_release, on_update, on_acquire, on_finish)) { diff --git a/src/tools/rbd_mirror/ImageMap.h b/src/tools/rbd_mirror/ImageMap.h index 07ed766dafb..ad1d8e93deb 100644 --- a/src/tools/rbd_mirror/ImageMap.h +++ b/src/tools/rbd_mirror/ImageMap.h @@ -109,6 +109,8 @@ private: } }; + // context callbacks which are retry-able get deleted after + // transiting to the next state. struct C_UpdateMap : Context { ImageMap *image_map; std::string global_image_id; diff --git a/src/tools/rbd_mirror/image_map/Action.cc b/src/tools/rbd_mirror/image_map/Action.cc index a31dc6dbc1e..8fd19e8ef87 100644 --- a/src/tools/rbd_mirror/image_map/Action.cc +++ b/src/tools/rbd_mirror/image_map/Action.cc @@ -58,12 +58,32 @@ void Action::execute_state_callback(StateTransition::State state) { } } -void Action::execute_completion_callback(int r) { - auto it = context_map.find(StateTransition::STATE_COMPLETE); - assert(it != context_map.end()); +void Action::state_callback_complete(StateTransition::State state, bool delete_context) { + Context *on_state = nullptr; + auto it = context_map.find(state); + if (it != context_map.end()) { + std::swap(it->second, on_state); + } + + if (on_state && delete_context) { + delete on_state; + } +} + +void Action::execute_completion_callback(int r) { Context *on_finish = nullptr; - std::swap(it->second, on_finish); // just called once so its swap'd + + for (auto &ctx : context_map) { + Context *on_state = nullptr; + std::swap(ctx.second, on_state); + + if (ctx.first == StateTransition::STATE_COMPLETE) { + on_finish = on_state; + } else if (on_state != nullptr) { + delete on_state; + } + } if (on_finish != nullptr) { on_finish->complete(r); diff --git a/src/tools/rbd_mirror/image_map/Action.h b/src/tools/rbd_mirror/image_map/Action.h index 5cdd7ce2360..ba9c2c29763 100644 --- a/src/tools/rbd_mirror/image_map/Action.h +++ b/src/tools/rbd_mirror/image_map/Action.h @@ -21,6 +21,7 @@ public: Context *on_finish); void execute_state_callback(StateTransition::State state); + void state_callback_complete(StateTransition::State state, bool delete_context); void execute_completion_callback(int r); StateTransition::ActionType get_action_type() const; diff --git a/src/tools/rbd_mirror/image_map/Policy.cc b/src/tools/rbd_mirror/image_map/Policy.cc index d23794cc9a9..0a8cc952174 100644 --- a/src/tools/rbd_mirror/image_map/Policy.cc +++ b/src/tools/rbd_mirror/image_map/Policy.cc @@ -173,9 +173,9 @@ bool Policy::finish_action(const std::string &global_image_id, int r) { bool complete; if (r == 0) { post_execute_state_callback(global_image_id, action_state.transition.next_state); - complete = perform_transition(&action_state, action.get_action_type()); + complete = perform_transition(&action_state, &action); } else { - complete = abort_or_retry(&action_state); + complete = abort_or_retry(&action_state, &action); } if (complete) { @@ -229,13 +229,15 @@ bool Policy::is_transition_complete(StateTransition::ActionType action_type, Sta return complete; } -bool Policy::perform_transition(ActionState *action_state, StateTransition::ActionType action_type) { +bool Policy::perform_transition(ActionState *action_state, Action *action) { dout(20) << dendl; assert(m_map_lock.is_wlocked()); StateTransition::State state = action_state->transition.next_state; + // delete context based on retry_on_error flag + action->state_callback_complete(state, action_state->transition.retry_on_error); - bool complete = is_transition_complete(action_type, &state); + bool complete = is_transition_complete(action->get_action_type(), &state); dout(10) << ": advancing state: " << action_state->current_state << " -> " << state << dendl; @@ -248,15 +250,19 @@ bool Policy::perform_transition(ActionState *action_state, StateTransition::Acti return complete; } -bool Policy::abort_or_retry(ActionState *action_state) { +bool Policy::abort_or_retry(ActionState *action_state, Action *action) { dout(20) << dendl; assert(m_map_lock.is_wlocked()); bool complete = !action_state->transition.retry_on_error; - if (complete && action_state->last_idle_state) { - dout(10) << ": using last idle state=" << action_state->last_idle_state.get() - << " as current state" << dendl; - action_state->current_state = action_state->last_idle_state.get(); + if (complete) { + // we aborted, so the context need not be freed + action->state_callback_complete(action_state->transition.next_state, false); + if (action_state->last_idle_state) { + dout(10) << ": using last idle state=" << action_state->last_idle_state.get() + << " as current state" << dendl; + action_state->current_state = action_state->last_idle_state.get(); + } } return complete; diff --git a/src/tools/rbd_mirror/image_map/Policy.h b/src/tools/rbd_mirror/image_map/Policy.h index 636f0266202..2b38aa8b11c 100644 --- a/src/tools/rbd_mirror/image_map/Policy.h +++ b/src/tools/rbd_mirror/image_map/Policy.h @@ -114,8 +114,8 @@ private: void post_execute_state_callback(const std::string &global_image_id, StateTransition::State state); bool is_transition_complete(StateTransition::ActionType action_type, StateTransition::State *state); - bool perform_transition(ActionState *action_state, StateTransition::ActionType action_type); - bool abort_or_retry(ActionState *action_state); + bool perform_transition(ActionState *action_state, Action *action); + bool abort_or_retry(ActionState *action_state, Action *action); protected: typedef std::map > InstanceToImageMap;