From f140d4d5847d497e85297e2299d8e06292934606 Mon Sep 17 00:00:00 2001 From: Rafay Kamran Date: Wed, 5 Jan 2022 10:10:51 +0000 Subject: [PATCH 1/2] Revert "libsnapshot: Fix libsnapshot_fuzzer_test." Revert submission 1935201 Reason for revert: DroidMonitor: Potential culprit for Bug http://b/213259099 - verifying through Forrest before revert submission. This is part of the standard investigation process, and does not mean your CL will be reverted. Reverted Changes: I219f956ba:libsnapshot: Fix libsnapshot_fuzzer_test. I281937e26:libsnapshot: Fix checks for compression to work wi... Change-Id: I33731556761cbdf603424fda94117973d3f21e21 --- 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, 16 insertions(+), 25 deletions(-) diff --git a/fs_mgr/libsnapshot/device_info.cpp b/fs_mgr/libsnapshot/device_info.cpp index 5c1b291c5..a6d96ed82 100644 --- a/fs_mgr/libsnapshot/device_info.cpp +++ b/fs_mgr/libsnapshot/device_info.cpp @@ -19,8 +19,6 @@ #include #include -#include "utility.h" - namespace android { namespace snapshot { @@ -145,9 +143,5 @@ 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 a07f554a8..8aefb8507 100644 --- a/fs_mgr/libsnapshot/device_info.h +++ b/fs_mgr/libsnapshot/device_info.h @@ -41,7 +41,6 @@ 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 8c4161c22..573a85b24 100644 --- a/fs_mgr/libsnapshot/include/libsnapshot/mock_device_info.h +++ b/fs_mgr/libsnapshot/include/libsnapshot/mock_device_info.h @@ -34,7 +34,6 @@ 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 f7e37bbcc..41c6ef576 100644 --- a/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h +++ b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h @@ -111,7 +111,6 @@ 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 07c3ec5b5..c3b40dca4 100644 --- a/fs_mgr/libsnapshot/include_test/libsnapshot/test_helpers.h +++ b/fs_mgr/libsnapshot/include_test/libsnapshot/test_helpers.h @@ -107,7 +107,6 @@ 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 c7c5da1f3..dfa467ee6 100644 --- a/fs_mgr/libsnapshot/snapshot.cpp +++ b/fs_mgr/libsnapshot/snapshot.cpp @@ -3134,15 +3134,14 @@ Return SnapshotManager::CreateUpdateSnapshots(const DeltaArchiveManifest& manife .compression_algorithm = compression_algorithm, }; - 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 (!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"; + } - if ((use_compression || is_snapshot_userspace_) && !device()->IsTestDevice()) { // Terminate stale daemon if any std::unique_ptr snapuserd_client = SnapuserdClient::Connect(kSnapuserdSocket, 10s); @@ -3157,8 +3156,17 @@ 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 63159dc9d..c1a5af77d 100644 --- a/fs_mgr/libsnapshot/snapshot_fuzz_utils.h +++ b/fs_mgr/libsnapshot/snapshot_fuzz_utils.h @@ -130,7 +130,6 @@ 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 2cd13e044..e3e3af853 100644 --- a/fs_mgr/libsnapshot/test_helpers.cpp +++ b/fs_mgr/libsnapshot/test_helpers.cpp @@ -25,8 +25,6 @@ #include #include -#include "utility.h" - namespace android { namespace snapshot { @@ -322,9 +320,5 @@ bool IsVirtualAbEnabled() { return android::base::GetBoolProperty("ro.virtual_ab.enabled", false); } -bool TestDeviceInfo::UseUserspaceSnapshots() const { - return !IsDmSnapshotTestingEnabled(); -} - } // namespace snapshot } // namespace android From 146f07e07b793be75dd328d26d5dcdb197b87b9a Mon Sep 17 00:00:00 2001 From: Rafay Kamran Date: Wed, 5 Jan 2022 10:10:51 +0000 Subject: [PATCH 2/2] Revert "libsnapshot: Fix checks for compression to work with new..." Revert submission 1935201 Reason for revert: DroidMonitor: Potential culprit for Bug http://b/213259099 - verifying through Forrest before revert submission. This is part of the standard investigation process, and does not mean your CL will be reverted. Reverted Changes: I219f956ba:libsnapshot: Fix libsnapshot_fuzzer_test. I281937e26:libsnapshot: Fix checks for compression to work wi... Change-Id: I88955a533ce7103111509785ed5bb40e1779b130 --- 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, 71 insertions(+), 90 deletions(-) diff --git a/fs_mgr/libsnapshot/partition_cow_creator.cpp b/fs_mgr/libsnapshot/partition_cow_creator.cpp index 5fcbdfe11..5569da038 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 || userspace_snapshots_enabled) { + if (compression_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 1f3417778..34b39ca72 100644 --- a/fs_mgr/libsnapshot/partition_cow_creator.h +++ b/fs_mgr/libsnapshot/partition_cow_creator.h @@ -58,7 +58,6 @@ 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 dfa467ee6..e6e17bdf3 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() && !UpdateUsesUserSnapshots(lock)) { + if (!status.compression_enabled()) { // Do not try to verify old-style COWs yet. return MergeFailureCode::Ok; } @@ -2345,15 +2345,13 @@ bool SnapshotManager::MapPartitionWithSnapshot(LockedFile* lock, remaining_time = GetRemainingTime(params.timeout_ms, begin); if (remaining_time.count() < 0) return false; - 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 (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 (UpdateUsesUserSnapshots(lock) || live_snapshot_status->compression_enabled()) { + if (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) { @@ -3134,61 +3132,6 @@ 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; @@ -3210,11 +3153,64 @@ 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); - status.set_userspace_snapshots(cow_creator.userspace_snapshots_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"; + } + // 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(); @@ -3378,8 +3374,6 @@ 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, @@ -3409,7 +3403,7 @@ Return SnapshotManager::InitializeUpdateSnapshots( return Return::Error(); } - if (userspace_merges || it->second.compression_enabled()) { + if (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 " @@ -3455,8 +3449,8 @@ bool SnapshotManager::MapUpdateSnapshot(const CreateLogicalPartitionParams& para if (!ReadSnapshotStatus(lock.get(), params.GetPartitionName(), &status)) { return false; } - if (status.compression_enabled() || UpdateUsesUserSnapshots(lock.get())) { - LOG(ERROR) << "Cannot use MapUpdateSnapshot with user snapshots"; + if (status.compression_enabled()) { + LOG(ERROR) << "Cannot use MapUpdateSnapshot with compressed snapshots"; return false; } @@ -3513,7 +3507,7 @@ std::unique_ptr SnapshotManager::OpenSnapshotWriter( return nullptr; } - if (status.compression_enabled() || UpdateUsesUserSnapshots(lock.get())) { + if (status.compression_enabled()) { return OpenCompressedSnapshotWriter(lock.get(), source_device, params.GetPartitionName(), status, paths); } @@ -3653,7 +3647,6 @@ 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; @@ -3854,10 +3847,6 @@ 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."; @@ -3870,7 +3859,6 @@ bool SnapshotManager::EnsureNoOverflowSnapshot(LockedFile* lock) { return false; } if (status.compression_enabled()) { - // Compressed snapshots are never written through dm-snapshot. continue; } @@ -4034,10 +4022,7 @@ bool SnapshotManager::IsSnapuserdRequired() { if (!lock) return false; auto status = ReadSnapshotUpdateStatus(lock.get()); - if (status.state() != UpdateState::None && status.compression_enabled()) { - return true; - } - return UpdateUsesUserSnapshots(lock.get()); + return status.state() != UpdateState::None && status.compression_enabled(); } bool SnapshotManager::DetachSnapuserdForSelinux(std::vector* snapuserd_argv) { @@ -4063,9 +4048,6 @@ 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 c70b353ed..14f2d45be 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(ShouldUseCompression()); + dynamic_partition_metadata->set_vabc_enabled(IsCompressionEnabled()); 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 (!ShouldUseUserspaceSnapshots()) { + } else if (!IsCompressionEnabled()) { 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 (ShouldUseUserspaceSnapshots()) { + if (ShouldUseCompression()) { std::unique_ptr writer; return MapUpdateSnapshot(name, &writer); } else { @@ -1040,7 +1040,7 @@ class SnapshotUpdateTest : public SnapshotTest { } AssertionResult WriteSnapshotAndHash(const std::string& name) { - if (ShouldUseUserspaceSnapshots()) { + if (ShouldUseCompression()) { 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 (ShouldUseUserspaceSnapshots()) { + if (ShouldUseCompression()) { GTEST_SKIP() << "No overflow bit set for userspace COWs"; }