Freezer: don't skip processes with file locks
To prevent deadlock, freezer had to skip all processes holding any file
locks. As SQLite is using exclusive locks, a lot of applications won't
be frozen. The new logic will detect the lock dependencies and only
unfreeze the processes that block others.
This patch also adds a rewind() function to ProcFileReader class so that
ProcLocksReader doesn't open/close procfs nodes and create new instance
each time /proc/locks is parsed.
Bug: 176927978
Test: atest ProcFileReaderTest
Test: atest ProcLocksReaderTest
Test: verified processes with file locks could be frozen
Test: verified processes with blocking file locks were unfrozen
Change-Id: Ic2c89e6dcc9ee186840731c911be2aa3247b1352
diff --git a/core/java/com/android/internal/os/ProcLocksReader.java b/core/java/com/android/internal/os/ProcLocksReader.java
index bd3115fc5..2143bc1 100644
--- a/core/java/com/android/internal/os/ProcLocksReader.java
+++ b/core/java/com/android/internal/os/ProcLocksReader.java
@@ -18,8 +18,6 @@
import com.android.internal.util.ProcFileReader;
-import libcore.io.IoUtils;
-
import java.io.FileInputStream;
import java.io.IOException;
@@ -36,6 +34,7 @@
*/
public class ProcLocksReader {
private final String mPath;
+ private ProcFileReader mReader = null;
public ProcLocksReader() {
mPath = "/proc/locks";
@@ -46,44 +45,54 @@
}
/**
- * Checks if a process corresponding to a specific pid owns any file locks.
- * @param pid The process ID for which we want to know the existence of file locks.
- * @return true If the process holds any file locks, false otherwise.
- * @throws IOException if /proc/locks can't be accessed.
+ * This interface is for AMS to run callback function on every processes one by one
+ * that hold file locks blocking other processes.
*/
- public boolean hasFileLocks(int pid) throws Exception {
- ProcFileReader reader = null;
+ public interface ProcLocksReaderCallback {
+ /**
+ * Call the callback function of handleBlockingFileLocks().
+ * @param pid Each process that hold file locks blocking other processes.
+ */
+ void onBlockingFileLock(int pid);
+ }
+
+ /**
+ * Checks if a process corresponding to a specific pid owns any file locks.
+ * @param callback Callback function, accepting pid as the input parameter.
+ * @throws IOException if /proc/locks can't be accessed or correctly parsed.
+ */
+ public void handleBlockingFileLocks(ProcLocksReaderCallback callback) throws IOException {
long last = -1;
long id; // ordinal position of the lock in the list
- int owner; // the PID of the process that owns the lock
+ int owner = -1; // the PID of the process that owns the lock
+ int pid = -1; // the PID of the process blocking others
- try {
- reader = new ProcFileReader(new FileInputStream(mPath));
-
- while (reader.hasMoreData()) {
- id = reader.nextLong(true); // lock id
- if (id == last) {
- reader.finishLine(); // blocked lock
- continue;
- }
-
- reader.nextIgnored(); // lock type: POSIX?
- reader.nextIgnored(); // lock type: MANDATORY?
- reader.nextIgnored(); // lock type: RW?
-
- owner = reader.nextInt(); // pid
- if (owner == pid) {
- return true;
- }
- reader.finishLine();
- last = id;
- }
- } catch (IOException e) {
- // TODO: let ProcFileReader log the failed line
- throw new Exception("Exception parsing /proc/locks");
- } finally {
- IoUtils.closeQuietly(reader);
+ if (mReader == null) {
+ mReader = new ProcFileReader(new FileInputStream(mPath));
+ } else {
+ mReader.rewind();
}
- return false;
+
+ while (mReader.hasMoreData()) {
+ id = mReader.nextLong(true); // lock id
+ if (id == last) {
+ mReader.finishLine(); // blocked lock
+ if (pid < 0) {
+ pid = owner; // get pid from the previous line
+ callback.onBlockingFileLock(pid);
+ }
+ continue;
+ } else {
+ pid = -1; // a new lock
+ }
+
+ mReader.nextIgnored(); // lock type: POSIX?
+ mReader.nextIgnored(); // lock type: MANDATORY?
+ mReader.nextIgnored(); // lock type: RW?
+
+ owner = mReader.nextInt(); // pid
+ mReader.finishLine();
+ last = id;
+ }
}
}
diff --git a/core/java/com/android/internal/util/ProcFileReader.java b/core/java/com/android/internal/util/ProcFileReader.java
index 0dd8ad8..b726d5d 100644
--- a/core/java/com/android/internal/util/ProcFileReader.java
+++ b/core/java/com/android/internal/util/ProcFileReader.java
@@ -17,6 +17,7 @@
package com.android.internal.util;
import java.io.Closeable;
+import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.net.ProtocolException;
@@ -47,6 +48,9 @@
public ProcFileReader(InputStream stream, int bufferSize) throws IOException {
mStream = stream;
mBuffer = new byte[bufferSize];
+ if (stream.markSupported()) {
+ mStream.mark(0);
+ }
// read enough to answer hasMoreData
fillBuf();
@@ -257,6 +261,24 @@
}
}
+ /**
+ * Reset file position and internal buffer
+ * @throws IOException
+ */
+ public void rewind() throws IOException {
+ if (mStream instanceof FileInputStream) {
+ ((FileInputStream) mStream).getChannel().position(0);
+ } else if (mStream.markSupported()) {
+ mStream.reset();
+ } else {
+ throw new IOException("The InputStream is NOT markable");
+ }
+
+ mTail = 0;
+ mLineFinished = false;
+ fillBuf();
+ }
+
@Override
public void close() throws IOException {
mStream.close();
diff --git a/core/tests/coretests/src/com/android/internal/os/ProcLocksReaderTest.java b/core/tests/coretests/src/com/android/internal/os/ProcLocksReaderTest.java
index d800c2c..b34554c 100644
--- a/core/tests/coretests/src/com/android/internal/os/ProcLocksReaderTest.java
+++ b/core/tests/coretests/src/com/android/internal/os/ProcLocksReaderTest.java
@@ -16,7 +16,6 @@
package com.android.internal.os;
-import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import android.content.Context;
@@ -33,11 +32,14 @@
import java.io.File;
import java.nio.file.Files;
+import java.util.ArrayList;
@SmallTest
@RunWith(AndroidJUnit4.class)
-public class ProcLocksReaderTest {
+public class ProcLocksReaderTest implements
+ ProcLocksReader.ProcLocksReaderCallback {
private File mProcDirectory;
+ private ArrayList<Integer> mPids = new ArrayList<>();
@Before
public void setUp() {
@@ -55,14 +57,12 @@
String simpleLocks =
"1: POSIX ADVISORY READ 18403 fd:09:9070 1073741826 1073742335\n" +
"2: POSIX ADVISORY WRITE 18292 fd:09:34062 0 EOF\n";
- assertFalse(runHasFileLocks(simpleLocks, 18402));
- assertFalse(runHasFileLocks(simpleLocks, 18404));
- assertTrue(runHasFileLocks(simpleLocks, 18403));
- assertTrue(runHasFileLocks(simpleLocks, 18292));
+ runHandleBlockingFileLocks(simpleLocks);
+ assertTrue(mPids.isEmpty());
}
@Test
- public void testRunBlockedLocks() throws Exception {
+ public void testRunBlockingLocks() throws Exception {
String blockedLocks =
"1: POSIX ADVISORY READ 18403 fd:09:9070 1073741826 1073742335\n" +
"2: POSIX ADVISORY WRITE 18292 fd:09:34062 0 EOF\n" +
@@ -70,21 +70,43 @@
"2: -> POSIX ADVISORY WRITE 18293 fd:09:34062 0 EOF\n" +
"3: POSIX ADVISORY READ 3888 fd:09:13992 128 128\n" +
"4: POSIX ADVISORY READ 3888 fd:09:14230 1073741826 1073742335\n";
- assertFalse(runHasFileLocks(blockedLocks, 18402));
- assertFalse(runHasFileLocks(blockedLocks, 18404));
- assertTrue(runHasFileLocks(blockedLocks, 18403));
- assertTrue(runHasFileLocks(blockedLocks, 18292));
-
- assertFalse(runHasFileLocks(blockedLocks, 18291));
- assertFalse(runHasFileLocks(blockedLocks, 18293));
- assertTrue(runHasFileLocks(blockedLocks, 3888));
+ runHandleBlockingFileLocks(blockedLocks);
+ assertTrue(mPids.remove(0).equals(18292));
+ assertTrue(mPids.isEmpty());
}
- private boolean runHasFileLocks(String fileContents, int pid) throws Exception {
+ @Test
+ public void testRunMultipleBlockingLocks() throws Exception {
+ String blockedLocks =
+ "1: POSIX ADVISORY READ 18403 fd:09:9070 1073741826 1073742335\n" +
+ "2: POSIX ADVISORY WRITE 18292 fd:09:34062 0 EOF\n" +
+ "2: -> POSIX ADVISORY WRITE 18291 fd:09:34062 0 EOF\n" +
+ "2: -> POSIX ADVISORY WRITE 18293 fd:09:34062 0 EOF\n" +
+ "3: POSIX ADVISORY READ 3888 fd:09:13992 128 128\n" +
+ "4: FLOCK ADVISORY WRITE 3840 fe:01:5111809 0 EOF\n" +
+ "4: -> FLOCK ADVISORY WRITE 3841 fe:01:5111809 0 EOF\n" +
+ "5: POSIX ADVISORY READ 3888 fd:09:14230 1073741826 1073742335\n";
+ runHandleBlockingFileLocks(blockedLocks);
+ assertTrue(mPids.remove(0).equals(18292));
+ assertTrue(mPids.remove(0).equals(3840));
+ assertTrue(mPids.isEmpty());
+ }
+
+ private void runHandleBlockingFileLocks(String fileContents) throws Exception {
File tempFile = File.createTempFile("locks", null, mProcDirectory);
Files.write(tempFile.toPath(), fileContents.getBytes());
- boolean result = new ProcLocksReader(tempFile.toString()).hasFileLocks(pid);
+ mPids.clear();
+ new ProcLocksReader(tempFile.toString()).handleBlockingFileLocks(this);
Files.delete(tempFile.toPath());
- return result;
+ }
+
+ /**
+ * Call the callback function of handleBlockingFileLocks().
+ *
+ * @param pid Each process that hold file locks blocking other processes.
+ */
+ @Override
+ public void onBlockingFileLock(int pid) {
+ mPids.add(pid);
}
}
diff --git a/core/tests/utiltests/src/com/android/internal/util/ProcFileReaderTest.java b/core/tests/utiltests/src/com/android/internal/util/ProcFileReaderTest.java
index 0532628..b932760 100644
--- a/core/tests/utiltests/src/com/android/internal/util/ProcFileReaderTest.java
+++ b/core/tests/utiltests/src/com/android/internal/util/ProcFileReaderTest.java
@@ -19,8 +19,11 @@
import android.test.AndroidTestCase;
import java.io.ByteArrayInputStream;
+import java.io.File;
+import java.io.FileInputStream;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
/**
* Tests for {@link ProcFileReader}.
@@ -206,6 +209,41 @@
assertFalse(reader.hasMoreData());
}
+ public void testRewind() throws Exception {
+ final ProcFileReader reader = buildReader("abc\n");
+
+ assertEquals("abc", reader.nextString());
+ reader.finishLine();
+ assertFalse(reader.hasMoreData());
+
+ reader.rewind();
+ assertTrue(reader.hasMoreData());
+
+ assertEquals("abc", reader.nextString());
+ reader.finishLine();
+ assertFalse(reader.hasMoreData());
+ }
+
+
+ public void testRewindFileInputStream() throws Exception {
+ File tempFile = File.createTempFile("procfile", null, null);
+ Files.write(tempFile.toPath(), "abc\n".getBytes(StandardCharsets.US_ASCII));
+ final ProcFileReader reader = new ProcFileReader(new FileInputStream(tempFile));
+
+ assertEquals("abc", reader.nextString());
+ reader.finishLine();
+ assertFalse(reader.hasMoreData());
+
+ reader.rewind();
+ assertTrue(reader.hasMoreData());
+
+ assertEquals("abc", reader.nextString());
+ reader.finishLine();
+ assertFalse(reader.hasMoreData());
+
+ Files.delete(tempFile.toPath());
+ }
+
private static ProcFileReader buildReader(String string) throws IOException {
return buildReader(string, 2048);
}
diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java
index da6eeb6..9f2b1ae 100644
--- a/services/core/java/com/android/server/am/ActivityManagerService.java
+++ b/services/core/java/com/android/server/am/ActivityManagerService.java
@@ -11830,8 +11830,8 @@
restart = app.onCleanupApplicationRecordLSP(mProcessStats, allowRestart,
fromBinderDied || app.isolated /* unlinkDeath */);
- // Cancel pending frozen task if there is any.
- mOomAdjuster.mCachedAppOptimizer.unscheduleFreezeAppLSP(app);
+ // Cancel pending frozen task and clean up frozen record if there is any.
+ mOomAdjuster.mCachedAppOptimizer.onCleanupApplicationRecordLocked(app);
}
mAppProfiler.onCleanupApplicationRecordLocked(app);
skipCurrentReceiverLocked(app);
diff --git a/services/core/java/com/android/server/am/CachedAppOptimizer.java b/services/core/java/com/android/server/am/CachedAppOptimizer.java
index 583fb89..0c94fbb 100644
--- a/services/core/java/com/android/server/am/CachedAppOptimizer.java
+++ b/services/core/java/com/android/server/am/CachedAppOptimizer.java
@@ -38,6 +38,7 @@
import android.text.TextUtils;
import android.util.EventLog;
import android.util.Slog;
+import android.util.SparseArray;
import com.android.internal.annotations.GuardedBy;
import com.android.internal.annotations.VisibleForTesting;
@@ -173,6 +174,10 @@
private final ArrayList<ProcessRecord> mPendingCompactionProcesses =
new ArrayList<ProcessRecord>();
+ @GuardedBy("mProcLock")
+ private final SparseArray<ProcessRecord> mFrozenProcesses =
+ new SparseArray<>();
+
private final ActivityManagerService mAm;
private final ActivityManagerGlobalLock mProcLock;
@@ -424,6 +429,15 @@
pw.println(" " + KEY_USE_FREEZER + "=" + mUseFreezer);
pw.println(" " + KEY_FREEZER_STATSD_SAMPLE_RATE + "=" + mFreezerStatsdSampleRate);
pw.println(" " + KEY_FREEZER_DEBOUNCE_TIMEOUT + "=" + mFreezerDebounceTimeout);
+ synchronized (mProcLock) {
+ int size = mFrozenProcesses.size();
+ pw.println(" Apps frozen: " + size);
+ for (int i = 0; i < size; i++) {
+ ProcessRecord app = mFrozenProcesses.valueAt(i);
+ pw.println(" " + app.mOptRecord.getFreezeUnfreezeTime()
+ + ": " + app.getPid() + " " + app.processName);
+ }
+ }
if (DEBUG_COMPACTION) {
for (Map.Entry<Integer, LastCompactionStats> entry
: mLastCompactionStats.entrySet()) {
@@ -1001,6 +1015,7 @@
opt.setFreezeUnfreezeTime(SystemClock.uptimeMillis());
opt.setFrozen(false);
+ mFrozenProcesses.delete(pid);
} catch (Exception e) {
Slog.e(TAG_AM, "Unable to unfreeze " + pid + " " + app.processName
+ ". This might cause inconsistency or UI hangs.");
@@ -1021,7 +1036,7 @@
* To be called when the given app is killed.
*/
@GuardedBy({"mAm", "mProcLock"})
- void unscheduleFreezeAppLSP(ProcessRecord app) {
+ void onCleanupApplicationRecordLocked(ProcessRecord app) {
if (mUseFreezer) {
final ProcessCachedOptimizerRecord opt = app.mOptRecord;
if (opt.isPendingFreeze()) {
@@ -1029,6 +1044,8 @@
mFreezeHandler.removeMessages(SET_FROZEN_PROCESS_MSG, app);
opt.setPendingFreeze(false);
}
+
+ mFrozenProcesses.delete(app.getPid());
}
}
@@ -1293,7 +1310,8 @@
}
}
- private final class FreezeHandler extends Handler {
+ private final class FreezeHandler extends Handler implements
+ ProcLocksReader.ProcLocksReaderCallback {
private FreezeHandler() {
super(mCachedAppOptimizerThread.getLooper());
}
@@ -1336,20 +1354,6 @@
opt.setPendingFreeze(false);
- try {
- // pre-check for locks to avoid unnecessary freeze/unfreeze operations
- if (mProcLocksReader.hasFileLocks(pid)) {
- if (DEBUG_FREEZER) {
- Slog.d(TAG_AM, name + " (" + pid + ") holds file locks, not freezing");
- }
- return;
- }
- } catch (Exception e) {
- Slog.e(TAG_AM, "Not freezing. Unable to check file locks for " + name + "(" + pid
- + "): " + e);
- return;
- }
-
synchronized (mProcLock) {
pid = proc.getPid();
if (proc.mState.getCurAdj() < ProcessList.CACHED_APP_MIN_ADJ
@@ -1403,6 +1407,7 @@
opt.setFreezeUnfreezeTime(SystemClock.uptimeMillis());
opt.setFrozen(true);
+ mFrozenProcesses.put(pid, proc);
} catch (Exception e) {
Slog.w(TAG_AM, "Unable to freeze " + pid + " " + name);
}
@@ -1450,16 +1455,13 @@
}
try {
- // post-check to prevent races
- if (mProcLocksReader.hasFileLocks(pid)) {
- if (DEBUG_FREEZER) {
- Slog.d(TAG_AM, name + " (" + pid + ") holds file locks, reverting freeze");
- }
- unfreezeAppLSP(proc);
- }
+ // post-check to prevent deadlock
+ mProcLocksReader.handleBlockingFileLocks(this);
} catch (Exception e) {
Slog.e(TAG_AM, "Unable to check file locks for " + name + "(" + pid + "): " + e);
- unfreezeAppLSP(proc);
+ synchronized (mProcLock) {
+ unfreezeAppLSP(proc);
+ }
}
}
@@ -1477,6 +1479,21 @@
frozenDuration);
}
}
+
+ @GuardedBy({"mAm"})
+ @Override
+ public void onBlockingFileLock(int pid) {
+ if (DEBUG_FREEZER) {
+ Slog.d(TAG_AM, "Process (pid=" + pid + ") holds blocking file lock");
+ }
+ synchronized (mProcLock) {
+ ProcessRecord app = mFrozenProcesses.get(pid);
+ if (app != null) {
+ Slog.i(TAG_AM, app.processName + " (" + pid + ") holds blocking file lock");
+ unfreezeAppLSP(app);
+ }
+ }
+ }
}
/**