From 15969e0d88ed2a2c03e207d3fe60ec43ae4362ef Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Mon, 14 Sep 2020 13:49:59 -0300 Subject: [PATCH] Cache folder contents in K/V backup/restore This speeds up things significantly and was needed due to poor performance of call log backup. --- .../com/stevesoltys/seedvault/PluginTest.kt | 7 +- .../plugins/saf/DocumentsProviderKVBackup.kt | 86 ++++++++++++------- .../saf/DocumentsProviderKVRestorePlugin.kt | 27 ++++-- .../seedvault/plugins/saf/DocumentsStorage.kt | 2 +- .../seedvault/transport/backup/KVBackup.kt | 10 +-- .../transport/backup/KVBackupPlugin.kt | 20 +++-- .../transport/restore/KVRestorePlugin.kt | 3 +- .../transport/CoordinatorIntegrationTest.kt | 4 +- .../transport/backup/KVBackupTest.kt | 23 ++--- 9 files changed, 110 insertions(+), 72 deletions(-) diff --git a/app/src/androidTest/java/com/stevesoltys/seedvault/PluginTest.kt b/app/src/androidTest/java/com/stevesoltys/seedvault/PluginTest.kt index 7bbc04e9..dbf6bd7a 100644 --- a/app/src/androidTest/java/com/stevesoltys/seedvault/PluginTest.kt +++ b/app/src/androidTest/java/com/stevesoltys/seedvault/PluginTest.kt @@ -202,7 +202,6 @@ class PluginTest : KoinComponent { val record3 = Pair(getRandomBase64(128), getRandomByteArray(5 * 1024 * 1024)) // write first record - kvBackup.ensureRecordStorageForPackage(packageInfo) kvBackup.getOutputStreamForRecord(packageInfo, record1.first).writeAndClose(record1.second) // data is now available for current token and given package, but not for different token @@ -220,11 +219,11 @@ class PluginTest : KoinComponent { ) // write second and third record - kvBackup.ensureRecordStorageForPackage(packageInfo) kvBackup.getOutputStreamForRecord(packageInfo, record2.first).writeAndClose(record2.second) kvBackup.getOutputStreamForRecord(packageInfo, record3.first).writeAndClose(record3.second) // all records for package are found and returned properly + assertTrue(kvRestore.hasDataForPackage(token, packageInfo)) records = kvRestore.listRecords(token, packageInfo) assertEquals(listOf(record1.first, record2.first, record3.first).sorted(), records.sorted()) assertReadEquals( @@ -242,6 +241,7 @@ class PluginTest : KoinComponent { // delete record3 and ensure that the other two are still found kvBackup.deleteRecord(packageInfo, record3.first) + assertTrue(kvRestore.hasDataForPackage(token, packageInfo)) records = kvRestore.listRecords(token, packageInfo) assertEquals(listOf(record1.first, record2.first).sorted(), records.sorted()) @@ -259,6 +259,7 @@ class PluginTest : KoinComponent { // initialize storage with given token initStorage(token) + assertFalse(kvBackup.hasDataForPackage(packageInfo)) // FIXME get Nextcloud to have the same limit // Since Nextcloud is using WebDAV and that seems to have undefined lower file name limits @@ -271,7 +272,6 @@ class PluginTest : KoinComponent { val recordOver = Pair(getRandomBase64(maxOver), getRandomByteArray(1024)) // write max record - kvBackup.ensureRecordStorageForPackage(packageInfo) kvBackup.getOutputStreamForRecord(packageInfo, recordMax.first) .writeAndClose(recordMax.second) @@ -281,7 +281,6 @@ class PluginTest : KoinComponent { assertEquals(listOf(recordMax.first), records) // write exceeding key length record - kvBackup.ensureRecordStorageForPackage(packageInfo) if (isNextcloud()) { // Nextcloud simply refuses to write long filenames coAssertThrows(IOException::class.java) { diff --git a/app/src/main/java/com/stevesoltys/seedvault/plugins/saf/DocumentsProviderKVBackup.kt b/app/src/main/java/com/stevesoltys/seedvault/plugins/saf/DocumentsProviderKVBackup.kt index 0da6066f..ca617764 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/plugins/saf/DocumentsProviderKVBackup.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/plugins/saf/DocumentsProviderKVBackup.kt @@ -19,39 +19,21 @@ internal class DocumentsProviderKVBackup( ) : KVBackupPlugin { private var packageFile: DocumentFile? = null + private var packageChildren: List? = null override fun getQuota(): Long = DEFAULT_QUOTA_KEY_VALUE_BACKUP @Throws(IOException::class) override suspend fun hasDataForPackage(packageInfo: PackageInfo): Boolean { - val packageFile = - storage.currentKvBackupDir?.findFileBlocking(context, packageInfo.packageName) - ?: return false - return packageFile.listFilesBlocking(context).isNotEmpty() - } - - @Throws(IOException::class) - override suspend fun ensureRecordStorageForPackage(packageInfo: PackageInfo) { - // remember package file for subsequent operations - packageFile = + // get the folder for the package (or create it) and all files in it + val dir = storage.getOrCreateKVBackupDir().createOrGetDirectory(context, packageInfo.packageName) - } - - @Throws(IOException::class) - override suspend fun removeDataOfPackage(packageInfo: PackageInfo) { - // we cannot use the cached this.packageFile here, - // because this can be called before [ensureRecordStorageForPackage] - val packageFile = - storage.currentKvBackupDir?.findFileBlocking(context, packageInfo.packageName) ?: return - packageFile.delete() - } - - @Throws(IOException::class) - override suspend fun deleteRecord(packageInfo: PackageInfo, key: String) { - val packageFile = this.packageFile ?: throw AssertionError() - packageFile.assertRightFile(packageInfo) - val keyFile = packageFile.findFileBlocking(context, key) ?: return - keyFile.delete() + val children = dir.listFilesBlocking(context) + // cache package file for subsequent operations + packageFile = dir + // also cache children as doing this for every record is super slow + packageChildren = children + return children.isNotEmpty() } @Throws(IOException::class) @@ -59,6 +41,7 @@ internal class DocumentsProviderKVBackup( packageInfo: PackageInfo, key: String ): OutputStream { + // check maximum key lengths check(key.length <= MAX_KEY_LENGTH) { "Key $key for ${packageInfo.packageName} is too long: ${key.length} chars." } @@ -68,10 +51,55 @@ internal class DocumentsProviderKVBackup( "Key $key for ${packageInfo.packageName} is too long: ${key.length} chars." ) } - val packageFile = this.packageFile ?: throw AssertionError() + // get dir and children from cache + val packageFile = this.packageFile + ?: throw AssertionError("No cached packageFile for ${packageInfo.packageName}") packageFile.assertRightFile(packageInfo) - val keyFile = packageFile.createOrGetFile(context, key) + val children = packageChildren + ?: throw AssertionError("No cached children for ${packageInfo.packageName}") + + // get file for key from cache, + val keyFile = children.find { it.name == key } // try cache first + ?: packageFile.createFile(MIME_TYPE, key) // assume it doesn't exist, create it + ?: packageFile.createOrGetFile(context, key) // cache was stale, so try to find it + check(keyFile.name == key) { "Key file named ${keyFile.name}, but should be $key" } return storage.getOutputStream(keyFile) } + @Throws(IOException::class) + override suspend fun deleteRecord(packageInfo: PackageInfo, key: String) { + val packageFile = this.packageFile + ?: throw AssertionError("No cached packageFile for ${packageInfo.packageName}") + packageFile.assertRightFile(packageInfo) + + val children = packageChildren + ?: throw AssertionError("No cached children for ${packageInfo.packageName}") + + // try to find file for given key and delete it if found + val keyFile = children.find { it.name == key } // try to find in cache + ?: packageFile.findFileBlocking(context, key) // fall-back to provider + ?: return // not found, nothing left to do + keyFile.delete() + + // we don't update the children cache as deleted records + // are not expected to get re-added in the same backup pass + } + + @Throws(IOException::class) + override suspend fun removeDataOfPackage(packageInfo: PackageInfo) { + val packageFile = this.packageFile + ?: throw AssertionError("No cached packageFile for ${packageInfo.packageName}") + packageFile.assertRightFile(packageInfo) + // We are not using the cached children here in case they are stale. + // This operation isn't frequent, so we don't need to heavily optimize it. + packageFile.deleteContents(context) + // clear children cache + packageChildren = ArrayList() + } + + override fun packageFinished(packageInfo: PackageInfo) { + packageFile = null + packageChildren = null + } + } diff --git a/app/src/main/java/com/stevesoltys/seedvault/plugins/saf/DocumentsProviderKVRestorePlugin.kt b/app/src/main/java/com/stevesoltys/seedvault/plugins/saf/DocumentsProviderKVRestorePlugin.kt index be57ca24..648fed54 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/plugins/saf/DocumentsProviderKVRestorePlugin.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/plugins/saf/DocumentsProviderKVRestorePlugin.kt @@ -14,13 +14,19 @@ internal class DocumentsProviderKVRestorePlugin( ) : KVRestorePlugin { private var packageDir: DocumentFile? = null + private var packageChildren: List? = null override suspend fun hasDataForPackage(token: Long, packageInfo: PackageInfo): Boolean { return try { val backupDir = storage.getKVBackupDir(token) ?: return false + val dir = backupDir.findFileBlocking(context, packageInfo.packageName) ?: return false + val children = dir.listFilesBlocking(context) // remember package file for subsequent operations - packageDir = backupDir.findFileBlocking(context, packageInfo.packageName) - packageDir != null + packageDir = dir + // remember package children for subsequent operations + packageChildren = children + // we have data if we have a non-empty list of children + children.isNotEmpty() } catch (e: IOException) { false } @@ -28,11 +34,13 @@ internal class DocumentsProviderKVRestorePlugin( @Throws(IOException::class) override suspend fun listRecords(token: Long, packageInfo: PackageInfo): List { - val packageDir = this.packageDir ?: throw AssertionError() + val packageDir = this.packageDir + ?: throw AssertionError("No cached packageDir for ${packageInfo.packageName}") packageDir.assertRightFile(packageInfo) - return packageDir.listFilesBlocking(context) - .filter { file -> file.name != null } - .map { file -> file.name!! } + return packageChildren + ?.filter { file -> file.name != null } + ?.map { file -> file.name!! } + ?: throw AssertionError("No cached children for ${packageInfo.packageName}") } @Throws(IOException::class) @@ -41,9 +49,12 @@ internal class DocumentsProviderKVRestorePlugin( packageInfo: PackageInfo, key: String ): InputStream { - val packageDir = this.packageDir ?: throw AssertionError() + val packageDir = this.packageDir + ?: throw AssertionError("No cached packageDir for ${packageInfo.packageName}") packageDir.assertRightFile(packageInfo) - val keyFile = packageDir.findFileBlocking(context, key) ?: throw IOException() + val keyFile = packageChildren?.find { it.name == key } + ?: packageDir.findFileBlocking(context, key) + ?: throw IOException() return storage.getInputStream(keyFile) } diff --git a/app/src/main/java/com/stevesoltys/seedvault/plugins/saf/DocumentsStorage.kt b/app/src/main/java/com/stevesoltys/seedvault/plugins/saf/DocumentsStorage.kt index d1e281d5..3fb0c1c7 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/plugins/saf/DocumentsStorage.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/plugins/saf/DocumentsStorage.kt @@ -32,7 +32,7 @@ const val DIRECTORY_FULL_BACKUP = "full" const val DIRECTORY_KEY_VALUE_BACKUP = "kv" const val FILE_BACKUP_METADATA = ".backup.metadata" const val FILE_NO_MEDIA = ".nomedia" -private const val MIME_TYPE = "application/octet-stream" +const val MIME_TYPE = "application/octet-stream" private val TAG = DocumentsStorage::class.java.simpleName diff --git a/app/src/main/java/com/stevesoltys/seedvault/transport/backup/KVBackup.kt b/app/src/main/java/com/stevesoltys/seedvault/transport/backup/KVBackup.kt index f02af892..c8f42361 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/transport/backup/KVBackup.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/transport/backup/KVBackup.kt @@ -91,14 +91,6 @@ internal class KVBackup( } } - // ensure there's a place to store K/V for the given package - try { - plugin.ensureRecordStorageForPackage(packageInfo) - } catch (e: IOException) { - Log.e(TAG, "Error ensuring storage for ${packageInfo.packageName}.", e) - return backupError(TRANSPORT_ERROR) - } - // parse and store the K/V updates return storeRecords(packageInfo, data) } @@ -221,6 +213,7 @@ internal class KVBackup( fun finishBackup(): Int { Log.i(TAG, "Finish K/V Backup of ${state!!.packageInfo.packageName}") + plugin.packageFinished(state!!.packageInfo) state = null return TRANSPORT_OK } @@ -233,6 +226,7 @@ internal class KVBackup( "Resetting state because of K/V Backup error of ${state!!.packageInfo.packageName}".let { Log.i(TAG, it) } + plugin.packageFinished(state!!.packageInfo) state = null return result } diff --git a/app/src/main/java/com/stevesoltys/seedvault/transport/backup/KVBackupPlugin.kt b/app/src/main/java/com/stevesoltys/seedvault/transport/backup/KVBackupPlugin.kt index 5ec97537..a7f5b7d4 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/transport/backup/KVBackupPlugin.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/transport/backup/KVBackupPlugin.kt @@ -14,18 +14,13 @@ interface KVBackupPlugin { // TODO consider using a salted hash for the package name (and key) to not leak it to the storage server /** * Return true if there are records stored for the given package. - */ - @Throws(IOException::class) - suspend fun hasDataForPackage(packageInfo: PackageInfo): Boolean - - /** - * This marks the beginning of a backup operation. + * This is always called first per [PackageInfo], before subsequent methods. * - * Make sure that there is a place to store K/V pairs for the given package. + * Independent of the return value, the storage should now be prepared to store K/V pairs. * E.g. file-based plugins should a create a directory for the package, if none exists. */ @Throws(IOException::class) - suspend fun ensureRecordStorageForPackage(packageInfo: PackageInfo) + suspend fun hasDataForPackage(packageInfo: PackageInfo): Boolean /** * Return an [OutputStream] for the given package and key @@ -41,9 +36,16 @@ interface KVBackupPlugin { suspend fun deleteRecord(packageInfo: PackageInfo, key: String) /** - * Remove all data associated with the given package. + * Remove all data associated with the given package, + * but be prepared to receive new records afterwards with [getOutputStreamForRecord]. */ @Throws(IOException::class) suspend fun removeDataOfPackage(packageInfo: PackageInfo) + /** + * The package finished backup. + * This can be an opportunity to clear existing caches or to do other clean-up work. + */ + fun packageFinished(packageInfo: PackageInfo) + } diff --git a/app/src/main/java/com/stevesoltys/seedvault/transport/restore/KVRestorePlugin.kt b/app/src/main/java/com/stevesoltys/seedvault/transport/restore/KVRestorePlugin.kt index fcf85065..fa2b0ae0 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/transport/restore/KVRestorePlugin.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/transport/restore/KVRestorePlugin.kt @@ -15,7 +15,8 @@ interface KVRestorePlugin { /** * Return all record keys for the given token and package. * - * Note: Implementations might expect that you call [hasDataForPackage] before. + * Note: Implementations usually expect that you call [hasDataForPackage] + * with the same parameters before. * * For file-based plugins, this is usually a list of file names in the package directory. */ diff --git a/app/src/test/java/com/stevesoltys/seedvault/transport/CoordinatorIntegrationTest.kt b/app/src/test/java/com/stevesoltys/seedvault/transport/CoordinatorIntegrationTest.kt index 26347c13..85be2aac 100644 --- a/app/src/test/java/com/stevesoltys/seedvault/transport/CoordinatorIntegrationTest.kt +++ b/app/src/test/java/com/stevesoltys/seedvault/transport/CoordinatorIntegrationTest.kt @@ -126,7 +126,6 @@ internal class CoordinatorIntegrationTest : TransportTest() { // read one key/value record and write it to output stream coEvery { kvBackupPlugin.hasDataForPackage(packageInfo) } returns false - coEvery { kvBackupPlugin.ensureRecordStorageForPackage(packageInfo) } just Runs every { inputFactory.getBackupDataInput(fileDescriptor) } returns backupDataInput every { backupDataInput.readNextHeader() } returns true andThen true andThen false every { backupDataInput.key } returns key andThen key2 @@ -151,6 +150,7 @@ internal class CoordinatorIntegrationTest : TransportTest() { key264 ) } returns bOutputStream2 + every { kvBackupPlugin.packageFinished(packageInfo) } just Runs coEvery { apkBackup.backupApkIfNecessary( packageInfo, @@ -219,7 +219,6 @@ internal class CoordinatorIntegrationTest : TransportTest() { // read one key/value record and write it to output stream coEvery { kvBackupPlugin.hasDataForPackage(packageInfo) } returns false - coEvery { kvBackupPlugin.ensureRecordStorageForPackage(packageInfo) } just Runs every { inputFactory.getBackupDataInput(fileDescriptor) } returns backupDataInput every { backupDataInput.readNextHeader() } returns true andThen false every { backupDataInput.key } returns key @@ -234,6 +233,7 @@ internal class CoordinatorIntegrationTest : TransportTest() { key64 ) } returns bOutputStream + every { kvBackupPlugin.packageFinished(packageInfo) } just Runs coEvery { apkBackup.backupApkIfNecessary(packageInfo, UNKNOWN_ERROR, any()) } returns null coEvery { backupPlugin.getMetadataOutputStream() } returns metadataOutputStream every { metadataManager.onPackageBackedUp(packageInfo, metadataOutputStream) } just Runs diff --git a/app/src/test/java/com/stevesoltys/seedvault/transport/backup/KVBackupTest.kt b/app/src/test/java/com/stevesoltys/seedvault/transport/backup/KVBackupTest.kt index 6e44a73b..5bf820e1 100644 --- a/app/src/test/java/com/stevesoltys/seedvault/transport/backup/KVBackupTest.kt +++ b/app/src/test/java/com/stevesoltys/seedvault/transport/backup/KVBackupTest.kt @@ -90,6 +90,9 @@ internal class KVBackupTest : BackupTest() { assertEquals(TRANSPORT_OK, backup.performBackup(pmPackageInfo, data, 0)) assertTrue(backup.hasState()) + + every { plugin.packageFinished(pmPackageInfo) } just Runs + assertEquals(TRANSPORT_OK, backup.finishBackup()) assertFalse(backup.hasState()) @@ -103,6 +106,7 @@ internal class KVBackupTest : BackupTest() { @Test fun `incremental backup with no data gets rejected`() = runBlocking { coEvery { plugin.hasDataForPackage(packageInfo) } returns false + every { plugin.packageFinished(packageInfo) } just Runs assertEquals( TRANSPORT_NON_INCREMENTAL_BACKUP_REQUIRED, @@ -114,6 +118,7 @@ internal class KVBackupTest : BackupTest() { @Test fun `check for existing data throws exception`() = runBlocking { coEvery { plugin.hasDataForPackage(packageInfo) } throws IOException() + every { plugin.packageFinished(packageInfo) } just Runs assertEquals(TRANSPORT_ERROR, backup.performBackup(packageInfo, data, 0)) assertFalse(backup.hasState()) @@ -145,20 +150,12 @@ internal class KVBackupTest : BackupTest() { assertFalse(backup.hasState()) } - @Test - fun `ensuring storage throws exception`() = runBlocking { - coEvery { plugin.hasDataForPackage(packageInfo) } returns false - coEvery { plugin.ensureRecordStorageForPackage(packageInfo) } throws IOException() - - assertEquals(TRANSPORT_ERROR, backup.performBackup(packageInfo, data, 0)) - assertFalse(backup.hasState()) - } - @Test fun `exception while reading next header`() = runBlocking { initPlugin(false) createBackupDataInput() every { dataInput.readNextHeader() } throws IOException() + every { plugin.packageFinished(packageInfo) } just Runs assertEquals(TRANSPORT_ERROR, backup.performBackup(packageInfo, data, 0)) assertFalse(backup.hasState()) @@ -172,6 +169,7 @@ internal class KVBackupTest : BackupTest() { every { dataInput.key } returns key every { dataInput.dataSize } returns value.size every { dataInput.readEntityData(any(), 0, value.size) } throws IOException() + every { plugin.packageFinished(packageInfo) } just Runs assertEquals(TRANSPORT_ERROR, backup.performBackup(packageInfo, data, 0)) assertFalse(backup.hasState()) @@ -181,6 +179,7 @@ internal class KVBackupTest : BackupTest() { fun `no data records`() = runBlocking { initPlugin(false) getDataInput(listOf(false)) + every { plugin.packageFinished(packageInfo) } just Runs assertEquals(TRANSPORT_OK, backup.performBackup(packageInfo, data, 0)) assertTrue(backup.hasState()) @@ -195,6 +194,7 @@ internal class KVBackupTest : BackupTest() { coEvery { plugin.getOutputStreamForRecord(packageInfo, key64) } returns outputStream every { headerWriter.writeVersion(outputStream, versionHeader) } throws IOException() every { outputStream.close() } just Runs + every { plugin.packageFinished(packageInfo) } just Runs assertEquals(TRANSPORT_ERROR, backup.performBackup(packageInfo, data, 0)) assertFalse(backup.hasState()) @@ -211,6 +211,7 @@ internal class KVBackupTest : BackupTest() { every { headerWriter.writeVersion(outputStream, versionHeader) } just Runs every { crypto.encryptMultipleSegments(outputStream, any()) } throws IOException() every { outputStream.close() } just Runs + every { plugin.packageFinished(packageInfo) } just Runs assertEquals(TRANSPORT_ERROR, backup.performBackup(packageInfo, data, 0)) assertFalse(backup.hasState()) @@ -226,6 +227,7 @@ internal class KVBackupTest : BackupTest() { every { outputStream.write(value) } just Runs every { outputStream.flush() } throws IOException() every { outputStream.close() } just Runs + every { plugin.packageFinished(packageInfo) } just Runs assertEquals(TRANSPORT_ERROR, backup.performBackup(packageInfo, data, 0)) assertFalse(backup.hasState()) @@ -241,6 +243,7 @@ internal class KVBackupTest : BackupTest() { every { outputStream.write(value) } just Runs every { outputStream.flush() } just Runs every { outputStream.close() } throws IOException() + every { plugin.packageFinished(packageInfo) } just Runs assertEquals(TRANSPORT_OK, backup.performBackup(packageInfo, data, 0)) assertTrue(backup.hasState()) @@ -255,11 +258,11 @@ internal class KVBackupTest : BackupTest() { every { outputStream.write(value) } just Runs every { outputStream.flush() } just Runs every { outputStream.close() } just Runs + every { plugin.packageFinished(packageInfo) } just Runs } private fun initPlugin(hasDataForPackage: Boolean = false, pi: PackageInfo = packageInfo) { coEvery { plugin.hasDataForPackage(pi) } returns hasDataForPackage - coEvery { plugin.ensureRecordStorageForPackage(pi) } just Runs } private fun createBackupDataInput() {