From 16378996320164254a4f4db42b56c6adb3130872 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Sat, 4 Dec 2021 16:48:53 -0800 Subject: [PATCH 1/2] libsnapshot: Fix checks for compression to work with new snapuserd. Most checks for compression are used as a gate to decide whether dm-user is enabled. However, dm-user can now be enabled without compression, so these checks have to be audited and adjusted. Once the "old" compression path is removed, these checks will become simpler. Bug: 208944665 Test: vts_libsnapshot_test -force_config vab Change-Id: I281937e26427044f1ef2e1164d86d67e4170163a --- fs_mgr/libsnapshot/partition_cow_creator.cpp | 2 +- fs_mgr/libsnapshot/partition_cow_creator.h | 1 + fs_mgr/libsnapshot/snapshot.cpp | 148 +++++++++++-------- fs_mgr/libsnapshot/snapshot_test.cpp | 10 +- 4 files changed, 90 insertions(+), 71 deletions(-) diff --git a/fs_mgr/libsnapshot/partition_cow_creator.cpp b/fs_mgr/libsnapshot/partition_cow_creator.cpp index 5569da038..5fcbdfe11 100644 --- a/fs_mgr/libsnapshot/partition_cow_creator.cpp +++ b/fs_mgr/libsnapshot/partition_cow_creator.cpp @@ -143,7 +143,7 @@ void WriteExtent(DmSnapCowSizeCalculator* sc, const chromeos_update_engine::Exte } std::optional PartitionCowCreator::GetCowSize() { - if (compression_enabled) { + if (compression_enabled || userspace_snapshots_enabled) { if (update == nullptr || !update->has_estimate_cow_size()) { LOG(ERROR) << "Update manifest does not include a COW size"; return std::nullopt; diff --git a/fs_mgr/libsnapshot/partition_cow_creator.h b/fs_mgr/libsnapshot/partition_cow_creator.h index 34b39ca72..1f3417778 100644 --- a/fs_mgr/libsnapshot/partition_cow_creator.h +++ b/fs_mgr/libsnapshot/partition_cow_creator.h @@ -58,6 +58,7 @@ struct PartitionCowCreator { std::vector extra_extents = {}; // True if compression is enabled. bool compression_enabled = false; + bool userspace_snapshots_enabled = false; std::string compression_algorithm; struct Return { diff --git a/fs_mgr/libsnapshot/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp index e6e17bdf3..dfa467ee6 100644 --- a/fs_mgr/libsnapshot/snapshot.cpp +++ b/fs_mgr/libsnapshot/snapshot.cpp @@ -1329,7 +1329,7 @@ MergeFailureCode SnapshotManager::CheckMergeConsistency(LockedFile* lock, const const SnapshotStatus& status) { CHECK(lock); - if (!status.compression_enabled()) { + if (!status.compression_enabled() && !UpdateUsesUserSnapshots(lock)) { // Do not try to verify old-style COWs yet. return MergeFailureCode::Ok; } @@ -2345,13 +2345,15 @@ bool SnapshotManager::MapPartitionWithSnapshot(LockedFile* lock, remaining_time = GetRemainingTime(params.timeout_ms, begin); if (remaining_time.count() < 0) return false; - if (context == SnapshotContext::Update && live_snapshot_status->compression_enabled()) { - // Stop here, we can't run dm-user yet, the COW isn't built. - created_devices.Release(); - return true; + if (context == SnapshotContext::Update) { + if (UpdateUsesUserSnapshots(lock) || live_snapshot_status->compression_enabled()) { + // Stop here, we can't run dm-user yet, the COW isn't built. + created_devices.Release(); + return true; + } } - if (live_snapshot_status->compression_enabled()) { + if (UpdateUsesUserSnapshots(lock) || live_snapshot_status->compression_enabled()) { // Get the source device (eg the view of the partition from before it was resized). std::string source_device_path; if (live_snapshot_status->old_partition_size() > 0) { @@ -3132,6 +3134,61 @@ Return SnapshotManager::CreateUpdateSnapshots(const DeltaArchiveManifest& manife .compression_algorithm = compression_algorithm, }; + if (!device()->IsTestDevice()) { + cow_creator.userspace_snapshots_enabled = IsUserspaceSnapshotsEnabled(); + if (cow_creator.userspace_snapshots_enabled) { + LOG(INFO) << "User-space snapshots enabled"; + } else { + LOG(INFO) << "User-space snapshots disabled"; + } + + // Terminate stale daemon if any + std::unique_ptr snapuserd_client = + SnapuserdClient::Connect(kSnapuserdSocket, 10s); + if (snapuserd_client) { + snapuserd_client->DetachSnapuserd(); + snapuserd_client->CloseConnection(); + snapuserd_client = nullptr; + } + + // Clear the cached client if any + if (snapuserd_client_) { + snapuserd_client_->CloseConnection(); + snapuserd_client_ = nullptr; + } + } else { + cow_creator.userspace_snapshots_enabled = !IsDmSnapshotTestingEnabled(); + if (cow_creator.userspace_snapshots_enabled) { + LOG(INFO) << "User-space snapshots disabled for testing"; + } else { + LOG(INFO) << "User-space snapshots enabled for testing"; + } + } + + is_snapshot_userspace_ = cow_creator.userspace_snapshots_enabled; + + // If compression is enabled, we need to retain a copy of the old metadata + // so we can access original blocks in case they are moved around. We do + // not want to rely on the old super metadata slot because we don't + // guarantee its validity after the slot switch is successful. + // + // Note that we do this for userspace merges even if compression is + // disabled, since the code path expects it even if the source device will + // be unused. + if (cow_creator.compression_enabled || cow_creator.userspace_snapshots_enabled) { + auto metadata = current_metadata->Export(); + if (!metadata) { + LOG(ERROR) << "Could not export current metadata"; + return Return::Error(); + } + + auto path = GetOldPartitionMetadataPath(); + if (!android::fs_mgr::WriteToImageFile(path, *metadata.get())) { + LOG(ERROR) << "Cannot write old metadata to " << path; + return Return::Error(); + } + } + auto ret = CreateUpdateSnapshotsInternal(lock.get(), manifest, &cow_creator, &created_devices, &all_snapshot_status); if (!ret.is_ok()) return ret; @@ -3153,64 +3210,11 @@ Return SnapshotManager::CreateUpdateSnapshots(const DeltaArchiveManifest& manife return Return::Error(); } - // If compression is enabled, we need to retain a copy of the old metadata - // so we can access original blocks in case they are moved around. We do - // not want to rely on the old super metadata slot because we don't - // guarantee its validity after the slot switch is successful. - if (cow_creator.compression_enabled) { - auto metadata = current_metadata->Export(); - if (!metadata) { - LOG(ERROR) << "Could not export current metadata"; - return Return::Error(); - } - - auto path = GetOldPartitionMetadataPath(); - if (!android::fs_mgr::WriteToImageFile(path, *metadata.get())) { - LOG(ERROR) << "Cannot write old metadata to " << path; - return Return::Error(); - } - } - SnapshotUpdateStatus status = ReadSnapshotUpdateStatus(lock.get()); status.set_state(update_state); status.set_compression_enabled(cow_creator.compression_enabled); - if (cow_creator.compression_enabled) { - if (!device()->IsTestDevice()) { - // Userspace snapshots is enabled only if compression is enabled - status.set_userspace_snapshots(IsUserspaceSnapshotsEnabled()); - if (IsUserspaceSnapshotsEnabled()) { - is_snapshot_userspace_ = true; - LOG(INFO) << "User-space snapshots enabled"; - } else { - is_snapshot_userspace_ = false; - LOG(INFO) << "User-space snapshots disabled"; - } + status.set_userspace_snapshots(cow_creator.userspace_snapshots_enabled); - // Terminate stale daemon if any - std::unique_ptr snapuserd_client = - SnapuserdClient::Connect(kSnapuserdSocket, 10s); - if (snapuserd_client) { - snapuserd_client->DetachSnapuserd(); - snapuserd_client->CloseConnection(); - snapuserd_client = nullptr; - } - - // Clear the cached client if any - if (snapuserd_client_) { - snapuserd_client_->CloseConnection(); - snapuserd_client_ = nullptr; - } - } else { - status.set_userspace_snapshots(!IsDmSnapshotTestingEnabled()); - if (IsDmSnapshotTestingEnabled()) { - is_snapshot_userspace_ = false; - LOG(INFO) << "User-space snapshots disabled for testing"; - } else { - is_snapshot_userspace_ = true; - LOG(INFO) << "User-space snapshots enabled for testing"; - } - } - } if (!WriteSnapshotUpdateStatus(lock.get(), status)) { LOG(ERROR) << "Unable to write new update state"; return Return::Error(); @@ -3374,6 +3378,8 @@ Return SnapshotManager::InitializeUpdateSnapshots( const std::map& all_snapshot_status) { CHECK(lock); + bool userspace_merges = UpdateUsesUserSnapshots(lock); + CreateLogicalPartitionParams cow_params{ .block_device = LP_METADATA_DEFAULT_PARTITION_NAME, .metadata = exported_target_metadata, @@ -3403,7 +3409,7 @@ Return SnapshotManager::InitializeUpdateSnapshots( return Return::Error(); } - if (it->second.compression_enabled()) { + if (userspace_merges || it->second.compression_enabled()) { unique_fd fd(open(cow_path.c_str(), O_RDWR | O_CLOEXEC)); if (fd < 0) { PLOG(ERROR) << "open " << cow_path << " failed for snapshot " @@ -3449,8 +3455,8 @@ bool SnapshotManager::MapUpdateSnapshot(const CreateLogicalPartitionParams& para if (!ReadSnapshotStatus(lock.get(), params.GetPartitionName(), &status)) { return false; } - if (status.compression_enabled()) { - LOG(ERROR) << "Cannot use MapUpdateSnapshot with compressed snapshots"; + if (status.compression_enabled() || UpdateUsesUserSnapshots(lock.get())) { + LOG(ERROR) << "Cannot use MapUpdateSnapshot with user snapshots"; return false; } @@ -3507,7 +3513,7 @@ std::unique_ptr SnapshotManager::OpenSnapshotWriter( return nullptr; } - if (status.compression_enabled()) { + if (status.compression_enabled() || UpdateUsesUserSnapshots(lock.get())) { return OpenCompressedSnapshotWriter(lock.get(), source_device, params.GetPartitionName(), status, paths); } @@ -3647,6 +3653,7 @@ bool SnapshotManager::Dump(std::ostream& os) { << (access(GetForwardMergeIndicatorPath().c_str(), F_OK) == 0 ? "exists" : strerror(errno)) << std::endl; ss << "Source build fingerprint: " << update_status.source_build_fingerprint() << std::endl; + ss << "Using userspace snapshots: " << UpdateUsesUserSnapshots(file.get()) << std::endl; bool ok = true; std::vector snapshots; @@ -3847,6 +3854,10 @@ UpdateState SnapshotManager::ProcessUpdateStateOnDataWipe(bool allow_forward_mer bool SnapshotManager::EnsureNoOverflowSnapshot(LockedFile* lock) { CHECK(lock); + if (UpdateUsesUserSnapshots(lock)) { + return true; + } + std::vector snapshots; if (!ListSnapshots(lock, &snapshots)) { LOG(ERROR) << "Could not list snapshots."; @@ -3859,6 +3870,7 @@ bool SnapshotManager::EnsureNoOverflowSnapshot(LockedFile* lock) { return false; } if (status.compression_enabled()) { + // Compressed snapshots are never written through dm-snapshot. continue; } @@ -4022,7 +4034,10 @@ bool SnapshotManager::IsSnapuserdRequired() { if (!lock) return false; auto status = ReadSnapshotUpdateStatus(lock.get()); - return status.state() != UpdateState::None && status.compression_enabled(); + if (status.state() != UpdateState::None && status.compression_enabled()) { + return true; + } + return UpdateUsesUserSnapshots(lock.get()); } bool SnapshotManager::DetachSnapuserdForSelinux(std::vector* snapuserd_argv) { @@ -4048,6 +4063,9 @@ const LpMetadata* SnapshotManager::ReadOldPartitionMetadata(LockedFile* lock) { } MergePhase SnapshotManager::DecideMergePhase(const SnapshotStatus& status) { + // Note: disabling compression disables move operations, so we don't need + // separate phases when compression is disabled (irrespective of userspace + // merges). if (status.compression_enabled() && status.device_size() < status.old_partition_size()) { return MergePhase::FIRST_PHASE; } diff --git a/fs_mgr/libsnapshot/snapshot_test.cpp b/fs_mgr/libsnapshot/snapshot_test.cpp index 14f2d45be..c70b353ed 100644 --- a/fs_mgr/libsnapshot/snapshot_test.cpp +++ b/fs_mgr/libsnapshot/snapshot_test.cpp @@ -357,7 +357,7 @@ class SnapshotTest : public ::testing::Test { DeltaArchiveManifest manifest; auto dynamic_partition_metadata = manifest.mutable_dynamic_partition_metadata(); - dynamic_partition_metadata->set_vabc_enabled(IsCompressionEnabled()); + dynamic_partition_metadata->set_vabc_enabled(ShouldUseCompression()); dynamic_partition_metadata->set_cow_version(android::snapshot::kCowVersionMajor); auto group = dynamic_partition_metadata->add_groups(); @@ -396,7 +396,7 @@ class SnapshotTest : public ::testing::Test { if (!res) { return res; } - } else if (!IsCompressionEnabled()) { + } else if (!ShouldUseUserspaceSnapshots()) { std::string ignore; if (!MapUpdateSnapshot("test_partition_b", &ignore)) { return AssertionFailure() << "Failed to map test_partition_b"; @@ -1030,7 +1030,7 @@ class SnapshotUpdateTest : public SnapshotTest { } AssertionResult MapOneUpdateSnapshot(const std::string& name) { - if (ShouldUseCompression()) { + if (ShouldUseUserspaceSnapshots()) { std::unique_ptr writer; return MapUpdateSnapshot(name, &writer); } else { @@ -1040,7 +1040,7 @@ class SnapshotUpdateTest : public SnapshotTest { } AssertionResult WriteSnapshotAndHash(const std::string& name) { - if (ShouldUseCompression()) { + if (ShouldUseUserspaceSnapshots()) { std::unique_ptr writer; auto res = MapUpdateSnapshot(name, &writer); if (!res) { @@ -2072,7 +2072,7 @@ TEST_F(SnapshotUpdateTest, Hashtree) { // Test for overflow bit after update TEST_F(SnapshotUpdateTest, Overflow) { - if (ShouldUseCompression()) { + if (ShouldUseUserspaceSnapshots()) { GTEST_SKIP() << "No overflow bit set for userspace COWs"; } From 34b726ddbe5ffc24912b91e8fb31e7c06ab0be75 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Wed, 15 Dec 2021 19:55:10 -0800 Subject: [PATCH 2/2] libsnapshot: Fix libsnapshot_fuzzer_test. Rather than fix this up to work in all configurations, just force-disable snapuserd testing for these tests. This patch also adds a "UseUserspaceSnapshots" helper to IDeviceInfo so we can simplify some checks in snapshot.cpp. Bug: 208944665 Test: libsnapshot_fuzzer_test Change-Id: I219f956ba09b090158d5ac757ef1f1d137d512e0 --- fs_mgr/libsnapshot/device_info.cpp | 6 +++++ fs_mgr/libsnapshot/device_info.h | 1 + .../include/libsnapshot/mock_device_info.h | 1 + .../include/libsnapshot/snapshot.h | 1 + .../include_test/libsnapshot/test_helpers.h | 1 + fs_mgr/libsnapshot/snapshot.cpp | 24 +++++++------------ fs_mgr/libsnapshot/snapshot_fuzz_utils.h | 1 + fs_mgr/libsnapshot/test_helpers.cpp | 6 +++++ 8 files changed, 25 insertions(+), 16 deletions(-) diff --git a/fs_mgr/libsnapshot/device_info.cpp b/fs_mgr/libsnapshot/device_info.cpp index a6d96ed82..5c1b291c5 100644 --- a/fs_mgr/libsnapshot/device_info.cpp +++ b/fs_mgr/libsnapshot/device_info.cpp @@ -19,6 +19,8 @@ #include #include +#include "utility.h" + namespace android { namespace snapshot { @@ -143,5 +145,9 @@ android::dm::IDeviceMapper& DeviceInfo::GetDeviceMapper() { return android::dm::DeviceMapper::Instance(); } +bool DeviceInfo::UseUserspaceSnapshots() const { + return IsUserspaceSnapshotsEnabled(); +} + } // namespace snapshot } // namespace android diff --git a/fs_mgr/libsnapshot/device_info.h b/fs_mgr/libsnapshot/device_info.h index 8aefb8507..a07f554a8 100644 --- a/fs_mgr/libsnapshot/device_info.h +++ b/fs_mgr/libsnapshot/device_info.h @@ -41,6 +41,7 @@ class DeviceInfo final : public SnapshotManager::IDeviceInfo { std::unique_ptr OpenImageManager() const override; bool IsFirstStageInit() const override; android::dm::IDeviceMapper& GetDeviceMapper() override; + bool UseUserspaceSnapshots() const override; void set_first_stage_init(bool value) { first_stage_init_ = value; } diff --git a/fs_mgr/libsnapshot/include/libsnapshot/mock_device_info.h b/fs_mgr/libsnapshot/include/libsnapshot/mock_device_info.h index 573a85b24..8c4161c22 100644 --- a/fs_mgr/libsnapshot/include/libsnapshot/mock_device_info.h +++ b/fs_mgr/libsnapshot/include/libsnapshot/mock_device_info.h @@ -34,6 +34,7 @@ class MockDeviceInfo : public SnapshotManager::IDeviceInfo { MOCK_METHOD(bool, IsFirstStageInit, (), (const, override)); MOCK_METHOD(std::unique_ptr, OpenImageManager, (), (const, override)); + MOCK_METHOD(bool, UseUserspaceSnapshots, (), (const, override)); }; } // namespace android::snapshot diff --git a/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h index 41c6ef576..f7e37bbcc 100644 --- a/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h +++ b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h @@ -111,6 +111,7 @@ class ISnapshotManager { virtual bool IsFirstStageInit() const = 0; virtual std::unique_ptr OpenImageManager() const = 0; virtual android::dm::IDeviceMapper& GetDeviceMapper() = 0; + virtual bool UseUserspaceSnapshots() const = 0; // Helper method for implementing OpenImageManager. std::unique_ptr OpenImageManager(const std::string& gsid_dir) const; diff --git a/fs_mgr/libsnapshot/include_test/libsnapshot/test_helpers.h b/fs_mgr/libsnapshot/include_test/libsnapshot/test_helpers.h index c3b40dca4..07c3ec5b5 100644 --- a/fs_mgr/libsnapshot/include_test/libsnapshot/test_helpers.h +++ b/fs_mgr/libsnapshot/include_test/libsnapshot/test_helpers.h @@ -107,6 +107,7 @@ class TestDeviceInfo : public SnapshotManager::IDeviceInfo { } bool IsSlotUnbootable(uint32_t slot) { return unbootable_slots_.count(slot) != 0; } + bool UseUserspaceSnapshots() const override; void set_slot_suffix(const std::string& suffix) { slot_suffix_ = suffix; } void set_fake_super(const std::string& path) { diff --git a/fs_mgr/libsnapshot/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp index dfa467ee6..c7c5da1f3 100644 --- a/fs_mgr/libsnapshot/snapshot.cpp +++ b/fs_mgr/libsnapshot/snapshot.cpp @@ -3134,14 +3134,15 @@ Return SnapshotManager::CreateUpdateSnapshots(const DeltaArchiveManifest& manife .compression_algorithm = compression_algorithm, }; - if (!device()->IsTestDevice()) { - cow_creator.userspace_snapshots_enabled = IsUserspaceSnapshotsEnabled(); - if (cow_creator.userspace_snapshots_enabled) { - LOG(INFO) << "User-space snapshots enabled"; - } else { - LOG(INFO) << "User-space snapshots disabled"; - } + cow_creator.userspace_snapshots_enabled = device_->UseUserspaceSnapshots(); + if (cow_creator.userspace_snapshots_enabled) { + LOG(INFO) << "User-space snapshots enabled, compression = " << compression_algorithm; + } else { + LOG(INFO) << "User-space snapshots disabled, compression = " << compression_algorithm; + } + is_snapshot_userspace_ = cow_creator.userspace_snapshots_enabled; + if ((use_compression || is_snapshot_userspace_) && !device()->IsTestDevice()) { // Terminate stale daemon if any std::unique_ptr snapuserd_client = SnapuserdClient::Connect(kSnapuserdSocket, 10s); @@ -3156,17 +3157,8 @@ Return SnapshotManager::CreateUpdateSnapshots(const DeltaArchiveManifest& manife snapuserd_client_->CloseConnection(); snapuserd_client_ = nullptr; } - } else { - cow_creator.userspace_snapshots_enabled = !IsDmSnapshotTestingEnabled(); - if (cow_creator.userspace_snapshots_enabled) { - LOG(INFO) << "User-space snapshots disabled for testing"; - } else { - LOG(INFO) << "User-space snapshots enabled for testing"; - } } - is_snapshot_userspace_ = cow_creator.userspace_snapshots_enabled; - // If compression is enabled, we need to retain a copy of the old metadata // so we can access original blocks in case they are moved around. We do // not want to rely on the old super metadata slot because we don't diff --git a/fs_mgr/libsnapshot/snapshot_fuzz_utils.h b/fs_mgr/libsnapshot/snapshot_fuzz_utils.h index c1a5af77d..63159dc9d 100644 --- a/fs_mgr/libsnapshot/snapshot_fuzz_utils.h +++ b/fs_mgr/libsnapshot/snapshot_fuzz_utils.h @@ -130,6 +130,7 @@ class SnapshotFuzzDeviceInfo : public ISnapshotManager::IDeviceInfo { std::unique_ptr OpenImageManager() const { return env_->CheckCreateFakeImageManager(); } + bool UseUserspaceSnapshots() const override { return false; } void SwitchSlot() { switched_slot_ = !switched_slot_; } diff --git a/fs_mgr/libsnapshot/test_helpers.cpp b/fs_mgr/libsnapshot/test_helpers.cpp index e3e3af853..2cd13e044 100644 --- a/fs_mgr/libsnapshot/test_helpers.cpp +++ b/fs_mgr/libsnapshot/test_helpers.cpp @@ -25,6 +25,8 @@ #include #include +#include "utility.h" + namespace android { namespace snapshot { @@ -320,5 +322,9 @@ bool IsVirtualAbEnabled() { return android::base::GetBoolProperty("ro.virtual_ab.enabled", false); } +bool TestDeviceInfo::UseUserspaceSnapshots() const { + return !IsDmSnapshotTestingEnabled(); +} + } // namespace snapshot } // namespace android