Finer-grained locking for size operations.
Disk space measurements are read-only and don't perform mutations,
so other installd operations shouldn't block them.
If there's an ongoing parallel operation (such as a dexopt) that
could race and skew the results, that's no different than an actively
running app changing it's disk usage during the measurement.
This change also allows measurements to happen in parallel, so we can
no longer rely on getcwd() being stable, which means all fts(3) users
now need to use FTS_NOCHDIR.
Bug: 36032444, 35706513
Test: runtest -x frameworks/base/services/tests/servicestests/src/com/android/server/pm/InstallerTest.java
Change-Id: I67d303d3ecce148052d41444cef67381b1d34ab0
diff --git a/cmds/installd/CacheItem.cpp b/cmds/installd/CacheItem.cpp
index 6caf149..0349b0f 100644
--- a/cmds/installd/CacheItem.cpp
+++ b/cmds/installd/CacheItem.cpp
@@ -72,7 +72,7 @@
FTS *fts;
FTSENT *p;
char *argv[] = { (char*) path.c_str(), nullptr };
- if (!(fts = fts_open(argv, FTS_PHYSICAL | FTS_XDEV, NULL))) {
+ if (!(fts = fts_open(argv, FTS_PHYSICAL | FTS_NOCHDIR | FTS_XDEV, NULL))) {
PLOG(WARNING) << "Failed to fts_open " << path;
return -1;
}
diff --git a/cmds/installd/CacheTracker.cpp b/cmds/installd/CacheTracker.cpp
index 9377836..fada699 100644
--- a/cmds/installd/CacheTracker.cpp
+++ b/cmds/installd/CacheTracker.cpp
@@ -83,7 +83,7 @@
FTS *fts;
FTSENT *p;
char *argv[] = { (char*) path.c_str(), nullptr };
- if (!(fts = fts_open(argv, FTS_PHYSICAL | FTS_XDEV, NULL))) {
+ if (!(fts = fts_open(argv, FTS_PHYSICAL | FTS_NOCHDIR | FTS_XDEV, NULL))) {
PLOG(WARNING) << "Failed to fts_open " << path;
return;
}
diff --git a/cmds/installd/InstalldNativeService.cpp b/cmds/installd/InstalldNativeService.cpp
index fbb30db..bffbd54 100644
--- a/cmds/installd/InstalldNativeService.cpp
+++ b/cmds/installd/InstalldNativeService.cpp
@@ -203,14 +203,20 @@
out << "installd is happy!" << endl;
- out << endl << "Devices with quota support:" << endl;
- for (const auto& n : mQuotaDevices) {
- out << " " << n.first << " = " << n.second << endl;
+ {
+ std::lock_guard<std::recursive_mutex> lock(mQuotaDevicesLock);
+ out << endl << "Devices with quota support:" << endl;
+ for (const auto& n : mQuotaDevices) {
+ out << " " << n.first << " = " << n.second << endl;
+ }
}
- out << endl << "Per-UID cache quotas:" << endl;
- for (const auto& n : mCacheQuotas) {
- out << " " << n.first << " = " << n.second << endl;
+ {
+ std::lock_guard<std::recursive_mutex> lock(mCacheQuotasLock);
+ out << endl << "Per-UID cache quotas:" << endl;
+ for (const auto& n : mCacheQuotas) {
+ out << " " << n.first << " = " << n.second << endl;
+ }
}
out << endl;
@@ -755,9 +761,6 @@
CHECK_ARGUMENT_UUID(uuid);
std::lock_guard<std::recursive_mutex> lock(mLock);
- // TODO: remove this once framework is more robust
- invalidateMounts();
-
const char* uuid_ = uuid ? uuid->c_str() : nullptr;
auto data_path = create_data_path(uuid_);
auto device = findQuotaDeviceForUuid(uuid);
@@ -789,7 +792,7 @@
(char*) create_data_user_de_path(uuid_, user).c_str(),
nullptr
};
- if (!(fts = fts_open(argv, FTS_PHYSICAL | FTS_XDEV, NULL))) {
+ if (!(fts = fts_open(argv, FTS_PHYSICAL | FTS_NOCHDIR | FTS_XDEV, NULL))) {
return error("Failed to fts_open");
}
while ((p = fts_read(fts)) != NULL) {
@@ -802,7 +805,10 @@
auto tracker = std::shared_ptr<CacheTracker>(new CacheTracker(
multiuser_get_user_id(uid), multiuser_get_app_id(uid), device));
tracker->addDataPath(p->fts_path);
- tracker->cacheQuota = mCacheQuotas[uid];
+ {
+ std::lock_guard<std::recursive_mutex> lock(mCacheQuotasLock);
+ tracker->cacheQuota = mCacheQuotas[uid];
+ }
if (tracker->cacheQuota == 0) {
LOG(WARNING) << "UID " << uid << " has no cache quota; assuming 64MB";
tracker->cacheQuota = 67108864;
@@ -1122,7 +1128,7 @@
FTS *fts;
FTSENT *p;
char *argv[] = { (char*) path.c_str(), nullptr };
- if (!(fts = fts_open(argv, FTS_PHYSICAL | FTS_XDEV, NULL))) {
+ if (!(fts = fts_open(argv, FTS_PHYSICAL | FTS_NOCHDIR | FTS_XDEV, NULL))) {
PLOG(ERROR) << "Failed to fts_open " << path;
return;
}
@@ -1161,7 +1167,8 @@
for (auto packageName : packageNames) {
CHECK_ARGUMENT_PACKAGE_NAME(packageName);
}
- std::lock_guard<std::recursive_mutex> lock(mLock);
+ // NOTE: Locking is relaxed on this method, since it's limited to
+ // read-only measurements without mutation.
// When modifying this logic, always verify using tests:
// runtest -x frameworks/base/services/tests/servicestests/src/com/android/server/pm/InstallerTest.java -m testGetAppSize
@@ -1276,7 +1283,8 @@
std::vector<int64_t>* _aidl_return) {
ENFORCE_UID(AID_SYSTEM);
CHECK_ARGUMENT_UUID(uuid);
- std::lock_guard<std::recursive_mutex> lock(mLock);
+ // NOTE: Locking is relaxed on this method, since it's limited to
+ // read-only measurements without mutation.
// When modifying this logic, always verify using tests:
// runtest -x frameworks/base/services/tests/servicestests/src/com/android/server/pm/InstallerTest.java -m testGetUserSize
@@ -1422,7 +1430,8 @@
int32_t userId, int32_t flags, std::vector<int64_t>* _aidl_return) {
ENFORCE_UID(AID_SYSTEM);
CHECK_ARGUMENT_UUID(uuid);
- std::lock_guard<std::recursive_mutex> lock(mLock);
+ // NOTE: Locking is relaxed on this method, since it's limited to
+ // read-only measurements without mutation.
// When modifying this logic, always verify using tests:
// runtest -x frameworks/base/services/tests/servicestests/src/com/android/server/pm/InstallerTest.java -m testGetExternalSize
@@ -1488,7 +1497,7 @@
FTSENT *p;
auto path = create_data_media_path(uuid_, userId);
char *argv[] = { (char*) path.c_str(), nullptr };
- if (!(fts = fts_open(argv, FTS_PHYSICAL | FTS_XDEV, NULL))) {
+ if (!(fts = fts_open(argv, FTS_PHYSICAL | FTS_NOCHDIR | FTS_XDEV, NULL))) {
return error("Failed to fts_open " + path);
}
while ((p = fts_read(fts)) != NULL) {
@@ -1535,7 +1544,7 @@
int32_t userId, int32_t appId, int64_t cacheQuota) {
ENFORCE_UID(AID_SYSTEM);
CHECK_ARGUMENT_UUID(uuid);
- std::lock_guard<std::recursive_mutex> lock(mLock);
+ std::lock_guard<std::recursive_mutex> lock(mCacheQuotasLock);
int32_t uid = multiuser_get_uid(userId, appId);
mCacheQuotas[uid] = cacheQuota;
@@ -1988,7 +1997,7 @@
binder::Status InstalldNativeService::invalidateMounts() {
ENFORCE_UID(AID_SYSTEM);
- std::lock_guard<std::recursive_mutex> lock(mLock);
+ std::lock_guard<std::recursive_mutex> lock(mQuotaDevicesLock);
mQuotaDevices.clear();
@@ -2019,6 +2028,7 @@
std::string InstalldNativeService::findQuotaDeviceForUuid(
const std::unique_ptr<std::string>& uuid) {
+ std::lock_guard<std::recursive_mutex> lock(mQuotaDevicesLock);
auto path = create_data_path(uuid ? uuid->c_str() : nullptr);
return mQuotaDevices[path];
}
diff --git a/cmds/installd/InstalldNativeService.h b/cmds/installd/InstalldNativeService.h
index feb2219..7ad8687 100644
--- a/cmds/installd/InstalldNativeService.h
+++ b/cmds/installd/InstalldNativeService.h
@@ -117,6 +117,9 @@
private:
std::recursive_mutex mLock;
+ std::recursive_mutex mQuotaDevicesLock;
+ std::recursive_mutex mCacheQuotasLock;
+
/* Map from mount point to underlying device node */
std::unordered_map<std::string, std::string> mQuotaDevices;
/* Map from UID to cache quota size */
diff --git a/cmds/installd/utils.cpp b/cmds/installd/utils.cpp
index af7a7c6..c2dcb36 100644
--- a/cmds/installd/utils.cpp
+++ b/cmds/installd/utils.cpp
@@ -284,7 +284,7 @@
FTSENT *p;
int64_t matchedSize = 0;
char *argv[] = { (char*) path.c_str(), nullptr };
- if (!(fts = fts_open(argv, FTS_PHYSICAL | FTS_XDEV, NULL))) {
+ if (!(fts = fts_open(argv, FTS_PHYSICAL | FTS_NOCHDIR | FTS_XDEV, NULL))) {
if (errno != ENOENT) {
PLOG(ERROR) << "Failed to fts_open " << path;
}
@@ -1440,7 +1440,7 @@
FTS *fts;
FTSENT *p;
char *argv[] = { (char*) path.c_str(), nullptr };
- if (!(fts = fts_open(argv, FTS_PHYSICAL | FTS_XDEV, NULL))) {
+ if (!(fts = fts_open(argv, FTS_PHYSICAL | FTS_NOCHDIR | FTS_XDEV, NULL))) {
PLOG(ERROR) << "Failed to fts_open " << path;
return -1;
}