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
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_);
diff --git a/common/utils.cc b/common/utils.cc
index 15ee05c..f0c045f 100644
--- a/common/utils.cc
+++ b/common/utils.cc
@@ -412,6 +412,17 @@
return true;
}
+bool DeleteDirectory(const char* dirname) {
+ const std::string tmpdir = std::string(dirname) + "_deleted";
+ std::filesystem::remove_all(tmpdir);
+ if (rename(dirname, tmpdir.c_str()) != 0) {
+ PLOG(ERROR) << "Failed to rename " << dirname << " to " << tmpdir;
+ return false;
+ }
+ std::filesystem::remove_all(tmpdir);
+ return true;
+}
+
bool FsyncDirectory(const char* dirname) {
android::base::unique_fd fd(
TEMP_FAILURE_RETRY(open(dirname, O_RDONLY | O_CLOEXEC)));
diff --git a/common/utils.h b/common/utils.h
index 0c8c13f..6bb89f1 100644
--- a/common/utils.h
+++ b/common/utils.h
@@ -162,6 +162,7 @@
bool SendFile(int out_fd, int in_fd, size_t count);
bool FsyncDirectory(const char* dirname);
+bool DeleteDirectory(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,