Skip to content

Commit 27984de

Browse files
Christoph Fieheprovokateurin
Christoph Fiehe
and
provokateurin
committed
Fix (slow) move on same object bucket.
This commit fixes the issue nextcloud#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 nextcloud#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 <cfiehe@users.noreply.github.com>
1 parent 6a6910f commit 27984de

File tree

4 files changed

+183
-0
lines changed

4 files changed

+183
-0
lines changed

‎lib/private/Files/ObjectStore/ObjectStoreStorage.php

+5
Original file line numberDiff line numberDiff line change
@@ -596,6 +596,11 @@ public function copyFromStorage(
596596

597597
public function moveFromStorage(IStorage $sourceStorage, $sourceInternalPath, $targetInternalPath, ?ICacheEntry $sourceCacheEntry = null): bool {
598598
$sourceCache = $sourceStorage->getCache();
599+
if ($sourceStorage->instanceOfStorage(ObjectStoreStorage::class) && $sourceStorage->getObjectStore()->getStorageId() === $this->getObjectStore()->getStorageId()) {
600+
$this->getCache()->moveFromCache($sourceCache, $sourceInternalPath, $targetInternalPath);
601+
// Do not import any data when source and target bucket are identical.
602+
return true;
603+
}
599604
if (!$sourceCacheEntry) {
600605
$sourceCacheEntry = $sourceCache->get($sourceInternalPath);
601606
}

‎tests/lib/Files/ObjectStore/ObjectStoreStoragesDifferentBucketTest.php

+39
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
<?php
2+
/**
3+
* SPDX-FileCopyrightText: 2016-2024 Nextcloud GmbH and Nextcloud contributors
4+
* SPDX-FileCopyrightText: 2016 ownCloud, Inc.
5+
* SPDX-License-Identifier: AGPL-3.0-or-later
6+
*/
7+
8+
namespace Test\Files\ObjectStore;
9+
10+
use Test\Files\Storage\StoragesTest;
11+
12+
/**
13+
* @group DB
14+
*/
15+
class ObjectStoreStoragesDifferentBucketTest extends StoragesTest {
16+
/**
17+
* @var \OCP\Files\ObjectStore\IObjectStore
18+
*/
19+
private $objectStore1;
20+
21+
/**
22+
* @var \OCP\Files\ObjectStore\IObjectStore
23+
*/
24+
private $objectStore2;
25+
26+
protected function setUp(): void {
27+
parent::setUp();
28+
29+
$baseStorage1 = new Temporary();
30+
$this->objectStore1 = new StorageObjectStore($baseStorage1);
31+
$config['objectstore'] = $this->objectStore1;
32+
$this->storage1 = new ObjectStoreStorageOverwrite($config);
33+
34+
$baseStorage2 = new Temporary();
35+
$this->objectStore2 = new StorageObjectStore($baseStorage2);
36+
$config['objectstore'] = $this->objectStore2;
37+
$this->storage2 = new ObjectStoreStorageOverwrite($config);
38+
}
39+
}

‎tests/lib/Files/ObjectStore/ObjectStoreStoragesSameBucketTest.php

+31
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
<?php
2+
/**
3+
* SPDX-FileCopyrightText: 2016-2024 Nextcloud GmbH and Nextcloud contributors
4+
* SPDX-FileCopyrightText: 2016 ownCloud, Inc.
5+
* SPDX-License-Identifier: AGPL-3.0-or-later
6+
*/
7+
8+
namespace Test\Files\ObjectStore;
9+
10+
use Test\Files\Storage\StoragesTest;
11+
12+
/**
13+
* @group DB
14+
*/
15+
class ObjectStoreStoragesSameBucketTest extends StoragesTest {
16+
/**
17+
* @var \OCP\Files\ObjectStore\IObjectStore
18+
*/
19+
private $objectStore;
20+
21+
protected function setUp(): void {
22+
parent::setUp();
23+
24+
$baseStorage = new Temporary();
25+
$this->objectStore = new StorageObjectStore($baseStorage);
26+
$config['objectstore'] = $this->objectStore;
27+
// storage1 and storage2 share the same object store.
28+
$this->storage1 = new ObjectStoreStorageOverwrite($config);
29+
$this->storage2 = new ObjectStoreStorageOverwrite($config);
30+
}
31+
}

‎tests/lib/Files/Storage/StoragesTest.php

+108
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
<?php
2+
/**
3+
* SPDX-FileCopyrightText: 2016-2024 Nextcloud GmbH and Nextcloud contributors
4+
* SPDX-FileCopyrightText: 2016 ownCloud, Inc.
5+
* SPDX-License-Identifier: AGPL-3.0-or-later
6+
*/
7+
8+
namespace Test\Files\Storage;
9+
10+
use Test\TestCase;
11+
12+
abstract class StoragesTest extends TestCase {
13+
/**
14+
* @var \OC\Files\Storage\Storage
15+
*/
16+
protected $storage1;
17+
18+
/**
19+
* @var \OC\Files\Storage\Storage
20+
*/
21+
protected $storage2;
22+
23+
protected function tearDown(): void {
24+
if (is_null($this->storage1) && is_null($this->storage2)) {
25+
return;
26+
}
27+
$this->storage1->getCache()->clear();
28+
$this->storage2->getCache()->clear();
29+
30+
parent::tearDown();
31+
}
32+
33+
public function testMoveFileFromStorage() {
34+
$source = 'source.txt';
35+
$target = 'target.txt';
36+
$storage2->file_put_contents($source, 'foo');
37+
38+
$storage1->moveFromStorage($storage2, $source, $target);
39+
40+
$this->assertTrue($storage1->file_exists($target), $target.' was not created');
41+
$this->assertFalse($storage2->file_exists($source), $source.' still exists');
42+
$this->assertEquals('foo', $storage1->file_get_contents($target));
43+
}
44+
45+
public function testMoveDirectoryFromStorage() {
46+
$storage2->mkdir('source');
47+
$storage2->file_put_contents('source/test1.txt', 'foo');
48+
$storage2->file_put_contents('source/test2.txt', 'qwerty');
49+
$storage2->mkdir('source/subfolder');
50+
$storage2->file_put_contents('source/subfolder/test.txt', 'bar');
51+
52+
$storage1->moveFromStorage($storage2, 'source', 'target');
53+
54+
$this->assertTrue($storage1->file_exists('target'));
55+
$this->assertTrue($storage1->file_exists('target/test1.txt'));
56+
$this->assertTrue($storage1->file_exists('target/test2.txt'));
57+
$this->assertTrue($storage1->file_exists('target/subfolder'));
58+
$this->assertTrue($storage1->file_exists('target/subfolder/test.txt'));
59+
60+
$this->assertFalse($storage2->file_exists('source'));
61+
$this->assertFalse($storage2->file_exists('source/test1.txt'));
62+
$this->assertFalse($storage2->file_exists('source/test2.txt'));
63+
$this->assertFalse($storage2->file_exists('source/subfolder'));
64+
$this->assertFalse($storage2->file_exists('source/subfolder/test.txt'));
65+
66+
$this->assertEquals('foo', $storage1->file_get_contents('target/test1.txt'));
67+
$this->assertEquals('qwerty', $storage1->file_get_contents('target/test2.txt'));
68+
$this->assertEquals('bar', $storage1->file_get_contents('target/subfolder/test.txt'));
69+
}
70+
71+
public function testCopyFileFromStorage() {
72+
$source = 'source.txt';
73+
$target = 'target.txt';
74+
$storage2->file_put_contents($source, 'foo');
75+
76+
$storage1->copyFromStorage($storage2, $source, $target);
77+
78+
$this->assertTrue($storage1->file_exists($target), $target.' was not created');
79+
$this->assertTrue($storage2->file_exists($source), $source.' was deleted');
80+
$this->assertEquals('foo', $storage1->file_get_contents($target));
81+
}
82+
83+
public function testCopyDirectoryFromStorage() {
84+
$storage2->mkdir('source');
85+
$storage2->file_put_contents('source/test1.txt', 'foo');
86+
$storage2->file_put_contents('source/test2.txt', 'qwerty');
87+
$storage2->mkdir('source/subfolder');
88+
$storage2->file_put_contents('source/subfolder/test.txt', 'bar');
89+
90+
$storage1->copyFromStorage($storage2, 'source', 'target');
91+
92+
$this->assertTrue($storage1->file_exists('target'));
93+
$this->assertTrue($storage1->file_exists('target/test1.txt'));
94+
$this->assertTrue($storage1->file_exists('target/test2.txt'));
95+
$this->assertTrue($storage1->file_exists('target/subfolder'));
96+
$this->assertTrue($storage1->file_exists('target/subfolder/test.txt'));
97+
98+
$this->assertTrue($storage2->file_exists('source'));
99+
$this->assertTrue($storage2->file_exists('source/test1.txt'));
100+
$this->assertTrue($storage2->file_exists('source/test2.txt'));
101+
$this->assertTrue($storage2->file_exists('source/subfolder'));
102+
$this->assertTrue($storage2->file_exists('source/subfolder/test.txt'));
103+
104+
$this->assertEquals('foo', $storage1->file_get_contents('target/test1.txt'));
105+
$this->assertEquals('qwerty', $storage1->file_get_contents('target/test2.txt'));
106+
$this->assertEquals('bar', $storage1->file_get_contents('target/subfolder/test.txt'));
107+
}
108+
}

0 commit comments

Comments
 (0)