diff --git a/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h index 397ff2e7d..1d7b6031c 100644 --- a/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h +++ b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h @@ -569,12 +569,6 @@ class SnapshotManager final : public ISnapshotManager { std::string GetRollbackIndicatorPath(); std::string GetForwardMergeIndicatorPath(); - // Return the name of the device holding the "snapshot" or "snapshot-merge" - // target. This may not be the final device presented via MapSnapshot(), if - // for example there is a linear segment. - std::string GetSnapshotDeviceName(const std::string& snapshot_name, - const SnapshotStatus& status); - bool MapAllPartitions(LockedFile* lock, const std::string& super_device, uint32_t slot, const std::chrono::milliseconds& timeout_ms); @@ -673,8 +667,8 @@ class SnapshotManager final : public ISnapshotManager { // Helper for RemoveAllSnapshots. // Check whether |name| should be deleted as a snapshot name. - bool ShouldDeleteSnapshot(LockedFile* lock, const std::map& flashing_status, - Slot current_slot, const std::string& name); + bool ShouldDeleteSnapshot(const std::map& flashing_status, Slot current_slot, + const std::string& name); // Create or delete forward merge indicator given |wipe|. Iff wipe is scheduled, // allow forward merge on FDR. diff --git a/fs_mgr/libsnapshot/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp index 21b720e32..64e45ede0 100644 --- a/fs_mgr/libsnapshot/snapshot.cpp +++ b/fs_mgr/libsnapshot/snapshot.cpp @@ -132,10 +132,6 @@ static std::string GetBaseDeviceName(const std::string& partition_name) { return partition_name + "-base"; } -static std::string GetSnapshotExtraDeviceName(const std::string& snapshot_name) { - return snapshot_name + "-inner"; -} - bool SnapshotManager::BeginUpdate() { bool needs_merge = false; if (!TryCancelUpdate(&needs_merge)) { @@ -465,8 +461,13 @@ bool SnapshotManager::MapSnapshot(LockedFile* lock, const std::string& name, LOG(ERROR) << "Invalid snapshot size for " << base_device << ": " << status.snapshot_size(); return false; } + if (status.device_size() != status.snapshot_size()) { + LOG(ERROR) << "Device size and snapshot size must be the same (device size = " + << status.device_size() << ", snapshot size = " << status.snapshot_size(); + return false; + } + uint64_t snapshot_sectors = status.snapshot_size() / kSectorSize; - uint64_t linear_sectors = (status.device_size() - status.snapshot_size()) / kSectorSize; auto& dm = DeviceMapper::Instance(); @@ -491,45 +492,13 @@ bool SnapshotManager::MapSnapshot(LockedFile* lock, const std::string& name, break; } - // The kernel (tested on 4.19) crashes horribly if a device has both a snapshot - // and a linear target in the same table. Instead, we stack them, and give the - // snapshot device a different name. It is not exposed to the caller in this - // case. - auto snap_name = (linear_sectors > 0) ? GetSnapshotExtraDeviceName(name) : name; - DmTable table; table.Emplace(0, snapshot_sectors, base_device, cow_device, mode, kSnapshotChunkSize); - if (!dm.CreateDevice(snap_name, table, dev_path, timeout_ms)) { - LOG(ERROR) << "Could not create snapshot device: " << snap_name; + if (!dm.CreateDevice(name, table, dev_path, timeout_ms)) { + LOG(ERROR) << "Could not create snapshot device: " << name; return false; } - - if (linear_sectors) { - std::string snap_dev; - if (!dm.GetDeviceString(snap_name, &snap_dev)) { - LOG(ERROR) << "Cannot determine major/minor for: " << snap_name; - return false; - } - - // Our stacking will looks like this: - // [linear, linear] ; to snapshot, and non-snapshot region of base device - // [snapshot-inner] - // [base device] [cow] - DmTable table; - table.Emplace(0, snapshot_sectors, snap_dev, 0); - table.Emplace(snapshot_sectors, linear_sectors, base_device, - snapshot_sectors); - if (!dm.CreateDevice(name, table, dev_path, timeout_ms)) { - LOG(ERROR) << "Could not create outer snapshot device: " << name; - dm.DeleteDevice(snap_name); - return false; - } - } - - // :TODO: when merging is implemented, we need to add an argument to the - // status indicating how much progress is left to merge. (device-mapper - // does not retain the initial values, so we can't derive them.) return true; } @@ -565,13 +534,6 @@ bool SnapshotManager::UnmapSnapshot(LockedFile* lock, const std::string& name) { LOG(ERROR) << "Could not delete snapshot device: " << name; return false; } - - auto snapshot_extra_device = GetSnapshotExtraDeviceName(name); - if (!dm.DeleteDeviceIfExists(snapshot_extra_device)) { - LOG(ERROR) << "Could not delete snapshot inner device: " << snapshot_extra_device; - return false; - } - return true; } @@ -749,16 +711,15 @@ bool SnapshotManager::SwitchSnapshotToMerge(LockedFile* lock, const std::string& // After this, we return true because we technically did switch to a merge // target. Everything else we do here is just informational. - auto dm_name = GetSnapshotDeviceName(name, status); - if (!RewriteSnapshotDeviceTable(dm_name)) { + if (!RewriteSnapshotDeviceTable(name)) { return false; } status.set_state(SnapshotState::MERGING); DmTargetSnapshot::Status dm_status; - if (!QuerySnapshotStatus(dm_name, nullptr, &dm_status)) { - LOG(ERROR) << "Could not query merge status for snapshot: " << dm_name; + if (!QuerySnapshotStatus(name, nullptr, &dm_status)) { + LOG(ERROR) << "Could not query merge status for snapshot: " << name; } status.set_sectors_allocated(dm_status.sectors_allocated); status.set_metadata_sectors(dm_status.metadata_sectors); @@ -768,33 +729,33 @@ bool SnapshotManager::SwitchSnapshotToMerge(LockedFile* lock, const std::string& return true; } -bool SnapshotManager::RewriteSnapshotDeviceTable(const std::string& dm_name) { +bool SnapshotManager::RewriteSnapshotDeviceTable(const std::string& name) { auto& dm = DeviceMapper::Instance(); std::vector old_targets; - if (!dm.GetTableInfo(dm_name, &old_targets)) { - LOG(ERROR) << "Could not read snapshot device table: " << dm_name; + if (!dm.GetTableInfo(name, &old_targets)) { + LOG(ERROR) << "Could not read snapshot device table: " << name; return false; } if (old_targets.size() != 1 || DeviceMapper::GetTargetType(old_targets[0].spec) != "snapshot") { - LOG(ERROR) << "Unexpected device-mapper table for snapshot: " << dm_name; + LOG(ERROR) << "Unexpected device-mapper table for snapshot: " << name; return false; } std::string base_device, cow_device; if (!DmTargetSnapshot::GetDevicesFromParams(old_targets[0].data, &base_device, &cow_device)) { - LOG(ERROR) << "Could not derive underlying devices for snapshot: " << dm_name; + LOG(ERROR) << "Could not derive underlying devices for snapshot: " << name; return false; } DmTable table; table.Emplace(0, old_targets[0].spec.length, base_device, cow_device, SnapshotStorageMode::Merge, kSnapshotChunkSize); - if (!dm.LoadTableAndActivate(dm_name, table)) { - LOG(ERROR) << "Could not swap device-mapper tables on snapshot device " << dm_name; + if (!dm.LoadTableAndActivate(name, table)) { + LOG(ERROR) << "Could not swap device-mapper tables on snapshot device " << name; return false; } - LOG(INFO) << "Successfully switched snapshot device to a merge target: " << dm_name; + LOG(INFO) << "Successfully switched snapshot device to a merge target: " << name; return true; } @@ -1003,11 +964,9 @@ UpdateState SnapshotManager::CheckTargetMergeState(LockedFile* lock, const std:: return UpdateState::MergeFailed; } - std::string dm_name = GetSnapshotDeviceName(name, snapshot_status); - std::unique_ptr current_metadata; - if (!IsSnapshotDevice(dm_name)) { + if (!IsSnapshotDevice(name)) { if (!current_metadata) { current_metadata = ReadCurrentMetadata(); } @@ -1029,7 +988,7 @@ UpdateState SnapshotManager::CheckTargetMergeState(LockedFile* lock, const std:: return UpdateState::MergeCompleted; } - LOG(ERROR) << "Expected snapshot or snapshot-merge for device: " << dm_name; + LOG(ERROR) << "Expected snapshot or snapshot-merge for device: " << name; return UpdateState::MergeFailed; } @@ -1039,7 +998,7 @@ UpdateState SnapshotManager::CheckTargetMergeState(LockedFile* lock, const std:: std::string target_type; DmTargetSnapshot::Status status; - if (!QuerySnapshotStatus(dm_name, &target_type, &status)) { + if (!QuerySnapshotStatus(name, &target_type, &status)) { return UpdateState::MergeFailed; } if (target_type != "snapshot-merge") { @@ -1124,21 +1083,20 @@ void SnapshotManager::AcknowledgeMergeFailure() { bool SnapshotManager::OnSnapshotMergeComplete(LockedFile* lock, const std::string& name, const SnapshotStatus& status) { - auto dm_name = GetSnapshotDeviceName(name, status); - if (IsSnapshotDevice(dm_name)) { + if (IsSnapshotDevice(name)) { // We are extra-cautious here, to avoid deleting the wrong table. std::string target_type; DmTargetSnapshot::Status dm_status; - if (!QuerySnapshotStatus(dm_name, &target_type, &dm_status)) { + if (!QuerySnapshotStatus(name, &target_type, &dm_status)) { return false; } if (target_type != "snapshot-merge") { LOG(ERROR) << "Unexpected target type " << target_type - << " for snapshot device: " << dm_name; + << " for snapshot device: " << name; return false; } if (dm_status.sectors_allocated != dm_status.metadata_sectors) { - LOG(ERROR) << "Merge is unexpectedly incomplete for device " << dm_name; + LOG(ERROR) << "Merge is unexpectedly incomplete for device " << name; return false; } if (!CollapseSnapshotDevice(name, status)) { @@ -1159,23 +1117,21 @@ bool SnapshotManager::OnSnapshotMergeComplete(LockedFile* lock, const std::strin bool SnapshotManager::CollapseSnapshotDevice(const std::string& name, const SnapshotStatus& status) { auto& dm = DeviceMapper::Instance(); - auto dm_name = GetSnapshotDeviceName(name, status); // Verify we have a snapshot-merge device. DeviceMapper::TargetInfo target; - if (!GetSingleTarget(dm_name, TableQuery::Table, &target)) { + if (!GetSingleTarget(name, TableQuery::Table, &target)) { return false; } if (DeviceMapper::GetTargetType(target.spec) != "snapshot-merge") { // This should be impossible, it was checked earlier. - LOG(ERROR) << "Snapshot device has invalid target type: " << dm_name; + LOG(ERROR) << "Snapshot device has invalid target type: " << name; return false; } std::string base_device, cow_device; if (!DmTargetSnapshot::GetDevicesFromParams(target.data, &base_device, &cow_device)) { - LOG(ERROR) << "Could not parse snapshot device " << dm_name - << " parameters: " << target.data; + LOG(ERROR) << "Could not parse snapshot device " << name << " parameters: " << target.data; return false; } @@ -1186,42 +1142,6 @@ bool SnapshotManager::CollapseSnapshotDevice(const std::string& name, return false; } - if (dm_name != name) { - // We've derived the base device, but we actually need to replace the - // table of the outermost device. Do a quick verification that this - // device looks like we expect it to. - std::vector outer_table; - if (!dm.GetTableInfo(name, &outer_table)) { - LOG(ERROR) << "Could not validate outer snapshot table: " << name; - return false; - } - if (outer_table.size() != 2) { - LOG(ERROR) << "Expected 2 dm-linear targets for table " << name - << ", got: " << outer_table.size(); - return false; - } - for (const auto& target : outer_table) { - auto target_type = DeviceMapper::GetTargetType(target.spec); - if (target_type != "linear") { - LOG(ERROR) << "Outer snapshot table may only contain linear targets, but " << name - << " has target: " << target_type; - return false; - } - } - if (outer_table[0].spec.length != snapshot_sectors) { - LOG(ERROR) << "dm-snapshot " << name << " should have " << snapshot_sectors - << " sectors, got: " << outer_table[0].spec.length; - return false; - } - uint64_t expected_device_sectors = status.device_size() / kSectorSize; - uint64_t actual_device_sectors = outer_table[0].spec.length + outer_table[1].spec.length; - if (expected_device_sectors != actual_device_sectors) { - LOG(ERROR) << "Outer device " << name << " should have " << expected_device_sectors - << " sectors, got: " << actual_device_sectors; - return false; - } - } - uint32_t slot = SlotNumberForSlotSuffix(device_->GetSlotSuffix()); // Create a DmTable that is identical to the base device. CreateLogicalPartitionParams base_device_params{ @@ -1236,7 +1156,6 @@ bool SnapshotManager::CollapseSnapshotDevice(const std::string& name, return false; } - // Note: we are replacing the *outer* table here, so we do not use dm_name. if (!dm.LoadTableAndActivate(name, table)) { return false; } @@ -1246,18 +1165,9 @@ bool SnapshotManager::CollapseSnapshotDevice(const std::string& name, // flushed remaining I/O. We could in theory replace with dm-zero (or // re-use the table above), but for now it's better to know why this // would fail. - if (dm_name != name && !dm.DeleteDeviceIfExists(dm_name)) { - LOG(ERROR) << "Unable to delete snapshot device " << dm_name << ", COW cannot be " - << "reclaimed until after reboot."; - return false; - } - if (status.compression_enabled()) { UnmapDmUserDevice(name); } - - // Cleanup the base device as well, since it is no longer used. This does - // not block cleanup. auto base_name = GetBaseDeviceName(name); if (!dm.DeleteDeviceIfExists(base_name)) { LOG(ERROR) << "Unable to delete base device for snapshot: " << base_name; @@ -1548,7 +1458,7 @@ bool SnapshotManager::RemoveAllSnapshots(LockedFile* lock) { // Otherwise (UPDATED flag), only delete snapshots if they are not mapped // as dm-snapshot (for example, after merge completes). bool should_unmap = current_slot != Slot::Target; - bool should_delete = ShouldDeleteSnapshot(lock, flashing_status, current_slot, name); + bool should_delete = ShouldDeleteSnapshot(flashing_status, current_slot, name); bool partition_ok = true; if (should_unmap && !UnmapPartitionWithSnapshot(lock, name)) { @@ -1582,8 +1492,7 @@ bool SnapshotManager::RemoveAllSnapshots(LockedFile* lock) { } // See comments in RemoveAllSnapshots(). -bool SnapshotManager::ShouldDeleteSnapshot(LockedFile* lock, - const std::map& flashing_status, +bool SnapshotManager::ShouldDeleteSnapshot(const std::map& flashing_status, Slot current_slot, const std::string& name) { if (current_slot != Slot::Target) { return true; @@ -1597,16 +1506,7 @@ bool SnapshotManager::ShouldDeleteSnapshot(LockedFile* lock, // partition flashed, okay to delete obsolete snapshots return true; } - // partition updated, only delete if not dm-snapshot - SnapshotStatus status; - if (!ReadSnapshotStatus(lock, name, &status)) { - LOG(WARNING) << "Unable to read snapshot status for " << name - << ", guessing snapshot device name"; - auto extra_name = GetSnapshotExtraDeviceName(name); - return !IsSnapshotDevice(name) && !IsSnapshotDevice(extra_name); - } - auto dm_name = GetSnapshotDeviceName(name, status); - return !IsSnapshotDevice(dm_name); + return !IsSnapshotDevice(name); } UpdateState SnapshotManager::GetUpdateState(double* progress) { @@ -2409,14 +2309,6 @@ bool SnapshotManager::WriteSnapshotStatus(LockedFile* lock, const SnapshotStatus return true; } -std::string SnapshotManager::GetSnapshotDeviceName(const std::string& snapshot_name, - const SnapshotStatus& status) { - if (status.device_size() != status.snapshot_size()) { - return GetSnapshotExtraDeviceName(snapshot_name); - } - return snapshot_name; -} - bool SnapshotManager::EnsureImageManager() { if (images_) return true; diff --git a/fs_mgr/libsnapshot/snapshot_test.cpp b/fs_mgr/libsnapshot/snapshot_test.cpp index 80a60742c..8b72022a6 100644 --- a/fs_mgr/libsnapshot/snapshot_test.cpp +++ b/fs_mgr/libsnapshot/snapshot_test.cpp @@ -467,31 +467,6 @@ TEST_F(SnapshotTest, MapSnapshot) { ASSERT_TRUE(android::base::StartsWith(snap_device, "/dev/block/dm-")); } -TEST_F(SnapshotTest, MapPartialSnapshot) { - ASSERT_TRUE(AcquireLock()); - - static const uint64_t kSnapshotSize = 1024 * 1024; - static const uint64_t kDeviceSize = 1024 * 1024 * 2; - SnapshotStatus status; - status.set_name("test-snapshot"); - status.set_device_size(kDeviceSize); - status.set_snapshot_size(kSnapshotSize); - status.set_cow_file_size(kSnapshotSize); - ASSERT_TRUE(sm->CreateSnapshot(lock_.get(), &status)); - ASSERT_TRUE(CreateCowImage("test-snapshot")); - - std::string base_device; - ASSERT_TRUE(CreatePartition("base-device", kDeviceSize, &base_device)); - - std::string cow_device; - ASSERT_TRUE(MapCowImage("test-snapshot", 10s, &cow_device)); - - std::string snap_device; - ASSERT_TRUE(sm->MapSnapshot(lock_.get(), "test-snapshot", base_device, cow_device, 10s, - &snap_device)); - ASSERT_TRUE(android::base::StartsWith(snap_device, "/dev/block/dm-")); -} - TEST_F(SnapshotTest, NoMergeBeforeReboot) { ASSERT_TRUE(sm->FinishedSnapshotWrites(false)); @@ -590,8 +565,7 @@ TEST_F(SnapshotTest, FirstStageMountAndMerge) { ASSERT_EQ(status.state(), SnapshotState::CREATED); DeviceMapper::TargetInfo target; - auto dm_name = init->GetSnapshotDeviceName("test_partition_b", status); - ASSERT_TRUE(init->IsSnapshotDevice(dm_name, &target)); + ASSERT_TRUE(init->IsSnapshotDevice("test_partition_b", &target)); ASSERT_EQ(DeviceMapper::GetTargetType(target.spec), "snapshot"); } @@ -618,8 +592,7 @@ TEST_F(SnapshotTest, FlashSuperDuringUpdate) { // We should not get a snapshot device now. DeviceMapper::TargetInfo target; - auto dm_name = init->GetSnapshotDeviceName("test_partition_b", status); - ASSERT_FALSE(init->IsSnapshotDevice(dm_name, &target)); + ASSERT_FALSE(init->IsSnapshotDevice("test_partition_b", &target)); // We should see a cancelled update as well. lock_ = nullptr;