Unbind from DataLoader when not needed anymore.
+ simplify adding new callbacks on storage state
+ streamline lock story for ifs members
Bug: 183101753
Fixes: 183101753
Test: atest PackageManagerShellCommandTest PackageManagerShellCommandIncrementalTest IncrementalServiceTest PackageManagerServiceTest ChecksumsTest
Change-Id: I86fffa7101eeb42ebccca67ae7f5d133c1ab9dfa
diff --git a/services/incremental/IncrementalService.cpp b/services/incremental/IncrementalService.cpp
index a88f2b4..932e997 100644
--- a/services/incremental/IncrementalService.cpp
+++ b/services/incremental/IncrementalService.cpp
@@ -377,6 +377,7 @@
dprintf(fd, "Mounts (%d): {\n", int(mMounts.size()));
for (auto&& [id, ifs] : mMounts) {
+ std::unique_lock ll(ifs->lock);
const IncFsMount& mnt = *ifs;
dprintf(fd, " [%d]: {\n", id);
if (id != mnt.mountId) {
@@ -422,6 +423,9 @@
}
bool IncrementalService::needStartDataLoaderLocked(IncFsMount& ifs) {
+ if (!ifs.dataLoaderStub) {
+ return false;
+ }
if (ifs.dataLoaderStub->isSystemDataLoader()) {
return true;
}
@@ -439,6 +443,8 @@
std::lock_guard l(mLock);
mounts.reserve(mMounts.size());
for (auto&& [id, ifs] : mMounts) {
+ std::unique_lock ll(ifs->lock);
+
if (ifs->mountId != id) {
continue;
}
@@ -456,7 +462,10 @@
std::thread([this, mounts = std::move(mounts)]() {
mJni->initializeForCurrentThread();
for (auto&& ifs : mounts) {
- ifs->dataLoaderStub->requestStart();
+ std::unique_lock l(ifs->lock);
+ if (ifs->dataLoaderStub) {
+ ifs->dataLoaderStub->requestStart();
+ }
}
}).detach();
}
@@ -671,23 +680,36 @@
setUidReadTimeouts(storageId, std::move(perUidReadTimeouts));
}
- // Re-initialize DataLoader.
- std::unique_lock l(mLock);
- const auto ifs = getIfsLocked(storageId);
- if (!ifs) {
- return false;
- }
- if (ifs->dataLoaderStub) {
- ifs->dataLoaderStub->cleanupResources();
- ifs->dataLoaderStub = {};
- }
- l.unlock();
+ IfsMountPtr ifs;
+ DataLoaderStubPtr dataLoaderStub;
- // DataLoader.
- auto dataLoaderStub =
- prepareDataLoader(*ifs, std::move(dataLoaderParams), std::move(statusListener),
- healthCheckParams, std::move(healthListener));
- CHECK(dataLoaderStub);
+ // Re-initialize DataLoader.
+ {
+ ifs = getIfs(storageId);
+ if (!ifs) {
+ return false;
+ }
+
+ std::unique_lock l(ifs->lock);
+ dataLoaderStub = std::exchange(ifs->dataLoaderStub, nullptr);
+ }
+
+ if (dataLoaderStub) {
+ dataLoaderStub->cleanupResources();
+ dataLoaderStub = {};
+ }
+
+ {
+ std::unique_lock l(ifs->lock);
+ if (ifs->dataLoaderStub) {
+ LOG(INFO) << "Skipped data loader stub creation because it already exists";
+ return false;
+ }
+ prepareDataLoaderLocked(*ifs, std::move(dataLoaderParams), std::move(statusListener),
+ healthCheckParams, std::move(healthListener));
+ CHECK(ifs->dataLoaderStub);
+ dataLoaderStub = ifs->dataLoaderStub;
+ }
if (dataLoaderStub->isSystemDataLoader()) {
// Readlogs from system dataloader (adb) can always be collected.
@@ -705,13 +727,14 @@
<< storageId;
return;
}
+ std::unique_lock l(ifs->lock);
if (ifs->startLoadingTs != startLoadingTs) {
LOG(INFO) << "Can't disable the readlogs, timestamp mismatch (new "
"installation?): "
<< storageId;
return;
}
- setStorageParams(*ifs, storageId, /*enableReadLogs=*/false);
+ disableReadLogsLocked(*ifs);
});
}
@@ -733,17 +756,17 @@
}
void IncrementalService::disallowReadLogs(StorageId storageId) {
- std::unique_lock l(mLock);
- const auto ifs = getIfsLocked(storageId);
+ const auto ifs = getIfs(storageId);
if (!ifs) {
LOG(ERROR) << "disallowReadLogs failed, invalid storageId: " << storageId;
return;
}
+
+ std::unique_lock l(ifs->lock);
if (!ifs->readLogsAllowed()) {
return;
}
ifs->disallowReadLogs();
- l.unlock();
const auto metadata = constants().readLogsDisabledMarkerName;
if (auto err = mIncFs->makeFile(ifs->control,
@@ -755,7 +778,7 @@
return;
}
- setStorageParams(storageId, /*enableReadLogs=*/false);
+ disableReadLogsLocked(*ifs);
}
int IncrementalService::setStorageParams(StorageId storageId, bool enableReadLogs) {
@@ -764,61 +787,66 @@
LOG(ERROR) << "setStorageParams failed, invalid storageId: " << storageId;
return -EINVAL;
}
- return setStorageParams(*ifs, storageId, enableReadLogs);
-}
-int IncrementalService::setStorageParams(IncFsMount& ifs, StorageId storageId,
- bool enableReadLogs) {
- const auto& params = ifs.dataLoaderStub->params();
- if (enableReadLogs) {
- if (!ifs.readLogsAllowed()) {
- LOG(ERROR) << "setStorageParams failed, readlogs disallowed for storageId: "
- << storageId;
- return -EPERM;
- }
-
- // Check loader usage stats permission and apop.
- if (auto status = mAppOpsManager->checkPermission(kLoaderUsageStats, kOpUsage,
- params.packageName.c_str());
- !status.isOk()) {
- LOG(ERROR) << " Permission: " << kLoaderUsageStats
- << " check failed: " << status.toString8();
- return fromBinderStatus(status);
- }
-
- // Check multiuser permission.
- if (auto status = mAppOpsManager->checkPermission(kInteractAcrossUsers, nullptr,
- params.packageName.c_str());
- !status.isOk()) {
- LOG(ERROR) << " Permission: " << kInteractAcrossUsers
- << " check failed: " << status.toString8();
- return fromBinderStatus(status);
- }
-
- // Check installation time.
- const auto now = mClock->now();
- const auto startLoadingTs = ifs.startLoadingTs;
- if (startLoadingTs <= now && now - startLoadingTs > Constants::readLogsMaxInterval) {
- LOG(ERROR) << "setStorageParams failed, readlogs can't be enabled at this time, "
- "storageId: "
- << storageId;
- return -EPERM;
- }
+ std::unique_lock l(ifs->lock);
+ if (!enableReadLogs) {
+ return disableReadLogsLocked(*ifs);
}
- if (auto status = applyStorageParams(ifs, enableReadLogs); !status.isOk()) {
- LOG(ERROR) << "applyStorageParams failed: " << status.toString8();
+ if (!ifs->readLogsAllowed()) {
+ LOG(ERROR) << "enableReadLogs failed, readlogs disallowed for storageId: " << storageId;
+ return -EPERM;
+ }
+
+ if (!ifs->dataLoaderStub) {
+ // This should never happen - only DL can call enableReadLogs.
+ LOG(ERROR) << "enableReadLogs failed: invalid state";
+ return -EPERM;
+ }
+
+ // Check installation time.
+ const auto now = mClock->now();
+ const auto startLoadingTs = ifs->startLoadingTs;
+ if (startLoadingTs <= now && now - startLoadingTs > Constants::readLogsMaxInterval) {
+ LOG(ERROR) << "enableReadLogs failed, readlogs can't be enabled at this time, storageId: "
+ << storageId;
+ return -EPERM;
+ }
+
+ const auto& packageName = ifs->dataLoaderStub->params().packageName;
+
+ // Check loader usage stats permission and apop.
+ if (auto status =
+ mAppOpsManager->checkPermission(kLoaderUsageStats, kOpUsage, packageName.c_str());
+ !status.isOk()) {
+ LOG(ERROR) << " Permission: " << kLoaderUsageStats
+ << " check failed: " << status.toString8();
return fromBinderStatus(status);
}
- if (enableReadLogs) {
- registerAppOpsCallback(params.packageName);
+ // Check multiuser permission.
+ if (auto status =
+ mAppOpsManager->checkPermission(kInteractAcrossUsers, nullptr, packageName.c_str());
+ !status.isOk()) {
+ LOG(ERROR) << " Permission: " << kInteractAcrossUsers
+ << " check failed: " << status.toString8();
+ return fromBinderStatus(status);
}
+ if (auto status = applyStorageParamsLocked(*ifs, /*enableReadLogs=*/true); status != 0) {
+ return status;
+ }
+
+ registerAppOpsCallback(packageName);
+
return 0;
}
-binder::Status IncrementalService::applyStorageParams(IncFsMount& ifs, bool enableReadLogs) {
+int IncrementalService::disableReadLogsLocked(IncFsMount& ifs) {
+ return applyStorageParamsLocked(ifs, /*enableReadLogs=*/false);
+}
+
+int IncrementalService::applyStorageParamsLocked(IncFsMount& ifs, bool enableReadLogs) {
os::incremental::IncrementalFileSystemControlParcel control;
control.cmd.reset(dup(ifs.control.cmd()));
control.pendingReads.reset(dup(ifs.control.pendingReads()));
@@ -832,8 +860,10 @@
if (status.isOk()) {
// Store enabled state.
ifs.setReadLogsEnabled(enableReadLogs);
+ } else {
+ LOG(ERROR) << "applyStorageParams failed: " << status.toString8();
}
- return status;
+ return status.isOk() ? 0 : fromBinderStatus(status);
}
void IncrementalService::deleteStorage(StorageId storageId) {
@@ -1224,9 +1254,14 @@
return;
}
- const auto timeout = std::chrono::duration_cast<milliseconds>(maxPendingTimeUs) -
- Constants::perUidTimeoutOffset;
- updateUidReadTimeouts(storage, Clock::now() + timeout);
+ const auto timeout = Clock::now() + maxPendingTimeUs - Constants::perUidTimeoutOffset;
+ addIfsStateCallback(storage, [this, timeout](StorageId storageId, IfsState state) -> bool {
+ if (checkUidReadTimeouts(storageId, state, timeout)) {
+ return true;
+ }
+ clearUidReadTimeouts(storageId);
+ return false;
+ });
}
void IncrementalService::clearUidReadTimeouts(StorageId storage) {
@@ -1234,39 +1269,32 @@
if (!ifs) {
return;
}
-
mIncFs->setUidReadTimeouts(ifs->control, {});
}
-void IncrementalService::updateUidReadTimeouts(StorageId storage, Clock::time_point timeLimit) {
- // Reached maximum timeout.
+bool IncrementalService::checkUidReadTimeouts(StorageId storage, IfsState state,
+ Clock::time_point timeLimit) {
if (Clock::now() >= timeLimit) {
- return clearUidReadTimeouts(storage);
+ // Reached maximum timeout.
+ return false;
+ }
+ if (state.error) {
+ // Something is wrong, abort.
+ return false;
}
// Still loading?
- const auto state = isMountFullyLoaded(storage);
- if (int(state) < 0) {
- // Something is wrong, abort.
- return clearUidReadTimeouts(storage);
- }
-
- if (state == incfs::LoadingState::Full) {
- // Fully loaded, check readLogs collection.
- const auto ifs = getIfs(storage);
- if (!ifs->readLogsEnabled()) {
- return clearUidReadTimeouts(storage);
- }
+ if (state.fullyLoaded && !state.readLogsEnabled) {
+ return false;
}
const auto timeLeft = timeLimit - Clock::now();
if (timeLeft < Constants::progressUpdateInterval) {
// Don't bother.
- return clearUidReadTimeouts(storage);
+ return false;
}
- addTimedJob(*mTimedQueue, storage, Constants::progressUpdateInterval,
- [this, storage, timeLimit]() { updateUidReadTimeouts(storage, timeLimit); });
+ return true;
}
std::unordered_set<std::string_view> IncrementalService::adoptMountedInstances() {
@@ -1533,7 +1561,7 @@
dataLoaderParams.arguments = loader.arguments();
}
- prepareDataLoader(*ifs, std::move(dataLoaderParams));
+ prepareDataLoaderLocked(*ifs, std::move(dataLoaderParams));
CHECK(ifs->dataLoaderStub);
std::vector<std::pair<std::string, metadata::BindPoint>> bindPoints;
@@ -1615,24 +1643,10 @@
}
}
-IncrementalService::DataLoaderStubPtr IncrementalService::prepareDataLoader(
- IncFsMount& ifs, DataLoaderParamsParcel&& params, DataLoaderStatusListener&& statusListener,
- const StorageHealthCheckParams& healthCheckParams, StorageHealthListener&& healthListener) {
- std::unique_lock l(ifs.lock);
- prepareDataLoaderLocked(ifs, std::move(params), std::move(statusListener), healthCheckParams,
- std::move(healthListener));
- return ifs.dataLoaderStub;
-}
-
void IncrementalService::prepareDataLoaderLocked(IncFsMount& ifs, DataLoaderParamsParcel&& params,
DataLoaderStatusListener&& statusListener,
const StorageHealthCheckParams& healthCheckParams,
StorageHealthListener&& healthListener) {
- if (ifs.dataLoaderStub) {
- LOG(INFO) << "Skipped data loader preparation because it already exists";
- return;
- }
-
FileSystemControlParcel fsControlParcel;
fsControlParcel.incremental = std::make_optional<IncrementalFileSystemControlParcel>();
fsControlParcel.incremental->cmd.reset(dup(ifs.control.cmd()));
@@ -1647,6 +1661,29 @@
new DataLoaderStub(*this, ifs.mountId, std::move(params), std::move(fsControlParcel),
std::move(statusListener), healthCheckParams,
std::move(healthListener), path::join(ifs.root, constants().mount));
+
+ addIfsStateCallback(ifs.mountId, [this](StorageId storageId, IfsState state) -> bool {
+ if (!state.fullyLoaded || state.readLogsEnabled) {
+ return true;
+ }
+
+ DataLoaderStubPtr dataLoaderStub;
+ {
+ const auto ifs = getIfs(storageId);
+ if (!ifs) {
+ return false;
+ }
+
+ std::unique_lock l(ifs->lock);
+ dataLoaderStub = std::exchange(ifs->dataLoaderStub, nullptr);
+ }
+
+ if (dataLoaderStub) {
+ dataLoaderStub->cleanupResources();
+ }
+
+ return false;
+ });
}
template <class Duration>
@@ -2070,11 +2107,11 @@
StorageHealthListener healthListener) {
DataLoaderStubPtr dataLoaderStub;
{
- std::unique_lock l(mLock);
- const auto& ifs = getIfsLocked(storage);
+ const auto& ifs = getIfs(storage);
if (!ifs) {
return false;
}
+ std::unique_lock l(ifs->lock);
dataLoaderStub = ifs->dataLoaderStub;
if (!dataLoaderStub) {
return false;
@@ -2160,13 +2197,16 @@
std::lock_guard l(mLock);
affected.reserve(mMounts.size());
for (auto&& [id, ifs] : mMounts) {
- if (ifs->mountId == id && ifs->dataLoaderStub->params().packageName == packageName) {
+ std::unique_lock ll(ifs->lock);
+
+ if (ifs->mountId == id && ifs->dataLoaderStub &&
+ ifs->dataLoaderStub->params().packageName == packageName) {
affected.push_back(ifs);
}
}
}
for (auto&& ifs : affected) {
- applyStorageParams(*ifs, false);
+ applyStorageParamsLocked(*ifs, /*enableReadLogs=*/false);
}
}
@@ -2187,6 +2227,101 @@
return true;
}
+void IncrementalService::addIfsStateCallback(StorageId storageId, IfsStateCallback callback) {
+ bool wasEmpty;
+ {
+ std::lock_guard l(mIfsStateCallbacksLock);
+ wasEmpty = mIfsStateCallbacks.empty();
+ mIfsStateCallbacks[storageId].emplace_back(std::move(callback));
+ }
+ if (wasEmpty) {
+ addTimedJob(*mTimedQueue, kMaxStorageId, Constants::progressUpdateInterval,
+ [this]() { processIfsStateCallbacks(); });
+ }
+}
+
+void IncrementalService::processIfsStateCallbacks() {
+ StorageId storageId = kInvalidStorageId;
+ std::vector<IfsStateCallback> local;
+ while (true) {
+ {
+ std::lock_guard l(mIfsStateCallbacksLock);
+ if (mIfsStateCallbacks.empty()) {
+ return;
+ }
+ IfsStateCallbacks::iterator it;
+ if (storageId == kInvalidStorageId) {
+ // First entry, initialize the it.
+ it = mIfsStateCallbacks.begin();
+ } else {
+ // Subsequent entries, update the storageId, and shift to the new one.
+ it = mIfsStateCallbacks.find(storageId);
+ if (it == mIfsStateCallbacks.end()) {
+ // Was removed while processing, too bad.
+ break;
+ }
+
+ auto& callbacks = it->second;
+ if (callbacks.empty()) {
+ std::swap(callbacks, local);
+ } else {
+ callbacks.insert(callbacks.end(), local.begin(), local.end());
+ }
+ if (callbacks.empty()) {
+ it = mIfsStateCallbacks.erase(it);
+ if (mIfsStateCallbacks.empty()) {
+ return;
+ }
+ } else {
+ ++it;
+ }
+ }
+
+ if (it == mIfsStateCallbacks.end()) {
+ break;
+ }
+
+ storageId = it->first;
+ auto& callbacks = it->second;
+ if (callbacks.empty()) {
+ // Invalid case, one extra lookup should be ok.
+ continue;
+ }
+ std::swap(callbacks, local);
+ }
+
+ processIfsStateCallbacks(storageId, local);
+ }
+
+ addTimedJob(*mTimedQueue, kMaxStorageId, Constants::progressUpdateInterval,
+ [this]() { processIfsStateCallbacks(); });
+}
+
+void IncrementalService::processIfsStateCallbacks(StorageId storageId,
+ std::vector<IfsStateCallback>& callbacks) {
+ const auto state = isMountFullyLoaded(storageId);
+ IfsState storageState = {};
+ storageState.error = int(state) < 0;
+ storageState.fullyLoaded = state == incfs::LoadingState::Full;
+ if (storageState.fullyLoaded) {
+ const auto ifs = getIfs(storageId);
+ storageState.readLogsEnabled = ifs && ifs->readLogsEnabled();
+ }
+
+ for (auto cur = callbacks.begin(); cur != callbacks.end();) {
+ if ((*cur)(storageId, storageState)) {
+ ++cur;
+ } else {
+ cur = callbacks.erase(cur);
+ }
+ }
+}
+
+void IncrementalService::removeIfsStateCallbacks(StorageId storageId) {
+ std::lock_guard l(mIfsStateCallbacksLock);
+ mIfsStateCallbacks.erase(storageId);
+}
+
void IncrementalService::getMetrics(StorageId storageId, android::os::PersistableBundle* result) {
const auto duration = getMillsSinceOldestPendingRead(storageId);
if (duration >= 0) {
@@ -2197,12 +2332,12 @@
}
long IncrementalService::getMillsSinceOldestPendingRead(StorageId storageId) {
- std::unique_lock l(mLock);
- const auto ifs = getIfsLocked(storageId);
+ const auto ifs = getIfs(storageId);
if (!ifs) {
LOG(ERROR) << "getMillsSinceOldestPendingRead failed, invalid storageId: " << storageId;
return -EINVAL;
}
+ std::unique_lock l(ifs->lock);
if (!ifs->dataLoaderStub) {
LOG(ERROR) << "getMillsSinceOldestPendingRead failed, no data loader: " << storageId;
return -EINVAL;
@@ -2248,6 +2383,7 @@
resetHealthControl();
mService.removeTimedJobs(*mService.mTimedQueue, mId);
}
+ mService.removeIfsStateCallbacks(mId);
requestDestroy();
@@ -2758,7 +2894,7 @@
mService.mLooper->addFd(
pendingReadsFd, android::Looper::POLL_CALLBACK, android::Looper::EVENT_INPUT,
[](int, int, void* data) -> int {
- auto&& self = (DataLoaderStub*)data;
+ auto self = (DataLoaderStub*)data;
self->updateHealthStatus(/*baseline=*/true);
return 0;
},