From 65ceecfec66ad57f241243806dedfd4d2a5238ae Mon Sep 17 00:00:00 2001 From: Alessio Balsini Date: Wed, 26 Feb 2020 21:49:25 +0000 Subject: [PATCH 01/13] libsnapshot/test: Re-enable the failing tests Some tests were temporarily disabled due to errors in Cuttlefish. Now that the issues were fixed, the tests can be put back in presubmit. Bug: 148889015 Test: vts_libsnapshot_test (Cuttlefish + Pixel 4) Signed-off-by: Alessio Balsini Change-Id: Idcb2b7831b2183b2d734bdf8821fd34746f2beee Merged-In: Idcb2b7831b2183b2d734bdf8821fd34746f2beee --- fs_mgr/TEST_MAPPING | 2 +- fs_mgr/libsnapshot/Android.bp | 8 -------- fs_mgr/libsnapshot/snapshot_test.cpp | 18 ------------------ 3 files changed, 1 insertion(+), 27 deletions(-) diff --git a/fs_mgr/TEST_MAPPING b/fs_mgr/TEST_MAPPING index 705d4e313..676f446e7 100644 --- a/fs_mgr/TEST_MAPPING +++ b/fs_mgr/TEST_MAPPING @@ -13,7 +13,7 @@ "name": "fiemap_writer_test" }, { - "name": "vts_libsnapshot_test_presubmit" + "name": "vts_libsnapshot_test" } ] } diff --git a/fs_mgr/libsnapshot/Android.bp b/fs_mgr/libsnapshot/Android.bp index 2d6261731..6c739fc19 100644 --- a/fs_mgr/libsnapshot/Android.bp +++ b/fs_mgr/libsnapshot/Android.bp @@ -208,14 +208,6 @@ cc_test { defaults: ["libsnapshot_test_defaults"], } -cc_test { - name: "vts_libsnapshot_test_presubmit", - defaults: ["libsnapshot_test_defaults"], - cppflags: [ - "-DSKIP_TEST_IN_PRESUBMIT", - ], -} - cc_binary { name: "snapshotctl", srcs: [ diff --git a/fs_mgr/libsnapshot/snapshot_test.cpp b/fs_mgr/libsnapshot/snapshot_test.cpp index 5d2840f99..0ffaa71ba 100644 --- a/fs_mgr/libsnapshot/snapshot_test.cpp +++ b/fs_mgr/libsnapshot/snapshot_test.cpp @@ -506,9 +506,6 @@ TEST_F(SnapshotTest, Merge) { } TEST_F(SnapshotTest, FirstStageMountAndMerge) { -#ifdef SKIP_TEST_IN_PRESUBMIT - GTEST_SKIP() << "WIP failure b/148889015"; -#endif ASSERT_TRUE(AcquireLock()); static const uint64_t kDeviceSize = 1024 * 1024; @@ -565,9 +562,6 @@ TEST_F(SnapshotTest, FlashSuperDuringUpdate) { } TEST_F(SnapshotTest, FlashSuperDuringMerge) { -#ifdef SKIP_TEST_IN_PRESUBMIT - GTEST_SKIP() << "WIP failure b/148889015"; -#endif ASSERT_TRUE(AcquireLock()); static const uint64_t kDeviceSize = 1024 * 1024; @@ -979,9 +973,6 @@ class SnapshotUpdateTest : public SnapshotTest { // Also test UnmapUpdateSnapshot unmaps everything. // Also test first stage mount and merge after this. TEST_F(SnapshotUpdateTest, FullUpdateFlow) { -#ifdef SKIP_TEST_IN_PRESUBMIT - GTEST_SKIP() << "WIP failure b/148889015"; -#endif // 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)); @@ -1129,9 +1120,6 @@ TEST_F(SnapshotUpdateTest, SnapshotStatusFileWithoutCow) { // Test that the old partitions are not modified. TEST_F(SnapshotUpdateTest, TestRollback) { -#ifdef SKIP_TEST_IN_PRESUBMIT - GTEST_SKIP() << "WIP failure b/148889015"; -#endif // Execute the update. ASSERT_TRUE(sm->BeginUpdate()); ASSERT_TRUE(sm->UnmapUpdateSnapshot("sys_b")); @@ -1309,9 +1297,6 @@ TEST_F(SnapshotUpdateTest, RetrofitAfterRegularAb) { } TEST_F(SnapshotUpdateTest, MergeCannotRemoveCow) { -#ifdef SKIP_TEST_IN_PRESUBMIT - GTEST_SKIP() << "WIP failure b/148889015"; -#endif // Make source partitions as big as possible to force COW image to be created. SetSize(sys_, 5_MiB); SetSize(vnd_, 5_MiB); @@ -1586,9 +1571,6 @@ TEST_F(SnapshotUpdateTest, Overflow) { } TEST_F(SnapshotUpdateTest, WaitForMerge) { -#ifdef SKIP_TEST_IN_PRESUBMIT - GTEST_SKIP() << "WIP failure b/148889015"; -#endif AddOperationForPartitions(); // Execute the update. From c6411c2d37b2451948c5376e05ed60f347836648 Mon Sep 17 00:00:00 2001 From: Alessio Balsini Date: Tue, 21 Jan 2020 18:38:34 +0000 Subject: [PATCH 02/13] snapshotctl: init reports merge statistics Enable the --report flag in init rc script to collect and send snapshot merge statistics after OTA. Bug: 138817833 Test: statsd_testdrive Change-Id: Ie32a2c6d7d1671ca2b1846c6a8d33cea2ab22a4c Signed-off-by: Alessio Balsini Merged-In: Ie32a2c6d7d1671ca2b1846c6a8d33cea2ab22a4c --- fs_mgr/libsnapshot/snapshotctl.rc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs_mgr/libsnapshot/snapshotctl.rc b/fs_mgr/libsnapshot/snapshotctl.rc index 5dbe35255..ccb2c410b 100644 --- a/fs_mgr/libsnapshot/snapshotctl.rc +++ b/fs_mgr/libsnapshot/snapshotctl.rc @@ -1,2 +1,2 @@ on property:sys.boot_completed=1 - exec_background - root root -- /system/bin/snapshotctl merge --logcat --log-to-file + exec_background - root root -- /system/bin/snapshotctl merge --logcat --log-to-file --report From 91ffe19c4e3c6b36d98fdcad44e0dd7666c9dea9 Mon Sep 17 00:00:00 2001 From: Yifan Hong Date: Tue, 18 Feb 2020 13:47:26 -0800 Subject: [PATCH 03/13] snapshotctl don't auto-merge. update_engine is now responsible for initiating the merge. snapshotctl becomes a debugging tool for libsnapshot now. Bug: 147696014 Test: libsnapshot_test Change-Id: Ia2527a35e0c0f0789dbe5c477e174663ef406903 Merged-In: Ia2527a35e0c0f0789dbe5c477e174663ef406903 --- CleanSpec.mk | 1 + fs_mgr/libsnapshot/Android.bp | 3 --- fs_mgr/libsnapshot/snapshotctl.cpp | 1 + fs_mgr/libsnapshot/snapshotctl.rc | 2 -- 4 files changed, 2 insertions(+), 5 deletions(-) delete mode 100644 fs_mgr/libsnapshot/snapshotctl.rc diff --git a/CleanSpec.mk b/CleanSpec.mk index c84bd24c4..0a534a2bd 100644 --- a/CleanSpec.mk +++ b/CleanSpec.mk @@ -90,3 +90,4 @@ $(call add-clean-step, rm -rf $(PRODUCT_OUT)/recovery/root/product_services) $(call add-clean-step, rm -rf $(PRODUCT_OUT)/debug_ramdisk/product_services) $(call add-clean-step, find $(PRODUCT_OUT) -type l -name "charger" -print0 | xargs -0 rm -f) $(call add-clean-step, rm -f $(PRODUCT_OUT)/system/bin/adbd) +$(call add-clean-step, rm -rf $(PRODUCT_OUT)/system/etc/init/snapshotctl.rc) diff --git a/fs_mgr/libsnapshot/Android.bp b/fs_mgr/libsnapshot/Android.bp index 6c739fc19..0a0a21ddb 100644 --- a/fs_mgr/libsnapshot/Android.bp +++ b/fs_mgr/libsnapshot/Android.bp @@ -235,7 +235,4 @@ cc_binary { // TODO(b/148818798): remove when parent bug is fixed. "libutilscallstack", ], - init_rc: [ - "snapshotctl.rc", - ], } diff --git a/fs_mgr/libsnapshot/snapshotctl.cpp b/fs_mgr/libsnapshot/snapshotctl.cpp index 34d3d69d1..4670eeea0 100644 --- a/fs_mgr/libsnapshot/snapshotctl.cpp +++ b/fs_mgr/libsnapshot/snapshotctl.cpp @@ -178,6 +178,7 @@ bool MergeCmdHandler(int argc, char** argv) { } LOG(ERROR) << "Snapshot failed to merge with state \"" << state << "\"."; + return false; } diff --git a/fs_mgr/libsnapshot/snapshotctl.rc b/fs_mgr/libsnapshot/snapshotctl.rc deleted file mode 100644 index ccb2c410b..000000000 --- a/fs_mgr/libsnapshot/snapshotctl.rc +++ /dev/null @@ -1,2 +0,0 @@ -on property:sys.boot_completed=1 - exec_background - root root -- /system/bin/snapshotctl merge --logcat --log-to-file --report From 83fe0a3572bd4f9385a34e2e6b02de5fbbf2eeab Mon Sep 17 00:00:00 2001 From: Yifan Hong Date: Tue, 18 Feb 2020 15:04:28 -0800 Subject: [PATCH 04/13] libsnapshot: Add prolog to RemoveAllUpdateStates. Add an optional prolog arg (function) that is invoked before snapshots are deleted and update state set to none. This allows update_engine to delete markers before deleting snapshots to avoid depending on the erroneous markers. Otherwise, if update_engine delete markers after libsnapshot deletes update states, the device could technically get into a state where update_engine thinks the update has been applied, but snapshots are gone. Bug: 147696014 Test: libsnapshot_test Change-Id: I71bfc04a81ea4f94b3072558be50d2f80565113e Merged-In: I71bfc04a81ea4f94b3072558be50d2f80565113e --- .../include/libsnapshot/snapshot.h | 14 ++++---- fs_mgr/libsnapshot/snapshot.cpp | 35 ++++++++++++------- 2 files changed, 30 insertions(+), 19 deletions(-) diff --git a/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h index b440c7190..e2699f289 100644 --- a/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h +++ b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h @@ -169,7 +169,8 @@ class SnapshotManager final { // // The optional callback allows the caller to periodically check the // progress with GetUpdateState(). - UpdateState ProcessUpdateState(const std::function& callback = {}); + UpdateState ProcessUpdateState(const std::function& callback = {}, + const std::function& before_cancel = {}); public: // Initiate the merge if necessary, then wait for the merge to finish. @@ -179,7 +180,8 @@ class SnapshotManager final { // - Unverified if called on the source slot // - MergeCompleted if merge is completed // - other states indicating an error has occurred - UpdateState InitiateMergeAndWait(SnapshotMergeReport* report = nullptr); + UpdateState InitiateMergeAndWait(SnapshotMergeReport* report = nullptr, + const std::function& before_cancel = {}); // Wait for the merge if rebooted into the new slot. Does NOT initiate a // merge. If the merge has not been initiated (but should be), wait. @@ -375,14 +377,14 @@ class SnapshotManager final { // Check for a cancelled or rolled back merge, returning true if such a // condition was detected and handled. - bool HandleCancelledUpdate(LockedFile* lock); + bool HandleCancelledUpdate(LockedFile* lock, const std::function& before_cancel); // Helper for HandleCancelledUpdate. Assumes booting from new slot. bool AreAllSnapshotsCancelled(LockedFile* lock); // Remove artifacts created by the update process, such as snapshots, and // set the update state to None. - bool RemoveAllUpdateState(LockedFile* lock); + bool RemoveAllUpdateState(LockedFile* lock, const std::function& prolog = {}); // Interact with /metadata/ota. std::unique_ptr OpenLock(int lock_flags); @@ -437,8 +439,8 @@ class SnapshotManager final { // UpdateState::MergeCompleted // UpdateState::MergeFailed // UpdateState::MergeNeedsReboot - UpdateState CheckMergeState(); - UpdateState CheckMergeState(LockedFile* lock); + UpdateState CheckMergeState(const std::function& before_cancel); + UpdateState CheckMergeState(LockedFile* lock, const std::function& before_cancel); UpdateState CheckTargetMergeState(LockedFile* lock, const std::string& name); // Interact with status files under /metadata/ota/snapshots. diff --git a/fs_mgr/libsnapshot/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp index 2fe06fb22..b3aa9f0d5 100644 --- a/fs_mgr/libsnapshot/snapshot.cpp +++ b/fs_mgr/libsnapshot/snapshot.cpp @@ -219,7 +219,12 @@ static bool RemoveFileIfExists(const std::string& path) { return true; } -bool SnapshotManager::RemoveAllUpdateState(LockedFile* lock) { +bool SnapshotManager::RemoveAllUpdateState(LockedFile* lock, const std::function& prolog) { + if (prolog && !prolog()) { + LOG(WARNING) << "Can't RemoveAllUpdateState: prolog failed."; + return false; + } + LOG(INFO) << "Removing all update state."; #ifdef LIBSNAPSHOT_USE_CALLSTACK @@ -789,9 +794,10 @@ bool SnapshotManager::QuerySnapshotStatus(const std::string& dm_name, std::strin // Note that when a merge fails, we will *always* try again to complete the // merge each time the device boots. There is no harm in doing so, and if // the problem was transient, we might manage to get a new outcome. -UpdateState SnapshotManager::ProcessUpdateState(const std::function& callback) { +UpdateState SnapshotManager::ProcessUpdateState(const std::function& callback, + const std::function& before_cancel) { while (true) { - UpdateState state = CheckMergeState(); + UpdateState state = CheckMergeState(before_cancel); if (state == UpdateState::MergeFailed) { AcknowledgeMergeFailure(); } @@ -811,24 +817,25 @@ UpdateState SnapshotManager::ProcessUpdateState(const std::function& cal } } -UpdateState SnapshotManager::CheckMergeState() { +UpdateState SnapshotManager::CheckMergeState(const std::function& before_cancel) { auto lock = LockExclusive(); if (!lock) { return UpdateState::MergeFailed; } - UpdateState state = CheckMergeState(lock.get()); + UpdateState state = CheckMergeState(lock.get(), before_cancel); if (state == UpdateState::MergeCompleted) { // Do this inside the same lock. Failures get acknowledged without the // lock, because flock() might have failed. AcknowledgeMergeSuccess(lock.get()); } else if (state == UpdateState::Cancelled) { - RemoveAllUpdateState(lock.get()); + RemoveAllUpdateState(lock.get(), before_cancel); } return state; } -UpdateState SnapshotManager::CheckMergeState(LockedFile* lock) { +UpdateState SnapshotManager::CheckMergeState(LockedFile* lock, + const std::function& before_cancel) { UpdateState state = ReadUpdateState(lock); switch (state) { case UpdateState::None: @@ -849,7 +856,7 @@ UpdateState SnapshotManager::CheckMergeState(LockedFile* lock) { // This is an edge case. Normally cancelled updates are detected // via the merge poll below, but if we never started a merge, we // need to also check here. - if (HandleCancelledUpdate(lock)) { + if (HandleCancelledUpdate(lock, before_cancel)) { return UpdateState::Cancelled; } return state; @@ -1169,7 +1176,8 @@ bool SnapshotManager::CollapseSnapshotDevice(const std::string& name, return true; } -bool SnapshotManager::HandleCancelledUpdate(LockedFile* lock) { +bool SnapshotManager::HandleCancelledUpdate(LockedFile* lock, + const std::function& before_cancel) { auto slot = GetCurrentSlot(); if (slot == Slot::Unknown) { return false; @@ -1177,7 +1185,7 @@ bool SnapshotManager::HandleCancelledUpdate(LockedFile* lock) { // If all snapshots were reflashed, then cancel the entire update. if (AreAllSnapshotsCancelled(lock)) { - RemoveAllUpdateState(lock); + RemoveAllUpdateState(lock, before_cancel); return true; } @@ -2388,7 +2396,8 @@ std::unique_ptr SnapshotManager::EnsureMetadataMounted() { return AutoUnmountDevice::New(device_->GetMetadataDir()); } -UpdateState SnapshotManager::InitiateMergeAndWait(SnapshotMergeReport* stats_report) { +UpdateState SnapshotManager::InitiateMergeAndWait(SnapshotMergeReport* stats_report, + const std::function& before_cancel) { { auto lock = LockExclusive(); // Sync update state from file with bootloader. @@ -2413,7 +2422,7 @@ UpdateState SnapshotManager::InitiateMergeAndWait(SnapshotMergeReport* stats_rep LOG(INFO) << "Waiting for any previous merge request to complete. " << "This can take up to several minutes."; merge_stats.Resume(); - auto state = ProcessUpdateState(callback); + auto state = ProcessUpdateState(callback, before_cancel); merge_stats.set_state(state); if (state == UpdateState::None) { LOG(INFO) << "Can't find any snapshot to merge."; @@ -2436,7 +2445,7 @@ UpdateState SnapshotManager::InitiateMergeAndWait(SnapshotMergeReport* stats_rep // All other states can be handled by ProcessUpdateState. LOG(INFO) << "Waiting for merge to complete. This can take up to several minutes."; last_progress = 0; - state = ProcessUpdateState(callback); + state = ProcessUpdateState(callback, before_cancel); merge_stats.set_state(state); } From f6d4e74bc60736c22e452983ba5a4ca0a74ca150 Mon Sep 17 00:00:00 2001 From: Yifan Hong Date: Wed, 19 Feb 2020 17:45:49 -0800 Subject: [PATCH 05/13] libsnapshot: Re-expose InitiateMerge and ProcessUpdateState These functions are used directly in update_engine now that it is responsible for initiating the merge. update_engine has its own message loop, so WaitForMerge / InitiateMergeAndWait becomes useless and are deprecated. Bug: 147696014 Test: compiles Change-Id: Id8cb14f5f18b4893e2fc3d0d2b374a867a220e6f Merged-In: Id8cb14f5f18b4893e2fc3d0d2b374a867a220e6f --- fs_mgr/libsnapshot/include/libsnapshot/snapshot.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h index e2699f289..c8cbb1e7a 100644 --- a/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h +++ b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h @@ -140,7 +140,6 @@ class SnapshotManager final { // Before calling this function, all snapshots must be mapped. bool FinishedSnapshotWrites(); - private: // Initiate a merge on all snapshot devices. This should only be used after an // update has been marked successful after booting. bool InitiateMerge(); @@ -172,7 +171,6 @@ class SnapshotManager final { UpdateState ProcessUpdateState(const std::function& callback = {}, const std::function& before_cancel = {}); - public: // Initiate the merge if necessary, then wait for the merge to finish. // See InitiateMerge() and ProcessUpdateState() for details. // Returns: From ef9e44ae78b4e65a2ced2e3cf0617ce1280fda25 Mon Sep 17 00:00:00 2001 From: Yifan Hong Date: Wed, 19 Feb 2020 13:14:05 -0800 Subject: [PATCH 06/13] Allow ProcessUpdateState to be paused. Change callback type of ProcessUpdateState to bool() so that, when callback returns false, immediately returns Merging. Test: libsnapshot_test Bug: 147696014 Change-Id: I9dcb8e1658b95216c0f1991e2e4c1ea2e7e7b2e5 Merged-In: I9dcb8e1658b95216c0f1991e2e4c1ea2e7e7b2e5 --- fs_mgr/libsnapshot/include/libsnapshot/snapshot.h | 8 ++++++-- fs_mgr/libsnapshot/snapshot.cpp | 14 +++++++++----- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h index c8cbb1e7a..72f1d91de 100644 --- a/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h +++ b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h @@ -148,7 +148,11 @@ class SnapshotManager final { // /data is mounted. // // If a merge is in progress, this function will block until the merge is - // completed. If a merge or update was cancelled, this will clean up any + // completed. + // - Callback is called periodically during the merge. If callback() + // returns false during the merge, ProcessUpdateState() will pause + // and returns Merging. + // If a merge or update was cancelled, this will clean up any // update artifacts and return. // // Note that after calling this, GetUpdateState() may still return that a @@ -168,7 +172,7 @@ class SnapshotManager final { // // The optional callback allows the caller to periodically check the // progress with GetUpdateState(). - UpdateState ProcessUpdateState(const std::function& callback = {}, + UpdateState ProcessUpdateState(const std::function& callback = {}, const std::function& before_cancel = {}); // Initiate the merge if necessary, then wait for the merge to finish. diff --git a/fs_mgr/libsnapshot/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp index b3aa9f0d5..7a4f3dcad 100644 --- a/fs_mgr/libsnapshot/snapshot.cpp +++ b/fs_mgr/libsnapshot/snapshot.cpp @@ -794,7 +794,7 @@ bool SnapshotManager::QuerySnapshotStatus(const std::string& dm_name, std::strin // Note that when a merge fails, we will *always* try again to complete the // merge each time the device boots. There is no harm in doing so, and if // the problem was transient, we might manage to get a new outcome. -UpdateState SnapshotManager::ProcessUpdateState(const std::function& callback, +UpdateState SnapshotManager::ProcessUpdateState(const std::function& callback, const std::function& before_cancel) { while (true) { UpdateState state = CheckMergeState(before_cancel); @@ -807,8 +807,8 @@ UpdateState SnapshotManager::ProcessUpdateState(const std::function& cal return state; } - if (callback) { - callback(); + if (callback && !callback()) { + return state; } // This wait is not super time sensitive, so we have a relatively @@ -2410,13 +2410,14 @@ UpdateState SnapshotManager::InitiateMergeAndWait(SnapshotMergeReport* stats_rep SnapshotMergeStats merge_stats(*this); unsigned int last_progress = 0; - auto callback = [&]() -> void { + auto callback = [&]() -> bool { double progress; GetUpdateState(&progress); if (last_progress < static_cast(progress)) { last_progress = progress; LOG(INFO) << "Waiting for merge to complete: " << last_progress << "%."; } + return true; // continue }; LOG(INFO) << "Waiting for any previous merge request to complete. " @@ -2512,7 +2513,10 @@ bool SnapshotManager::HandleImminentDataWipe(const std::function& callba return false; } - UpdateState state = ProcessUpdateState(callback); + UpdateState state = ProcessUpdateState([&]() -> bool { + callback(); + return true; + }); LOG(INFO) << "Update state in recovery: " << state; switch (state) { case UpdateState::MergeFailed: From 1d32aa1a376eb7be0ac294357f4075429a1da305 Mon Sep 17 00:00:00 2001 From: Yifan Hong Date: Thu, 20 Feb 2020 17:09:55 -0800 Subject: [PATCH 07/13] libsnapshot: NeedSnapshotsInFirstStageMount don't test for IsRecovery Init skips first stage mount entirely in recovery, so the check is useless. Also, __ANDROID_RECOVERY__ is always set for first stage init. Also add logs if rollback indicator is set. Fixes: 149956852 Test: apply OTA, manually rollback, reboot, see rollback indicator Change-Id: Iac967baf4b51ea5ce9f6eb962a33cb7ee5819c1d Merged-In: Iac967baf4b51ea5ce9f6eb962a33cb7ee5819c1d --- fs_mgr/libsnapshot/snapshot.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/fs_mgr/libsnapshot/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp index 7a4f3dcad..152ea9c3e 100644 --- a/fs_mgr/libsnapshot/snapshot.cpp +++ b/fs_mgr/libsnapshot/snapshot.cpp @@ -1387,12 +1387,14 @@ bool SnapshotManager::NeedSnapshotsInFirstStageMount() { auto slot = GetCurrentSlot(); if (slot != Slot::Target) { - if (slot == Slot::Source && !device_->IsRecovery()) { + if (slot == Slot::Source) { // Device is rebooting into the original slot, so mark this as a // rollback. auto path = GetRollbackIndicatorPath(); if (!android::base::WriteStringToFile("1", path)) { PLOG(ERROR) << "Unable to write rollback indicator: " << path; + } else { + LOG(INFO) << "Rollback detected, writing rollback indicator to " << path; } } LOG(INFO) << "Not booting from new slot. Will not mount snapshots."; From ae2558d900d5eafe2a511f8debe72a8564f5f966 Mon Sep 17 00:00:00 2001 From: Yifan Hong Date: Tue, 18 Feb 2020 14:19:14 -0800 Subject: [PATCH 08/13] libsnapshot: RemoveUpdateState on rollback. If rollback is detected in ProcessUpdateState, call RemoveUpdateState and return UpdateState::Cancelled. Now that update_engine is reponsible for initiating the merge, it can react to this state and clean up markers appropriately. Test: libsnapshot_test Test: apply OTA, manually rollback (by setting the active slot), then inspect /metadata/ota as well as /data/misc/update_engine/prefs. Bug: 147696014 Change-Id: Ibfee11fb50e4f4fb7c6cf02b4921b35e77b8f5a5 Merged-In: Ibfee11fb50e4f4fb7c6cf02b4921b35e77b8f5a5 --- fs_mgr/libsnapshot/snapshot.cpp | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/fs_mgr/libsnapshot/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp index 152ea9c3e..c51c977db 100644 --- a/fs_mgr/libsnapshot/snapshot.cpp +++ b/fs_mgr/libsnapshot/snapshot.cpp @@ -1185,15 +1185,32 @@ bool SnapshotManager::HandleCancelledUpdate(LockedFile* lock, // If all snapshots were reflashed, then cancel the entire update. if (AreAllSnapshotsCancelled(lock)) { + LOG(WARNING) << "Detected re-flashing, cancelling unverified update."; RemoveAllUpdateState(lock, before_cancel); return true; } - // This unverified update might be rolled back, or it might not (b/147347110 - // comment #77). Take no action, as update_engine is responsible for deciding - // whether to cancel. - LOG(ERROR) << "Update state is being processed before reboot, taking no action."; - return false; + // If update has been rolled back, then cancel the entire update. + // Client (update_engine) is responsible for doing additional cleanup work on its own states + // when ProcessUpdateState() returns UpdateState::Cancelled. + auto current_slot = GetCurrentSlot(); + if (current_slot != Slot::Source) { + LOG(INFO) << "Update state is being processed while booting at " << current_slot + << " slot, taking no action."; + return false; + } + + // current_slot == Source. Attempt to detect rollbacks. + if (access(GetRollbackIndicatorPath().c_str(), F_OK) != 0) { + // This unverified update is not attempted. Take no action. + PLOG(INFO) << "Rollback indicator not detected. " + << "Update state is being processed before reboot, taking no action."; + return false; + } + + LOG(WARNING) << "Detected rollback, cancelling unverified update."; + RemoveAllUpdateState(lock, before_cancel); + return true; } std::unique_ptr SnapshotManager::ReadCurrentMetadata() { From 7c5ae0a12ef22f9dba005da9db13a8f37945f1a9 Mon Sep 17 00:00:00 2001 From: Yifan Hong Date: Thu, 20 Feb 2020 19:51:05 -0800 Subject: [PATCH 09/13] libsnapshot: remove snapshots properly after flashing If updated then immediately flashed target slot at unverified stage, update_engine attempts to call RemoveAllSnapshots, but it used to fail because unmapping fails. In reality, these devices are dm-linear targets, so unmapping them is optional. - Introduce a GetSnapshotFlashingStatus function that expose more information than AreAllSnapshotsCancelled. We can use the returned map to determine whether a partition is flashed or not. - Introduce ShouldUnmapDeleteSnapshot which determines whether unmapping / deleteting a snapshot is needed in RemoveAllSnapshots. Test: apply OTA, flash target slot, then inspect logs Bug: 147696014 Change-Id: I0853d1e213566af2a3401e46fac7d9586cee7aaf Merged-In: I0853d1e213566af2a3401e46fac7d9586cee7aaf --- .../include/libsnapshot/snapshot.h | 14 +++ fs_mgr/libsnapshot/snapshot.cpp | 95 +++++++++++++++++-- 2 files changed, 99 insertions(+), 10 deletions(-) diff --git a/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h index 72f1d91de..1c9b9e525 100644 --- a/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h +++ b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h @@ -384,6 +384,15 @@ class SnapshotManager final { // Helper for HandleCancelledUpdate. Assumes booting from new slot. bool AreAllSnapshotsCancelled(LockedFile* lock); + // Determine whether partition names in |snapshots| have been flashed and + // store result to |out|. + // Return true if values are successfully retrieved and false on error + // (e.g. super partition metadata cannot be read). When it returns true, + // |out| stores true for partitions that have been flashed and false for + // partitions that have not been flashed. + bool GetSnapshotFlashingStatus(LockedFile* lock, const std::vector& snapshots, + std::map* out); + // Remove artifacts created by the update process, such as snapshots, and // set the update state to None. bool RemoveAllUpdateState(LockedFile* lock, const std::function& prolog = {}); @@ -517,6 +526,11 @@ class SnapshotManager final { std::string ReadUpdateSourceSlotSuffix(); + // 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); + std::string gsid_dir_; std::string metadata_dir_; std::unique_ptr device_; diff --git a/fs_mgr/libsnapshot/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp index c51c977db..e13fc879f 100644 --- a/fs_mgr/libsnapshot/snapshot.cpp +++ b/fs_mgr/libsnapshot/snapshot.cpp @@ -1244,6 +1244,28 @@ bool SnapshotManager::AreAllSnapshotsCancelled(LockedFile* lock) { return true; } + std::map flashing_status; + + if (!GetSnapshotFlashingStatus(lock, snapshots, &flashing_status)) { + LOG(WARNING) << "Failed to determine whether partitions have been flashed. Not" + << "removing update states."; + return false; + } + + bool all_snapshots_cancelled = std::all_of(flashing_status.begin(), flashing_status.end(), + [](const auto& pair) { return pair.second; }); + + if (all_snapshots_cancelled) { + LOG(WARNING) << "All partitions are re-flashed after update, removing all update states."; + } + return all_snapshots_cancelled; +} + +bool SnapshotManager::GetSnapshotFlashingStatus(LockedFile* lock, + const std::vector& snapshots, + std::map* out) { + CHECK(lock); + auto source_slot_suffix = ReadUpdateSourceSlotSuffix(); if (source_slot_suffix.empty()) { return false; @@ -1269,20 +1291,17 @@ bool SnapshotManager::AreAllSnapshotsCancelled(LockedFile* lock) { return false; } - bool all_snapshots_cancelled = true; for (const auto& snapshot_name : snapshots) { if (GetMetadataPartitionState(*metadata, snapshot_name) == MetadataPartitionState::Updated) { - all_snapshots_cancelled = false; - continue; + out->emplace(snapshot_name, false); + } else { + // Delete snapshots for partitions that are re-flashed after the update. + LOG(WARNING) << "Detected re-flashing of partition " << snapshot_name << "."; + out->emplace(snapshot_name, true); } - // Delete snapshots for partitions that are re-flashed after the update. - LOG(WARNING) << "Detected re-flashing of partition " << snapshot_name << "."; } - if (all_snapshots_cancelled) { - LOG(WARNING) << "All partitions are re-flashed after update, removing all update states."; - } - return all_snapshots_cancelled; + return true; } bool SnapshotManager::RemoveAllSnapshots(LockedFile* lock) { @@ -1292,10 +1311,38 @@ bool SnapshotManager::RemoveAllSnapshots(LockedFile* lock) { return false; } + std::map flashing_status; + if (!GetSnapshotFlashingStatus(lock, snapshots, &flashing_status)) { + LOG(WARNING) << "Failed to get flashing status"; + } + + auto current_slot = GetCurrentSlot(); bool ok = true; bool has_mapped_cow_images = false; for (const auto& name : snapshots) { - if (!UnmapPartitionWithSnapshot(lock, name) || !DeleteSnapshot(lock, name)) { + // If booting off source slot, it is okay to unmap and delete all the snapshots. + // If boot indicator is missing, update state is None or Initiated, so + // it is also okay to unmap and delete all the snapshots. + // If booting off target slot, + // - should not unmap because: + // - In Android mode, snapshots are not mapped, but + // filesystems are mounting off dm-linear targets directly. + // - In recovery mode, assume nothing is mapped, so it is optional to unmap. + // - If partition is flashed or unknown, it is okay to delete snapshots. + // 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 partition_ok = true; + if (should_unmap && !UnmapPartitionWithSnapshot(lock, name)) { + partition_ok = false; + } + if (partition_ok && should_delete && !DeleteSnapshot(lock, name)) { + partition_ok = false; + } + + if (!partition_ok) { // Remember whether or not we were able to unmap the cow image. auto cow_image_device = GetCowImageDeviceName(name); has_mapped_cow_images |= @@ -1318,6 +1365,34 @@ bool SnapshotManager::RemoveAllSnapshots(LockedFile* lock) { return ok; } +// See comments in RemoveAllSnapshots(). +bool SnapshotManager::ShouldDeleteSnapshot(LockedFile* lock, + const std::map& flashing_status, + Slot current_slot, const std::string& name) { + if (current_slot != Slot::Target) { + return true; + } + auto it = flashing_status.find(name); + if (it == flashing_status.end()) { + LOG(WARNING) << "Can't determine flashing status for " << name; + return true; + } + if (it->second) { + // 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); +} + UpdateState SnapshotManager::GetUpdateState(double* progress) { // If we've never started an update, the state file won't exist. auto state_file = GetStateFilePath(); From d5675f672870600249ef518a91eeb895cbcba600 Mon Sep 17 00:00:00 2001 From: Yifan Hong Date: Thu, 20 Feb 2020 20:06:51 -0800 Subject: [PATCH 10/13] libsnapshot: handle errors in RemoveAllUpdateState appropriately. In CheckMergeState / HandleCancelledUpdate, if removing update state fails, just return the previous state. It used to return Cancelled, and the error goes unnoticed. Test: pass Bug: 147696014 Change-Id: I9cb3d20c5c886afa1913740c903eaad08f0cc041 Merged-In: I9cb3d20c5c886afa1913740c903eaad08f0cc041 --- fs_mgr/libsnapshot/snapshot.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/fs_mgr/libsnapshot/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp index e13fc879f..f1a90c59e 100644 --- a/fs_mgr/libsnapshot/snapshot.cpp +++ b/fs_mgr/libsnapshot/snapshot.cpp @@ -829,7 +829,9 @@ UpdateState SnapshotManager::CheckMergeState(const std::function& before // lock, because flock() might have failed. AcknowledgeMergeSuccess(lock.get()); } else if (state == UpdateState::Cancelled) { - RemoveAllUpdateState(lock.get(), before_cancel); + if (!RemoveAllUpdateState(lock.get(), before_cancel)) { + return ReadSnapshotUpdateStatus(lock.get()).state(); + } } return state; } @@ -1186,8 +1188,7 @@ bool SnapshotManager::HandleCancelledUpdate(LockedFile* lock, // If all snapshots were reflashed, then cancel the entire update. if (AreAllSnapshotsCancelled(lock)) { LOG(WARNING) << "Detected re-flashing, cancelling unverified update."; - RemoveAllUpdateState(lock, before_cancel); - return true; + return RemoveAllUpdateState(lock, before_cancel); } // If update has been rolled back, then cancel the entire update. @@ -1209,8 +1210,7 @@ bool SnapshotManager::HandleCancelledUpdate(LockedFile* lock, } LOG(WARNING) << "Detected rollback, cancelling unverified update."; - RemoveAllUpdateState(lock, before_cancel); - return true; + return RemoveAllUpdateState(lock, before_cancel); } std::unique_ptr SnapshotManager::ReadCurrentMetadata() { From 3634255e66d61f522f88ef0ad3b06c644f6a72e4 Mon Sep 17 00:00:00 2001 From: Yifan Hong Date: Wed, 19 Feb 2020 20:46:37 -0800 Subject: [PATCH 11/13] libsnapshot: delete WaitForMerge. Now that update_engine is responsible for initiating the merge, WaitForMerge function becomes useless. Bug: 147696014 Test: compiles Change-Id: I3779bdf9dd8d1716238e21d09acf0499e3fc90a3 Merged-In: I3779bdf9dd8d1716238e21d09acf0499e3fc90a3 --- .../libsnapshot/include/libsnapshot/return.h | 2 - .../include/libsnapshot/snapshot.h | 9 ----- fs_mgr/libsnapshot/return.cpp | 2 - fs_mgr/libsnapshot/snapshot.cpp | 26 ------------- fs_mgr/libsnapshot/snapshot_test.cpp | 39 ------------------- 5 files changed, 78 deletions(-) diff --git a/fs_mgr/libsnapshot/include/libsnapshot/return.h b/fs_mgr/libsnapshot/include/libsnapshot/return.h index 1f132fa84..dedc44501 100644 --- a/fs_mgr/libsnapshot/include/libsnapshot/return.h +++ b/fs_mgr/libsnapshot/include/libsnapshot/return.h @@ -30,7 +30,6 @@ class Return { enum class ErrorCode : int32_t { SUCCESS = static_cast(FiemapStatus::ErrorCode::SUCCESS), ERROR = static_cast(FiemapStatus::ErrorCode::ERROR), - NEEDS_REBOOT = ERROR + 1, NO_SPACE = static_cast(FiemapStatus::ErrorCode::NO_SPACE), }; ErrorCode error_code() const { return error_code_; } @@ -43,7 +42,6 @@ class Return { static Return Ok() { return Return(ErrorCode::SUCCESS); } static Return Error() { return Return(ErrorCode::ERROR); } static Return NoSpace(uint64_t size) { return Return(ErrorCode::NO_SPACE, size); } - static Return NeedsReboot() { return Return(ErrorCode::NEEDS_REBOOT); } // Does not set required_size_ properly even when status.error_code() == NO_SPACE. explicit Return(const FiemapStatus& status) : error_code_(FromFiemapStatusErrorCode(status.error_code())), required_size_(0) {} diff --git a/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h index 1c9b9e525..32345d26f 100644 --- a/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h +++ b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h @@ -185,15 +185,6 @@ class SnapshotManager final { UpdateState InitiateMergeAndWait(SnapshotMergeReport* report = nullptr, const std::function& before_cancel = {}); - // Wait for the merge if rebooted into the new slot. Does NOT initiate a - // merge. If the merge has not been initiated (but should be), wait. - // Returns: - // - Return::Ok(): there is no merge or merge finishes - // - Return::NeedsReboot(): merge finishes but need a reboot before - // applying the next update. - // - Return::Error(): other irrecoverable errors - Return WaitForMerge(); - // Find the status of the current update, if any. // // |progress| depends on the returned status: diff --git a/fs_mgr/libsnapshot/return.cpp b/fs_mgr/libsnapshot/return.cpp index 6559c12f1..cc64af5cb 100644 --- a/fs_mgr/libsnapshot/return.cpp +++ b/fs_mgr/libsnapshot/return.cpp @@ -24,8 +24,6 @@ std::string Return::string() const { switch (error_code()) { case ErrorCode::ERROR: return "Error"; - case ErrorCode::NEEDS_REBOOT: - return "Retry after reboot"; case ErrorCode::SUCCESS: [[fallthrough]]; case ErrorCode::NO_SPACE: diff --git a/fs_mgr/libsnapshot/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp index f1a90c59e..30993794e 100644 --- a/fs_mgr/libsnapshot/snapshot.cpp +++ b/fs_mgr/libsnapshot/snapshot.cpp @@ -2551,32 +2551,6 @@ UpdateState SnapshotManager::InitiateMergeAndWait(SnapshotMergeReport* stats_rep return state; } -Return SnapshotManager::WaitForMerge() { - LOG(INFO) << "Waiting for any previous merge request to complete. " - << "This can take up to several minutes."; - while (true) { - auto state = ProcessUpdateState(); - if (state == UpdateState::Unverified && GetCurrentSlot() == Slot::Target) { - LOG(INFO) << "Wait for merge to be initiated."; - std::this_thread::sleep_for(kUpdateStateCheckInterval); - continue; - } - LOG(INFO) << "Wait for merge exits with state " << state; - switch (state) { - case UpdateState::None: - [[fallthrough]]; - case UpdateState::MergeCompleted: - [[fallthrough]]; - case UpdateState::Cancelled: - return Return::Ok(); - case UpdateState::MergeNeedsReboot: - return Return::NeedsReboot(); - default: - return Return::Error(); - } - } -} - bool SnapshotManager::HandleImminentDataWipe(const std::function& callback) { if (!device_->IsRecovery()) { LOG(ERROR) << "Data wipes are only allowed in recovery."; diff --git a/fs_mgr/libsnapshot/snapshot_test.cpp b/fs_mgr/libsnapshot/snapshot_test.cpp index 0ffaa71ba..7d16ec280 100644 --- a/fs_mgr/libsnapshot/snapshot_test.cpp +++ b/fs_mgr/libsnapshot/snapshot_test.cpp @@ -1570,45 +1570,6 @@ TEST_F(SnapshotUpdateTest, Overflow) { << "FinishedSnapshotWrites should detect overflow of CoW device."; } -TEST_F(SnapshotUpdateTest, WaitForMerge) { - AddOperationForPartitions(); - - // 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)); - } - - ASSERT_TRUE(sm->FinishedSnapshotWrites()); - - // Simulate shutting down the device. - ASSERT_TRUE(UnmapAll()); - - // After reboot, init does first stage mount. - { - auto init = SnapshotManager::NewForFirstStageMount(new TestDeviceInfo(fake_super, "_b")); - ASSERT_NE(nullptr, init); - ASSERT_TRUE(init->CreateLogicalAndSnapshotPartitions("super", snapshot_timeout_)); - } - - auto new_sm = SnapshotManager::New(new TestDeviceInfo(fake_super, "_b")); - ASSERT_NE(nullptr, new_sm); - - auto waiter = std::async(std::launch::async, [&new_sm] { return new_sm->WaitForMerge(); }); - ASSERT_EQ(std::future_status::timeout, waiter.wait_for(1s)) - << "WaitForMerge should block when not initiated"; - - auto merger = - std::async(std::launch::async, [&new_sm] { return new_sm->InitiateMergeAndWait(); }); - // Small images, so should be merged pretty quickly. - ASSERT_EQ(std::future_status::ready, waiter.wait_for(3s)) << "WaitForMerge did not finish"; - ASSERT_TRUE(waiter.get()); - ASSERT_THAT(merger.get(), AnyOf(UpdateState::None, UpdateState::MergeCompleted)); -} - TEST_F(SnapshotUpdateTest, LowSpace) { static constexpr auto kMaxFree = 10_MiB; auto userdata = std::make_unique(); From 2cea7249a95cd02e27252e6938a249b59e93fe2f Mon Sep 17 00:00:00 2001 From: Yifan Hong Date: Tue, 25 Feb 2020 15:15:23 -0800 Subject: [PATCH 12/13] libsnapshot: Expose SnapshotMergeStats Test: builds Bug: 147696014 Change-Id: Ia59a3f9226628558bdd1fdb0966812c2f652190c Merged-In: Ia59a3f9226628558bdd1fdb0966812c2f652190c --- fs_mgr/libsnapshot/{ => include/libsnapshot}/snapshot_stats.h | 0 fs_mgr/libsnapshot/snapshot.cpp | 2 +- fs_mgr/libsnapshot/snapshot_stats.cpp | 2 +- 3 files changed, 2 insertions(+), 2 deletions(-) rename fs_mgr/libsnapshot/{ => include/libsnapshot}/snapshot_stats.h (100%) diff --git a/fs_mgr/libsnapshot/snapshot_stats.h b/fs_mgr/libsnapshot/include/libsnapshot/snapshot_stats.h similarity index 100% rename from fs_mgr/libsnapshot/snapshot_stats.h rename to fs_mgr/libsnapshot/include/libsnapshot/snapshot_stats.h diff --git a/fs_mgr/libsnapshot/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp index 30993794e..896857f4a 100644 --- a/fs_mgr/libsnapshot/snapshot.cpp +++ b/fs_mgr/libsnapshot/snapshot.cpp @@ -43,10 +43,10 @@ #endif #include +#include #include "device_info.h" #include "partition_cow_creator.h" #include "snapshot_metadata_updater.h" -#include "snapshot_stats.h" #include "utility.h" namespace android { diff --git a/fs_mgr/libsnapshot/snapshot_stats.cpp b/fs_mgr/libsnapshot/snapshot_stats.cpp index 635b47d80..f4ebae81e 100644 --- a/fs_mgr/libsnapshot/snapshot_stats.cpp +++ b/fs_mgr/libsnapshot/snapshot_stats.cpp @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "snapshot_stats.h" +#include #include From e53129819d1761cd62d5a07f6f8d25f7378e5871 Mon Sep 17 00:00:00 2001 From: Yifan Hong Date: Tue, 25 Feb 2020 14:47:38 -0800 Subject: [PATCH 13/13] libsnapshot: SnapshotMergeStats::Start/Finish Change API for SnapshotMergeStats so that it is easier for update_engine to use. Test: apply OTA and look at stats Bug: 147696014 Bug: 138817833 Change-Id: Ie4036ac7382102c00f0761f443d78e00b9e585d5 Merged-In: Ie4036ac7382102c00f0761f443d78e00b9e585d5 --- .../include/libsnapshot/snapshot_stats.h | 30 +++++-- fs_mgr/libsnapshot/snapshot.cpp | 19 +++-- fs_mgr/libsnapshot/snapshot_stats.cpp | 82 +++++++++++++------ fs_mgr/libsnapshot/snapshotctl.cpp | 2 +- 4 files changed, 91 insertions(+), 42 deletions(-) diff --git a/fs_mgr/libsnapshot/include/libsnapshot/snapshot_stats.h b/fs_mgr/libsnapshot/include/libsnapshot/snapshot_stats.h index 60109a4d1..91dd34f80 100644 --- a/fs_mgr/libsnapshot/include/libsnapshot/snapshot_stats.h +++ b/fs_mgr/libsnapshot/include/libsnapshot/snapshot_stats.h @@ -15,6 +15,7 @@ #pragma once #include +#include #include #include @@ -24,21 +25,34 @@ namespace snapshot { class SnapshotMergeStats { public: - SnapshotMergeStats(SnapshotManager& parent); - ~SnapshotMergeStats(); - void Start(); - void Resume(); + // Not thread safe. + static SnapshotMergeStats* GetInstance(SnapshotManager& manager); + + // Called when merge starts or resumes. + bool Start(); void set_state(android::snapshot::UpdateState state); - SnapshotMergeReport GetReport(); + + // Called when merge ends. Properly clean up permanent storage. + class Result { + public: + virtual ~Result() {} + virtual const SnapshotMergeReport& report() const = 0; + // Time between successful Start() / Resume() to Finish(). + virtual std::chrono::steady_clock::duration merge_time() const = 0; + }; + std::unique_ptr Finish(); private: bool ReadState(); bool WriteState(); + bool DeleteState(); + SnapshotMergeStats(const std::string& path); - const SnapshotManager& parent_; + std::string path_; SnapshotMergeReport report_; - std::chrono::time_point init_time_; - std::chrono::time_point end_time_; + // Time of the last successful Start() / Resume() call. + std::chrono::time_point start_time_; + bool running_{false}; }; } // namespace snapshot diff --git a/fs_mgr/libsnapshot/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp index 896857f4a..61fc2dff6 100644 --- a/fs_mgr/libsnapshot/snapshot.cpp +++ b/fs_mgr/libsnapshot/snapshot.cpp @@ -2501,7 +2501,7 @@ UpdateState SnapshotManager::InitiateMergeAndWait(SnapshotMergeReport* stats_rep } } - SnapshotMergeStats merge_stats(*this); + auto merge_stats = SnapshotMergeStats::GetInstance(*this); unsigned int last_progress = 0; auto callback = [&]() -> bool { @@ -2516,9 +2516,9 @@ UpdateState SnapshotManager::InitiateMergeAndWait(SnapshotMergeReport* stats_rep LOG(INFO) << "Waiting for any previous merge request to complete. " << "This can take up to several minutes."; - merge_stats.Resume(); + merge_stats->Start(); auto state = ProcessUpdateState(callback, before_cancel); - merge_stats.set_state(state); + merge_stats->set_state(state); if (state == UpdateState::None) { LOG(INFO) << "Can't find any snapshot to merge."; return state; @@ -2529,10 +2529,6 @@ UpdateState SnapshotManager::InitiateMergeAndWait(SnapshotMergeReport* stats_rep return state; } - // This is the first snapshot merge that is requested after OTA. We can - // initialize the merge duration statistics. - merge_stats.Start(); - if (!InitiateMerge()) { LOG(ERROR) << "Failed to initiate merge."; return state; @@ -2541,12 +2537,17 @@ UpdateState SnapshotManager::InitiateMergeAndWait(SnapshotMergeReport* stats_rep LOG(INFO) << "Waiting for merge to complete. This can take up to several minutes."; last_progress = 0; state = ProcessUpdateState(callback, before_cancel); - merge_stats.set_state(state); + merge_stats->set_state(state); } LOG(INFO) << "Merge finished with state \"" << state << "\"."; if (stats_report) { - *stats_report = merge_stats.GetReport(); + auto result = merge_stats->Finish(); + if (result) { + *stats_report = result->report(); + } else { + LOG(WARNING) << "SnapshotMergeStatus::Finish failed."; + } } return state; } diff --git a/fs_mgr/libsnapshot/snapshot_stats.cpp b/fs_mgr/libsnapshot/snapshot_stats.cpp index f4ebae81e..5da7b9873 100644 --- a/fs_mgr/libsnapshot/snapshot_stats.cpp +++ b/fs_mgr/libsnapshot/snapshot_stats.cpp @@ -23,22 +23,17 @@ namespace android { namespace snapshot { -SnapshotMergeStats::SnapshotMergeStats(SnapshotManager& parent) : parent_(parent) { - init_time_ = std::chrono::steady_clock::now(); +SnapshotMergeStats* SnapshotMergeStats::GetInstance(SnapshotManager& parent) { + static SnapshotMergeStats g_instance(parent.GetMergeStateFilePath()); + CHECK(g_instance.path_ == parent.GetMergeStateFilePath()); + return &g_instance; } -SnapshotMergeStats::~SnapshotMergeStats() { - std::string error; - auto file_path = parent_.GetMergeStateFilePath(); - if (!android::base::RemoveFileIfExists(file_path, &error)) { - LOG(ERROR) << "Failed to remove merge statistics file " << file_path << ": " << error; - return; - } -} +SnapshotMergeStats::SnapshotMergeStats(const std::string& path) : path_(path), running_(false) {} bool SnapshotMergeStats::ReadState() { std::string contents; - if (!android::base::ReadFileToString(parent_.GetMergeStateFilePath(), &contents)) { + if (!android::base::ReadFileToString(path_, &contents)) { PLOG(INFO) << "Read merge statistics file failed"; return false; } @@ -55,34 +50,73 @@ bool SnapshotMergeStats::WriteState() { LOG(ERROR) << "Unable to serialize SnapshotMergeStats."; return false; } - auto file_path = parent_.GetMergeStateFilePath(); - if (!WriteStringToFileAtomic(contents, file_path)) { + if (!WriteStringToFileAtomic(contents, path_)) { PLOG(ERROR) << "Could not write to merge statistics file"; return false; } return true; } -void SnapshotMergeStats::Start() { - report_.set_resume_count(0); - report_.set_state(UpdateState::None); - WriteState(); +bool SnapshotMergeStats::DeleteState() { + std::string error; + if (!android::base::RemoveFileIfExists(path_, &error)) { + LOG(ERROR) << "Failed to remove merge statistics file " << path_ << ": " << error; + return false; + } + return true; } -void SnapshotMergeStats::Resume() { - if (!ReadState()) { - return; +bool SnapshotMergeStats::Start() { + if (running_) { + LOG(ERROR) << "SnapshotMergeStats running_ == " << running_; + return false; } - report_.set_resume_count(report_.resume_count() + 1); - WriteState(); + running_ = true; + + start_time_ = std::chrono::steady_clock::now(); + if (ReadState()) { + report_.set_resume_count(report_.resume_count() + 1); + } else { + report_.set_resume_count(0); + report_.set_state(UpdateState::None); + } + + return WriteState(); } void SnapshotMergeStats::set_state(android::snapshot::UpdateState state) { report_.set_state(state); } -SnapshotMergeReport SnapshotMergeStats::GetReport() { - return report_; +class SnapshotMergeStatsResultImpl : public SnapshotMergeStats::Result { + public: + SnapshotMergeStatsResultImpl(const SnapshotMergeReport& report, + std::chrono::steady_clock::duration merge_time) + : report_(report), merge_time_(merge_time) {} + const SnapshotMergeReport& report() const override { return report_; } + std::chrono::steady_clock::duration merge_time() const override { return merge_time_; } + + private: + SnapshotMergeReport report_; + std::chrono::steady_clock::duration merge_time_; +}; + +std::unique_ptr SnapshotMergeStats::Finish() { + if (!running_) { + LOG(ERROR) << "SnapshotMergeStats running_ == " << running_; + return nullptr; + } + running_ = false; + + auto result = std::make_unique( + report_, std::chrono::steady_clock::now() - start_time_); + + // We still want to report result if state is not deleted. Just leave + // it there and move on. A side effect is that it may be reported over and + // over again in the future, but there is nothing we can do. + (void)DeleteState(); + + return result; } } // namespace snapshot diff --git a/fs_mgr/libsnapshot/snapshotctl.cpp b/fs_mgr/libsnapshot/snapshotctl.cpp index 4670eeea0..aa5e9c1fa 100644 --- a/fs_mgr/libsnapshot/snapshotctl.cpp +++ b/fs_mgr/libsnapshot/snapshotctl.cpp @@ -26,9 +26,9 @@ #include #include #include +#include #include -#include "snapshot_stats.h" #include "utility.h" using namespace std::string_literals;