From a025436bb2e1c4979292ec251ad84d8c35e59294 Mon Sep 17 00:00:00 2001 From: Christoph Fiehe Date: Sat, 14 Sep 2024 23:05:12 +0200 Subject: [PATCH] Fix (slow) move on same object bucket. This commit fixes the issue #47856. When you upload a file into a group folder and when you use a single S3 bucket as primary storage, the final move operation hangs for a long time. In the background, Nextcloud initiates a copy-delete sequence from the bucket into the bucket, with causes a lot unnecessary overhead. Nextcloud thinks that the file must be imported to another storage and does not recognize that everything is done on the same object bucket. In that case, the import step can be completely skipped, which saves time, network bandwidth and reduces the load on the object storage. The behavior improves a lot with https://github.com/nextcloud/server/pull/46013. However, there are still some put messages that are being sent to the object storage when you use an object storage as primary storage and upload files into a group folder. Co-authored-by: Kate <26026535+provokateurin@users.noreply.github.com> Signed-off-by: cfiehe --- .../Files/ObjectStore/ObjectStoreStorage.php | 5 + ...ObjectStoreStoragesDifferentBucketTest.php | 39 ++++++ .../ObjectStoreStoragesSameBucketTest.php | 31 +++++ tests/lib/Files/Storage/StoragesTest.php | 111 ++++++++++++++++++ 4 files changed, 186 insertions(+) create mode 100644 tests/lib/Files/ObjectStore/ObjectStoreStoragesDifferentBucketTest.php create mode 100644 tests/lib/Files/ObjectStore/ObjectStoreStoragesSameBucketTest.php create mode 100644 tests/lib/Files/Storage/StoragesTest.php diff --git a/lib/private/Files/ObjectStore/ObjectStoreStorage.php b/lib/private/Files/ObjectStore/ObjectStoreStorage.php index 228fc51667744..18a75e61b1c1b 100644 --- a/lib/private/Files/ObjectStore/ObjectStoreStorage.php +++ b/lib/private/Files/ObjectStore/ObjectStoreStorage.php @@ -596,6 +596,11 @@ public function copyFromStorage( public function moveFromStorage(IStorage $sourceStorage, $sourceInternalPath, $targetInternalPath, ?ICacheEntry $sourceCacheEntry = null): bool { $sourceCache = $sourceStorage->getCache(); + if ($sourceStorage->instanceOfStorage(ObjectStoreStorage::class) && $sourceStorage->getObjectStore()->getStorageId() === $this->getObjectStore()->getStorageId()) { + $this->getCache()->moveFromCache($sourceCache, $sourceInternalPath, $targetInternalPath); + // Do not import any data when source and target bucket are identical. + return true; + } if (!$sourceCacheEntry) { $sourceCacheEntry = $sourceCache->get($sourceInternalPath); } diff --git a/tests/lib/Files/ObjectStore/ObjectStoreStoragesDifferentBucketTest.php b/tests/lib/Files/ObjectStore/ObjectStoreStoragesDifferentBucketTest.php new file mode 100644 index 0000000000000..ca75966fa5998 --- /dev/null +++ b/tests/lib/Files/ObjectStore/ObjectStoreStoragesDifferentBucketTest.php @@ -0,0 +1,39 @@ +objectStorage1 = new StorageObjectStore($baseStorage1); + $config['objectstore'] = $this->objectStorage1; + $this->storage1 = new ObjectStoreStorageOverwrite($config); + + $baseStorage2 = new Temporary(); + $this->objectStorage2 = new StorageObjectStore($baseStorage2); + $config['objectstore'] = $this->objectStorage2; + $this->storage2 = new ObjectStoreStorageOverwrite($config); + } +} \ No newline at end of file diff --git a/tests/lib/Files/ObjectStore/ObjectStoreStoragesSameBucketTest.php b/tests/lib/Files/ObjectStore/ObjectStoreStoragesSameBucketTest.php new file mode 100644 index 0000000000000..60011797b8ed0 --- /dev/null +++ b/tests/lib/Files/ObjectStore/ObjectStoreStoragesSameBucketTest.php @@ -0,0 +1,31 @@ +objectStorage = new StorageObjectStore($baseStorage); + $config['objectstore'] = $this->objectStorage; + // storage1 and storage2 share the same object store. + $this->storage1 = new ObjectStoreStorageOverwrite($config); + $this->storage2 = new ObjectStoreStorageOverwrite($config); + } +} diff --git a/tests/lib/Files/Storage/StoragesTest.php b/tests/lib/Files/Storage/StoragesTest.php new file mode 100644 index 0000000000000..bbeffdcfba177 --- /dev/null +++ b/tests/lib/Files/Storage/StoragesTest.php @@ -0,0 +1,111 @@ +storage1) && is_null($this->storage2)) { + return; + } + $this->storage1->getCache()->clear(); + $this->storage2->getCache()->clear(); + + parent::tearDown(); + } + + public function testMoveFileFromStorage() { + $source = 'source.txt'; + $target = 'target.txt'; + $storage2->file_put_contents($source, 'foo'); + + $storage1->moveFromStorage($storage2, $source, $target); + + $this->assertTrue($storage1->file_exists($target), $target.' was not created'); + $this->assertFalse($storage2->file_exists($source), $source.' still exists'); + $this->assertEquals('foo', $storage1->file_get_contents($target)); + } + + public function testMoveDirectoryFromStorage() { + $storage2->mkdir('source'); + $storage2->file_put_contents('source/test1.txt', 'foo'); + $storage2->file_put_contents('source/test2.txt', 'qwerty'); + $storage2->mkdir('source/subfolder'); + $storage2->file_put_contents('source/subfolder/test.txt', 'bar'); + + $storage1->moveFromStorage($storage2, 'source', 'target'); + + $this->assertTrue($storage1->file_exists('target')); + $this->assertTrue($storage1->file_exists('target/test1.txt')); + $this->assertTrue($storage1->file_exists('target/test2.txt')); + $this->assertTrue($storage1->file_exists('target/subfolder')); + $this->assertTrue($storage1->file_exists('target/subfolder/test.txt')); + + $this->assertFalse($storage2->file_exists('source')); + $this->assertFalse($storage2->file_exists('source/test1.txt')); + $this->assertFalse($storage2->file_exists('source/test2.txt')); + $this->assertFalse($storage2->file_exists('source/subfolder')); + $this->assertFalse($storage2->file_exists('source/subfolder/test.txt')); + + $this->assertEquals('foo', $storage1->file_get_contents('target/test1.txt')); + $this->assertEquals('qwerty', $storage1->file_get_contents('target/test2.txt')); + $this->assertEquals('bar', $storage1->file_get_contents('target/subfolder/test.txt')); + } + + /** + * @dataProvider copyAndMoveProvider + */ + public function testCopyFileFromStorage($source, $target) { + $source = 'source.txt'; + $target = 'target.txt'; + $storage2->file_put_contents($source, 'foo'); + + $storage1->copyFromStorage($storage2, $source, $target); + + $this->assertTrue($storage1->file_exists($target), $target.' was not created'); + $this->assertTrue($storage2->file_exists($source), $source.' was deleted'); + $this->assertEquals('foo', $storage1->file_get_contents($target)); + } + + public function testCopyDirectoryFromStorage() { + $storage2->mkdir('source'); + $storage2->file_put_contents('source/test1.txt', 'foo'); + $storage2->file_put_contents('source/test2.txt', 'qwerty'); + $storage2->mkdir('source/subfolder'); + $storage2->file_put_contents('source/subfolder/test.txt', 'bar'); + + $storage1->copyFromStorage($storage2, 'source', 'target'); + + $this->assertTrue($storage1->file_exists('target')); + $this->assertTrue($storage1->file_exists('target/test1.txt')); + $this->assertTrue($storage1->file_exists('target/test2.txt')); + $this->assertTrue($storage1->file_exists('target/subfolder')); + $this->assertTrue($storage1->file_exists('target/subfolder/test.txt')); + + $this->assertTrue($storage2->file_exists('source')); + $this->assertTrue($storage2->file_exists('source/test1.txt')); + $this->assertTrue($storage2->file_exists('source/test2.txt')); + $this->assertTrue($storage2->file_exists('source/subfolder')); + $this->assertTrue($storage2->file_exists('source/subfolder/test.txt')); + + $this->assertEquals('foo', $storage1->file_get_contents('target/test1.txt')); + $this->assertEquals('qwerty', $storage1->file_get_contents('target/test2.txt')); + $this->assertEquals('bar', $storage1->file_get_contents('target/subfolder/test.txt')); + } +} \ No newline at end of file