From 0f707941d4c1fb5a9840dc7d795b45a769c6dc3b Mon Sep 17 00:00:00 2001 From: David Anderson Date: Wed, 14 Jul 2021 16:42:17 -0700 Subject: [PATCH] libsnapshot: Fix inconsistency in how merge ops are counted. A recent change to libsnapshot caused us to filter out duplicate COW ops. The merge consistency check relied on the old method of manually counting ops, causing it to come up with a different number. Fix this by using the already computed "official" count. Bug: 193532829 Test: new test case in vts_libsnapshot_test manual test with incremental OTA Change-Id: I68d1e41f5c140af20a04ba80e3db0780a916ecf8 --- fs_mgr/libsnapshot/cow_reader.cpp | 11 +++-- .../include/libsnapshot/cow_reader.h | 4 ++ fs_mgr/libsnapshot/snapshot.cpp | 6 +-- fs_mgr/libsnapshot/snapshot_test.cpp | 47 +++++++++++++++++++ fs_mgr/libsnapshot/snapuserd.cpp | 2 +- 5 files changed, 59 insertions(+), 11 deletions(-) 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; }