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/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];
}