From e722a1b169302cec30fd7e7c70990ce7232309ee Mon Sep 17 00:00:00 2001 From: Akilesh Kailash Date: Thu, 1 Apr 2021 19:19:50 +0000 Subject: [PATCH] libsnapshot:snapuserd: Fix memory leak When worker threads were created, snapuserd was converted to a shared_pointer. Earlier, memory was forcefully released by setting snapuserd to nullptr which worked as it was a unique pointer. Now, every worker thread holds a reference. Clear the vector once all the worker threads are terminated. Test: Apply OTA and verify memory is released after OTA is applied Bug: 183652708 Signed-off-by: Akilesh Kailash Change-Id: I256d26d98b02ad599aff49b92192226546c59b17 --- fs_mgr/libsnapshot/snapuserd.h | 1 + fs_mgr/libsnapshot/snapuserd_server.cpp | 8 +++++++- fs_mgr/libsnapshot/snapuserd_server.h | 10 +++++++++- 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/fs_mgr/libsnapshot/snapuserd.h b/fs_mgr/libsnapshot/snapuserd.h index 933536442..87c552824 100644 --- a/fs_mgr/libsnapshot/snapuserd.h +++ b/fs_mgr/libsnapshot/snapuserd.h @@ -158,6 +158,7 @@ class Snapuserd : public std::enable_shared_from_this { bool CommitMerge(int num_merge_ops); void CloseFds() { cow_fd_ = {}; } + void FreeResources() { worker_threads_.clear(); } size_t GetMetadataAreaSize() { return vec_.size(); } void* GetExceptionBuffer(size_t i) { return vec_[i].get(); } diff --git a/fs_mgr/libsnapshot/snapuserd_server.cpp b/fs_mgr/libsnapshot/snapuserd_server.cpp index 167895e8e..64332d191 100644 --- a/fs_mgr/libsnapshot/snapuserd_server.cpp +++ b/fs_mgr/libsnapshot/snapuserd_server.cpp @@ -219,7 +219,13 @@ void SnapuserdServer::RunThread(std::shared_ptr handler) { auto iter = FindHandler(&lock, handler->misc_name()); if (iter == dm_users_.end()) { // RemoveAndJoinHandler() already removed us from the list, and is - // now waiting on a join(), so just return. + // now waiting on a join(), so just return. Additionally, release + // all the resources held by snapuserd object which are shared + // by worker threads. This should be done when the last reference + // of "handler" is released; but we will explicitly release here + // to make sure snapuserd object is freed as it is the biggest + // consumer of memory in the daemon. + handler->FreeResources(); LOG(INFO) << "Exiting handler thread to allow for join: " << misc_name; return; } diff --git a/fs_mgr/libsnapshot/snapuserd_server.h b/fs_mgr/libsnapshot/snapuserd_server.h index e9d575d01..6699189ea 100644 --- a/fs_mgr/libsnapshot/snapuserd_server.h +++ b/fs_mgr/libsnapshot/snapuserd_server.h @@ -49,7 +49,15 @@ class DmUserHandler { public: explicit DmUserHandler(std::shared_ptr snapuserd); - void FreeResources() { snapuserd_ = nullptr; } + void FreeResources() { + // Each worker thread holds a reference to snapuserd. + // Clear them so that all the resources + // held by snapuserd is released + if (snapuserd_) { + snapuserd_->FreeResources(); + snapuserd_ = nullptr; + } + } const std::shared_ptr& snapuserd() const { return snapuserd_; } std::thread& thread() { return thread_; }