Addressing CL comments.
Bug: 153874006
Test: test PackageManagerShellCommandTest PackageManagerShellCommandIncrementalTest IncrementalServiceTest
Change-Id: Ie8b86cdc382955840cc3718d79a45991cf89b879
diff --git a/services/incremental/IncrementalService.cpp b/services/incremental/IncrementalService.cpp
index d412a19..a9dc92f 100644
--- a/services/incremental/IncrementalService.cpp
+++ b/services/incremental/IncrementalService.cpp
@@ -1356,16 +1356,9 @@
fsControlParcel.incremental->log.reset(dup(ifs.control.logs()));
fsControlParcel.service = new IncrementalServiceConnector(*this, ifs.mountId);
- incfs::UniqueControl healthControl = mIncFs->openMount(ifs.root.c_str());
- if (healthControl.pendingReads() < 0) {
- LOG(ERROR) << "Failed to open health control for: " << ifs.root << "("
- << healthControl.cmd() << ":" << healthControl.pendingReads() << ":"
- << healthControl.logs() << ")";
- }
-
ifs.dataLoaderStub =
new DataLoaderStub(*this, ifs.mountId, std::move(params), std::move(fsControlParcel),
- std::move(healthControl), externalListener);
+ externalListener, path::join(ifs.root, constants().mount));
}
template <class Duration>
@@ -1685,15 +1678,15 @@
IncrementalService::DataLoaderStub::DataLoaderStub(IncrementalService& service, MountId id,
DataLoaderParamsParcel&& params,
FileSystemControlParcel&& control,
- incfs::UniqueControl&& healthControl,
- const DataLoaderStatusListener* externalListener)
+ const DataLoaderStatusListener* externalListener,
+ std::string&& healthPath)
: mService(service),
mId(id),
mParams(std::move(params)),
mControl(std::move(control)),
- mHealthControl(std::move(healthControl)),
- mListener(externalListener ? *externalListener : DataLoaderStatusListener()) {
- addToCmdLooperLocked();
+ mListener(externalListener ? *externalListener : DataLoaderStatusListener()),
+ mHealthPath(std::move(healthPath)) {
+ healthStatusOk();
}
IncrementalService::DataLoaderStub::~DataLoaderStub() {
@@ -1708,11 +1701,10 @@
auto now = Clock::now();
std::unique_lock lock(mMutex);
- removeFromCmdLooperLocked();
+ unregisterFromPendingReads();
mParams = {};
mControl = {};
- mHealthControl = {};
mStatusCondition.wait_until(lock, now + 60s, [this] {
return mCurrentStatus == IDataLoaderStatusListener::DATA_LOADER_DESTROYED;
});
@@ -1832,7 +1824,7 @@
case IDataLoaderStatusListener::DATA_LOADER_STOPPED:
return start();
}
- // fallthrough
+ [[fallthrough]];
}
case IDataLoaderStatusListener::DATA_LOADER_CREATED:
switch (currentStatus) {
@@ -1895,23 +1887,48 @@
return binder::Status::ok();
}
-void IncrementalService::DataLoaderStub::addToCmdLooperLocked() {
- const auto pendingReadsFd = mHealthControl.pendingReads();
+void IncrementalService::DataLoaderStub::healthStatusOk() {
+ LOG(DEBUG) << "healthStatusOk: " << mId;
+ std::unique_lock lock(mMutex);
+ registerForPendingReads();
+}
+
+void IncrementalService::DataLoaderStub::healthStatusReadsPending() {
+ LOG(DEBUG) << "healthStatusReadsPending: " << mId;
+ requestStart();
+
+ std::unique_lock lock(mMutex);
+ unregisterFromPendingReads();
+}
+
+void IncrementalService::DataLoaderStub::healthStatusBlocked() {}
+
+void IncrementalService::DataLoaderStub::healthStatusUnhealthy() {}
+
+void IncrementalService::DataLoaderStub::registerForPendingReads() {
+ auto pendingReadsFd = mHealthControl.pendingReads();
if (pendingReadsFd < 0) {
- return;
+ mHealthControl = mService.mIncFs->openMount(mHealthPath);
+ pendingReadsFd = mHealthControl.pendingReads();
+ if (pendingReadsFd < 0) {
+ LOG(ERROR) << "Failed to open health control for: " << mId << ", path: " << mHealthPath
+ << "(" << mHealthControl.cmd() << ":" << mHealthControl.pendingReads() << ":"
+ << mHealthControl.logs() << ")";
+ return;
+ }
}
mService.mLooper->addFd(
pendingReadsFd, android::Looper::POLL_CALLBACK, android::Looper::EVENT_INPUT,
[](int, int, void* data) -> int {
auto&& self = (DataLoaderStub*)data;
- return self->onCmdLooperEvent();
+ return self->onPendingReads();
},
this);
mService.mLooper->wake();
}
-void IncrementalService::DataLoaderStub::removeFromCmdLooperLocked() {
+void IncrementalService::DataLoaderStub::unregisterFromPendingReads() {
const auto pendingReadsFd = mHealthControl.pendingReads();
if (pendingReadsFd < 0) {
return;
@@ -1919,41 +1936,17 @@
mService.mLooper->removeFd(pendingReadsFd);
mService.mLooper->wake();
+
+ mHealthControl = {};
}
-int IncrementalService::DataLoaderStub::onCmdLooperEvent() {
+int IncrementalService::DataLoaderStub::onPendingReads() {
if (!mService.mRunning.load(std::memory_order_relaxed)) {
return 0;
}
- bool pendingReadsOccur = false;
-
- {
- std::unique_lock lock(mMutex);
- const auto now = Clock::now();
- if (now < mEarliestMissingPageTs) {
- // Transition: duration::max -> now.
- mEarliestMissingPageTs = now;
- pendingReadsOccur = true;
- }
- }
-
- if (pendingReadsOccur) {
- LOG(INFO) << "Pending reads occur for, requesting start for: " << mId;
- requestStart();
- }
-
- {
- // Drop pending reads.
- static constexpr auto kMaxDropIterations = 3;
- std::unique_lock lock(mMutex);
- for (int i = 0; i < kMaxDropIterations; ++i) {
- if (IncFs_DropPendingReads(mHealthControl) <= 0) {
- break;
- }
- }
- }
- return 1;
+ healthStatusReadsPending();
+ return 0;
}
void IncrementalService::DataLoaderStub::onDump(int fd) {
@@ -1962,8 +1955,6 @@
dprintf(fd, " targetStatus: %d\n", mTargetStatus);
dprintf(fd, " targetStatusTs: %lldmcs\n",
(long long)(elapsedMcs(mTargetStatusTs, Clock::now())));
- dprintf(fd, " earliestMissingPageTs: %lldmcs\n",
- (long long)(elapsedMcs(mEarliestMissingPageTs, Clock::now())));
const auto& params = mParams;
dprintf(fd, " dataLoaderParams: {\n");
dprintf(fd, " type: %s\n", toString(params.type).c_str());
diff --git a/services/incremental/IncrementalService.h b/services/incremental/IncrementalService.h
index 640ca53..f3fde2a 100644
--- a/services/incremental/IncrementalService.h
+++ b/services/incremental/IncrementalService.h
@@ -161,8 +161,7 @@
DataLoaderStub(IncrementalService& service, MountId id,
content::pm::DataLoaderParamsParcel&& params,
content::pm::FileSystemControlParcel&& control,
- incfs::UniqueControl&& healthControl,
- const DataLoaderStatusListener* externalListener);
+ const DataLoaderStatusListener* externalListener, std::string&& healthPath);
~DataLoaderStub();
// Cleans up the internal state and invalidates DataLoaderStub. Any subsequent calls will
// result in an error.
@@ -180,9 +179,9 @@
private:
binder::Status onStatusChanged(MountId mount, int newStatus) final;
- void addToCmdLooperLocked();
- void removeFromCmdLooperLocked();
- int onCmdLooperEvent();
+ void registerForPendingReads();
+ void unregisterFromPendingReads();
+ int onPendingReads();
bool isValid() const { return mId != kInvalidStorageId; }
sp<content::pm::IDataLoader> getDataLoader();
@@ -197,13 +196,22 @@
bool fsmStep();
+ // Watching for pending reads.
+ void healthStatusOk();
+ // Pending reads detected, waiting for Xsecs to confirm blocked state.
+ void healthStatusReadsPending();
+ // There are reads pending for X+secs, waiting for additional Ysecs to confirm unhealthy
+ // state.
+ void healthStatusBlocked();
+ // There are reads pending for X+Ysecs, marking storage as unhealthy.
+ void healthStatusUnhealthy();
+
IncrementalService& mService;
std::mutex mMutex;
MountId mId = kInvalidStorageId;
content::pm::DataLoaderParamsParcel mParams;
content::pm::FileSystemControlParcel mControl;
- incfs::UniqueControl mHealthControl;
DataLoaderStatusListener mListener;
std::condition_variable mStatusCondition;
@@ -211,7 +219,8 @@
int mTargetStatus = content::pm::IDataLoaderStatusListener::DATA_LOADER_DESTROYED;
TimePoint mTargetStatusTs = {};
- TimePoint mEarliestMissingPageTs{Clock::duration::max()};
+ std::string mHealthPath;
+ incfs::UniqueControl mHealthControl;
};
using DataLoaderStubPtr = sp<DataLoaderStub>;
