From 3171dbf96c53a68745dd09c44b94ffd51aaef9a6 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Fri, 24 Jun 2022 17:30:40 +0000 Subject: [PATCH] LockSettingsStorage: fix user prefetching The user prefetching logic doesn't work as intended, for two reasons: - The key that Cache.setFetched() sets differs from the key that Cache.isFetched() looks for, so Cache.isFetched() always returns false. So, LockSettingsStorage.prefetchUser() always runs for a user when called, rather than just once per boot as intended. Fix this by making Cache.setFetched() use the same key as Cache.isFetched(). - Since Cache.putIfUnchanged() increments Cache.mVersion, only the first call to it within LockSettingsStorage.prefetchUser() has any effect. So only the first key-value pair for the user is prefetched, not all of them as intended. Fix this by making Cache.putIfUnchanged() *not* increment mVersion, as the backing storage isn't being modified. Found by code review; these bugs aren't known to have been causing any real-world problems. Test: atest --iterations 50 LockSettingsStorageTests Change-Id: Ife97ba855d014d8e291d46f6a702a41b28bd5707 --- .../locksettings/LockSettingsStorage.java | 47 +++++++++++++------ .../LockSettingsStorageTests.java | 14 +++++- 2 files changed, 44 insertions(+), 17 deletions(-) diff --git a/services/core/java/com/android/server/locksettings/LockSettingsStorage.java b/services/core/java/com/android/server/locksettings/LockSettingsStorage.java index 9ddf1efbaef21..a77cc5d8d5fdf 100644 --- a/services/core/java/com/android/server/locksettings/LockSettingsStorage.java +++ b/services/core/java/com/android/server/locksettings/LockSettingsStorage.java @@ -201,6 +201,16 @@ class LockSettingsStorage extends WatchableImpl { return result == DEFAULT ? defaultValue : (String) result; } + @VisibleForTesting + boolean isKeyValueCached(String key, int userId) { + return mCache.hasKeyValue(key, userId); + } + + @VisibleForTesting + boolean isUserPrefetched(int userId) { + return mCache.isFetched(userId); + } + @VisibleForTesting public void removeKey(String key, int userId) { removeKey(mOpenHelper.getWritableDatabase(), key, userId); @@ -762,19 +772,24 @@ class LockSettingsStorage extends WatchableImpl { } } - /** - * Cache consistency model: - * - Writes to storage write directly to the cache, but this MUST happen within the atomic - * section either provided by the database transaction or mWriteLock, such that writes to the - * cache and writes to the backing storage are guaranteed to occur in the same order + /* + * A cache for the following types of data: * - * - Reads can populate the cache, but because they are no strong ordering guarantees with - * respect to writes this precaution is taken: - * - The cache is assigned a version number that increases every time the cache is modified. - * Reads from backing storage can only populate the cache if the backing storage - * has not changed since the load operation has begun. - * This guarantees that no read operation can shadow a write to the cache that happens - * after it had begun. + * - Key-value entries from the locksettings database, where the key is the combination of a + * userId and a string key, and the value is a string. + * - File paths to file contents. + * - The per-user "prefetched" flag. + * + * Cache consistency model: + * - Writes to storage write directly to the cache, but this MUST happen within an atomic + * section either provided by the database transaction or mFileWriteLock, such that writes to + * the cache and writes to the backing storage are guaranteed to occur in the same order. + * - Reads can populate the cache, but because there are no strong ordering guarantees with + * respect to writes the following precaution is taken: The cache is assigned a version + * number that increases every time the backing storage is modified. Reads from backing + * storage can only populate the cache if the backing storage has not changed since the load + * operation has begun. This guarantees that a read operation can't clobber a different value + * that was written to the cache by a concurrent write operation. */ private static class Cache { private final ArrayMap mCache = new ArrayMap<>(); @@ -819,7 +834,7 @@ class LockSettingsStorage extends WatchableImpl { } void setFetched(int userId) { - put(CacheKey.TYPE_FETCHED, "isFetched", "true", userId); + put(CacheKey.TYPE_FETCHED, "", "true", userId); } boolean isFetched(int userId) { @@ -831,7 +846,7 @@ class LockSettingsStorage extends WatchableImpl { } private synchronized void put(int type, String key, Object value, int userId) { - // Create a new CachKey here because it may be saved in the map if the key is absent. + // Create a new CacheKey here because it may be saved in the map if the key is absent. mCache.put(new CacheKey().set(type, key, userId), value); mVersion++; } @@ -839,7 +854,9 @@ class LockSettingsStorage extends WatchableImpl { private synchronized void putIfUnchanged(int type, String key, Object value, int userId, int version) { if (!contains(type, key, userId) && mVersion == version) { - put(type, key, value, userId); + mCache.put(new CacheKey().set(type, key, userId), value); + // Don't increment mVersion, as this method should only be called in cases where the + // backing storage isn't being modified. } } diff --git a/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsStorageTests.java b/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsStorageTests.java index 609c05c040c29..15bbb10c6a1e5 100644 --- a/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsStorageTests.java +++ b/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsStorageTests.java @@ -232,12 +232,22 @@ public class LockSettingsStorageTests { @Test public void testPrefetch() { - mStorage.writeKeyValue("key", "toBeFetched", 0); + mStorage.writeKeyValue("key1", "value1", 0); + mStorage.writeKeyValue("key2", "value2", 0); mStorage.clearCache(); + + assertFalse(mStorage.isUserPrefetched(0)); + assertFalse(mStorage.isKeyValueCached("key1", 0)); + assertFalse(mStorage.isKeyValueCached("key2", 0)); + mStorage.prefetchUser(0); - assertEquals("toBeFetched", mStorage.readKeyValue("key", "default", 0)); + assertTrue(mStorage.isUserPrefetched(0)); + assertTrue(mStorage.isKeyValueCached("key1", 0)); + assertTrue(mStorage.isKeyValueCached("key2", 0)); + assertEquals("value1", mStorage.readKeyValue("key1", "default", 0)); + assertEquals("value2", mStorage.readKeyValue("key2", "default", 0)); } @Test