From 7d3246a195171989836859f298ac38a9e3092925 Mon Sep 17 00:00:00 2001 From: Ricky Wai Date: Wed, 24 Feb 2021 16:02:22 +0000 Subject: [PATCH] Check and create data and obb dirs after starting zygote process. When data or obb is not cached, it may slow down app starting speed as we wait the directory is paged before starting zygote. To improve this, we postpone the dir check after starting zygote process in ProcessList, so the directory paging and creation can be done in background without blocking app starting. As it's possible that dir is not created when zygote needs it, we also a busy waiting check in zygote to ensure data and obb dirs are created before we do our work. Bug: 177248242 Test: Able to boot device and start app after enabling app storage data isolation flag. Change-Id: I892b7a1a4f8c38331546f9129b8fb43333093aec Merged-In: I892b7a1a4f8c38331546f9129b8fb43333093aec (cherry picked from commit ff269b9e8f03a5e0698f2e3569b6c45ca4447136) --- .../os/storage/StorageManagerInternal.java | 5 +++ core/jni/com_android_internal_os_Zygote.cpp | 40 +++++++++++++++++++ .../android/server/StorageManagerService.java | 7 ++++ .../com/android/server/am/ProcessList.java | 18 ++++++--- 4 files changed, 65 insertions(+), 5 deletions(-) diff --git a/core/java/android/os/storage/StorageManagerInternal.java b/core/java/android/os/storage/StorageManagerInternal.java index 396ba2d3cea51..82c4c715f4b0f 100644 --- a/core/java/android/os/storage/StorageManagerInternal.java +++ b/core/java/android/os/storage/StorageManagerInternal.java @@ -51,6 +51,11 @@ public abstract class StorageManagerInternal { void onReset(IVold vold); } + /** + * Return true if fuse is mounted. + */ + public abstract boolean isFuseMounted(int userId); + /** * Create storage directories if it does not exist. * Return true if the directories were setup correctly, otherwise false. diff --git a/core/jni/com_android_internal_os_Zygote.cpp b/core/jni/com_android_internal_os_Zygote.cpp index 836074f1d5f74..cf96fe63f82c7 100644 --- a/core/jni/com_android_internal_os_Zygote.cpp +++ b/core/jni/com_android_internal_os_Zygote.cpp @@ -184,6 +184,17 @@ static constexpr int PROCESS_PRIORITY_MIN = 19; /** The numeric value for the normal priority a process should have. */ static constexpr int PROCESS_PRIORITY_DEFAULT = 0; +/** Exponential back off parameters for storage dir check. */ +static constexpr unsigned int STORAGE_DIR_CHECK_RETRY_MULTIPLIER = 2; +static constexpr unsigned int STORAGE_DIR_CHECK_INIT_INTERVAL_US = 50; +static constexpr unsigned int STORAGE_DIR_CHECK_MAX_INTERVAL_US = 1000; +/** + * Lower bound time we allow storage dir check to sleep. + * If it exceeds 2s, PROC_START_TIMEOUT_MSG will kill the starting app anyway, + * so it's fine to assume max retries is 5 mins. + */ +static constexpr int STORAGE_DIR_CHECK_TIMEOUT_US = 1000 * 60 * 5; + /** * A helper class containing accounting information for USAPs. */ @@ -1458,6 +1469,31 @@ static void isolateJitProfile(JNIEnv* env, jobjectArray pkg_data_info_list, } } +static void WaitUntilDirReady(const std::string& target, fail_fn_t fail_fn) { + unsigned int sleepIntervalUs = STORAGE_DIR_CHECK_INIT_INTERVAL_US; + + // This is just an approximate value as it doesn't need to be very accurate. + unsigned int sleepTotalUs = 0; + + const char* dir_path = target.c_str(); + while (sleepTotalUs < STORAGE_DIR_CHECK_TIMEOUT_US) { + if (access(dir_path, F_OK) == 0) { + return; + } + // Failed, so we add exponential backoff and retry + usleep(sleepIntervalUs); + sleepTotalUs += sleepIntervalUs; + sleepIntervalUs = std::min( + sleepIntervalUs * STORAGE_DIR_CHECK_RETRY_MULTIPLIER, + STORAGE_DIR_CHECK_MAX_INTERVAL_US); + } + // Last chance and get the latest errno if it fails. + if (access(dir_path, F_OK) == 0) { + return; + } + fail_fn(CREATE_ERROR("Error dir is not ready %s: %s", dir_path, strerror(errno))); +} + static void BindMountStorageToLowerFs(const userid_t user_id, const uid_t uid, const char* dir_name, const char* package, fail_fn_t fail_fn) { bool hasSdcardFs = IsSdcardfsUsed(); @@ -1468,6 +1504,10 @@ static void BindMountStorageToLowerFs(const userid_t user_id, const uid_t uid, source = StringPrintf("/mnt/pass_through/%d/emulated/%d/%s/%s", user_id, user_id, dir_name, package); } + + // Directory might be not ready, as prepareStorageDirs() is running asynchronously in ProcessList, + // so wait until dir is created. + WaitUntilDirReady(source, fail_fn); std::string target = StringPrintf("/storage/emulated/%d/%s/%s", user_id, dir_name, package); // As the parent is mounted as tmpfs, we need to create the target dir here. diff --git a/services/core/java/com/android/server/StorageManagerService.java b/services/core/java/com/android/server/StorageManagerService.java index 27b648e53a38c..740a1c16a4868 100644 --- a/services/core/java/com/android/server/StorageManagerService.java +++ b/services/core/java/com/android/server/StorageManagerService.java @@ -4553,6 +4553,13 @@ class StorageManagerService extends IStorageManager.Stub private final List mResetListeners = new ArrayList<>(); + @Override + public boolean isFuseMounted(int userId) { + synchronized (mLock) { + return mFuseMountedUser.contains(userId); + } + } + /** * Check if fuse is running in target user, if it's running then setup its storage dirs. * Return true if storage dirs are mounted. diff --git a/services/core/java/com/android/server/am/ProcessList.java b/services/core/java/com/android/server/am/ProcessList.java index 38330fe770fb3..ed8d696f98c4e 100644 --- a/services/core/java/com/android/server/am/ProcessList.java +++ b/services/core/java/com/android/server/am/ProcessList.java @@ -2315,11 +2315,12 @@ public final class ProcessList { StorageManagerInternal storageManagerInternal = LocalServices.getService( StorageManagerInternal.class); if (needsStorageDataIsolation(storageManagerInternal, app)) { - bindMountAppStorageDirs = true; - if (pkgDataInfoMap == null || - !storageManagerInternal.prepareStorageDirs(userId, pkgDataInfoMap.keySet(), - app.processName)) { - // Cannot prepare Android/app and Android/obb directory or inode == 0, + // We will run prepareStorageDirs() after we trigger zygote fork, so it won't + // slow down app starting speed as those dirs might not be cached. + if (pkgDataInfoMap != null && storageManagerInternal.isFuseMounted(userId)) { + bindMountAppStorageDirs = true; + } else { + // Fuse is not mounted or inode == 0, // so we won't mount it in zygote, but resume the mount after unlocking device. app.setBindMountPending(true); bindMountAppStorageDirs = false; @@ -2367,6 +2368,13 @@ public final class ProcessList { allowlistedAppDataInfoMap, bindMountAppsData, bindMountAppStorageDirs, new String[]{PROC_START_SEQ_IDENT + app.getStartSeq()}); } + // This runs after Process.start() as this method may block app process starting time + // if dir is not cached. Running this method after Process.start() can make it + // cache the dir asynchronously, so zygote can use it without waiting for it. + if (bindMountAppStorageDirs) { + storageManagerInternal.prepareStorageDirs(userId, pkgDataInfoMap.keySet(), + app.processName); + } checkSlow(startTime, "startProcess: returned from zygote!"); return startResult; } finally {