diff --git a/fs_mgr/libsnapshot/cow_reader.cpp b/fs_mgr/libsnapshot/cow_reader.cpp index b3198a092..af49c7d3e 100644 --- a/fs_mgr/libsnapshot/cow_reader.cpp +++ b/fs_mgr/libsnapshot/cow_reader.cpp @@ -381,10 +381,10 @@ bool CowReader::PrepMergeOps() { std::set> other_ops; auto seq_ops_set = std::unordered_set(); auto block_map = std::make_shared>(); - int num_seqs = 0; + size_t num_seqs = 0; size_t read; - for (int i = 0; i < ops_->size(); i++) { + for (size_t i = 0; i < ops_->size(); i++) { auto& current_op = ops_->data()[i]; if (current_op.type == kCowSequenceOp) { @@ -396,7 +396,7 @@ bool CowReader::PrepMergeOps() { PLOG(ERROR) << "Failed to read sequence op!"; return false; } - for (int j = num_seqs; j < num_seqs + seq_len; j++) { + for (size_t j = num_seqs; j < num_seqs + seq_len; j++) { seq_ops_set.insert(merge_op_blocks->data()[j]); } num_seqs += seq_len; @@ -413,10 +413,11 @@ bool CowReader::PrepMergeOps() { } block_map->insert({current_op.new_block, i}); } - if (merge_op_blocks->size() > header_.num_merge_ops) + if (merge_op_blocks->size() > header_.num_merge_ops) { num_ordered_ops_to_merge_ = merge_op_blocks->size() - header_.num_merge_ops; - else + } else { num_ordered_ops_to_merge_ = 0; + } merge_op_blocks->reserve(merge_op_blocks->size() + other_ops.size()); for (auto block : other_ops) { merge_op_blocks->emplace_back(block); diff --git a/fs_mgr/libsnapshot/include/libsnapshot/cow_reader.h b/fs_mgr/libsnapshot/include/libsnapshot/cow_reader.h index 05f70662c..6c3059c81 100644 --- a/fs_mgr/libsnapshot/include/libsnapshot/cow_reader.h +++ b/fs_mgr/libsnapshot/include/libsnapshot/cow_reader.h @@ -126,6 +126,10 @@ class CowReader : public ICowReader { bool GetRawBytes(uint64_t offset, void* buffer, size_t len, size_t* read); + // Returns the total number of data ops that should be merged. This is the + // count of the merge sequence before removing already-merged operations. + // It may be different than the actual data op count, for example, if there + // are duplicate ops in the stream. uint64_t get_num_total_data_ops() { return num_total_data_ops_; } uint64_t get_num_ordered_ops_to_merge() { return num_ordered_ops_to_merge_; } diff --git a/fs_mgr/libsnapshot/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp index 0e36da151..bed5f563d 100644 --- a/fs_mgr/libsnapshot/snapshot.cpp +++ b/fs_mgr/libsnapshot/snapshot.cpp @@ -1193,11 +1193,7 @@ MergeFailureCode SnapshotManager::CheckMergeConsistency(LockedFile* lock, const return MergeFailureCode::ParseCowConsistencyCheck; } - for (auto iter = reader.GetOpIter(); !iter->Done(); iter->Next()) { - if (!IsMetadataOp(iter->Get())) { - num_ops++; - } - } + num_ops = reader.get_num_total_data_ops(); } // Second pass, try as hard as we can to get the actual number of blocks diff --git a/fs_mgr/libsnapshot/snapshot_test.cpp b/fs_mgr/libsnapshot/snapshot_test.cpp index 60186434a..b2203fe63 100644 --- a/fs_mgr/libsnapshot/snapshot_test.cpp +++ b/fs_mgr/libsnapshot/snapshot_test.cpp @@ -1184,6 +1184,53 @@ TEST_F(SnapshotUpdateTest, FullUpdateFlow) { } } +TEST_F(SnapshotUpdateTest, DuplicateOps) { + if (!IsCompressionEnabled()) { + GTEST_SKIP() << "Compression-only test"; + } + + // OTA client blindly unmaps all partitions that are possibly mapped. + for (const auto& name : {"sys_b", "vnd_b", "prd_b"}) { + ASSERT_TRUE(sm->UnmapUpdateSnapshot(name)); + } + + // Execute the update. + ASSERT_TRUE(sm->BeginUpdate()); + ASSERT_TRUE(sm->CreateUpdateSnapshots(manifest_)); + + // Write some data to target partitions. + for (const auto& name : {"sys_b", "vnd_b", "prd_b"}) { + ASSERT_TRUE(WriteSnapshotAndHash(name)); + } + + std::vector partitions = {sys_, vnd_, prd_}; + for (auto* partition : partitions) { + AddOperation(partition); + + std::unique_ptr writer; + auto res = MapUpdateSnapshot(partition->partition_name() + "_b", &writer); + ASSERT_TRUE(res); + ASSERT_TRUE(writer->AddZeroBlocks(0, 1)); + ASSERT_TRUE(writer->AddZeroBlocks(0, 1)); + ASSERT_TRUE(writer->Finalize()); + } + + ASSERT_TRUE(sm->FinishedSnapshotWrites(false)); + + // Simulate shutting down the device. + ASSERT_TRUE(UnmapAll()); + + // After reboot, init does first stage mount. + auto init = NewManagerForFirstStageMount("_b"); + ASSERT_NE(init, nullptr); + ASSERT_TRUE(init->NeedSnapshotsInFirstStageMount()); + ASSERT_TRUE(init->CreateLogicalAndSnapshotPartitions("super", snapshot_timeout_)); + + // Initiate the merge and wait for it to be completed. + ASSERT_TRUE(init->InitiateMerge()); + ASSERT_EQ(UpdateState::MergeCompleted, init->ProcessUpdateState()); +} + // Test that shrinking and growing partitions at the same time is handled // correctly in VABC. TEST_F(SnapshotUpdateTest, SpaceSwapUpdate) { diff --git a/fs_mgr/libsnapshot/snapuserd.cpp b/fs_mgr/libsnapshot/snapuserd.cpp index e578a7676..a09b1111c 100644 --- a/fs_mgr/libsnapshot/snapuserd.cpp +++ b/fs_mgr/libsnapshot/snapuserd.cpp @@ -64,7 +64,7 @@ bool Snapuserd::CommitMerge(int num_merge_ops) { int ret = msync(mapped_addr_, BLOCK_SZ, MS_SYNC); if (ret < 0) { - PLOG(ERROR) << "msync header failed: " << ret; + SNAP_PLOG(ERROR) << "msync header failed: " << ret; return false; }