Merge "Addressing CL comments." into rvc-dev

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()),
-        mListener(externalListener ? *externalListener : DataLoaderStatusListener()) {
+        mHealthPath(std::move(healthPath)) {
-    addToCmdLooperLocked();
+    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() {
+void IncrementalService::DataLoaderStub::healthStatusOk() {
-    const auto pendingReadsFd = mHealthControl.pendingReads();
+    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) {
+        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;
+    healthStatusReadsPending();
-
+    return 0;
-    {
-        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;
 }
 
 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());

services/incremental/IncrementalService.h

@@ -161,8 +161,7 @@ private:
         DataLoaderStub(IncrementalService& service, MountId id,
                        content::pm::DataLoaderParamsParcel&& params,
                        content::pm::FileSystemControlParcel&& control,
-                       incfs::UniqueControl&& healthControl,
+                       const DataLoaderStatusListener* externalListener, std::string&& healthPath);
-                       const DataLoaderStatusListener* externalListener);
         ~DataLoaderStub();
         // Cleans up the internal state and invalidates DataLoaderStub. Any subsequent calls will
         // result in an error.
@@ -180,9 +179,9 @@ private:
     private:
         binder::Status onStatusChanged(MountId mount, int newStatus) final;
 
-        void addToCmdLooperLocked();
+        void registerForPendingReads();
-        void removeFromCmdLooperLocked();
+        void unregisterFromPendingReads();
-        int onCmdLooperEvent();
+        int onPendingReads();
 
         bool isValid() const { return mId != kInvalidStorageId; }
         sp<content::pm::IDataLoader> getDataLoader();
@@ -197,13 +196,22 @@ private:
 
         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 @@ private:
         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>;