From 1985e314e27987d8adb4a70f8895442aa34a96c5 Mon Sep 17 00:00:00 2001 From: Zim Date: Tue, 2 Mar 2021 09:30:41 +0000 Subject: [PATCH 1/2] Implement ANR delay with app IO blocked reason The ExternalStorageService now notifies the system_server of app IO blocked reasons, specifically transcoding. If an ANR occurs, we query these reasons for the uid to modify the ANR dialog behavior Bug: 170486601 Test: Manual Change-Id: I86f800c4a6c565a7bced0f2a5c5da7ba6c048168 --- .../os/storage/StorageManagerInternal.java | 7 +++ .../android/server/StorageManagerService.java | 60 ++++++++++++++++--- .../server/storage/StorageUserConnection.java | 12 +++- 3 files changed, 69 insertions(+), 10 deletions(-) diff --git a/core/java/android/os/storage/StorageManagerInternal.java b/core/java/android/os/storage/StorageManagerInternal.java index b12bb2ece4c2f..396ba2d3cea51 100644 --- a/core/java/android/os/storage/StorageManagerInternal.java +++ b/core/java/android/os/storage/StorageManagerInternal.java @@ -20,6 +20,7 @@ import android.annotation.NonNull; import android.annotation.Nullable; import android.os.IVold; +import java.util.List; import java.util.Set; /** @@ -112,4 +113,10 @@ public abstract class StorageManagerInternal { * @param bytes number of bytes which need to be freed */ public abstract void freeCache(@Nullable String volumeUuid, long bytes); + + /** + * Returns the {@link VolumeInfo#getId()} values for the volumes matching + * {@link VolumeInfo#isPrimary()} + */ + public abstract List getPrimaryVolumeIds(); } diff --git a/services/core/java/com/android/server/StorageManagerService.java b/services/core/java/com/android/server/StorageManagerService.java index 233a50d417ad1..5486ca67dd97f 100644 --- a/services/core/java/com/android/server/StorageManagerService.java +++ b/services/core/java/com/android/server/StorageManagerService.java @@ -573,6 +573,12 @@ class StorageManagerService extends IStorageManager.Stub */ private static final int PBKDF2_HASH_ROUNDS = 1024; + private static final String ANR_DELAY_MILLIS_DEVICE_CONFIG_KEY = + "anr_delay_millis"; + + private static final String ANR_DELAY_NOTIFY_EXTERNAL_STORAGE_SERVICE_DEVICE_CONFIG_KEY = + "anr_delay_notify_external_storage_service"; + /** * Mounted OBB tracking information. Used to track the current state of all * OBBs. @@ -943,25 +949,51 @@ class StorageManagerService extends IStorageManager.Stub } } - // TODO(b/170486601): Check transcoding status based on events pushed from the MediaProvider private class ExternalStorageServiceAnrController implements AnrController { @Override public long getAnrDelayMillis(String packageName, int uid) { - int delay = SystemProperties.getInt("sys.fuse.transcode_anr_delay", 0); - Log.d(TAG, "getAnrDelayMillis: " + packageName + ". Delaying for " + delay + "ms"); + if (!isAppIoBlocked(uid)) { + return 0; + } + + int delay = DeviceConfig.getInt(DeviceConfig.NAMESPACE_STORAGE_NATIVE_BOOT, + ANR_DELAY_MILLIS_DEVICE_CONFIG_KEY, 0); + Slog.v(TAG, "getAnrDelayMillis for " + packageName + ". " + delay + "ms"); return delay; } @Override public void onAnrDelayStarted(String packageName, int uid) { - Log.d(TAG, "onAnrDelayStarted: " + packageName); + if (!isAppIoBlocked(uid)) { + return; + } + + boolean notifyExternalStorageService = DeviceConfig.getBoolean( + DeviceConfig.NAMESPACE_STORAGE_NATIVE_BOOT, + ANR_DELAY_NOTIFY_EXTERNAL_STORAGE_SERVICE_DEVICE_CONFIG_KEY, true); + if (notifyExternalStorageService) { + Slog.d(TAG, "onAnrDelayStarted for " + packageName + + ". Notifying external storage service"); + try { + mStorageSessionController.notifyAnrDelayStarted(packageName, uid, 0 /* tid */, + StorageManager.APP_IO_BLOCKED_REASON_TRANSCODING); + } catch (ExternalStorageServiceException e) { + Slog.e(TAG, "Failed to notify ANR delay started for " + packageName, e); + } + } else { + // TODO(b/170973510): Implement framework spinning dialog for ANR delay + } } @Override public boolean onAnrDelayCompleted(String packageName, int uid) { - boolean show = SystemProperties.getBoolean("sys.fuse.transcode_anr_dialog_show", true); - Log.d(TAG, "onAnrDelayCompleted: " + packageName + ". Show: " + show); - return show; + if (isAppIoBlocked(uid)) { + Slog.d(TAG, "onAnrDelayCompleted for " + packageName + ". Showing ANR dialog..."); + return true; + } else { + Slog.d(TAG, "onAnrDelayCompleted for " + packageName + ". Skipping ANR dialog..."); + return false; + } } } @@ -4637,5 +4669,19 @@ class StorageManagerService extends IStorageManager.Stub Binder.restoreCallingIdentity(token); } } + + @Override + public List getPrimaryVolumeIds() { + final List primaryVolumeIds = new ArrayList<>(); + synchronized (mLock) { + for (int i = 0; i < mVolumes.size(); i++) { + final VolumeInfo vol = mVolumes.valueAt(i); + if (vol.isPrimary()) { + primaryVolumeIds.add(vol.getId()); + } + } + } + return primaryVolumeIds; + } } } diff --git a/services/core/java/com/android/server/storage/StorageUserConnection.java b/services/core/java/com/android/server/storage/StorageUserConnection.java index d2b05c0914d7e..797f0f2e677be 100644 --- a/services/core/java/com/android/server/storage/StorageUserConnection.java +++ b/services/core/java/com/android/server/storage/StorageUserConnection.java @@ -53,6 +53,7 @@ import java.io.IOException; import java.util.ArrayList; import java.util.HashMap; import java.util.HashSet; +import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Set; @@ -72,6 +73,7 @@ public final class StorageUserConnection { private final Context mContext; private final int mUserId; private final StorageSessionController mSessionController; + private final StorageManagerInternal mSmInternal; private final ActiveConnection mActiveConnection = new ActiveConnection(); @GuardedBy("mLock") private final Map mSessions = new HashMap<>(); @GuardedBy("mLock") private final Set mUidsBlockedOnIo = new ArraySet<>(); @@ -81,6 +83,7 @@ public final class StorageUserConnection { mContext = Objects.requireNonNull(context); mUserId = Preconditions.checkArgumentNonnegative(userId); mSessionController = controller; + mSmInternal = LocalServices.getService(StorageManagerInternal.class); mHandlerThread = new HandlerThread("StorageUserConnectionThread-" + mUserId); mHandlerThread.start(); } @@ -152,9 +155,13 @@ public final class StorageUserConnection { */ public void notifyAnrDelayStarted(String packageName, int uid, int tid, int reason) throws ExternalStorageServiceException { + List primarySessionIds = mSmInternal.getPrimaryVolumeIds(); synchronized (mSessionsLock) { for (String sessionId : mSessions.keySet()) { - mActiveConnection.notifyAnrDelayStarted(packageName, uid, tid, reason); + if (primarySessionIds.contains(sessionId)) { + mActiveConnection.notifyAnrDelayStarted(packageName, uid, tid, reason); + return; + } } } } @@ -201,8 +208,7 @@ public final class StorageUserConnection { return; } } - StorageManagerInternal sm = LocalServices.getService(StorageManagerInternal.class); - sm.resetUser(mUserId); + mSmInternal.resetUser(mUserId); } /** From e3a9d5f065751a5ca1fc6364123db5a12e0b7165 Mon Sep 17 00:00:00 2001 From: Zim Date: Wed, 3 Mar 2021 13:13:26 +0000 Subject: [PATCH 2/2] Avoid waiting for the async notifyAnrDelayStarted The binder interface is already one-way, but previously, we were blocking for x seconds for a result. Now, notifyAnrDelayStarted is purely async without any blocking. It is also a best effort request because we don't attempt to connect if the service is unavailable. Test: Manual Bug: 170486601 Change-Id: I2a817d71970c0c03fe0e3e3d3a4871396f887c93 --- .../storage/ExternalStorageService.java | 7 ++-- .../storage/IExternalStorageService.aidl | 3 +- .../server/storage/StorageUserConnection.java | 32 +++++++++++++++---- 3 files changed, 29 insertions(+), 13 deletions(-) diff --git a/core/java/android/service/storage/ExternalStorageService.java b/core/java/android/service/storage/ExternalStorageService.java index 1e07a8748af9c..bbe184bd1a8c0 100644 --- a/core/java/android/service/storage/ExternalStorageService.java +++ b/core/java/android/service/storage/ExternalStorageService.java @@ -239,14 +239,13 @@ public abstract class ExternalStorageService extends Service { } @Override - public void notifyAnrDelayStarted(String packageName, int uid, int tid, int reason, - RemoteCallback callback) throws RemoteException { + public void notifyAnrDelayStarted(String packageName, int uid, int tid, int reason) + throws RemoteException { mHandler.post(() -> { try { onAnrDelayStarted(packageName, uid, tid, reason); - sendResult(packageName, null /* throwable */, callback); } catch (Throwable t) { - sendResult(packageName, t, callback); + // Ignored } }); } diff --git a/core/java/android/service/storage/IExternalStorageService.aidl b/core/java/android/service/storage/IExternalStorageService.aidl index ba98efa58f7c0..0766b754e57de 100644 --- a/core/java/android/service/storage/IExternalStorageService.aidl +++ b/core/java/android/service/storage/IExternalStorageService.aidl @@ -32,6 +32,5 @@ oneway interface IExternalStorageService in RemoteCallback callback); void freeCache(@utf8InCpp String sessionId, in String volumeUuid, long bytes, in RemoteCallback callback); - void notifyAnrDelayStarted(String packageName, int uid, int tid, int reason, - in RemoteCallback callback); + void notifyAnrDelayStarted(String packageName, int uid, int tid, int reason); } \ No newline at end of file diff --git a/services/core/java/com/android/server/storage/StorageUserConnection.java b/services/core/java/com/android/server/storage/StorageUserConnection.java index 797f0f2e677be..9409eb5d1ad99 100644 --- a/services/core/java/com/android/server/storage/StorageUserConnection.java +++ b/services/core/java/com/android/server/storage/StorageUserConnection.java @@ -59,6 +59,7 @@ import java.util.Objects; import java.util.Set; import java.util.concurrent.CompletableFuture; import java.util.concurrent.TimeUnit; +import java.util.function.Consumer; /** * Controls the lifecycle of the {@link ActiveConnection} to an {@link ExternalStorageService} @@ -323,6 +324,23 @@ public final class StorageUserConnection { } } + private void asyncBestEffort(Consumer consumer) { + synchronized (mLock) { + if (mRemoteFuture == null) { + Slog.w(TAG, "Dropping async request service is not bound"); + return; + } + + IExternalStorageService service = mRemoteFuture.getNow(null); + if (service == null) { + Slog.w(TAG, "Dropping async request service is not connected"); + return; + } + + consumer.accept(service); + } + } + private void waitForAsyncVoid(AsyncStorageServiceCall asyncCall) throws Exception { CompletableFuture opFuture = new CompletableFuture<>(); RemoteCallback callback = new RemoteCallback(result -> setResult(result, opFuture)); @@ -407,13 +425,13 @@ public final class StorageUserConnection { public void notifyAnrDelayStarted(String packgeName, int uid, int tid, int reason) throws ExternalStorageServiceException { - try { - waitForAsyncVoid((service, callback) -> - service.notifyAnrDelayStarted(packgeName, uid, tid, reason, callback)); - } catch (Exception e) { - throw new ExternalStorageServiceException("Failed to notify ANR delay started: " - + packgeName, e); - } + asyncBestEffort(service -> { + try { + service.notifyAnrDelayStarted(packgeName, uid, tid, reason); + } catch (RemoteException e) { + Slog.w(TAG, "Failed to notify ANR delay started", e); + } + }); } private void setResult(Bundle result, CompletableFuture future) {