Merge "libsnapshot: Fix inconsistency in how merge ops are counted." am: 16aa3c9573
Original change: https://android-review.googlesource.com/c/platform/system/core/+/1767246
Change-Id: I073d1478291d824c83e51f7f5f838e2a8c03c3ee
diff --git a/fs_mgr/libsnapshot/cow_reader.cpp b/fs_mgr/libsnapshot/cow_reader.cpp
index b3198a0..af49c7d 100644
--- a/fs_mgr/libsnapshot/cow_reader.cpp
+++ b/fs_mgr/libsnapshot/cow_reader.cpp
@@ -381,10 +381,10 @@
std::set<int, std::greater<int>> other_ops;
auto seq_ops_set = std::unordered_set<uint32_t>();
auto block_map = std::make_shared<std::unordered_map<uint32_t, int>>();
- 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 @@
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 @@
}
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 05f7066..6c3059c 100644
--- a/fs_mgr/libsnapshot/include/libsnapshot/cow_reader.h
+++ b/fs_mgr/libsnapshot/include/libsnapshot/cow_reader.h
@@ -126,6 +126,10 @@
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 0e36da1..bed5f56 100644
--- a/fs_mgr/libsnapshot/snapshot.cpp
+++ b/fs_mgr/libsnapshot/snapshot.cpp
@@ -1193,11 +1193,7 @@
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 6018643..b2203fe 100644
--- a/fs_mgr/libsnapshot/snapshot_test.cpp
+++ b/fs_mgr/libsnapshot/snapshot_test.cpp
@@ -1184,6 +1184,53 @@
}
}
+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<PartitionUpdate*> partitions = {sys_, vnd_, prd_};
+ for (auto* partition : partitions) {
+ AddOperation(partition);
+
+ std::unique_ptr<ISnapshotWriter> 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 e578a76..a09b111 100644
--- a/fs_mgr/libsnapshot/snapuserd.cpp
+++ b/fs_mgr/libsnapshot/snapuserd.cpp
@@ -64,7 +64,7 @@
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;
}