From da208016d65c331218ed3a1acd2f45d5ca4ce006 Mon Sep 17 00:00:00 2001 From: Yurii Zubrytskyi Date: Tue, 7 Apr 2020 15:35:21 -0700 Subject: [PATCH 1/2] [incfs] Make native library extraction async IncrementalService can create the library files beforehand, but delay filling in their data. As it takes quite a while in general (over a second in cases when the phone is busy), it's better to run the unzipping and filling in a separate thread and only make sure it finishes before the whole installation process is complete. This speeds up the megacity.apk installation by ~250-300ms, 1000-1100ms -> 750-800ms Bug: 153513507 Test: adb install megacity.apk Change-Id: Ia44f7e45b9e0abaebdfb6fe5352f9dcf29ab4ece --- .../os/incremental/IIncrementalService.aidl | 5 + .../os/incremental/IncrementalStorage.java | 14 + .../internal/content/NativeLibraryHelper.java | 16 +- .../server/pm/PackageManagerService.java | 11 + .../incremental/BinderIncrementalService.cpp | 22 +- .../incremental/BinderIncrementalService.h | 7 +- services/incremental/IncrementalService.cpp | 241 ++++++++++++------ services/incremental/IncrementalService.h | 25 +- 8 files changed, 245 insertions(+), 96 deletions(-) diff --git a/core/java/android/os/incremental/IIncrementalService.aidl b/core/java/android/os/incremental/IIncrementalService.aidl index 2dbaea860e2a4..25cb0400a38d8 100644 --- a/core/java/android/os/incremental/IIncrementalService.aidl +++ b/core/java/android/os/incremental/IIncrementalService.aidl @@ -109,4 +109,9 @@ interface IIncrementalService { * Setting up native library directories and extract native libs onto a storage. */ boolean configureNativeBinaries(int storageId, in @utf8InCpp String apkFullPath, in @utf8InCpp String libDirRelativePath, in @utf8InCpp String abi); + + /** + * Waits until all native library extraction is done for the storage + */ + boolean waitForNativeBinariesExtraction(int storageId); } diff --git a/core/java/android/os/incremental/IncrementalStorage.java b/core/java/android/os/incremental/IncrementalStorage.java index 7092751c0d7ee..70ebbaa326b81 100644 --- a/core/java/android/os/incremental/IncrementalStorage.java +++ b/core/java/android/os/incremental/IncrementalStorage.java @@ -480,4 +480,18 @@ public final class IncrementalStorage { return false; } } + + /** + * Waits for all native binary extraction operations to complete on the storage. + * + * @return Success of not. + */ + public boolean waitForNativeBinariesExtraction() { + try { + return mService.waitForNativeBinariesExtraction(mId); + } catch (RemoteException e) { + e.rethrowFromSystemServer(); + return false; + } + } } diff --git a/core/java/com/android/internal/content/NativeLibraryHelper.java b/core/java/com/android/internal/content/NativeLibraryHelper.java index 04bf915679860..02cf25a7c3d2a 100644 --- a/core/java/com/android/internal/content/NativeLibraryHelper.java +++ b/core/java/com/android/internal/content/NativeLibraryHelper.java @@ -26,7 +26,6 @@ import static android.system.OsConstants.S_IXGRP; import static android.system.OsConstants.S_IXOTH; import android.content.Context; -import android.content.pm.ApplicationInfo; import android.content.pm.PackageManager; import android.content.pm.PackageParser; import android.content.pm.PackageParser.PackageLite; @@ -40,6 +39,7 @@ import android.os.incremental.IncrementalManager; import android.os.incremental.IncrementalStorage; import android.system.ErrnoException; import android.system.Os; +import android.util.ArraySet; import android.util.Slog; import dalvik.system.CloseGuard; @@ -545,4 +545,18 @@ public class NativeLibraryHelper { } return false; } + + /** + * Wait for all native library extraction to complete for the passed storages. + * + * @param incrementalStorages A list of the storages to wait for. + */ + public static void waitForNativeBinariesExtraction( + ArraySet incrementalStorages) { + for (int i = 0; i < incrementalStorages.size(); ++i) { + IncrementalStorage storage = incrementalStorages.valueAtUnchecked(i); + storage.waitForNativeBinariesExtraction(); + } + } + } diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java index 9c41e6d5c6a31..59ac603875e2b 100644 --- a/services/core/java/com/android/server/pm/PackageManagerService.java +++ b/services/core/java/com/android/server/pm/PackageManagerService.java @@ -263,6 +263,7 @@ import android.os.UserHandle; import android.os.UserManager; import android.os.UserManagerInternal; import android.os.incremental.IncrementalManager; +import android.os.incremental.IncrementalStorage; import android.os.storage.DiskInfo; import android.os.storage.IStorageManager; import android.os.storage.StorageEventListener; @@ -16596,6 +16597,7 @@ public class PackageManagerService extends IPackageManager.Stub * locks on {@link #mLock}. */ private void executePostCommitSteps(CommitRequest commitRequest) { + final ArraySet incrementalStorages = new ArraySet<>(); for (ReconciledPackage reconciledPkg : commitRequest.reconciledPackages.values()) { final boolean instantApp = ((reconciledPkg.scanResult.request.scanFlags & PackageManagerService.SCAN_AS_INSTANT_APP) != 0); @@ -16603,6 +16605,14 @@ public class PackageManagerService extends IPackageManager.Stub final String packageName = pkg.getPackageName(); final boolean onIncremental = mIncrementalManager != null && isIncrementalPath(pkg.getCodePath()); + if (onIncremental) { + IncrementalStorage storage = mIncrementalManager.openStorage(pkg.getCodePath()); + if (storage == null) { + throw new IllegalArgumentException( + "Install: null storage for incremental package " + packageName); + } + incrementalStorages.add(storage); + } prepareAppDataAfterInstallLIF(pkg); if (reconciledPkg.prepareResult.clearCodeCache) { clearAppDataLIF(pkg, UserHandle.USER_ALL, FLAG_STORAGE_DE | FLAG_STORAGE_CE @@ -16700,6 +16710,7 @@ public class PackageManagerService extends IPackageManager.Stub notifyPackageChangeObserversOnUpdate(reconciledPkg); } + NativeLibraryHelper.waitForNativeBinariesExtraction(incrementalStorages); } private void notifyPackageChangeObserversOnUpdate(ReconciledPackage reconciledPkg) { diff --git a/services/incremental/BinderIncrementalService.cpp b/services/incremental/BinderIncrementalService.cpp index aabc58c3a2977..ebebf606b1cc4 100644 --- a/services/incremental/BinderIncrementalService.cpp +++ b/services/incremental/BinderIncrementalService.cpp @@ -116,11 +116,14 @@ binder::Status BinderIncrementalService::openStorage(const std::string& path, return ok(); } -binder::Status BinderIncrementalService::createStorage(const std::string& path, - const DataLoaderParamsParcel& params, - const ::android::sp<::android::content::pm::IDataLoaderStatusListener>& listener, - int32_t createMode, int32_t* _aidl_return) { - *_aidl_return = mImpl.createStorage(path, const_cast(params), listener, android::incremental::IncrementalService::CreateOptions(createMode)); +binder::Status BinderIncrementalService::createStorage( + const std::string& path, const DataLoaderParamsParcel& params, + const ::android::sp<::android::content::pm::IDataLoaderStatusListener>& listener, + int32_t createMode, int32_t* _aidl_return) { + *_aidl_return = + mImpl.createStorage(path, const_cast(params), listener, + android::incremental::IncrementalService::CreateOptions( + createMode)); return ok(); } @@ -181,7 +184,8 @@ static std::tuple toMakeFileParams( if (!params.signature) { nfp.signature = {}; } else { - nfp.signature = {(const char*)params.signature->data(), (IncFsSize)params.signature->size()}; + nfp.signature = {(const char*)params.signature->data(), + (IncFsSize)params.signature->size()}; } return {0, id, nfp}; } @@ -278,6 +282,12 @@ binder::Status BinderIncrementalService::configureNativeBinaries( return ok(); } +binder::Status BinderIncrementalService::waitForNativeBinariesExtraction(int storageId, + bool* _aidl_return) { + *_aidl_return = mImpl.waitForNativeBinariesExtraction(storageId); + return ok(); +} + } // namespace android::os::incremental jlong Incremental_IncrementalService_Start() { diff --git a/services/incremental/BinderIncrementalService.h b/services/incremental/BinderIncrementalService.h index 28613e101b7c1..aca10ab6d5915 100644 --- a/services/incremental/BinderIncrementalService.h +++ b/services/incremental/BinderIncrementalService.h @@ -38,7 +38,10 @@ public: void onInvalidStorage(int mountId); binder::Status openStorage(const std::string& path, int32_t* _aidl_return) final; - binder::Status createStorage(const ::std::string& path, const ::android::content::pm::DataLoaderParamsParcel& params, const ::android::sp<::android::content::pm::IDataLoaderStatusListener>& listener, int32_t createMode, int32_t* _aidl_return) final; + binder::Status createStorage( + const ::std::string& path, const ::android::content::pm::DataLoaderParamsParcel& params, + const ::android::sp<::android::content::pm::IDataLoaderStatusListener>& listener, + int32_t createMode, int32_t* _aidl_return) final; binder::Status createLinkedStorage(const std::string& path, int32_t otherStorageId, int32_t createMode, int32_t* _aidl_return) final; binder::Status makeBindMount(int32_t storageId, const std::string& sourcePath, @@ -68,9 +71,11 @@ public: std::vector* _aidl_return) final; binder::Status startLoading(int32_t storageId, bool* _aidl_return) final; binder::Status deleteStorage(int32_t storageId) final; + binder::Status configureNativeBinaries(int32_t storageId, const std::string& apkFullPath, const std::string& libDirRelativePath, const std::string& abi, bool* _aidl_return) final; + binder::Status waitForNativeBinariesExtraction(int storageId, bool* _aidl_return) final; private: android::incremental::IncrementalService mImpl; diff --git a/services/incremental/IncrementalService.cpp b/services/incremental/IncrementalService.cpp index eb65a2ddc5f19..400388e932622 100644 --- a/services/incremental/IncrementalService.cpp +++ b/services/incremental/IncrementalService.cpp @@ -26,23 +26,18 @@ #include #include #include -#include -#include #include #include #include #include #include #include -#include #include #include #include #include #include -#include -#include #include #include "Metadata.pb.h" @@ -252,6 +247,10 @@ IncrementalService::IncrementalService(ServiceManagerWrapper&& sm, std::string_v if (!mAppOpsManager) { LOG(FATAL) << "AppOpsManager is unavailable"; } + + mJobQueue.reserve(16); + mJobProcessor = std::thread([this]() { runJobProcessing(); }); + mountExistingImages(); } @@ -259,7 +258,14 @@ FileId IncrementalService::idFromMetadata(std::span metadata) { return IncFs_FileIdFromMetadata({(const char*)metadata.data(), metadata.size()}); } -IncrementalService::~IncrementalService() = default; +IncrementalService::~IncrementalService() { + { + std::lock_guard lock(mJobMutex); + mRunning = false; + } + mJobCondition.notify_all(); + mJobProcessor.join(); +} inline const char* toString(TimePoint t) { using SystemClock = std::chrono::system_clock; @@ -1158,8 +1164,6 @@ static long elapsedMcs(Duration start, Duration end) { bool IncrementalService::configureNativeBinaries(StorageId storage, std::string_view apkFullPath, std::string_view libDirRelativePath, std::string_view abi) { - namespace sc = std::chrono; - using Clock = sc::steady_clock; auto start = Clock::now(); const auto ifs = getIfs(storage); @@ -1176,33 +1180,35 @@ bool IncrementalService::configureNativeBinaries(StorageId storage, std::string_ } auto mkDirsTs = Clock::now(); - - std::unique_ptr zipFile(ZipFileRO::open(path::c_str(apkFullPath))); - if (!zipFile) { + ZipArchiveHandle zipFileHandle; + if (OpenArchive(path::c_str(apkFullPath), &zipFileHandle)) { LOG(ERROR) << "Failed to open zip file at " << apkFullPath; return false; } + + // Need a shared pointer: will be passing it into all unpacking jobs. + std::shared_ptr zipFile(zipFileHandle, [](ZipArchiveHandle h) { CloseArchive(h); }); void* cookie = nullptr; const auto libFilePrefix = path::join(constants().libDir, abi); - if (!zipFile->startIteration(&cookie, libFilePrefix.c_str() /* prefix */, - constants().libSuffix.data() /* suffix */)) { + if (StartIteration(zipFile.get(), &cookie, libFilePrefix, constants().libSuffix)) { LOG(ERROR) << "Failed to start zip iteration for " << apkFullPath; return false; } - auto endIteration = [&zipFile](void* cookie) { zipFile->endIteration(cookie); }; + auto endIteration = [](void* cookie) { EndIteration(cookie); }; auto iterationCleaner = std::unique_ptr(cookie, endIteration); auto openZipTs = Clock::now(); - std::vector instructions; - ZipEntryRO entry = nullptr; - while ((entry = zipFile->nextEntry(cookie)) != nullptr) { - auto startFileTs = Clock::now(); - - char fileName[PATH_MAX]; - if (zipFile->getEntryFileName(entry, fileName, sizeof(fileName))) { + std::vector jobQueue; + ZipEntry entry; + std::string_view fileName; + while (!Next(cookie, &entry, &fileName)) { + if (fileName.empty()) { continue; } + + auto startFileTs = Clock::now(); + const auto libName = path::basename(fileName); const auto targetLibPath = path::join(libDirRelativePath, libName); const auto targetLibPathAbsolute = normalizePathToStorage(ifs, storage, targetLibPath); @@ -1216,16 +1222,9 @@ bool IncrementalService::configureNativeBinaries(StorageId storage, std::string_ continue; } - uint32_t uncompressedLen, compressedLen; - if (!zipFile->getEntryInfo(entry, nullptr, &uncompressedLen, &compressedLen, nullptr, - nullptr, nullptr)) { - LOG(ERROR) << "Failed to read native lib entry: " << fileName; - return false; - } - // Create new lib file without signature info incfs::NewFileParams libFileParams = { - .size = uncompressedLen, + .size = entry.uncompressed_length, .signature = {}, // Metadata of the new lib file is its relative path .metadata = {targetLibPath.c_str(), (IncFsSize)targetLibPath.size()}, @@ -1241,81 +1240,152 @@ bool IncrementalService::configureNativeBinaries(StorageId storage, std::string_ auto makeFileTs = Clock::now(); // If it is a zero-byte file, skip data writing - if (uncompressedLen == 0) { + if (entry.uncompressed_length == 0) { if (sEnablePerfLogging) { - LOG(INFO) << "incfs: Extracted " << libName << "(" << compressedLen << " -> " - << uncompressedLen << " bytes): " << elapsedMcs(startFileTs, makeFileTs) - << "mcs, make: " << elapsedMcs(startFileTs, makeFileTs); + LOG(INFO) << "incfs: Extracted " << libName + << "(0 bytes): " << elapsedMcs(startFileTs, makeFileTs) << "mcs"; } continue; } - // Write extracted data to new file - // NOTE: don't zero-initialize memory, it may take a while - auto libData = std::unique_ptr(new uint8_t[uncompressedLen]); - if (!zipFile->uncompressEntry(entry, libData.get(), uncompressedLen)) { - LOG(ERROR) << "Failed to extract native lib zip entry: " << fileName; - return false; - } - - auto extractFileTs = Clock::now(); - - const auto writeFd = mIncFs->openForSpecialOps(ifs->control, libFileId); - if (!writeFd.ok()) { - LOG(ERROR) << "Failed to open write fd for: " << targetLibPath << " errno: " << writeFd; - return false; - } - - auto openFileTs = Clock::now(); - - const int numBlocks = (uncompressedLen + constants().blockSize - 1) / constants().blockSize; - instructions.clear(); - instructions.reserve(numBlocks); - auto remainingData = std::span(libData.get(), uncompressedLen); - for (int i = 0; i < numBlocks; i++) { - const auto blockSize = std::min(constants().blockSize, remainingData.size()); - auto inst = IncFsDataBlock{ - .fileFd = writeFd.get(), - .pageIndex = static_cast(i), - .compression = INCFS_COMPRESSION_KIND_NONE, - .kind = INCFS_BLOCK_KIND_DATA, - .dataSize = blockSize, - .data = reinterpret_cast(remainingData.data()), - }; - instructions.push_back(inst); - remainingData = remainingData.subspan(blockSize); - } - auto prepareInstsTs = Clock::now(); - - size_t res = mIncFs->writeBlocks(instructions); - if (res != instructions.size()) { - LOG(ERROR) << "Failed to write data into: " << targetLibPath; - return false; - } + jobQueue.emplace_back([this, zipFile, entry, ifs, libFileId, + libPath = std::move(targetLibPath), makeFileTs]() mutable { + extractZipFile(ifs, zipFile.get(), entry, libFileId, libPath, makeFileTs); + }); if (sEnablePerfLogging) { - auto endFileTs = Clock::now(); - LOG(INFO) << "incfs: Extracted " << libName << "(" << compressedLen << " -> " - << uncompressedLen << " bytes): " << elapsedMcs(startFileTs, endFileTs) - << "mcs, make: " << elapsedMcs(startFileTs, makeFileTs) - << " extract: " << elapsedMcs(makeFileTs, extractFileTs) - << " open: " << elapsedMcs(extractFileTs, openFileTs) - << " prepare: " << elapsedMcs(openFileTs, prepareInstsTs) - << " write:" << elapsedMcs(prepareInstsTs, endFileTs); + auto prepareJobTs = Clock::now(); + LOG(INFO) << "incfs: Processed " << libName << ": " + << elapsedMcs(startFileTs, prepareJobTs) + << "mcs, make file: " << elapsedMcs(startFileTs, makeFileTs) + << " prepare job: " << elapsedMcs(makeFileTs, prepareJobTs); } } + auto processedTs = Clock::now(); + + if (!jobQueue.empty()) { + { + std::lock_guard lock(mJobMutex); + if (mRunning) { + auto& existingJobs = mJobQueue[storage]; + if (existingJobs.empty()) { + existingJobs = std::move(jobQueue); + } else { + existingJobs.insert(existingJobs.end(), std::move_iterator(jobQueue.begin()), + std::move_iterator(jobQueue.end())); + } + } + } + mJobCondition.notify_all(); + } + if (sEnablePerfLogging) { auto end = Clock::now(); LOG(INFO) << "incfs: configureNativeBinaries complete in " << elapsedMcs(start, end) << "mcs, make dirs: " << elapsedMcs(start, mkDirsTs) << " open zip: " << elapsedMcs(mkDirsTs, openZipTs) - << " extract all: " << elapsedMcs(openZipTs, end); + << " make files: " << elapsedMcs(openZipTs, processedTs) + << " schedule jobs: " << elapsedMcs(processedTs, end); } return true; } +void IncrementalService::extractZipFile(const IfsMountPtr& ifs, ZipArchiveHandle zipFile, + ZipEntry& entry, const incfs::FileId& libFileId, + std::string_view targetLibPath, + Clock::time_point scheduledTs) { + auto libName = path::basename(targetLibPath); + auto startedTs = Clock::now(); + + // Write extracted data to new file + // NOTE: don't zero-initialize memory, it may take a while for nothing + auto libData = std::unique_ptr(new uint8_t[entry.uncompressed_length]); + if (ExtractToMemory(zipFile, &entry, libData.get(), entry.uncompressed_length)) { + LOG(ERROR) << "Failed to extract native lib zip entry: " << libName; + return; + } + + auto extractFileTs = Clock::now(); + + const auto writeFd = mIncFs->openForSpecialOps(ifs->control, libFileId); + if (!writeFd.ok()) { + LOG(ERROR) << "Failed to open write fd for: " << targetLibPath << " errno: " << writeFd; + return; + } + + auto openFileTs = Clock::now(); + const int numBlocks = + (entry.uncompressed_length + constants().blockSize - 1) / constants().blockSize; + std::vector instructions(numBlocks); + auto remainingData = std::span(libData.get(), entry.uncompressed_length); + for (int i = 0; i < numBlocks; i++) { + const auto blockSize = std::min(constants().blockSize, remainingData.size()); + instructions[i] = IncFsDataBlock{ + .fileFd = writeFd.get(), + .pageIndex = static_cast(i), + .compression = INCFS_COMPRESSION_KIND_NONE, + .kind = INCFS_BLOCK_KIND_DATA, + .dataSize = blockSize, + .data = reinterpret_cast(remainingData.data()), + }; + remainingData = remainingData.subspan(blockSize); + } + auto prepareInstsTs = Clock::now(); + + size_t res = mIncFs->writeBlocks(instructions); + if (res != instructions.size()) { + LOG(ERROR) << "Failed to write data into: " << targetLibPath; + return; + } + + if (sEnablePerfLogging) { + auto endFileTs = Clock::now(); + LOG(INFO) << "incfs: Extracted " << libName << "(" << entry.compressed_length << " -> " + << entry.uncompressed_length << " bytes): " << elapsedMcs(startedTs, endFileTs) + << "mcs, scheduling delay: " << elapsedMcs(scheduledTs, startedTs) + << " extract: " << elapsedMcs(startedTs, extractFileTs) + << " open: " << elapsedMcs(extractFileTs, openFileTs) + << " prepare: " << elapsedMcs(openFileTs, prepareInstsTs) + << " write: " << elapsedMcs(prepareInstsTs, endFileTs); + } +} + +bool IncrementalService::waitForNativeBinariesExtraction(StorageId storage) { + std::unique_lock lock(mJobMutex); + mJobCondition.wait(lock, [this, storage] { + return !mRunning || + (mPendingJobsStorage != storage && mJobQueue.find(storage) == mJobQueue.end()); + }); + return mPendingJobsStorage != storage && mJobQueue.find(storage) == mJobQueue.end(); +} + +void IncrementalService::runJobProcessing() { + for (;;) { + std::unique_lock lock(mJobMutex); + mJobCondition.wait(lock, [this]() { return !mRunning || !mJobQueue.empty(); }); + if (!mRunning) { + return; + } + + auto it = mJobQueue.begin(); + mPendingJobsStorage = it->first; + auto queue = std::move(it->second); + mJobQueue.erase(it); + lock.unlock(); + + for (auto&& job : queue) { + job(); + } + + lock.lock(); + mPendingJobsStorage = kInvalidStorageId; + lock.unlock(); + mJobCondition.notify_all(); + } +} + void IncrementalService::registerAppOpsCallback(const std::string& packageName) { sp listener; { @@ -1328,7 +1398,8 @@ void IncrementalService::registerAppOpsCallback(const std::string& packageName) listener = cb; } - mAppOpsManager->startWatchingMode(AppOpsManager::OP_GET_USAGE_STATS, String16(packageName.c_str()), listener); + mAppOpsManager->startWatchingMode(AppOpsManager::OP_GET_USAGE_STATS, + String16(packageName.c_str()), listener); } bool IncrementalService::unregisterAppOpsCallback(const std::string& packageName) { diff --git a/services/incremental/IncrementalService.h b/services/incremental/IncrementalService.h index 27d40f1506ca2..4fdce4bd92dcc 100644 --- a/services/incremental/IncrementalService.h +++ b/services/incremental/IncrementalService.h @@ -23,16 +23,19 @@ #include #include #include +#include #include #include -#include +#include +#include #include #include #include #include #include #include +#include #include #include #include @@ -132,12 +135,15 @@ public: std::vector listFiles(StorageId storage) const; bool startLoading(StorageId storage) const; + bool configureNativeBinaries(StorageId storage, std::string_view apkFullPath, std::string_view libDirRelativePath, std::string_view abi); + bool waitForNativeBinariesExtraction(StorageId storage); class AppOpsListener : public android::BnAppOpsCallback { public: - AppOpsListener(IncrementalService& incrementalService, std::string packageName) : incrementalService(incrementalService), packageName(std::move(packageName)) {} + AppOpsListener(IncrementalService& incrementalService, std::string packageName) + : incrementalService(incrementalService), packageName(std::move(packageName)) {} void opChanged(int32_t op, const String16& packageName) final; private: @@ -277,7 +283,12 @@ private: bool unregisterAppOpsCallback(const std::string& packageName); void onAppOpChanged(const std::string& packageName); - // Member variables + void runJobProcessing(); + void extractZipFile(const IfsMountPtr& ifs, ZipArchiveHandle zipFile, ZipEntry& entry, + const incfs::FileId& libFileId, std::string_view targetLibPath, + Clock::time_point scheduledTs); + +private: std::unique_ptr const mVold; std::unique_ptr const mDataLoaderManager; std::unique_ptr const mIncFs; @@ -294,6 +305,14 @@ private: std::atomic_bool mSystemReady = false; StorageId mNextId = 0; + + using Job = std::function; + std::unordered_map> mJobQueue; + StorageId mPendingJobsStorage = kInvalidStorageId; + std::condition_variable mJobCondition; + std::mutex mJobMutex; + std::thread mJobProcessor; + bool mRunning = true; }; } // namespace android::incremental From 86321400385172e6bb938d53ce733f1fd7984b20 Mon Sep 17 00:00:00 2001 From: Yurii Zubrytskyi Date: Thu, 9 Apr 2020 19:22:30 -0700 Subject: [PATCH 2/2] [incfs] Fix a crash in worker thread calling JNI Worker thread has to initialize JNI separately to be able to call into managed binders implemented in the same system_server process, e.g. DataLoaderManager Bug: 153513507 Test: adb install megacity.nov4.apk; adb install megacity.v4.apk Change-Id: I668e8664361cd2fb3353ec50efd689c7d613658f --- .../jni/com_android_server_SystemServer.cpp | 2 +- ...m_PackageManagerShellCommandDataLoader.cpp | 2 + .../incremental/BinderIncrementalService.cpp | 12 +- .../incremental/BinderIncrementalService.h | 5 +- services/incremental/IncrementalService.cpp | 18 +- services/incremental/IncrementalService.h | 9 +- services/incremental/ServiceWrappers.cpp | 180 +++++++++++++++++- services/incremental/ServiceWrappers.h | 116 ++--------- .../incremental/include/incremental_service.h | 2 +- .../test/IncrementalServiceTest.cpp | 28 ++- 10 files changed, 243 insertions(+), 131 deletions(-) diff --git a/services/core/jni/com_android_server_SystemServer.cpp b/services/core/jni/com_android_server_SystemServer.cpp index e99a264c42e4c..76b171337bb93 100644 --- a/services/core/jni/com_android_server_SystemServer.cpp +++ b/services/core/jni/com_android_server_SystemServer.cpp @@ -135,7 +135,7 @@ static void android_server_SystemServer_spawnFdLeakCheckThread(JNIEnv*, jobject) static jlong android_server_SystemServer_startIncrementalService(JNIEnv* env, jclass klass, jobject self) { - return Incremental_IncrementalService_Start(); + return Incremental_IncrementalService_Start(env); } static void android_server_SystemServer_setIncrementalServiceSystemReady(JNIEnv* env, jclass klass, diff --git a/services/core/jni/com_android_server_pm_PackageManagerShellCommandDataLoader.cpp b/services/core/jni/com_android_server_pm_PackageManagerShellCommandDataLoader.cpp index 853eba71d88ae..e32a343433a89 100644 --- a/services/core/jni/com_android_server_pm_PackageManagerShellCommandDataLoader.cpp +++ b/services/core/jni/com_android_server_pm_PackageManagerShellCommandDataLoader.cpp @@ -410,6 +410,8 @@ private: // Installation. bool onPrepareImage(dataloader::DataLoaderInstallationFiles addedFiles) final { + ALOGE("onPrepareImage: start."); + JNIEnv* env = GetOrAttachJNIEnvironment(mJvm); const auto& jni = jniIds(env); diff --git a/services/incremental/BinderIncrementalService.cpp b/services/incremental/BinderIncrementalService.cpp index ebebf606b1cc4..fc8c6feac22b9 100644 --- a/services/incremental/BinderIncrementalService.cpp +++ b/services/incremental/BinderIncrementalService.cpp @@ -58,10 +58,10 @@ static bool incFsValid(const sp& vold) { return true; } -BinderIncrementalService::BinderIncrementalService(const sp& sm) - : mImpl(RealServiceManager(sm), getIncrementalDir()) {} +BinderIncrementalService::BinderIncrementalService(const sp& sm, JNIEnv* env) + : mImpl(RealServiceManager(sm, env), getIncrementalDir()) {} -BinderIncrementalService* BinderIncrementalService::start() { +BinderIncrementalService* BinderIncrementalService::start(JNIEnv* env) { if (!incFsEnabled()) { return nullptr; } @@ -81,7 +81,7 @@ BinderIncrementalService* BinderIncrementalService::start() { return nullptr; } - sp self(new BinderIncrementalService(sm)); + sp self(new BinderIncrementalService(sm, env)); status_t ret = sm->addService(String16{getServiceName()}, self); if (ret != android::OK) { return nullptr; @@ -290,8 +290,8 @@ binder::Status BinderIncrementalService::waitForNativeBinariesExtraction(int sto } // namespace android::os::incremental -jlong Incremental_IncrementalService_Start() { - return (jlong)android::os::incremental::BinderIncrementalService::start(); +jlong Incremental_IncrementalService_Start(JNIEnv* env) { + return (jlong)android::os::incremental::BinderIncrementalService::start(env); } void Incremental_IncrementalService_OnSystemReady(jlong self) { if (self) { diff --git a/services/incremental/BinderIncrementalService.h b/services/incremental/BinderIncrementalService.h index aca10ab6d5915..5a7d5da56f184 100644 --- a/services/incremental/BinderIncrementalService.h +++ b/services/incremental/BinderIncrementalService.h @@ -18,6 +18,7 @@ #include #include +#include #include "IncrementalService.h" #include "android/os/incremental/BnIncrementalService.h" @@ -28,9 +29,9 @@ namespace android::os::incremental { class BinderIncrementalService : public BnIncrementalService, public BinderService { public: - BinderIncrementalService(const sp& sm); + BinderIncrementalService(const sp& sm, JNIEnv* env); - static BinderIncrementalService* start(); + static BinderIncrementalService* start(JNIEnv* env); static const char16_t* getServiceName() { return u"incremental"; } status_t dump(int fd, const Vector& args) final; diff --git a/services/incremental/IncrementalService.cpp b/services/incremental/IncrementalService.cpp index 400388e932622..2c6bf0a80fe01 100644 --- a/services/incremental/IncrementalService.cpp +++ b/services/incremental/IncrementalService.cpp @@ -237,6 +237,7 @@ IncrementalService::IncrementalService(ServiceManagerWrapper&& sm, std::string_v mDataLoaderManager(sm.getDataLoaderManager()), mIncFs(sm.getIncFs()), mAppOpsManager(sm.getAppOpsManager()), + mJni(sm.getJni()), mIncrementalDir(rootDir) { if (!mVold) { LOG(FATAL) << "Vold service is unavailable"; @@ -249,7 +250,10 @@ IncrementalService::IncrementalService(ServiceManagerWrapper&& sm, std::string_v } mJobQueue.reserve(16); - mJobProcessor = std::thread([this]() { runJobProcessing(); }); + mJobProcessor = std::thread([this]() { + mJni->initializeForCurrentThread(); + runJobProcessing(); + }); mountExistingImages(); } @@ -1248,9 +1252,10 @@ bool IncrementalService::configureNativeBinaries(StorageId storage, std::string_ continue; } - jobQueue.emplace_back([this, zipFile, entry, ifs, libFileId, - libPath = std::move(targetLibPath), makeFileTs]() mutable { - extractZipFile(ifs, zipFile.get(), entry, libFileId, libPath, makeFileTs); + jobQueue.emplace_back([this, zipFile, entry, ifs = std::weak_ptr(ifs), + libFileId, libPath = std::move(targetLibPath), + makeFileTs]() mutable { + extractZipFile(ifs.lock(), zipFile.get(), entry, libFileId, libPath, makeFileTs); }); if (sEnablePerfLogging) { @@ -1296,6 +1301,11 @@ void IncrementalService::extractZipFile(const IfsMountPtr& ifs, ZipArchiveHandle ZipEntry& entry, const incfs::FileId& libFileId, std::string_view targetLibPath, Clock::time_point scheduledTs) { + if (!ifs) { + LOG(INFO) << "Skipping zip file " << targetLibPath << " extraction for an expired mount"; + return; + } + auto libName = path::basename(targetLibPath); auto startedTs = Clock::now(); diff --git a/services/incremental/IncrementalService.h b/services/incremental/IncrementalService.h index 4fdce4bd92dcc..e7705df633d12 100644 --- a/services/incremental/IncrementalService.h +++ b/services/incremental/IncrementalService.h @@ -289,10 +289,11 @@ private: Clock::time_point scheduledTs); private: - std::unique_ptr const mVold; - std::unique_ptr const mDataLoaderManager; - std::unique_ptr const mIncFs; - std::unique_ptr const mAppOpsManager; + const std::unique_ptr mVold; + const std::unique_ptr mDataLoaderManager; + const std::unique_ptr mIncFs; + const std::unique_ptr mAppOpsManager; + const std::unique_ptr mJni; const std::string mIncrementalDir; mutable std::mutex mLock; diff --git a/services/incremental/ServiceWrappers.cpp b/services/incremental/ServiceWrappers.cpp index 9f4192fbf5316..bf8e696a264cf 100644 --- a/services/incremental/ServiceWrappers.cpp +++ b/services/incremental/ServiceWrappers.cpp @@ -14,8 +14,11 @@ * limitations under the License. */ +#define LOG_TAG "IncrementalService" + #include "ServiceWrappers.h" +#include #include using namespace std::literals; @@ -25,8 +28,123 @@ namespace android::os::incremental { static constexpr auto kVoldServiceName = "vold"sv; static constexpr auto kDataLoaderManagerName = "dataloader_manager"sv; -RealServiceManager::RealServiceManager(sp serviceManager) - : mServiceManager(std::move(serviceManager)) {} +class RealVoldService : public VoldServiceWrapper { +public: + RealVoldService(const sp vold) : mInterface(std::move(vold)) {} + ~RealVoldService() = default; + binder::Status mountIncFs(const std::string& backingPath, const std::string& targetDir, + int32_t flags, + IncrementalFileSystemControlParcel* _aidl_return) const final { + return mInterface->mountIncFs(backingPath, targetDir, flags, _aidl_return); + } + binder::Status unmountIncFs(const std::string& dir) const final { + return mInterface->unmountIncFs(dir); + } + binder::Status bindMount(const std::string& sourceDir, + const std::string& targetDir) const final { + return mInterface->bindMount(sourceDir, targetDir); + } + binder::Status setIncFsMountOptions( + const ::android::os::incremental::IncrementalFileSystemControlParcel& control, + bool enableReadLogs) const final { + return mInterface->setIncFsMountOptions(control, enableReadLogs); + } + +private: + sp mInterface; +}; + +class RealDataLoaderManager : public DataLoaderManagerWrapper { +public: + RealDataLoaderManager(const sp manager) + : mInterface(manager) {} + ~RealDataLoaderManager() = default; + binder::Status initializeDataLoader(MountId mountId, const DataLoaderParamsParcel& params, + const FileSystemControlParcel& control, + const sp& listener, + bool* _aidl_return) const final { + return mInterface->initializeDataLoader(mountId, params, control, listener, _aidl_return); + } + binder::Status getDataLoader(MountId mountId, sp* _aidl_return) const final { + return mInterface->getDataLoader(mountId, _aidl_return); + } + binder::Status destroyDataLoader(MountId mountId) const final { + return mInterface->destroyDataLoader(mountId); + } + +private: + sp mInterface; +}; + +class RealAppOpsManager : public AppOpsManagerWrapper { +public: + ~RealAppOpsManager() = default; + binder::Status checkPermission(const char* permission, const char* operation, + const char* package) const final { + return android::incremental::CheckPermissionForDataDelivery(permission, operation, package); + } + void startWatchingMode(int32_t op, const String16& packageName, + const sp& callback) final { + mAppOpsManager.startWatchingMode(op, packageName, callback); + } + void stopWatchingMode(const sp& callback) final { + mAppOpsManager.stopWatchingMode(callback); + } + +private: + android::AppOpsManager mAppOpsManager; +}; + +class RealJniWrapper final : public JniWrapper { +public: + RealJniWrapper(JavaVM* jvm); + void initializeForCurrentThread() const final; + + static JavaVM* getJvm(JNIEnv* env); + +private: + JavaVM* const mJvm; +}; + +class RealIncFs : public IncFsWrapper { +public: + RealIncFs() = default; + ~RealIncFs() = default; + Control createControl(IncFsFd cmd, IncFsFd pendingReads, IncFsFd logs) const final { + return incfs::createControl(cmd, pendingReads, logs); + } + ErrorCode makeFile(const Control& control, std::string_view path, int mode, FileId id, + NewFileParams params) const final { + return incfs::makeFile(control, path, mode, id, params); + } + ErrorCode makeDir(const Control& control, std::string_view path, int mode) const final { + return incfs::makeDir(control, path, mode); + } + RawMetadata getMetadata(const Control& control, FileId fileid) const final { + return incfs::getMetadata(control, fileid); + } + RawMetadata getMetadata(const Control& control, std::string_view path) const final { + return incfs::getMetadata(control, path); + } + FileId getFileId(const Control& control, std::string_view path) const final { + return incfs::getFileId(control, path); + } + ErrorCode link(const Control& control, std::string_view from, std::string_view to) const final { + return incfs::link(control, from, to); + } + ErrorCode unlink(const Control& control, std::string_view path) const final { + return incfs::unlink(control, path); + } + base::unique_fd openForSpecialOps(const Control& control, FileId id) const final { + return base::unique_fd{incfs::openForSpecialOps(control, id).release()}; + } + ErrorCode writeBlocks(Span blocks) const final { + return incfs::writeBlocks(blocks); + } +}; + +RealServiceManager::RealServiceManager(sp serviceManager, JNIEnv* env) + : mServiceManager(std::move(serviceManager)), mJvm(RealJniWrapper::getJvm(env)) {} template sp RealServiceManager::getRealService(std::string_view serviceName) const { @@ -63,4 +181,62 @@ std::unique_ptr RealServiceManager::getAppOpsManager() { return std::make_unique(); } +std::unique_ptr RealServiceManager::getJni() { + return std::make_unique(mJvm); +} + +static JavaVM* getJavaVm(JNIEnv* env) { + CHECK(env); + JavaVM* jvm = nullptr; + env->GetJavaVM(&jvm); + CHECK(jvm); + return jvm; +} + +static JNIEnv* getJniEnv(JavaVM* vm) { + JNIEnv* env; + if (vm->GetEnv(reinterpret_cast(&env), JNI_VERSION_1_6) != JNI_OK) { + return nullptr; + } + return env; +} + +static JNIEnv* getOrAttachJniEnv(JavaVM* jvm) { + if (!jvm) { + LOG(ERROR) << "No JVM instance"; + return nullptr; + } + + JNIEnv* env = getJniEnv(jvm); + if (!env) { + int result = jvm->AttachCurrentThread(&env, nullptr); + if (result != JNI_OK) { + LOG(ERROR) << "JVM thread attach failed: " << result; + return nullptr; + } + struct VmDetacher { + VmDetacher(JavaVM* vm) : mVm(vm) {} + ~VmDetacher() { mVm->DetachCurrentThread(); } + + private: + JavaVM* const mVm; + }; + static thread_local VmDetacher detacher(jvm); + } + + return env; +} + +RealJniWrapper::RealJniWrapper(JavaVM* jvm) : mJvm(jvm) { + CHECK(!!mJvm) << "JVM is unavailable"; +} + +void RealJniWrapper::initializeForCurrentThread() const { + (void)getOrAttachJniEnv(mJvm); +} + +JavaVM* RealJniWrapper::getJvm(JNIEnv* env) { + return getJavaVm(env); +} + } // namespace android::os::incremental diff --git a/services/incremental/ServiceWrappers.h b/services/incremental/ServiceWrappers.h index 84bf1ffaf45cc..142bf2ef32f30 100644 --- a/services/incremental/ServiceWrappers.h +++ b/services/incremental/ServiceWrappers.h @@ -29,6 +29,7 @@ #include #include #include +#include #include #include @@ -93,6 +94,12 @@ public: virtual void stopWatchingMode(const sp& callback) = 0; }; +class JniWrapper { +public: + virtual ~JniWrapper() = default; + virtual void initializeForCurrentThread() const = 0; +}; + class ServiceManagerWrapper { public: virtual ~ServiceManagerWrapper() = default; @@ -100,127 +107,26 @@ public: virtual std::unique_ptr getDataLoaderManager() = 0; virtual std::unique_ptr getIncFs() = 0; virtual std::unique_ptr getAppOpsManager() = 0; + virtual std::unique_ptr getJni() = 0; }; // --- Real stuff --- -class RealVoldService : public VoldServiceWrapper { -public: - RealVoldService(const sp vold) : mInterface(std::move(vold)) {} - ~RealVoldService() = default; - binder::Status mountIncFs(const std::string& backingPath, const std::string& targetDir, - int32_t flags, - IncrementalFileSystemControlParcel* _aidl_return) const final { - return mInterface->mountIncFs(backingPath, targetDir, flags, _aidl_return); - } - binder::Status unmountIncFs(const std::string& dir) const final { - return mInterface->unmountIncFs(dir); - } - binder::Status bindMount(const std::string& sourceDir, - const std::string& targetDir) const final { - return mInterface->bindMount(sourceDir, targetDir); - } - binder::Status setIncFsMountOptions( - const ::android::os::incremental::IncrementalFileSystemControlParcel& control, - bool enableReadLogs) const final { - return mInterface->setIncFsMountOptions(control, enableReadLogs); - } - -private: - sp mInterface; -}; - -class RealDataLoaderManager : public DataLoaderManagerWrapper { -public: - RealDataLoaderManager(const sp manager) - : mInterface(manager) {} - ~RealDataLoaderManager() = default; - binder::Status initializeDataLoader(MountId mountId, const DataLoaderParamsParcel& params, - const FileSystemControlParcel& control, - const sp& listener, - bool* _aidl_return) const final { - return mInterface->initializeDataLoader(mountId, params, control, listener, _aidl_return); - } - binder::Status getDataLoader(MountId mountId, sp* _aidl_return) const final { - return mInterface->getDataLoader(mountId, _aidl_return); - } - binder::Status destroyDataLoader(MountId mountId) const final { - return mInterface->destroyDataLoader(mountId); - } - -private: - sp mInterface; -}; - -class RealAppOpsManager : public AppOpsManagerWrapper { -public: - ~RealAppOpsManager() = default; - binder::Status checkPermission(const char* permission, const char* operation, - const char* package) const final { - return android::incremental::CheckPermissionForDataDelivery(permission, operation, package); - } - void startWatchingMode(int32_t op, const String16& packageName, - const sp& callback) final { - mAppOpsManager.startWatchingMode(op, packageName, callback); - } - void stopWatchingMode(const sp& callback) final { - mAppOpsManager.stopWatchingMode(callback); - } - -private: - android::AppOpsManager mAppOpsManager; -}; - class RealServiceManager : public ServiceManagerWrapper { public: - RealServiceManager(sp serviceManager); + RealServiceManager(sp serviceManager, JNIEnv* env); ~RealServiceManager() = default; std::unique_ptr getVoldService() final; std::unique_ptr getDataLoaderManager() final; std::unique_ptr getIncFs() final; std::unique_ptr getAppOpsManager() final; + std::unique_ptr getJni() final; private: template sp getRealService(std::string_view serviceName) const; sp mServiceManager; -}; - -class RealIncFs : public IncFsWrapper { -public: - RealIncFs() = default; - ~RealIncFs() = default; - Control createControl(IncFsFd cmd, IncFsFd pendingReads, IncFsFd logs) const final { - return incfs::createControl(cmd, pendingReads, logs); - } - ErrorCode makeFile(const Control& control, std::string_view path, int mode, FileId id, - NewFileParams params) const final { - return incfs::makeFile(control, path, mode, id, params); - } - ErrorCode makeDir(const Control& control, std::string_view path, int mode) const final { - return incfs::makeDir(control, path, mode); - } - RawMetadata getMetadata(const Control& control, FileId fileid) const final { - return incfs::getMetadata(control, fileid); - } - RawMetadata getMetadata(const Control& control, std::string_view path) const final { - return incfs::getMetadata(control, path); - } - FileId getFileId(const Control& control, std::string_view path) const final { - return incfs::getFileId(control, path); - } - ErrorCode link(const Control& control, std::string_view from, std::string_view to) const final { - return incfs::link(control, from, to); - } - ErrorCode unlink(const Control& control, std::string_view path) const final { - return incfs::unlink(control, path); - } - base::unique_fd openForSpecialOps(const Control& control, FileId id) const final { - return base::unique_fd{incfs::openForSpecialOps(control, id).release()}; - } - ErrorCode writeBlocks(Span blocks) const final { - return incfs::writeBlocks(blocks); - } + JavaVM* const mJvm; }; } // namespace android::os::incremental diff --git a/services/incremental/include/incremental_service.h b/services/incremental/include/incremental_service.h index 4a34b11261b9b..321387531694a 100644 --- a/services/incremental/include/incremental_service.h +++ b/services/incremental/include/incremental_service.h @@ -24,7 +24,7 @@ __BEGIN_DECLS #define INCREMENTAL_LIBRARY_NAME "service.incremental.so" -jlong Incremental_IncrementalService_Start(); +jlong Incremental_IncrementalService_Start(JNIEnv* env); void Incremental_IncrementalService_OnSystemReady(jlong self); void Incremental_IncrementalService_OnDump(jlong self, jint fd); diff --git a/services/incremental/test/IncrementalServiceTest.cpp b/services/incremental/test/IncrementalServiceTest.cpp index 991131950531b..117dca8c37b3a 100644 --- a/services/incremental/test/IncrementalServiceTest.cpp +++ b/services/incremental/test/IncrementalServiceTest.cpp @@ -261,28 +261,39 @@ public: sp mStoredCallback; }; +class MockJniWrapper : public JniWrapper { +public: + MOCK_CONST_METHOD0(initializeForCurrentThread, void()); + + MockJniWrapper() { EXPECT_CALL(*this, initializeForCurrentThread()).Times(1); } +}; + class MockServiceManager : public ServiceManagerWrapper { public: MockServiceManager(std::unique_ptr vold, - std::unique_ptr manager, + std::unique_ptr dataLoaderManager, std::unique_ptr incfs, - std::unique_ptr appOpsManager) + std::unique_ptr appOpsManager, + std::unique_ptr jni) : mVold(std::move(vold)), - mDataLoaderManager(std::move(manager)), + mDataLoaderManager(std::move(dataLoaderManager)), mIncFs(std::move(incfs)), - mAppOpsManager(std::move(appOpsManager)) {} + mAppOpsManager(std::move(appOpsManager)), + mJni(std::move(jni)) {} std::unique_ptr getVoldService() final { return std::move(mVold); } std::unique_ptr getDataLoaderManager() final { return std::move(mDataLoaderManager); } std::unique_ptr getIncFs() final { return std::move(mIncFs); } std::unique_ptr getAppOpsManager() final { return std::move(mAppOpsManager); } + std::unique_ptr getJni() final { return std::move(mJni); } private: std::unique_ptr mVold; std::unique_ptr mDataLoaderManager; std::unique_ptr mIncFs; std::unique_ptr mAppOpsManager; + std::unique_ptr mJni; }; // --- IncrementalServiceTest --- @@ -298,11 +309,15 @@ public: mIncFs = incFs.get(); auto appOps = std::make_unique>(); mAppOpsManager = appOps.get(); + auto jni = std::make_unique>(); + mJni = jni.get(); mIncrementalService = std::make_unique(MockServiceManager(std::move(vold), - std::move(dataloaderManager), + std::move( + dataloaderManager), std::move(incFs), - std::move(appOps)), + std::move(appOps), + std::move(jni)), mRootDir.path); mDataLoaderParcel.packageName = "com.test"; mDataLoaderParcel.arguments = "uri"; @@ -336,6 +351,7 @@ protected: NiceMock* mIncFs; NiceMock* mDataLoaderManager; NiceMock* mAppOpsManager; + NiceMock* mJni; std::unique_ptr mIncrementalService; TemporaryDir mRootDir; DataLoaderParamsParcel mDataLoaderParcel;