Fsync data after writing prefs
update_engine is relying on prefs stored on filesystem to resume OTA
progress. Partners report loss of update progress after forced reboot.
This might be I/O cache issue.
Test: th
Bug: 259174530
Change-Id: I7b4b8281cfe2cc45c51e4d6d60e09edb12f3ed86
diff --git a/aosp/cleanup_previous_update_action.cc b/aosp/cleanup_previous_update_action.cc
index fb7bac1..7b0c9bb 100644
--- a/aosp/cleanup_previous_update_action.cc
+++ b/aosp/cleanup_previous_update_action.cc
@@ -379,10 +379,7 @@
}
bool CleanupPreviousUpdateAction::BeforeCancel() {
- if (DeltaPerformer::ResetUpdateProgress(
- prefs_,
- false /* quick */,
- false /* skip dynamic partitions metadata*/)) {
+ if (DeltaPerformer::ResetUpdateProgress(prefs_, false /* quick */)) {
return true;
}
diff --git a/aosp/dynamic_partition_control_android.cc b/aosp/dynamic_partition_control_android.cc
index cb38506..7818086 100644
--- a/aosp/dynamic_partition_control_android.cc
+++ b/aosp/dynamic_partition_control_android.cc
@@ -1314,8 +1314,8 @@
// ResetUpdateProgress may pass but CancelUpdate fails.
// This is expected. A scheduled CleanupPreviousUpdateAction should free
// space when it is done.
- TEST_AND_RETURN_FALSE(DeltaPerformer::ResetUpdateProgress(
- prefs, false /* quick */, false /* skip dynamic partitions metadata */));
+ TEST_AND_RETURN_FALSE(
+ DeltaPerformer::ResetUpdateProgress(prefs, false /* quick */));
if (ExpectMetadataMounted()) {
TEST_AND_RETURN_FALSE(snapshot_->CancelUpdate());
diff --git a/common/prefs.cc b/common/prefs.cc
index f33a8a9..41a7b24 100644
--- a/common/prefs.cc
+++ b/common/prefs.cc
@@ -25,6 +25,7 @@
#include <base/strings/string_split.h>
#include <base/strings/string_util.h>
+#include "brillo/file_utils.h"
#include "update_engine/common/utils.h"
using std::string;
@@ -202,8 +203,8 @@
// to parent directories where we might not have permission to write to.
TEST_AND_RETURN_FALSE(base::CreateDirectory(filename.DirName()));
}
- TEST_AND_RETURN_FALSE(base::WriteFile(filename, value.data(), value.size()) ==
- static_cast<int>(value.size()));
+ TEST_AND_RETURN_FALSE(
+ utils::WriteStringToFileAtomic(filename.value(), value));
return true;
}
diff --git a/common/utils.cc b/common/utils.cc
index e3ffa1c..6b8bc54 100644
--- a/common/utils.cc
+++ b/common/utils.cc
@@ -35,6 +35,7 @@
#include <unistd.h>
#include <algorithm>
+#include <filesystem>
#include <utility>
#include <vector>
@@ -409,6 +410,53 @@
return true;
}
+bool FsyncDirectory(const char* dirname) {
+ android::base::unique_fd fd(
+ TEMP_FAILURE_RETRY(open(dirname, O_RDONLY | O_CLOEXEC)));
+ if (fd == -1) {
+ PLOG(ERROR) << "Failed to open " << dirname;
+ return false;
+ }
+ if (fsync(fd) == -1) {
+ if (errno == EROFS || errno == EINVAL) {
+ PLOG(WARNING) << "Skip fsync " << dirname
+ << " on a file system does not support synchronization";
+ } else {
+ PLOG(ERROR) << "Failed to fsync " << dirname;
+ return false;
+ }
+ }
+ return true;
+}
+
+bool WriteStringToFileAtomic(const std::string& path,
+ std::string_view content) {
+ const std::string tmp_path = path + ".tmp";
+ {
+ const int flags = O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC;
+ android::base::unique_fd fd(
+ TEMP_FAILURE_RETRY(open(tmp_path.c_str(), flags, 0644)));
+ if (fd == -1) {
+ PLOG(ERROR) << "Failed to open " << path;
+ return false;
+ }
+ if (!WriteAll(fd.get(), content.data(), content.size())) {
+ PLOG(ERROR) << "Failed to write to fd " << fd;
+ return false;
+ }
+ // rename() without fsync() is not safe. Data could still be living on page
+ // cache. To ensure atomiticity, call fsync()
+ if (fsync(fd) != 0) {
+ PLOG(ERROR) << "Failed to fsync " << tmp_path;
+ }
+ }
+ if (rename(tmp_path.c_str(), path.c_str()) == -1) {
+ PLOG(ERROR) << "rename failed from " << tmp_path << " to " << path;
+ return false;
+ }
+ return FsyncDirectory(std::filesystem::path(path).parent_path().c_str());
+}
+
void HexDumpArray(const uint8_t* const arr, const size_t length) {
LOG(INFO) << "Logging array of length: " << length;
const unsigned int bytes_per_line = 16;
diff --git a/common/utils.h b/common/utils.h
index 201e47e..0087794 100644
--- a/common/utils.h
+++ b/common/utils.h
@@ -162,6 +162,9 @@
bool SendFile(int out_fd, int in_fd, size_t count);
+bool FsyncDirectory(const char* dirname);
+bool WriteStringToFileAtomic(const std::string& path, std::string_view content);
+
// Returns true if the file exists for sure. Returns false if it doesn't exist,
// or an error occurs.
bool FileExists(const char* path);
diff --git a/payload_consumer/delta_performer.cc b/payload_consumer/delta_performer.cc
index 7a1272a..5754cae 100644
--- a/payload_consumer/delta_performer.cc
+++ b/payload_consumer/delta_performer.cc
@@ -1320,10 +1320,7 @@
return true;
}
-bool DeltaPerformer::ResetUpdateProgress(
- PrefsInterface* prefs,
- bool quick,
- bool skip_dynamic_partititon_metadata_updated) {
+bool DeltaPerformer::ResetUpdateProgress(PrefsInterface* prefs, bool quick) {
TEST_AND_RETURN_FALSE(prefs->SetInt64(kPrefsUpdateStateNextOperation,
kUpdateStateOperationInvalid));
if (!quick) {
@@ -1338,10 +1335,8 @@
prefs->Delete(kPrefsPostInstallSucceeded);
prefs->Delete(kPrefsVerityWritten);
- if (!skip_dynamic_partititon_metadata_updated) {
- LOG(INFO) << "Resetting recorded hash for prepared partitions.";
- prefs->Delete(kPrefsDynamicPartitionMetadataUpdated);
- }
+ LOG(INFO) << "Resetting recorded hash for prepared partitions.";
+ prefs->Delete(kPrefsDynamicPartitionMetadataUpdated);
}
return true;
}
@@ -1414,6 +1409,7 @@
}
TEST_AND_RETURN_FALSE(
prefs_->SetInt64(kPrefsUpdateStateNextOperation, next_operation_num_));
+ LOG(INFO) << "Update progress successfully checkpointed.";
return true;
}
diff --git a/payload_consumer/delta_performer.h b/payload_consumer/delta_performer.h
index 633c533..7365519 100644
--- a/payload_consumer/delta_performer.h
+++ b/payload_consumer/delta_performer.h
@@ -147,10 +147,7 @@
// If |skip_dynamic_partititon_metadata_updated| is true, do not reset
// dynamic-partition-metadata-updated.
// Returns true on success, false otherwise.
- static bool ResetUpdateProgress(
- PrefsInterface* prefs,
- bool quick,
- bool skip_dynamic_partititon_metadata_updated = false);
+ static bool ResetUpdateProgress(PrefsInterface* prefs, bool quick);
// Attempts to parse the update metadata starting from the beginning of
// |payload|. On success, returns kMetadataParseSuccess. Returns