From 7f1141f7e9805005c813d5ffd39ebdfa715dd290 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Mon, 26 Mar 2018 16:34:19 +0200 Subject: [PATCH 1/7] Make the MountManager strict Signed-off-by: Roeland Jago Douma --- lib/private/Files/Mount/Manager.php | 27 ++++++++++++------------ lib/public/Files/Mount/IMountManager.php | 15 +++++++------ 2 files changed, 22 insertions(+), 20 deletions(-) diff --git a/lib/private/Files/Mount/Manager.php b/lib/private/Files/Mount/Manager.php index 7bd888a6389..9f0d7647dab 100644 --- a/lib/private/Files/Mount/Manager.php +++ b/lib/private/Files/Mount/Manager.php @@ -1,4 +1,5 @@ 1) { $mountPoint .= '/'; @@ -57,7 +58,7 @@ class Manager implements IMountManager { * @param string $mountPoint * @param string $target */ - public function moveMount($mountPoint, $target){ + public function moveMount(string $mountPoint, string $target){ $this->mounts[$target] = $this->mounts[$mountPoint]; unset($this->mounts[$mountPoint]); } @@ -68,14 +69,14 @@ class Manager implements IMountManager { * @param string $path * @return MountPoint */ - public function find($path) { + public function find(string $path): IMountPoint { \OC_Util::setupFS(); $path = $this->formatPath($path); if (isset($this->mounts[$path])) { return $this->mounts[$path]; } - \OC_Hook::emit('OC_Filesystem', 'get_mountpoint', array('path' => $path)); + \OC_Hook::emit('OC_Filesystem', 'get_mountpoint', ['path' => $path]); $foundMountPoint = ''; $mountPoints = array_keys($this->mounts); foreach ($mountPoints as $mountpoint) { @@ -96,10 +97,10 @@ class Manager implements IMountManager { * @param string $path * @return MountPoint[] */ - public function findIn($path) { + public function findIn(string $path): array { \OC_Util::setupFS(); $path = $this->formatPath($path); - $result = array(); + $result = []; $pathLength = strlen($path); $mountPoints = array_keys($this->mounts); foreach ($mountPoints as $mountPoint) { @@ -111,7 +112,7 @@ class Manager implements IMountManager { } public function clear() { - $this->mounts = array(); + $this->mounts = []; } /** @@ -120,12 +121,12 @@ class Manager implements IMountManager { * @param string $id * @return MountPoint[] */ - public function findByStorageId($id) { + public function findByStorageId(string $id): array { \OC_Util::setupFS(); if (strlen($id) > 64) { $id = md5($id); } - $result = array(); + $result = []; foreach ($this->mounts as $mount) { if ($mount->getStorageId() === $id) { $result[] = $mount; @@ -137,7 +138,7 @@ class Manager implements IMountManager { /** * @return MountPoint[] */ - public function getAll() { + public function getAll(): array { return $this->mounts; } @@ -147,7 +148,7 @@ class Manager implements IMountManager { * @param int $id * @return MountPoint[] */ - public function findByNumericId($id) { + public function findByNumericId(int $id): array { $storageId = \OC\Files\Cache\Storage::getStorageId($id); return $this->findByStorageId($storageId); } @@ -156,7 +157,7 @@ class Manager implements IMountManager { * @param string $path * @return string */ - private function formatPath($path) { + private function formatPath(string $path): string { $path = Filesystem::normalizePath($path); if (strlen($path) > 1) { $path .= '/'; diff --git a/lib/public/Files/Mount/IMountManager.php b/lib/public/Files/Mount/IMountManager.php index 2b6458bf7c8..da40419d9e1 100644 --- a/lib/public/Files/Mount/IMountManager.php +++ b/lib/public/Files/Mount/IMountManager.php @@ -1,4 +1,5 @@ Date: Mon, 26 Mar 2018 16:35:47 +0200 Subject: [PATCH 2/7] Inspections Signed-off-by: Roeland Jago Douma --- lib/private/Files/Mount/Manager.php | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/lib/private/Files/Mount/Manager.php b/lib/private/Files/Mount/Manager.php index 9f0d7647dab..eb5d4c4a458 100644 --- a/lib/private/Files/Mount/Manager.php +++ b/lib/private/Files/Mount/Manager.php @@ -48,7 +48,7 @@ class Manager implements IMountManager { */ public function removeMount(string $mountPoint) { $mountPoint = Filesystem::normalizePath($mountPoint); - if (strlen($mountPoint) > 1) { + if (\strlen($mountPoint) > 1) { $mountPoint .= '/'; } unset($this->mounts[$mountPoint]); @@ -80,15 +80,16 @@ class Manager implements IMountManager { $foundMountPoint = ''; $mountPoints = array_keys($this->mounts); foreach ($mountPoints as $mountpoint) { - if (strpos($path, $mountpoint) === 0 and strlen($mountpoint) > strlen($foundMountPoint)) { + if (strpos($path, $mountpoint) === 0 && \strlen($mountpoint) > \strlen($foundMountPoint)) { $foundMountPoint = $mountpoint; } } + if (isset($this->mounts[$foundMountPoint])) { return $this->mounts[$foundMountPoint]; - } else { - return null; } + + return null; } /** @@ -101,10 +102,10 @@ class Manager implements IMountManager { \OC_Util::setupFS(); $path = $this->formatPath($path); $result = []; - $pathLength = strlen($path); + $pathLength = \strlen($path); $mountPoints = array_keys($this->mounts); foreach ($mountPoints as $mountPoint) { - if (substr($mountPoint, 0, $pathLength) === $path and strlen($mountPoint) > $pathLength) { + if (substr($mountPoint, 0, $pathLength) === $path && \strlen($mountPoint) > $pathLength) { $result[] = $this->mounts[$mountPoint]; } } @@ -123,7 +124,7 @@ class Manager implements IMountManager { */ public function findByStorageId(string $id): array { \OC_Util::setupFS(); - if (strlen($id) > 64) { + if (\strlen($id) > 64) { $id = md5($id); } $result = []; @@ -159,7 +160,7 @@ class Manager implements IMountManager { */ private function formatPath(string $path): string { $path = Filesystem::normalizePath($path); - if (strlen($path) > 1) { + if (\strlen($path) > 1) { $path .= '/'; } return $path; From e48d4c4aad1216a64d52ad5d87b0478944ce577f Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Mon, 26 Mar 2018 16:47:46 +0200 Subject: [PATCH 3/7] Cache the $foundMountPointLength Signed-off-by: Roeland Jago Douma --- lib/private/Files/Mount/Manager.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/private/Files/Mount/Manager.php b/lib/private/Files/Mount/Manager.php index eb5d4c4a458..7f21a1e7f99 100644 --- a/lib/private/Files/Mount/Manager.php +++ b/lib/private/Files/Mount/Manager.php @@ -79,9 +79,11 @@ class Manager implements IMountManager { \OC_Hook::emit('OC_Filesystem', 'get_mountpoint', ['path' => $path]); $foundMountPoint = ''; $mountPoints = array_keys($this->mounts); + $foundMountPointLength = 0; foreach ($mountPoints as $mountpoint) { - if (strpos($path, $mountpoint) === 0 && \strlen($mountpoint) > \strlen($foundMountPoint)) { + if (strpos($path, $mountpoint) === 0 && \strlen($mountpoint) > $foundMountPointLength) { $foundMountPoint = $mountpoint; + $foundMountPointLength = \strlen($foundMountPoint); } } From dc222aa3a50c32fb1aa9801ad7fedf98325477e8 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Mon, 26 Mar 2018 16:48:20 +0200 Subject: [PATCH 4/7] Comparing stringlength is cheaper than strpos Signed-off-by: Roeland Jago Douma --- lib/private/Files/Mount/Manager.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/Files/Mount/Manager.php b/lib/private/Files/Mount/Manager.php index 7f21a1e7f99..d98d45ae0b7 100644 --- a/lib/private/Files/Mount/Manager.php +++ b/lib/private/Files/Mount/Manager.php @@ -81,7 +81,7 @@ class Manager implements IMountManager { $mountPoints = array_keys($this->mounts); $foundMountPointLength = 0; foreach ($mountPoints as $mountpoint) { - if (strpos($path, $mountpoint) === 0 && \strlen($mountpoint) > $foundMountPointLength) { + if (\strlen($mountpoint) > $foundMountPointLength && strpos($path, $mountpoint) === 0) { $foundMountPoint = $mountpoint; $foundMountPointLength = \strlen($foundMountPoint); } From 6868da9958f25bbf0212d1cd2d5d918152e50d78 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Mon, 26 Mar 2018 19:47:38 +0200 Subject: [PATCH 5/7] Make normalized cache path larger On larger instances with a large number of shares this can make a real impact as the default 512 entries are easily filled. Making this contain max 2048 entries has basically no effect on smaller installations (as they probably never hit the 512 now). But makes sure we don't evict entries in the larger case. Signed-off-by: Roeland Jago Douma --- lib/private/Files/Filesystem.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/Files/Filesystem.php b/lib/private/Files/Filesystem.php index 95703eab925..b9f0813c84a 100644 --- a/lib/private/Files/Filesystem.php +++ b/lib/private/Files/Filesystem.php @@ -794,7 +794,7 @@ class Filesystem { */ public static function normalizePath($path, $stripTrailingSlash = true, $isAbsolutePath = false, $keepUnicode = false) { if (is_null(self::$normalizedPathCache)) { - self::$normalizedPathCache = new CappedMemoryCache(); + self::$normalizedPathCache = new CappedMemoryCache(2048); } /** From 37233471b6698f798c9f0b9bf0286c4d9c342204 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Mon, 26 Mar 2018 19:53:10 +0200 Subject: [PATCH 6/7] Add pathcache * If we find the mountpoint for a path cache it * If we modify the mount points empty the pathCache Signed-off-by: Roeland Jago Douma --- lib/private/Files/Mount/Manager.php | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/lib/private/Files/Mount/Manager.php b/lib/private/Files/Mount/Manager.php index d98d45ae0b7..f27dfaacc8b 100644 --- a/lib/private/Files/Mount/Manager.php +++ b/lib/private/Files/Mount/Manager.php @@ -26,21 +26,28 @@ declare(strict_types=1); namespace OC\Files\Mount; -use \OC\Files\Filesystem; +use OC\Cache\CappedMemoryCache; +use OC\Files\Filesystem; use OCP\Files\Mount\IMountManager; use OCP\Files\Mount\IMountPoint; class Manager implements IMountManager { - /** - * @var MountPoint[] - */ + /** @var MountPoint[] */ private $mounts = []; + /** @var CappedMemoryCache */ + private $pathCache; + + public function __construct() { + $this->pathCache = new CappedMemoryCache(); + } + /** * @param IMountPoint $mount */ public function addMount(IMountPoint $mount) { $this->mounts[$mount->getMountPoint()] = $mount; + $this->pathCache->clear(); } /** @@ -52,6 +59,7 @@ class Manager implements IMountManager { $mountPoint .= '/'; } unset($this->mounts[$mountPoint]); + $this->pathCache->clear(); } /** @@ -61,6 +69,7 @@ class Manager implements IMountManager { public function moveMount(string $mountPoint, string $target){ $this->mounts[$target] = $this->mounts[$mountPoint]; unset($this->mounts[$mountPoint]); + $this->pathCache->clear(); } /** @@ -76,6 +85,10 @@ class Manager implements IMountManager { return $this->mounts[$path]; } + if (isset($this->pathCache[$path])) { + return $this->pathCache[$path]; + } + \OC_Hook::emit('OC_Filesystem', 'get_mountpoint', ['path' => $path]); $foundMountPoint = ''; $mountPoints = array_keys($this->mounts); @@ -88,6 +101,7 @@ class Manager implements IMountManager { } if (isset($this->mounts[$foundMountPoint])) { + $this->pathCache[$path] = $this->mounts[$foundMountPoint]; return $this->mounts[$foundMountPoint]; } @@ -116,6 +130,7 @@ class Manager implements IMountManager { public function clear() { $this->mounts = []; + $this->pathCache->clear(); } /** From 73e6eea57e0454a74b6b461e15d2c4fac61ed524 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Wed, 4 Apr 2018 12:50:54 +0200 Subject: [PATCH 7/7] Fix tests Signed-off-by: Roeland Jago Douma --- lib/private/Files/Mount/Manager.php | 4 ++-- lib/public/Files/Mount/IMountManager.php | 4 ++-- tests/lib/Files/Storage/Wrapper/EncryptionTest.php | 4 ++++ 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/private/Files/Mount/Manager.php b/lib/private/Files/Mount/Manager.php index f27dfaacc8b..019dda03a40 100644 --- a/lib/private/Files/Mount/Manager.php +++ b/lib/private/Files/Mount/Manager.php @@ -76,9 +76,9 @@ class Manager implements IMountManager { * Find the mount for $path * * @param string $path - * @return MountPoint + * @return MountPoint|null */ - public function find(string $path): IMountPoint { + public function find(string $path) { \OC_Util::setupFS(); $path = $this->formatPath($path); if (isset($this->mounts[$path])) { diff --git a/lib/public/Files/Mount/IMountManager.php b/lib/public/Files/Mount/IMountManager.php index da40419d9e1..b9ec85bc1d4 100644 --- a/lib/public/Files/Mount/IMountManager.php +++ b/lib/public/Files/Mount/IMountManager.php @@ -61,10 +61,10 @@ interface IMountManager { * Find the mount for $path * * @param string $path - * @return \OCP\Files\Mount\IMountPoint + * @return \OCP\Files\Mount\IMountPoint|null * @since 8.2.0 */ - public function find(string $path): IMountPoint; + public function find(string $path); /** * Find all mounts in $path diff --git a/tests/lib/Files/Storage/Wrapper/EncryptionTest.php b/tests/lib/Files/Storage/Wrapper/EncryptionTest.php index 082c08b3e43..22c93ee0a91 100644 --- a/tests/lib/Files/Storage/Wrapper/EncryptionTest.php +++ b/tests/lib/Files/Storage/Wrapper/EncryptionTest.php @@ -730,6 +730,8 @@ class EncryptionTest extends Storage { $temp = \OC::$server->getTempManager(); return fopen($temp->getTemporaryFile(), $mode); }); + $storage2->method('getId') + ->willReturn('stroage2'); $cache = $this->createMock(ICache::class); $cache->expects($this->once()) ->method('get') @@ -777,6 +779,8 @@ class EncryptionTest extends Storage { $temp = \OC::$server->getTempManager(); return fopen($temp->getTemporaryFile(), $mode); }); + $storage2->method('getId') + ->willReturn('stroage2'); if($expectedEncrypted) { $cache = $this->createMock(ICache::class); $cache->expects($this->once())