Addressing CL comments.

Bug: 153874006
Test: test PackageManagerShellCommandTest PackageManagerShellCommandIncrementalTest IncrementalServiceTest
Change-Id: Ie8b86cdc382955840cc3718d79a45991cf89b879

1 files changed, 43 insertions, 52 deletions

diff --git a/services/incremental/IncrementalService.cpp b/services/incremental/IncrementalService.cpp
index d412a19f241f..a9dc92fca971 100644
--- a/services/incremental/IncrementalService.cpp
+++ b/services/incremental/IncrementalService.cpp
@@ -1356,16 +1356,9 @@ void IncrementalService::prepareDataLoaderLocked(IncFsMount& ifs, DataLoaderPara
     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 @@ void IncrementalService::onAppOpChanged(const std::string& packageName) {
 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 @@ void IncrementalService::DataLoaderStub::cleanupResources() {
     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 @@ bool IncrementalService::DataLoaderStub::fsmStep() {
                 case IDataLoaderStatusListener::DATA_LOADER_STOPPED:
                     return start();
             }
-            // fallthrough
+            [[fallthrough]];
         }
         case IDataLoaderStatusListener::DATA_LOADER_CREATED:
             switch (currentStatus) {
@@ -1895,23 +1887,48 @@ binder::Status IncrementalService::DataLoaderStub::onStatusChanged(MountId mount
     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 @@ void IncrementalService::DataLoaderStub::removeFromCmdLooperLocked() {
 
     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 @@ void IncrementalService::DataLoaderStub::onDump(int fd) {
     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());