From c28150f56f47d79e4b4802e082a51318af04f524 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Fri, 23 Jun 2023 12:19:57 -0700 Subject: [PATCH 1/5] snapuserd: Create a MergeWorker class. Merge threads and read threads share some common state but not much. Splitting into separate classes will help isolate dm-user specific code. Bug: 288273605 Test: snapuserd_test Change-Id: I612374bb0072b1eedf32c30270913dbe907cc6ab --- .../user-space-merge/handler_manager.cpp | 1 + .../user-space-merge/snapuserd_core.cpp | 15 ++++- .../user-space-merge/snapuserd_core.h | 37 ++---------- .../user-space-merge/snapuserd_merge.cpp | 31 ++++++---- .../user-space-merge/snapuserd_merge.h | 56 +++++++++++++++++++ 5 files changed, 92 insertions(+), 48 deletions(-) create mode 100644 fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_merge.h diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/handler_manager.cpp b/fs_mgr/libsnapshot/snapuserd/user-space-merge/handler_manager.cpp index bdba5c0cc..734e84f3c 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/handler_manager.cpp +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/handler_manager.cpp @@ -19,6 +19,7 @@ #include #include "snapuserd_core.h" +#include "snapuserd_merge.h" namespace android { namespace snapshot { diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.cpp b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.cpp index 8e1212b7b..4f7495c0a 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.cpp +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.cpp @@ -23,6 +23,8 @@ #include #include +#include "snapuserd_merge.h" + namespace android { namespace snapshot { @@ -57,8 +59,9 @@ bool SnapshotHandler::InitializeWorkers() { worker_threads_.push_back(std::move(wt)); } - merge_thread_ = std::make_unique(cow_device_, backing_store_device_, control_device_, - misc_name_, base_path_merge_, GetSharedPtr()); + merge_thread_ = + std::make_unique(cow_device_, backing_store_device_, control_device_, + misc_name_, base_path_merge_, GetSharedPtr()); read_ahead_thread_ = std::make_unique(cow_device_, backing_store_device_, misc_name_, GetSharedPtr()); @@ -316,7 +319,7 @@ bool SnapshotHandler::Start() { } std::future merge_thread = - std::async(std::launch::async, &Worker::RunMergeThread, merge_thread_.get()); + std::async(std::launch::async, &MergeWorker::Run, merge_thread_.get()); // Now that the worker threads are up, scan the partitions. if (perform_verification_) { @@ -452,5 +455,11 @@ bool SnapshotHandler::CheckPartitionVerification() { return update_verify_->CheckPartitionVerification(); } +void SnapshotHandler::FreeResources() { + worker_threads_.clear(); + read_ahead_thread_ = nullptr; + merge_thread_ = nullptr; +} + } // namespace snapshot } // namespace android diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.h b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.h index c984a61f8..fe10edfee 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.h +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.h @@ -75,6 +75,7 @@ enum class MERGE_IO_TRANSITION { READ_AHEAD_FAILURE, }; +class MergeWorker; class SnapshotHandler; enum class MERGE_GROUP_STATE { @@ -104,10 +105,9 @@ class Worker { const std::string& control_device, const std::string& misc_name, const std::string& base_path_merge, std::shared_ptr snapuserd); bool RunThread(); - bool RunMergeThread(); bool Init(); - private: + protected: // Initialization void InitializeBufsink(); bool InitializeFds(); @@ -145,22 +145,9 @@ class Worker { bool ProcessXorOp(const CowOperation* cow_op); bool ProcessOrderedOp(const CowOperation* cow_op); - // Merge related ops - bool Merge(); - bool AsyncMerge(); - bool SyncMerge(); - bool MergeOrderedOps(); - bool MergeOrderedOpsAsync(); - bool MergeReplaceZeroOps(); - int PrepareMerge(uint64_t* source_offset, int* pending_ops, - std::vector* replace_zero_vec = nullptr); - sector_t ChunkToSector(chunk_t chunk) { return chunk << CHUNK_SHIFT; } chunk_t SectorToChunk(sector_t sector) { return sector >> CHUNK_SHIFT; } - bool InitializeIouring(); - void FinalizeIouring(); - std::unique_ptr reader_; BufferSink bufsink_; XorSink xorsink_; @@ -178,18 +165,6 @@ class Worker { bool header_response_ = false; std::unique_ptr cowop_iter_; - size_t ra_block_index_ = 0; - uint64_t blocks_merged_in_group_ = 0; - bool merge_async_ = false; - // Queue depth of 8 seems optimal. We don't want - // to have a huge depth as it may put more memory pressure - // on the kernel worker threads given that we use - // IOSQE_ASYNC flag - ASYNC flags can potentially - // result in EINTR; Since we don't restart - // syscalls and fallback to synchronous I/O, we - // don't want huge queue depth - int queue_depth_ = 8; - std::unique_ptr ring_; std::shared_ptr snapuserd_; }; @@ -212,11 +187,7 @@ class SnapshotHandler : public std::enable_shared_from_this { bool CommitMerge(int num_merge_ops); void CloseFds() { cow_fd_ = {}; } - void FreeResources() { - worker_threads_.clear(); - read_ahead_thread_ = nullptr; - merge_thread_ = nullptr; - } + void FreeResources(); bool InitializeWorkers(); std::unique_ptr CloneReaderForWorker(); @@ -330,7 +301,7 @@ class SnapshotHandler : public std::enable_shared_from_this { // Merge Block state std::vector> merge_blk_state_; - std::unique_ptr merge_thread_; + std::unique_ptr merge_thread_; double merge_completion_percentage_; bool merge_initiated_ = false; diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_merge.cpp b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_merge.cpp index ce95b7659..ee7101129 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_merge.cpp +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_merge.cpp @@ -13,6 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +#include "snapuserd_merge.h" #include "snapuserd_core.h" @@ -23,8 +24,14 @@ using namespace android; using namespace android::dm; using android::base::unique_fd; -int Worker::PrepareMerge(uint64_t* source_offset, int* pending_ops, - std::vector* replace_zero_vec) { +MergeWorker::MergeWorker(const std::string& cow_device, const std::string& backing_device, + const std::string& control_device, const std::string& misc_name, + const std::string& base_path_merge, + std::shared_ptr snapuserd) + : Worker(cow_device, backing_device, control_device, misc_name, base_path_merge, snapuserd) {} + +int MergeWorker::PrepareMerge(uint64_t* source_offset, int* pending_ops, + std::vector* replace_zero_vec) { int num_ops = *pending_ops; int nr_consecutive = 0; bool checkOrderedOp = (replace_zero_vec == nullptr); @@ -70,7 +77,7 @@ int Worker::PrepareMerge(uint64_t* source_offset, int* pending_ops, return nr_consecutive; } -bool Worker::MergeReplaceZeroOps() { +bool MergeWorker::MergeReplaceZeroOps() { // Flush after merging 2MB. Since all ops are independent and there is no // dependency between COW ops, we will flush the data and the number // of ops merged in COW block device. If there is a crash, we will @@ -149,7 +156,7 @@ bool Worker::MergeReplaceZeroOps() { if (snapuserd_->IsIOTerminated()) { SNAP_LOG(ERROR) - << "MergeReplaceZeroOps: Worker threads terminated - shutting down merge"; + << "MergeReplaceZeroOps: MergeWorker threads terminated - shutting down merge"; return false; } } @@ -173,7 +180,7 @@ bool Worker::MergeReplaceZeroOps() { return true; } -bool Worker::MergeOrderedOpsAsync() { +bool MergeWorker::MergeOrderedOpsAsync() { void* mapped_addr = snapuserd_->GetMappedAddr(); void* read_ahead_buffer = static_cast((char*)mapped_addr + snapuserd_->GetBufferDataOffset()); @@ -354,7 +361,7 @@ bool Worker::MergeOrderedOpsAsync() { return true; } -bool Worker::MergeOrderedOps() { +bool MergeWorker::MergeOrderedOps() { void* mapped_addr = snapuserd_->GetMappedAddr(); void* read_ahead_buffer = static_cast((char*)mapped_addr + snapuserd_->GetBufferDataOffset()); @@ -439,7 +446,7 @@ bool Worker::MergeOrderedOps() { return true; } -bool Worker::AsyncMerge() { +bool MergeWorker::AsyncMerge() { if (!MergeOrderedOpsAsync()) { SNAP_LOG(ERROR) << "MergeOrderedOpsAsync failed - Falling back to synchronous I/O"; // Reset the iter so that we retry the merge @@ -455,7 +462,7 @@ bool Worker::AsyncMerge() { return true; } -bool Worker::SyncMerge() { +bool MergeWorker::SyncMerge() { if (!MergeOrderedOps()) { SNAP_LOG(ERROR) << "Merge failed for ordered ops"; return false; @@ -465,7 +472,7 @@ bool Worker::SyncMerge() { return true; } -bool Worker::Merge() { +bool MergeWorker::Merge() { cowop_iter_ = reader_->GetOpIter(true); bool retry = false; @@ -511,7 +518,7 @@ bool Worker::Merge() { return true; } -bool Worker::InitializeIouring() { +bool MergeWorker::InitializeIouring() { if (!snapuserd_->IsIouringSupported()) { return false; } @@ -530,13 +537,13 @@ bool Worker::InitializeIouring() { return true; } -void Worker::FinalizeIouring() { +void MergeWorker::FinalizeIouring() { if (merge_async_) { io_uring_queue_exit(ring_.get()); } } -bool Worker::RunMergeThread() { +bool MergeWorker::Run() { SNAP_LOG(DEBUG) << "Waiting for merge begin..."; if (!snapuserd_->WaitForMergeBegin()) { SNAP_LOG(ERROR) << "Merge terminated early..."; diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_merge.h b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_merge.h new file mode 100644 index 000000000..08192a0ef --- /dev/null +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_merge.h @@ -0,0 +1,56 @@ +// Copyright (C) 2023 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +#pragma once + +#include "snapuserd_core.h" + +namespace android { +namespace snapshot { + +class MergeWorker : public Worker { + public: + MergeWorker(const std::string& cow_device, const std::string& backing_device, + const std::string& control_device, const std::string& misc_name, + const std::string& base_path_merge, std::shared_ptr snapuserd); + bool Run(); + + private: + int PrepareMerge(uint64_t* source_offset, int* pending_ops, + std::vector* replace_zero_vec = nullptr); + bool MergeReplaceZeroOps(); + bool MergeOrderedOps(); + bool MergeOrderedOpsAsync(); + bool Merge(); + bool AsyncMerge(); + bool SyncMerge(); + bool InitializeIouring(); + void FinalizeIouring(); + + private: + std::unique_ptr ring_; + size_t ra_block_index_ = 0; + uint64_t blocks_merged_in_group_ = 0; + bool merge_async_ = false; + // Queue depth of 8 seems optimal. We don't want + // to have a huge depth as it may put more memory pressure + // on the kernel worker threads given that we use + // IOSQE_ASYNC flag - ASYNC flags can potentially + // result in EINTR; Since we don't restart + // syscalls and fallback to synchronous I/O, we + // don't want huge queue depth + int queue_depth_ = 8; +}; + +} // namespace snapshot +} // namespace android From d967591434f4e8467fce590286a7c18c1c06c64d Mon Sep 17 00:00:00 2001 From: David Anderson Date: Fri, 23 Jun 2023 12:39:19 -0700 Subject: [PATCH 2/5] snapuserd: Create a ReadWorker class. This splits the dm-user specific parts of Worker into a derived class. Bug: 288273605 Test: snapuserd_test Change-Id: Ic0ed1a8dff30018fa8466e7dc6e92469f1c87579 --- fs_mgr/libsnapshot/snapuserd/Android.bp | 2 +- .../user-space-merge/handler_manager.cpp | 1 + ...{snapuserd_dm_user.cpp => read_worker.cpp} | 41 +++++++++----- .../snapuserd/user-space-merge/read_worker.h | 53 +++++++++++++++++++ .../user-space-merge/snapuserd_core.cpp | 8 +-- .../user-space-merge/snapuserd_core.h | 27 ++-------- 6 files changed, 92 insertions(+), 40 deletions(-) rename fs_mgr/libsnapshot/snapuserd/user-space-merge/{snapuserd_dm_user.cpp => read_worker.cpp} (94%) create mode 100644 fs_mgr/libsnapshot/snapuserd/user-space-merge/read_worker.h diff --git a/fs_mgr/libsnapshot/snapuserd/Android.bp b/fs_mgr/libsnapshot/snapuserd/Android.bp index 9fe567acb..38ec23fc6 100644 --- a/fs_mgr/libsnapshot/snapuserd/Android.bp +++ b/fs_mgr/libsnapshot/snapuserd/Android.bp @@ -63,8 +63,8 @@ cc_library_static { "dm-snapshot-merge/snapuserd_readahead.cpp", "snapuserd_buffer.cpp", "user-space-merge/handler_manager.cpp", + "user-space-merge/read_worker.cpp", "user-space-merge/snapuserd_core.cpp", - "user-space-merge/snapuserd_dm_user.cpp", "user-space-merge/snapuserd_merge.cpp", "user-space-merge/snapuserd_readahead.cpp", "user-space-merge/snapuserd_transitions.cpp", diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/handler_manager.cpp b/fs_mgr/libsnapshot/snapuserd/user-space-merge/handler_manager.cpp index 734e84f3c..4105b4b48 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/handler_manager.cpp +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/handler_manager.cpp @@ -18,6 +18,7 @@ #include +#include "read_worker.h" #include "snapuserd_core.h" #include "snapuserd_merge.h" diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_dm_user.cpp b/fs_mgr/libsnapshot/snapuserd/user-space-merge/read_worker.cpp similarity index 94% rename from fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_dm_user.cpp rename to fs_mgr/libsnapshot/snapuserd/user-space-merge/read_worker.cpp index 2b9d14e9c..49a83608e 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_dm_user.cpp +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/read_worker.cpp @@ -14,6 +14,8 @@ * limitations under the License. */ +#include "read_worker.h" + #include "snapuserd_core.h" namespace android { @@ -34,6 +36,12 @@ Worker::Worker(const std::string& cow_device, const std::string& backing_device, snapuserd_ = snapuserd; } +ReadWorker::ReadWorker(const std::string& cow_device, const std::string& backing_device, + const std::string& control_device, const std::string& misc_name, + const std::string& base_path_merge, + std::shared_ptr snapuserd) + : Worker(cow_device, backing_device, control_device, misc_name, base_path_merge, snapuserd) {} + bool Worker::InitializeFds() { backing_store_fd_.reset(open(backing_store_device_.c_str(), O_RDONLY)); if (backing_store_fd_ < 0) { @@ -118,7 +126,7 @@ bool Worker::ReadFromSourceDevice(const CowOperation* cow_op) { // Start the copy operation. This will read the backing // block device which is represented by cow_op->source. -bool Worker::ProcessCopyOp(const CowOperation* cow_op) { +bool ReadWorker::ProcessCopyOp(const CowOperation* cow_op) { if (!ReadFromSourceDevice(cow_op)) { return false; } @@ -126,7 +134,7 @@ bool Worker::ProcessCopyOp(const CowOperation* cow_op) { return true; } -bool Worker::ProcessXorOp(const CowOperation* cow_op) { +bool ReadWorker::ProcessXorOp(const CowOperation* cow_op) { if (!ReadFromSourceDevice(cow_op)) { return false; } @@ -165,7 +173,7 @@ bool Worker::ProcessZeroOp() { return true; } -bool Worker::ProcessOrderedOp(const CowOperation* cow_op) { +bool ReadWorker::ProcessOrderedOp(const CowOperation* cow_op) { void* buffer = bufsink_.GetPayloadBuffer(BLOCK_SZ); if (buffer == nullptr) { SNAP_LOG(ERROR) << "ProcessOrderedOp: Failed to get payload buffer"; @@ -218,7 +226,7 @@ bool Worker::ProcessOrderedOp(const CowOperation* cow_op) { return false; } -bool Worker::ProcessCowOp(const CowOperation* cow_op) { +bool ReadWorker::ProcessCowOp(const CowOperation* cow_op) { if (cow_op == nullptr) { SNAP_LOG(ERROR) << "ProcessCowOp: Invalid cow_op"; return false; @@ -257,7 +265,6 @@ void Worker::InitializeBufsink() { bool Worker::Init() { InitializeBufsink(); - xorsink_.Initialize(&bufsink_, BLOCK_SZ); if (!InitializeFds()) { return false; @@ -270,7 +277,15 @@ bool Worker::Init() { return true; } -bool Worker::RunThread() { +bool ReadWorker::Init() { + if (!Worker::Init()) { + return false; + } + xorsink_.Initialize(&bufsink_, BLOCK_SZ); + return true; +} + +bool ReadWorker::Run() { SNAP_LOG(INFO) << "Processing snapshot I/O requests...."; if (setpriority(PRIO_PROCESS, gettid(), kNiceValueForMergeThreads)) { @@ -291,7 +306,7 @@ bool Worker::RunThread() { } // Send the payload/data back to dm-user misc device. -bool Worker::WriteDmUserPayload(size_t size) { +bool ReadWorker::WriteDmUserPayload(size_t size) { size_t payload_size = size; void* buf = bufsink_.GetPayloadBufPtr(); if (header_response_) { @@ -329,7 +344,7 @@ bool Worker::ReadDataFromBaseDevice(sector_t sector, size_t read_size) { return true; } -bool Worker::ReadAlignedSector(sector_t sector, size_t sz) { +bool ReadWorker::ReadAlignedSector(sector_t sector, size_t sz) { size_t remaining_size = sz; std::vector>& chunk_vec = snapuserd_->GetChunkVec(); int ret = 0; @@ -389,7 +404,7 @@ bool Worker::ReadAlignedSector(sector_t sector, size_t sz) { return true; } -int Worker::ReadUnalignedSector( +int ReadWorker::ReadUnalignedSector( sector_t sector, size_t size, std::vector>::iterator& it) { size_t skip_sector_size = 0; @@ -424,7 +439,7 @@ int Worker::ReadUnalignedSector( return std::min(size, (BLOCK_SZ - skip_sector_size)); } -bool Worker::ReadUnalignedSector(sector_t sector, size_t size) { +bool ReadWorker::ReadUnalignedSector(sector_t sector, size_t size) { bufsink_.ResetBufferOffset(); std::vector>& chunk_vec = snapuserd_->GetChunkVec(); @@ -563,7 +578,7 @@ bool Worker::ReadUnalignedSector(sector_t sector, size_t size) { return true; } -void Worker::RespondIOError() { +void ReadWorker::RespondIOError() { struct dm_user_header* header = bufsink_.GetHeaderPtr(); header->type = DM_USER_RESP_ERROR; // This is an issue with the dm-user interface. There @@ -580,7 +595,7 @@ void Worker::RespondIOError() { WriteDmUserPayload(0); } -bool Worker::DmuserReadRequest() { +bool ReadWorker::DmuserReadRequest() { struct dm_user_header* header = bufsink_.GetHeaderPtr(); // Unaligned I/O request @@ -591,7 +606,7 @@ bool Worker::DmuserReadRequest() { return ReadAlignedSector(header->sector, header->len); } -bool Worker::ProcessIORequest() { +bool ReadWorker::ProcessIORequest() { // Read Header from dm-user misc device. This gives // us the sector number for which IO is issued by dm-snapshot device struct dm_user_header* header = bufsink_.GetHeaderPtr(); diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/read_worker.h b/fs_mgr/libsnapshot/snapuserd/user-space-merge/read_worker.h new file mode 100644 index 000000000..262f8adfe --- /dev/null +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/read_worker.h @@ -0,0 +1,53 @@ +// Copyright (C) 2023 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#pragma once + +#include "snapuserd_core.h" + +namespace android { +namespace snapshot { + +class ReadWorker : public Worker { + public: + ReadWorker(const std::string& cow_device, const std::string& backing_device, + const std::string& control_device, const std::string& misc_name, + const std::string& base_path_merge, std::shared_ptr snapuserd); + + bool Run(); + bool Init() override; + + private: + // Functions interacting with dm-user + bool ProcessIORequest(); + bool WriteDmUserPayload(size_t size); + bool DmuserReadRequest(); + void RespondIOError(); + + bool ProcessCowOp(const CowOperation* cow_op); + bool ProcessXorOp(const CowOperation* cow_op); + bool ProcessOrderedOp(const CowOperation* cow_op); + bool ProcessCopyOp(const CowOperation* cow_op); + + bool ReadAlignedSector(sector_t sector, size_t sz); + bool ReadUnalignedSector(sector_t sector, size_t size); + int ReadUnalignedSector(sector_t sector, size_t size, + std::vector>::iterator& it); + + XorSink xorsink_; + bool header_response_ = false; +}; + +} // namespace snapshot +} // namespace android diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.cpp b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.cpp index 4f7495c0a..baf06b393 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.cpp +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.cpp @@ -23,6 +23,7 @@ #include #include +#include "read_worker.h" #include "snapuserd_merge.h" namespace android { @@ -48,9 +49,8 @@ SnapshotHandler::SnapshotHandler(std::string misc_name, std::string cow_device, bool SnapshotHandler::InitializeWorkers() { for (int i = 0; i < num_worker_threads_; i++) { - std::unique_ptr wt = - std::make_unique(cow_device_, backing_store_device_, control_device_, - misc_name_, base_path_merge_, GetSharedPtr()); + auto wt = std::make_unique(cow_device_, backing_store_device_, control_device_, + misc_name_, base_path_merge_, GetSharedPtr()); if (!wt->Init()) { SNAP_LOG(ERROR) << "Thread initialization failed"; return false; @@ -315,7 +315,7 @@ bool SnapshotHandler::Start() { // Launch worker threads for (int i = 0; i < worker_threads_.size(); i++) { threads.emplace_back( - std::async(std::launch::async, &Worker::RunThread, worker_threads_[i].get())); + std::async(std::launch::async, &ReadWorker::Run, worker_threads_[i].get())); } std::future merge_thread = diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.h b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.h index fe10edfee..0c30eac75 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.h +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.h @@ -76,6 +76,7 @@ enum class MERGE_IO_TRANSITION { }; class MergeWorker; +class ReadWorker; class SnapshotHandler; enum class MERGE_GROUP_STATE { @@ -104,8 +105,9 @@ class Worker { Worker(const std::string& cow_device, const std::string& backing_device, const std::string& control_device, const std::string& misc_name, const std::string& base_path_merge, std::shared_ptr snapuserd); - bool RunThread(); - bool Init(); + virtual ~Worker() = default; + + virtual bool Init(); protected: // Initialization @@ -118,39 +120,21 @@ class Worker { base_path_merge_fd_ = {}; } - // Functions interacting with dm-user - bool WriteDmUserPayload(size_t size); - bool DmuserReadRequest(); - // IO Path - bool ProcessIORequest(); bool IsBlockAligned(size_t size) { return ((size & (BLOCK_SZ - 1)) == 0); } bool ReadDataFromBaseDevice(sector_t sector, size_t read_size); bool ReadFromSourceDevice(const CowOperation* cow_op); - bool ReadAlignedSector(sector_t sector, size_t sz); - bool ReadUnalignedSector(sector_t sector, size_t size); - int ReadUnalignedSector(sector_t sector, size_t size, - std::vector>::iterator& it); - void RespondIOError(); - // Processing COW operations - bool ProcessCowOp(const CowOperation* cow_op); bool ProcessReplaceOp(const CowOperation* cow_op); bool ProcessZeroOp(); - // Handles Copy and Xor - bool ProcessCopyOp(const CowOperation* cow_op); - bool ProcessXorOp(const CowOperation* cow_op); - bool ProcessOrderedOp(const CowOperation* cow_op); - sector_t ChunkToSector(chunk_t chunk) { return chunk << CHUNK_SHIFT; } chunk_t SectorToChunk(sector_t sector) { return sector >> CHUNK_SHIFT; } std::unique_ptr reader_; BufferSink bufsink_; - XorSink xorsink_; std::string cow_device_; std::string backing_store_device_; @@ -162,7 +146,6 @@ class Worker { unique_fd backing_store_fd_; unique_fd base_path_merge_fd_; unique_fd ctrl_fd_; - bool header_response_ = false; std::unique_ptr cowop_iter_; @@ -286,7 +269,7 @@ class SnapshotHandler : public std::enable_shared_from_this { void* mapped_addr_; size_t total_mapped_addr_length_; - std::vector> worker_threads_; + std::vector> worker_threads_; // Read-ahead related bool populate_data_from_cow_ = false; bool ra_thread_ = false; From ab57c8a5774a5ba59e6144e06ecb33d9df690ea3 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Fri, 23 Jun 2023 13:00:34 -0700 Subject: [PATCH 3/5] snapuserd: Split more methods out of Worker. This moves ReadWorker-specific methods out of Worker, and moves remaining Worker methods into a separate worker.cpp file. Bug: 288273605 Test: snapuserd_test Change-Id: I59c31318e127db61a5f3a673956865dac97a6e5f --- fs_mgr/libsnapshot/snapuserd/Android.bp | 1 + .../user-space-merge/read_worker.cpp | 76 +-------------- .../snapuserd/user-space-merge/read_worker.h | 7 +- .../user-space-merge/snapuserd_core.h | 53 ----------- .../user-space-merge/snapuserd_merge.h | 4 +- .../snapuserd/user-space-merge/worker.cpp | 95 +++++++++++++++++++ .../snapuserd/user-space-merge/worker.h | 86 +++++++++++++++++ 7 files changed, 193 insertions(+), 129 deletions(-) create mode 100644 fs_mgr/libsnapshot/snapuserd/user-space-merge/worker.cpp create mode 100644 fs_mgr/libsnapshot/snapuserd/user-space-merge/worker.h diff --git a/fs_mgr/libsnapshot/snapuserd/Android.bp b/fs_mgr/libsnapshot/snapuserd/Android.bp index 38ec23fc6..78a3b9bf2 100644 --- a/fs_mgr/libsnapshot/snapuserd/Android.bp +++ b/fs_mgr/libsnapshot/snapuserd/Android.bp @@ -69,6 +69,7 @@ cc_library_static { "user-space-merge/snapuserd_readahead.cpp", "user-space-merge/snapuserd_transitions.cpp", "user-space-merge/snapuserd_verify.cpp", + "user-space-merge/worker.cpp", ], static_libs: [ "libbase", diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/read_worker.cpp b/fs_mgr/libsnapshot/snapuserd/user-space-merge/read_worker.cpp index 49a83608e..c47abe3fd 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/read_worker.cpp +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/read_worker.cpp @@ -25,61 +25,12 @@ using namespace android; using namespace android::dm; using android::base::unique_fd; -Worker::Worker(const std::string& cow_device, const std::string& backing_device, - const std::string& control_device, const std::string& misc_name, - const std::string& base_path_merge, std::shared_ptr snapuserd) { - cow_device_ = cow_device; - backing_store_device_ = backing_device; - control_device_ = control_device; - misc_name_ = misc_name; - base_path_merge_ = base_path_merge; - snapuserd_ = snapuserd; -} - ReadWorker::ReadWorker(const std::string& cow_device, const std::string& backing_device, const std::string& control_device, const std::string& misc_name, const std::string& base_path_merge, std::shared_ptr snapuserd) : Worker(cow_device, backing_device, control_device, misc_name, base_path_merge, snapuserd) {} -bool Worker::InitializeFds() { - backing_store_fd_.reset(open(backing_store_device_.c_str(), O_RDONLY)); - if (backing_store_fd_ < 0) { - SNAP_PLOG(ERROR) << "Open Failed: " << backing_store_device_; - return false; - } - - cow_fd_.reset(open(cow_device_.c_str(), O_RDWR)); - if (cow_fd_ < 0) { - SNAP_PLOG(ERROR) << "Open Failed: " << cow_device_; - return false; - } - - ctrl_fd_.reset(open(control_device_.c_str(), O_RDWR)); - if (ctrl_fd_ < 0) { - SNAP_PLOG(ERROR) << "Unable to open " << control_device_; - return false; - } - - // Base device used by merge thread - base_path_merge_fd_.reset(open(base_path_merge_.c_str(), O_RDWR)); - if (base_path_merge_fd_ < 0) { - SNAP_PLOG(ERROR) << "Open Failed: " << base_path_merge_; - return false; - } - - return true; -} - -bool Worker::InitReader() { - reader_ = snapuserd_->CloneReaderForWorker(); - - if (!reader_->InitForMerge(std::move(cow_fd_))) { - return false; - } - return true; -} - // Start the replace operation. This will read the // internal COW format and if the block is compressed, // it will be de-compressed. @@ -96,7 +47,7 @@ bool Worker::ProcessReplaceOp(const CowOperation* cow_op) { return true; } -bool Worker::ReadFromSourceDevice(const CowOperation* cow_op) { +bool ReadWorker::ReadFromSourceDevice(const CowOperation* cow_op) { void* buffer = bufsink_.GetPayloadBuffer(BLOCK_SZ); if (buffer == nullptr) { SNAP_LOG(ERROR) << "ReadFromBaseDevice: Failed to get payload buffer"; @@ -254,29 +205,6 @@ bool ReadWorker::ProcessCowOp(const CowOperation* cow_op) { return false; } -void Worker::InitializeBufsink() { - // Allocate the buffer which is used to communicate between - // daemon and dm-user. The buffer comprises of header and a fixed payload. - // If the dm-user requests a big IO, the IO will be broken into chunks - // of PAYLOAD_BUFFER_SZ. - size_t buf_size = sizeof(struct dm_user_header) + PAYLOAD_BUFFER_SZ; - bufsink_.Initialize(buf_size); -} - -bool Worker::Init() { - InitializeBufsink(); - - if (!InitializeFds()) { - return false; - } - - if (!InitReader()) { - return false; - } - - return true; -} - bool ReadWorker::Init() { if (!Worker::Init()) { return false; @@ -325,7 +253,7 @@ bool ReadWorker::WriteDmUserPayload(size_t size) { return true; } -bool Worker::ReadDataFromBaseDevice(sector_t sector, size_t read_size) { +bool ReadWorker::ReadDataFromBaseDevice(sector_t sector, size_t read_size) { CHECK(read_size <= BLOCK_SZ); void* buffer = bufsink_.GetPayloadBuffer(BLOCK_SZ); diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/read_worker.h b/fs_mgr/libsnapshot/snapuserd/user-space-merge/read_worker.h index 262f8adfe..b12dab24c 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/read_worker.h +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/read_worker.h @@ -14,7 +14,10 @@ #pragma once -#include "snapuserd_core.h" +#include +#include + +#include "worker.h" namespace android { namespace snapshot { @@ -44,6 +47,8 @@ class ReadWorker : public Worker { bool ReadUnalignedSector(sector_t sector, size_t size); int ReadUnalignedSector(sector_t sector, size_t size, std::vector>::iterator& it); + bool ReadFromSourceDevice(const CowOperation* cow_op); + bool ReadDataFromBaseDevice(sector_t sector, size_t read_size); XorSink xorsink_; bool header_response_ = false; diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.h b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.h index 0c30eac75..cdc38c016 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.h +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.h @@ -77,7 +77,6 @@ enum class MERGE_IO_TRANSITION { class MergeWorker; class ReadWorker; -class SnapshotHandler; enum class MERGE_GROUP_STATE { GROUP_MERGE_PENDING, @@ -100,58 +99,6 @@ struct MergeGroupState { : merge_state_(state), num_ios_in_progress(n_ios) {} }; -class Worker { - public: - Worker(const std::string& cow_device, const std::string& backing_device, - const std::string& control_device, const std::string& misc_name, - const std::string& base_path_merge, std::shared_ptr snapuserd); - virtual ~Worker() = default; - - virtual bool Init(); - - protected: - // Initialization - void InitializeBufsink(); - bool InitializeFds(); - bool InitReader(); - void CloseFds() { - ctrl_fd_ = {}; - backing_store_fd_ = {}; - base_path_merge_fd_ = {}; - } - - // IO Path - bool IsBlockAligned(size_t size) { return ((size & (BLOCK_SZ - 1)) == 0); } - - bool ReadDataFromBaseDevice(sector_t sector, size_t read_size); - bool ReadFromSourceDevice(const CowOperation* cow_op); - - // Processing COW operations - bool ProcessReplaceOp(const CowOperation* cow_op); - bool ProcessZeroOp(); - - sector_t ChunkToSector(chunk_t chunk) { return chunk << CHUNK_SHIFT; } - chunk_t SectorToChunk(sector_t sector) { return sector >> CHUNK_SHIFT; } - - std::unique_ptr reader_; - BufferSink bufsink_; - - std::string cow_device_; - std::string backing_store_device_; - std::string control_device_; - std::string misc_name_; - std::string base_path_merge_; - - unique_fd cow_fd_; - unique_fd backing_store_fd_; - unique_fd base_path_merge_fd_; - unique_fd ctrl_fd_; - - std::unique_ptr cowop_iter_; - - std::shared_ptr snapuserd_; -}; - class SnapshotHandler : public std::enable_shared_from_this { public: SnapshotHandler(std::string misc_name, std::string cow_device, std::string backing_device, diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_merge.h b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_merge.h index 08192a0ef..3da73c24d 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_merge.h +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_merge.h @@ -13,7 +13,9 @@ // limitations under the License. #pragma once -#include "snapuserd_core.h" +#include "worker.h" + +#include namespace android { namespace snapshot { diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/worker.cpp b/fs_mgr/libsnapshot/snapuserd/user-space-merge/worker.cpp new file mode 100644 index 000000000..ef6781a61 --- /dev/null +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/worker.cpp @@ -0,0 +1,95 @@ +// Copyright (C) 2023 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "worker.h" + +#include "snapuserd_core.h" + +namespace android { +namespace snapshot { + +Worker::Worker(const std::string& cow_device, const std::string& backing_device, + const std::string& control_device, const std::string& misc_name, + const std::string& base_path_merge, std::shared_ptr snapuserd) { + cow_device_ = cow_device; + backing_store_device_ = backing_device; + control_device_ = control_device; + misc_name_ = misc_name; + base_path_merge_ = base_path_merge; + snapuserd_ = snapuserd; +} + +void Worker::InitializeBufsink() { + // Allocate the buffer which is used to communicate between + // daemon and dm-user. The buffer comprises of header and a fixed payload. + // If the dm-user requests a big IO, the IO will be broken into chunks + // of PAYLOAD_BUFFER_SZ. + size_t buf_size = sizeof(struct dm_user_header) + PAYLOAD_BUFFER_SZ; + bufsink_.Initialize(buf_size); +} + +bool Worker::Init() { + InitializeBufsink(); + + if (!InitializeFds()) { + return false; + } + + if (!InitReader()) { + return false; + } + + return true; +} + +bool Worker::InitReader() { + reader_ = snapuserd_->CloneReaderForWorker(); + + if (!reader_->InitForMerge(std::move(cow_fd_))) { + return false; + } + return true; +} + +bool Worker::InitializeFds() { + backing_store_fd_.reset(open(backing_store_device_.c_str(), O_RDONLY)); + if (backing_store_fd_ < 0) { + SNAP_PLOG(ERROR) << "Open Failed: " << backing_store_device_; + return false; + } + + cow_fd_.reset(open(cow_device_.c_str(), O_RDWR)); + if (cow_fd_ < 0) { + SNAP_PLOG(ERROR) << "Open Failed: " << cow_device_; + return false; + } + + ctrl_fd_.reset(open(control_device_.c_str(), O_RDWR)); + if (ctrl_fd_ < 0) { + SNAP_PLOG(ERROR) << "Unable to open " << control_device_; + return false; + } + + // Base device used by merge thread + base_path_merge_fd_.reset(open(base_path_merge_.c_str(), O_RDWR)); + if (base_path_merge_fd_ < 0) { + SNAP_PLOG(ERROR) << "Open Failed: " << base_path_merge_; + return false; + } + + return true; +} + +} // namespace snapshot +} // namespace android diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/worker.h b/fs_mgr/libsnapshot/snapuserd/user-space-merge/worker.h new file mode 100644 index 000000000..d38ce5d18 --- /dev/null +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/worker.h @@ -0,0 +1,86 @@ +// Copyright (C) 2023 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#pragma once + +#include + +#include +#include + +#include +#include +#include +#include + +namespace android { +namespace snapshot { + +using android::base::unique_fd; + +class SnapshotHandler; + +class Worker { + public: + Worker(const std::string& cow_device, const std::string& backing_device, + const std::string& control_device, const std::string& misc_name, + const std::string& base_path_merge, std::shared_ptr snapuserd); + virtual ~Worker() = default; + + virtual bool Init(); + + protected: + // Initialization + void InitializeBufsink(); + bool InitializeFds(); + bool InitReader(); + void CloseFds() { + ctrl_fd_ = {}; + backing_store_fd_ = {}; + base_path_merge_fd_ = {}; + } + + // IO Path + bool IsBlockAligned(size_t size) { return ((size & (BLOCK_SZ - 1)) == 0); } + + bool ReadDataFromBaseDevice(sector_t sector, size_t read_size); + + // Processing COW operations + bool ProcessReplaceOp(const CowOperation* cow_op); + bool ProcessZeroOp(); + + sector_t ChunkToSector(chunk_t chunk) { return chunk << CHUNK_SHIFT; } + chunk_t SectorToChunk(sector_t sector) { return sector >> CHUNK_SHIFT; } + + std::unique_ptr reader_; + BufferSink bufsink_; + + std::string cow_device_; + std::string backing_store_device_; + std::string control_device_; + std::string misc_name_; + std::string base_path_merge_; + + unique_fd cow_fd_; + unique_fd backing_store_fd_; + unique_fd base_path_merge_fd_; + unique_fd ctrl_fd_; + + std::unique_ptr cowop_iter_; + + std::shared_ptr snapuserd_; +}; + +} // namespace snapshot +} // namespace android From c2d5a19d269bfcde34f4e4ea5db9733f437305f6 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Fri, 23 Jun 2023 13:30:00 -0700 Subject: [PATCH 4/5] snapuserd: Move more fields out of Worker. These fields are specific to either ReadWorker or MergeWorker, but not both. Bug: 288273605 Test: snapuserd_test Change-Id: I2db9cfa2a8f034249879517bd90a40babe97bc64 --- .../user-space-merge/read_worker.cpp | 23 +++++++++++++- .../snapuserd/user-space-merge/read_worker.h | 10 ++++++ .../user-space-merge/snapuserd_core.cpp | 5 ++- .../user-space-merge/snapuserd_merge.cpp | 5 ++- .../user-space-merge/snapuserd_merge.h | 4 +-- .../snapuserd/user-space-merge/worker.cpp | 17 +--------- .../snapuserd/user-space-merge/worker.h | 31 +++++-------------- 7 files changed, 47 insertions(+), 48 deletions(-) diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/read_worker.cpp b/fs_mgr/libsnapshot/snapuserd/user-space-merge/read_worker.cpp index c47abe3fd..4c8223f1e 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/read_worker.cpp +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/read_worker.cpp @@ -25,11 +25,19 @@ using namespace android; using namespace android::dm; using android::base::unique_fd; +void ReadWorker::CloseFds() { + ctrl_fd_ = {}; + backing_store_fd_ = {}; + Worker::CloseFds(); +} + ReadWorker::ReadWorker(const std::string& cow_device, const std::string& backing_device, const std::string& control_device, const std::string& misc_name, const std::string& base_path_merge, std::shared_ptr snapuserd) - : Worker(cow_device, backing_device, control_device, misc_name, base_path_merge, snapuserd) {} + : Worker(cow_device, misc_name, base_path_merge, snapuserd), + backing_store_device_(backing_device), + control_device_(control_device) {} // Start the replace operation. This will read the // internal COW format and if the block is compressed, @@ -209,6 +217,19 @@ bool ReadWorker::Init() { if (!Worker::Init()) { return false; } + + backing_store_fd_.reset(open(backing_store_device_.c_str(), O_RDONLY)); + if (backing_store_fd_ < 0) { + SNAP_PLOG(ERROR) << "Open Failed: " << backing_store_device_; + return false; + } + + ctrl_fd_.reset(open(control_device_.c_str(), O_RDWR)); + if (ctrl_fd_ < 0) { + SNAP_PLOG(ERROR) << "Unable to open " << control_device_; + return false; + } + xorsink_.Initialize(&bufsink_, BLOCK_SZ); return true; } diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/read_worker.h b/fs_mgr/libsnapshot/snapuserd/user-space-merge/read_worker.h index b12dab24c..bc0474f4b 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/read_worker.h +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/read_worker.h @@ -30,6 +30,7 @@ class ReadWorker : public Worker { bool Run(); bool Init() override; + void CloseFds() override; private: // Functions interacting with dm-user @@ -50,6 +51,15 @@ class ReadWorker : public Worker { bool ReadFromSourceDevice(const CowOperation* cow_op); bool ReadDataFromBaseDevice(sector_t sector, size_t read_size); + constexpr bool IsBlockAligned(size_t size) { return ((size & (BLOCK_SZ - 1)) == 0); } + constexpr sector_t ChunkToSector(chunk_t chunk) { return chunk << CHUNK_SHIFT; } + + std::string backing_store_device_; + unique_fd backing_store_fd_; + + std::string control_device_; + unique_fd ctrl_fd_; + XorSink xorsink_; bool header_response_ = false; }; diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.cpp b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.cpp index baf06b393..e52d752a0 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.cpp +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.cpp @@ -59,9 +59,8 @@ bool SnapshotHandler::InitializeWorkers() { worker_threads_.push_back(std::move(wt)); } - merge_thread_ = - std::make_unique(cow_device_, backing_store_device_, control_device_, - misc_name_, base_path_merge_, GetSharedPtr()); + merge_thread_ = std::make_unique(cow_device_, misc_name_, base_path_merge_, + GetSharedPtr()); read_ahead_thread_ = std::make_unique(cow_device_, backing_store_device_, misc_name_, GetSharedPtr()); diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_merge.cpp b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_merge.cpp index ee7101129..510e27d29 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_merge.cpp +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_merge.cpp @@ -24,11 +24,10 @@ using namespace android; using namespace android::dm; using android::base::unique_fd; -MergeWorker::MergeWorker(const std::string& cow_device, const std::string& backing_device, - const std::string& control_device, const std::string& misc_name, +MergeWorker::MergeWorker(const std::string& cow_device, const std::string& misc_name, const std::string& base_path_merge, std::shared_ptr snapuserd) - : Worker(cow_device, backing_device, control_device, misc_name, base_path_merge, snapuserd) {} + : Worker(cow_device, misc_name, base_path_merge, snapuserd) {} int MergeWorker::PrepareMerge(uint64_t* source_offset, int* pending_ops, std::vector* replace_zero_vec) { diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_merge.h b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_merge.h index 3da73c24d..f35147f2b 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_merge.h +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_merge.h @@ -22,8 +22,7 @@ namespace snapshot { class MergeWorker : public Worker { public: - MergeWorker(const std::string& cow_device, const std::string& backing_device, - const std::string& control_device, const std::string& misc_name, + MergeWorker(const std::string& cow_device, const std::string& misc_name, const std::string& base_path_merge, std::shared_ptr snapuserd); bool Run(); @@ -40,6 +39,7 @@ class MergeWorker : public Worker { void FinalizeIouring(); private: + std::unique_ptr cowop_iter_; std::unique_ptr ring_; size_t ra_block_index_ = 0; uint64_t blocks_merged_in_group_ = 0; diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/worker.cpp b/fs_mgr/libsnapshot/snapuserd/user-space-merge/worker.cpp index ef6781a61..aa156301f 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/worker.cpp +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/worker.cpp @@ -19,12 +19,9 @@ namespace android { namespace snapshot { -Worker::Worker(const std::string& cow_device, const std::string& backing_device, - const std::string& control_device, const std::string& misc_name, +Worker::Worker(const std::string& cow_device, const std::string& misc_name, const std::string& base_path_merge, std::shared_ptr snapuserd) { cow_device_ = cow_device; - backing_store_device_ = backing_device; - control_device_ = control_device; misc_name_ = misc_name; base_path_merge_ = base_path_merge; snapuserd_ = snapuserd; @@ -63,24 +60,12 @@ bool Worker::InitReader() { } bool Worker::InitializeFds() { - backing_store_fd_.reset(open(backing_store_device_.c_str(), O_RDONLY)); - if (backing_store_fd_ < 0) { - SNAP_PLOG(ERROR) << "Open Failed: " << backing_store_device_; - return false; - } - cow_fd_.reset(open(cow_device_.c_str(), O_RDWR)); if (cow_fd_ < 0) { SNAP_PLOG(ERROR) << "Open Failed: " << cow_device_; return false; } - ctrl_fd_.reset(open(control_device_.c_str(), O_RDWR)); - if (ctrl_fd_ < 0) { - SNAP_PLOG(ERROR) << "Unable to open " << control_device_; - return false; - } - // Base device used by merge thread base_path_merge_fd_.reset(open(base_path_merge_.c_str(), O_RDWR)); if (base_path_merge_fd_ < 0) { diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/worker.h b/fs_mgr/libsnapshot/snapuserd/user-space-merge/worker.h index d38ce5d18..1175ab714 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/worker.h +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/worker.h @@ -33,8 +33,7 @@ class SnapshotHandler; class Worker { public: - Worker(const std::string& cow_device, const std::string& backing_device, - const std::string& control_device, const std::string& misc_name, + Worker(const std::string& cow_device, const std::string& misc_name, const std::string& base_path_merge, std::shared_ptr snapuserd); virtual ~Worker() = default; @@ -45,14 +44,7 @@ class Worker { void InitializeBufsink(); bool InitializeFds(); bool InitReader(); - void CloseFds() { - ctrl_fd_ = {}; - backing_store_fd_ = {}; - base_path_merge_fd_ = {}; - } - - // IO Path - bool IsBlockAligned(size_t size) { return ((size & (BLOCK_SZ - 1)) == 0); } + virtual void CloseFds() { base_path_merge_fd_ = {}; } bool ReadDataFromBaseDevice(sector_t sector, size_t read_size); @@ -60,26 +52,19 @@ class Worker { bool ProcessReplaceOp(const CowOperation* cow_op); bool ProcessZeroOp(); - sector_t ChunkToSector(chunk_t chunk) { return chunk << CHUNK_SHIFT; } - chunk_t SectorToChunk(sector_t sector) { return sector >> CHUNK_SHIFT; } - std::unique_ptr reader_; BufferSink bufsink_; - std::string cow_device_; - std::string backing_store_device_; - std::string control_device_; - std::string misc_name_; - std::string base_path_merge_; + std::string misc_name_; // Needed for SNAP_LOG. - unique_fd cow_fd_; - unique_fd backing_store_fd_; unique_fd base_path_merge_fd_; - unique_fd ctrl_fd_; - - std::unique_ptr cowop_iter_; std::shared_ptr snapuserd_; + + private: + std::string cow_device_; + std::string base_path_merge_; + unique_fd cow_fd_; }; } // namespace snapshot From 8bc625e9cae82c798e0d2ef8233902663a638744 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Fri, 23 Jun 2023 13:52:44 -0700 Subject: [PATCH 5/5] snapuserd: Move Process ops out of Worker. These are so small they can be inlined into MergeWorker. Sharing these methods will be difficult after decoupling from dm-user, since acquisition of buffers will change. Bug: 288273605 Test: snapuserd_test Change-Id: I1625d1a6e55bcb2041f73453ca15a01f98263e8a --- .../snapuserd/user-space-merge/read_worker.cpp | 4 ++-- .../snapuserd/user-space-merge/read_worker.h | 2 ++ .../user-space-merge/snapuserd_merge.cpp | 15 +++++++++------ .../snapuserd/user-space-merge/worker.h | 6 ------ 4 files changed, 13 insertions(+), 14 deletions(-) diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/read_worker.cpp b/fs_mgr/libsnapshot/snapuserd/user-space-merge/read_worker.cpp index 4c8223f1e..dd2996b78 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/read_worker.cpp +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/read_worker.cpp @@ -42,7 +42,7 @@ ReadWorker::ReadWorker(const std::string& cow_device, const std::string& backing // Start the replace operation. This will read the // internal COW format and if the block is compressed, // it will be de-compressed. -bool Worker::ProcessReplaceOp(const CowOperation* cow_op) { +bool ReadWorker::ProcessReplaceOp(const CowOperation* cow_op) { void* buffer = bufsink_.GetPayloadBuffer(BLOCK_SZ); if (!buffer) { SNAP_LOG(ERROR) << "ProcessReplaceOp failed to allocate buffer"; @@ -120,7 +120,7 @@ bool ReadWorker::ProcessXorOp(const CowOperation* cow_op) { return true; } -bool Worker::ProcessZeroOp() { +bool ReadWorker::ProcessZeroOp() { // Zero out the entire block void* buffer = bufsink_.GetPayloadBuffer(BLOCK_SZ); if (buffer == nullptr) { diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/read_worker.h b/fs_mgr/libsnapshot/snapuserd/user-space-merge/read_worker.h index bc0474f4b..c3a4c346e 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/read_worker.h +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/read_worker.h @@ -43,6 +43,8 @@ class ReadWorker : public Worker { bool ProcessXorOp(const CowOperation* cow_op); bool ProcessOrderedOp(const CowOperation* cow_op); bool ProcessCopyOp(const CowOperation* cow_op); + bool ProcessReplaceOp(const CowOperation* cow_op); + bool ProcessZeroOp(); bool ReadAlignedSector(sector_t sector, size_t sz); bool ReadUnalignedSector(sector_t sector, size_t size); diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_merge.cpp b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_merge.cpp index 510e27d29..563f6ad42 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_merge.cpp +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_merge.cpp @@ -105,17 +105,20 @@ bool MergeWorker::MergeReplaceZeroOps() { for (size_t i = 0; i < replace_zero_vec.size(); i++) { const CowOperation* cow_op = replace_zero_vec[i]; + + void* buffer = bufsink_.GetPayloadBuffer(BLOCK_SZ); + if (!buffer) { + SNAP_LOG(ERROR) << "Failed to acquire buffer in merge"; + return false; + } if (cow_op->type == kCowReplaceOp) { - if (!ProcessReplaceOp(cow_op)) { - SNAP_LOG(ERROR) << "Merge - ReplaceOp failed for block: " << cow_op->new_block; + if (!reader_->ReadData(cow_op, buffer, BLOCK_SZ)) { + SNAP_LOG(ERROR) << "Failed to read COW in merge"; return false; } } else { CHECK(cow_op->type == kCowZeroOp); - if (!ProcessZeroOp()) { - SNAP_LOG(ERROR) << "Merge ZeroOp failed."; - return false; - } + memset(buffer, 0, BLOCK_SZ); } bufsink_.UpdateBufferOffset(BLOCK_SZ); diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/worker.h b/fs_mgr/libsnapshot/snapuserd/user-space-merge/worker.h index 1175ab714..813b159ea 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/worker.h +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/worker.h @@ -46,12 +46,6 @@ class Worker { bool InitReader(); virtual void CloseFds() { base_path_merge_fd_ = {}; } - bool ReadDataFromBaseDevice(sector_t sector, size_t read_size); - - // Processing COW operations - bool ProcessReplaceOp(const CowOperation* cow_op); - bool ProcessZeroOp(); - std::unique_ptr reader_; BufferSink bufsink_;