From: Aaron Schulz <aschulz@wikimedia.org>
Date: Sat, 7 Sep 2019 08:25:19 +0000 (-0700)
Subject: filebackend: optimize 'move' in FSFileBackend to avoid is_file() calls
X-Git-Tag: 1.34.0-rc.0~338
X-Git-Url: https://git.cyclocoop.org//%22%7B%7Blocalurle:Project:Pol%C3%ADtica_de_imagens%7D%7D/%22?a=commitdiff_plain;h=417dea8e9591c773985562c1bdc5d6831245c2c6;p=lhc%2Fweb%2Fwiklou.git

filebackend: optimize 'move' in FSFileBackend to avoid is_file() calls

Also, remove clearstatcache() call that is redundant with moveInternal()

Change-Id: I56e6c3d91427e7d0b49011f884b77daa5eb0b61c
---

diff --git a/includes/libs/filebackend/FSFileBackend.php b/includes/libs/filebackend/FSFileBackend.php
index 3256f93b40..0549d91bdb 100644
--- a/includes/libs/filebackend/FSFileBackend.php
+++ b/includes/libs/filebackend/FSFileBackend.php
@@ -366,34 +366,38 @@ class FSFileBackend extends FileBackendStore {
 	protected function doMoveInternal( array $params ) {
 		$status = $this->newStatus();
 
-		$source = $this->resolveToFSPath( $params['src'] );
-		if ( $source === null ) {
+		$fsSrcPath = $this->resolveToFSPath( $params['src'] );
+		if ( $fsSrcPath === null ) {
 			$status->fatal( 'backend-fail-invalidpath', $params['src'] );
 
 			return $status;
 		}
 
-		$dest = $this->resolveToFSPath( $params['dst'] );
-		if ( $dest === null ) {
+		$fsDstPath = $this->resolveToFSPath( $params['dst'] );
+		if ( $fsDstPath === null ) {
 			$status->fatal( 'backend-fail-invalidpath', $params['dst'] );
 
 			return $status;
 		}
 
-		if ( !is_file( $source ) ) {
-			if ( empty( $params['ignoreMissingSource'] ) ) {
-				$status->fatal( 'backend-fail-move', $params['src'] );
-			}
-
-			return $status; // do nothing; either OK or bad status
+		if ( $fsSrcPath === $fsDstPath ) {
+			return $status; // no-op
 		}
 
+		$ignoreMissing = !empty( $params['ignoreMissingSource'] );
+
 		if ( !empty( $params['async'] ) ) { // deferred
-			$cmd = implode( ' ', [
-				$this->isWindows ? 'MOVE /Y' : 'mv', // (overwrite)
-				escapeshellarg( $this->cleanPathSlashes( $source ) ),
-				escapeshellarg( $this->cleanPathSlashes( $dest ) )
-			] );
+			// https://manpages.debian.org/buster/coreutils/mv.1.en.html
+			// https://docs.microsoft.com/en-us/windows-server/administration/windows-commands/move
+			$encSrc = escapeshellarg( $this->cleanPathSlashes( $fsSrcPath ) );
+			$encDst	= escapeshellarg( $this->cleanPathSlashes( $fsDstPath ) );
+			if ( $this->isWindows ) {
+				$writeCmd = "MOVE /Y $encSrc $encDst";
+				$cmd = $ignoreMissing ? "IF EXIST $encSrc $writeCmd" : $writeCmd;
+			} else {
+				$writeCmd = "mv -f $encSrc $encDst";
+				$cmd = $ignoreMissing ? "test -f $encSrc && $writeCmd" : $writeCmd;
+			}
 			$handler = function ( $errors, StatusValue $status, array $params, $cmd ) {
 				if ( $errors !== '' && !( $this->isWindows && $errors[0] === " " ) ) {
 					$status->fatal( 'backend-fail-move', $params['src'], $params['dst'] );
@@ -402,11 +406,13 @@ class FSFileBackend extends FileBackendStore {
 			};
 			$status->value = new FSFileOpHandle( $this, $params, $handler, $cmd );
 		} else { // immediate write
-			$this->trapWarnings();
-			$ok = ( $source === $dest ) ? true : rename( $source, $dest );
-			$this->untrapWarnings();
-			clearstatcache(); // file no longer at source
-			if ( !$ok ) {
+			// Use rename() here since (a) this clears xattrs, (b) any threads still reading the
+			// old inode are unaffected since it writes to a new inode, and (c) this is fast and
+			// atomic within a file system volume (as is normally the case)
+			$this->trapWarnings( '/: No such file or directory$/' );
+			$moved = rename( $fsSrcPath, $fsDstPath );
+			$hadError = $this->untrapWarnings();
+			if ( $hadError || ( !$moved && !$ignoreMissing ) ) {
 				$status->fatal( 'backend-fail-move', $params['src'], $params['dst'] );
 
 				return $status;
diff --git a/tests/phpunit/includes/filebackend/FileBackendTest.php b/tests/phpunit/includes/filebackend/FileBackendTest.php
index ae46aa7ec2..3b30d7ef71 100644
--- a/tests/phpunit/includes/filebackend/FileBackendTest.php
+++ b/tests/phpunit/includes/filebackend/FileBackendTest.php
@@ -942,8 +942,7 @@ class FileBackendTest extends MediaWikiTestCase {
 			"$base/unittest-cont1/e/fileB.a",
 			"$base/unittest-cont1/e/fileC.a"
 		];
-		$createOps = [];
-		$delOps = [];
+		$createOps = $copyOps = $moveOps = $deleteOps = [];
 		foreach ( $files as $path ) {
 			$status = $this->prepare( [ 'dir' => dirname( $path ) ] );
 			$this->assertGoodStatus( $status,
@@ -951,11 +950,21 @@ class FileBackendTest extends MediaWikiTestCase {
 			$createOps[] = [ 'op' => 'create', 'dst' => $path, 'content' => mt_rand( 0, 50000 ) ];
 			$copyOps[] = [ 'op' => 'copy', 'src' => $path, 'dst' => "$path-2" ];
 			$moveOps[] = [ 'op' => 'move', 'src' => "$path-2", 'dst' => "$path-3" ];
-			$delOps[] = [ 'op' => 'delete', 'src' => $path ];
-			$delOps[] = [ 'op' => 'delete', 'src' => "$path-3" ];
-			$delOps[] = [ 'op' => 'delete', 'src' => "$path-gone", 'ignoreMissingSource' => true ];
+			$moveOps[] = [
+				'op' => 'move',
+				'src' => "$path-nothing",
+				'dst' => "$path-nowhere",
+				'ignoreMissingSource' => true
+			];
+			$deleteOps[] = [ 'op' => 'delete', 'src' => $path ];
+			$deleteOps[] = [ 'op' => 'delete', 'src' => "$path-3" ];
+			$deleteOps[] = [
+				'op' => 'delete',
+				'src' => "$path-gone",
+				'ignoreMissingSource' => true
+			];
 		}
-		$delOps[] = [ 'op' => 'null' ];
+		$deleteOps[] = [ 'op' => 'null' ];
 
 		$this->assertGoodStatus(
 			$this->backend->doQuickOperations( $createOps ),
@@ -996,7 +1005,7 @@ class FileBackendTest extends MediaWikiTestCase {
 			"File {$files[0]} still exists." );
 
 		$this->assertGoodStatus(
-			$this->backend->doQuickOperations( $delOps ),
+			$this->backend->doQuickOperations( $deleteOps ),
 			"Quick deletion of source files succeeded ($backendName)." );
 		foreach ( $files as $file ) {
 			$this->assertFalse( $this->backend->fileExists( [ 'src' => $file ] ),