commit c80d39b63b56fd4638ca5136acb60b720bee38f9
author Kelvin Zhang <zhangkelvin@google.com> Wed Oct 25 09:30:16 2023 -0700
committer Kelvin Zhang <zhangkelvin@google.com> Wed Oct 25 14:08:54 2023 -0700
tree a69839b9c9912755c530929e536d2ac777f71980
parent e53a31872942a949edcc6f667f051a51a405ed08

Improve atomiticy of update checkpointing

Current check point works by writing different prefs to different files
under a pending directory, and rename the pending directory to actual pref
directory afterwards to achieve atomicity. It has two pitfalls:

1. Before the rename() call, existing prefs dir must be rm -rf'ed , this
   deletion process isn't atomic. If device rebooted during rm -rf, we
   will end up with a partially deleted old pref.

2. fsync() on the parent directory is needed after rename()

This CL addresses both issues. For #1, we rename() the old pref dir to a
tmp dir first, and then rm -rf the tmp dir. Upon device restart, if the
current prefs dir is empty, we can simply rename() the pending directory
to actual pref directory.

Test: th
Bug: 295252766
Change-Id: Ic671a18245986c579b51d7443c3e8c10e206c448

common/prefs.cc

diff --git a/common/prefs.cc b/common/prefs.cc
index 57d4dcd..af4d318 100644
--- a/common/prefs.cc
+++ b/common/prefs.cc
@@ -20,6 +20,7 @@
 #include <filesystem>
 #include <unistd.h>
 
+#include <android-base/file.h>
 #include <base/files/file_enumerator.h>
 #include <base/files/file_util.h>
 #include <base/logging.h>
@@ -27,7 +28,6 @@
 #include <base/strings/string_split.h>
 #include <base/strings/string_util.h>
 
-#include "update_engine/common/platform_constants.h"
 #include "update_engine/common/utils.h"
 
 using std::string;
@@ -74,7 +74,16 @@
   if (!GetString(key, &str_value))
     return false;
   base::TrimWhitespaceASCII(str_value, base::TRIM_ALL, &str_value);
-  TEST_AND_RETURN_FALSE(base::StringToInt64(str_value, value));
+  if (str_value.empty()) {
+    LOG(ERROR) << "When reading pref " << key
+               << ", got an empty value after trim";
+    return false;
+  }
+  if (!base::StringToInt64(str_value, value)) {
+    LOG(ERROR) << "When reading pref " << key << ", failed to convert value "
+               << str_value << " to integer";
+    return false;
+  }
   return true;
 }
 
@@ -206,18 +215,30 @@
 }
 
 bool Prefs::FileStorage::SwapPrefs() {
-  if (std::filesystem::exists(prefs_dir_.value())) {
-    std::filesystem::remove_all(prefs_dir_.value());
+  if (!utils::DeleteDirectory(prefs_dir_.value().c_str())) {
+    LOG(ERROR) << "Failed to remove prefs dir " << prefs_dir_;
+    return false;
   }
   if (rename(GetTemporaryDir().c_str(), prefs_dir_.value().c_str()) != 0) {
     LOG(ERROR) << "Error replacing prefs with prefs_tmp" << strerror(errno);
     return false;
   }
+  if (!utils::FsyncDirectory(
+          android::base::Dirname(prefs_dir_.value()).c_str())) {
+    PLOG(ERROR) << "Failed to fsync prefs parent dir after swapping prefs";
+  }
   return true;
 }
 
 bool Prefs::FileStorage::Init(const base::FilePath& prefs_dir) {
   prefs_dir_ = prefs_dir;
+  if (!std::filesystem::exists(prefs_dir_.value())) {
+    LOG(INFO) << "Prefs dir does not exist, possibly due to an interrupted "
+                 "transaction.";
+    if (std::filesystem::exists(GetTemporaryDir())) {
+      SwapPrefs();
+    }
+  }
 
   // Delete empty directories. Ignore errors when deleting empty directories.
   DeleteEmptyDirectories(prefs_dir_);