From: Aaron Schulz <aschulz@wikimedia.org>
Date: Sun, 16 Jun 2013 20:48:17 +0000 (-0700)
Subject: lockmanager: QuorumLockManager subclasses can get EX/SH locks at once
X-Git-Tag: 1.31.0-rc.0~18436
X-Git-Url: http://git.cyclocoop.org/%22.%28%24lien.?a=commitdiff_plain;h=25d1af122d9de5a5c8890b170d330d1cf1e7f7a9;p=lhc%2Fweb%2Fwiklou.git

lockmanager: QuorumLockManager subclasses can get EX/SH locks at once

* Also reduced rount trips in doUnlockingRequestBucket().
* Also removed some redundant doc comments.

Change-Id: I81878e92332509bd7fda9ddeef950b774f5b015d
---

diff --git a/includes/filebackend/lockmanager/DBLockManager.php b/includes/filebackend/lockmanager/DBLockManager.php
index e081987851..3e934ba502 100644
--- a/includes/filebackend/lockmanager/DBLockManager.php
+++ b/includes/filebackend/lockmanager/DBLockManager.php
@@ -110,6 +110,19 @@ abstract class DBLockManager extends QuorumLockManager {
 		$this->session = wfRandomString( 31 );
 	}
 
+	// @TODO: change this code to work in one batch
+	protected function getLocksOnServer( $lockSrv, array $pathsByType ) {
+		$status = Status::newGood();
+		foreach ( $pathsByType as $type => $paths ) {
+			$status->merge( $this->doGetLocksOnServer( $lockSrv, $paths, $type ) );
+		}
+		return $status;
+	}
+
+	protected function freeLocksOnServer( $lockSrv, array $pathsByType ) {
+		return Status::newGood();
+	}
+
 	/**
 	 * @see QuorumLockManager::isServerUp()
 	 * @return bool
@@ -252,7 +265,7 @@ class MySqlLockManager extends DBLockManager {
 	 * @see DBLockManager::getLocksOnServer()
 	 * @return Status
 	 */
-	protected function getLocksOnServer( $lockSrv, array $paths, $type ) {
+	protected function doGetLocksOnServer( $lockSrv, array $paths, $type ) {
 		$status = Status::newGood();
 
 		$db = $this->getConnection( $lockSrv ); // checked in isServerUp()
@@ -318,14 +331,6 @@ class MySqlLockManager extends DBLockManager {
 		return $status;
 	}
 
-	/**
-	 * @see QuorumLockManager::freeLocksOnServer()
-	 * @return Status
-	 */
-	protected function freeLocksOnServer( $lockSrv, array $paths, $type ) {
-		return Status::newGood(); // not supported
-	}
-
 	/**
 	 * @see QuorumLockManager::releaseAllLocks()
 	 * @return Status
@@ -361,7 +366,7 @@ class PostgreSqlLockManager extends DBLockManager {
 		self::LOCK_EX => self::LOCK_EX
 	);
 
-	protected function getLocksOnServer( $lockSrv, array $paths, $type ) {
+	protected function doGetLocksOnServer( $lockSrv, array $paths, $type ) {
 		$status = Status::newGood();
 		if ( !count( $paths ) ) {
 			return $status; // nothing to lock
@@ -407,14 +412,6 @@ class PostgreSqlLockManager extends DBLockManager {
 		return $status;
 	}
 
-	/**
-	 * @see QuorumLockManager::freeLocksOnServer()
-	 * @return Status
-	 */
-	protected function freeLocksOnServer( $lockSrv, array $paths, $type ) {
-		return Status::newGood(); // not supported
-	}
-
 	/**
 	 * @see QuorumLockManager::releaseAllLocks()
 	 * @return Status
diff --git a/includes/filebackend/lockmanager/LockManager.php b/includes/filebackend/lockmanager/LockManager.php
index f0e54ec22f..dad8a62468 100644
--- a/includes/filebackend/lockmanager/LockManager.php
+++ b/includes/filebackend/lockmanager/LockManager.php
@@ -98,7 +98,7 @@ abstract class LockManager {
 	/**
 	 * Lock the resources at the given abstract paths
 	 *
-	 * @param array $paths Map of LockManager::LOCK_* constants to lists of storage paths
+	 * @param array $pathsByType Map of LockManager::LOCK_* constants to lists of paths
 	 * @param integer $timeout Timeout in seconds (0 means non-blocking) (since 1.21)
 	 * @return Status
 	 * @since 1.22
@@ -125,7 +125,7 @@ abstract class LockManager {
 	/**
 	 * Unlock the resources at the given abstract paths
 	 *
-	 * @param array $paths List of storage paths
+	 * @param array $paths List of paths
 	 * @param $type integer LockManager::LOCK_* constant
 	 * @return Status
 	 */
@@ -136,7 +136,7 @@ abstract class LockManager {
 	/**
 	 * Unlock the resources at the given abstract paths
 	 *
-	 * @param array $paths Map of LockManager::LOCK_* constants to lists of storage paths
+	 * @param array $pathsByType Map of LockManager::LOCK_* constants to lists of paths
 	 * @return Status
 	 * @since 1.22
 	 */
@@ -176,7 +176,7 @@ abstract class LockManager {
 	 * Normalize the $paths array by converting LOCK_UW locks into the
 	 * appropriate type and removing any duplicated paths for each lock type.
 	 *
-	 * @param array $paths Map of LockManager::LOCK_* constants to lists of storage paths
+	 * @param array $paths Map of LockManager::LOCK_* constants to lists of paths
 	 * @return Array
 	 * @since 1.22
 	 */
@@ -190,7 +190,7 @@ abstract class LockManager {
 
 	/**
 	 * @see LockManager::lockByType()
-	 * @param array $paths Map of LockManager::LOCK_* constants to lists of storage paths
+	 * @param array $paths Map of LockManager::LOCK_* constants to lists of paths
 	 * @return Status
 	 * @since 1.22
 	 */
@@ -215,7 +215,7 @@ abstract class LockManager {
 	/**
 	 * Lock resources with the given keys and lock type
 	 *
-	 * @param array $paths List of storage paths
+	 * @param array $paths List of paths
 	 * @param $type integer LockManager::LOCK_* constant
 	 * @return Status
 	 */
@@ -223,7 +223,7 @@ abstract class LockManager {
 
 	/**
 	 * @see LockManager::unlockByType()
-	 * @param array $paths Map of LockManager::LOCK_* constants to lists of storage paths
+	 * @param array $paths Map of LockManager::LOCK_* constants to lists of paths
 	 * @return Status
 	 * @since 1.22
 	 */
@@ -238,7 +238,7 @@ abstract class LockManager {
 	/**
 	 * Unlock resources with the given keys and lock type
 	 *
-	 * @param array $paths List of storage paths
+	 * @param array $paths List of paths
 	 * @param $type integer LockManager::LOCK_* constant
 	 * @return Status
 	 */
@@ -250,22 +250,10 @@ abstract class LockManager {
  * @since 1.19
  */
 class NullLockManager extends LockManager {
-	/**
-	 * @see LockManager::doLock()
-	 * @param $paths array
-	 * @param $type int
-	 * @return Status
-	 */
 	protected function doLock( array $paths, $type ) {
 		return Status::newGood();
 	}
 
-	/**
-	 * @see LockManager::doUnlock()
-	 * @param $paths array
-	 * @param $type int
-	 * @return Status
-	 */
 	protected function doUnlock( array $paths, $type ) {
 		return Status::newGood();
 	}
diff --git a/includes/filebackend/lockmanager/MemcLockManager.php b/includes/filebackend/lockmanager/MemcLockManager.php
index 757aeee1fa..5eab03eef0 100644
--- a/includes/filebackend/lockmanager/MemcLockManager.php
+++ b/includes/filebackend/lockmanager/MemcLockManager.php
@@ -88,11 +88,44 @@ class MemcLockManager extends QuorumLockManager {
 		$this->session = wfRandomString( 32 );
 	}
 
+	// @TODO: change this code to work in one batch
+	protected function getLocksOnServer( $lockSrv, array $pathsByType ) {
+		$status = Status::newGood();
+
+		$lockedPaths = array();
+		foreach ( $pathsByType as $type => $paths ) {
+			$status->merge( $this->doGetLocksOnServer( $lockSrv, $paths, $type ) );
+			if ( $status->isOK() ) {
+				$lockedPaths[$type] = isset( $lockedPaths[$type] )
+					? array_merge( $lockedPaths[$type], $paths )
+					: $paths;
+			} else {
+				foreach ( $lockedPaths as $type => $paths ) {
+					$status->merge( $this->doFreeLocksOnServer( $lockSrv, $paths, $type ) );
+				}
+				break;
+			}
+		}
+
+		return $status;
+	}
+
+	// @TODO: change this code to work in one batch
+	protected function freeLocksOnServer( $lockSrv, array $pathsByType ) {
+		$status = Status::newGood();
+
+		foreach ( $pathsByType as $type => $paths ) {
+			$status->merge( $this->doFreeLocksOnServer( $lockSrv, $paths, $type ) );
+		}
+
+		return $status;
+	}
+
 	/**
 	 * @see QuorumLockManager::getLocksOnServer()
 	 * @return Status
 	 */
-	protected function getLocksOnServer( $lockSrv, array $paths, $type ) {
+	protected function doGetLocksOnServer( $lockSrv, array $paths, $type ) {
 		$status = Status::newGood();
 
 		$memc = $this->getCache( $lockSrv );
@@ -164,7 +197,7 @@ class MemcLockManager extends QuorumLockManager {
 	 * @see QuorumLockManager::freeLocksOnServer()
 	 * @return Status
 	 */
-	protected function freeLocksOnServer( $lockSrv, array $paths, $type ) {
+	protected function doFreeLocksOnServer( $lockSrv, array $paths, $type ) {
 		$status = Status::newGood();
 
 		$memc = $this->getCache( $lockSrv );
diff --git a/includes/filebackend/lockmanager/QuorumLockManager.php b/includes/filebackend/lockmanager/QuorumLockManager.php
index 3b96ad6906..8356d32a64 100644
--- a/includes/filebackend/lockmanager/QuorumLockManager.php
+++ b/includes/filebackend/lockmanager/QuorumLockManager.php
@@ -31,81 +31,86 @@
 abstract class QuorumLockManager extends LockManager {
 	/** @var Array Map of bucket indexes to peer server lists */
 	protected $srvsByBucket = array(); // (bucket index => (lsrv1, lsrv2, ...))
+	/** @var Array Map of degraded buckets */
+	protected $degradedBuckets = array(); // (buckey index => UNIX timestamp)
 
-	/**
-	 * @see LockManager::doLock()
-	 * @param $paths array
-	 * @param $type int
-	 * @return Status
-	 */
 	final protected function doLock( array $paths, $type ) {
+		return $this->doLockByType( array( $type => $paths ) );
+	}
+
+	final protected function doUnlock( array $paths, $type ) {
+		return $this->doUnlockByType( array( $type => $paths ) );
+	}
+
+	protected function doLockByType( array $pathsByType ) {
 		$status = Status::newGood();
 
-		$pathsToLock = array(); // (bucket => paths)
+		$pathsToLock = array(); // (bucket => type => paths)
 		// Get locks that need to be acquired (buckets => locks)...
-		foreach ( $paths as $path ) {
-			if ( isset( $this->locksHeld[$path][$type] ) ) {
-				++$this->locksHeld[$path][$type];
-			} else {
-				$bucket = $this->getBucketFromPath( $path );
-				$pathsToLock[$bucket][] = $path;
+		foreach ( $pathsByType as $type => $paths ) {
+			foreach ( $paths as $path ) {
+				if ( isset( $this->locksHeld[$path][$type] ) ) {
+					++$this->locksHeld[$path][$type];
+				} else {
+					$bucket = $this->getBucketFromPath( $path );
+					$pathsToLock[$bucket][$type][] = $path;
+				}
 			}
 		}
 
-		$lockedPaths = array(); // files locked in this attempt
+		$lockedPaths = array(); // files locked in this attempt (type => paths)
 		// Attempt to acquire these locks...
-		foreach ( $pathsToLock as $bucket => $paths ) {
+		foreach ( $pathsToLock as $bucket => $pathsToLockByType ) {
 			// Try to acquire the locks for this bucket
-			$status->merge( $this->doLockingRequestBucket( $bucket, $paths, $type ) );
+			$status->merge( $this->doLockingRequestBucket( $bucket, $pathsToLockByType ) );
 			if ( !$status->isOK() ) {
-				$status->merge( $this->doUnlock( $lockedPaths, $type ) );
+				$status->merge( $this->doUnlockByType( $lockedPaths ) );
 				return $status;
 			}
 			// Record these locks as active
-			foreach ( $paths as $path ) {
-				$this->locksHeld[$path][$type] = 1; // locked
+			foreach ( $pathsToLockByType as $type => $paths ) {
+				foreach ( $paths as $path ) {
+					$this->locksHeld[$path][$type] = 1; // locked
+					// Keep track of what locks were made in this attempt
+					$lockedPaths[$type][] = $path;
+				}
 			}
-			// Keep track of what locks were made in this attempt
-			$lockedPaths = array_merge( $lockedPaths, $paths );
 		}
 
 		return $status;
 	}
 
-	/**
-	 * @see LockManager::doUnlock()
-	 * @param $paths array
-	 * @param $type int
-	 * @return Status
-	 */
-	final protected function doUnlock( array $paths, $type ) {
+	protected function doUnlockByType( array $pathsByType ) {
 		$status = Status::newGood();
 
-		$pathsToUnlock = array();
-		foreach ( $paths as $path ) {
-			if ( !isset( $this->locksHeld[$path][$type] ) ) {
-				$status->warning( 'lockmanager-notlocked', $path );
-			} else {
-				--$this->locksHeld[$path][$type];
-				// Reference count the locks held and release locks when zero
-				if ( $this->locksHeld[$path][$type] <= 0 ) {
-					unset( $this->locksHeld[$path][$type] );
-					$bucket = $this->getBucketFromPath( $path );
-					$pathsToUnlock[$bucket][] = $path;
-				}
-				if ( !count( $this->locksHeld[$path] ) ) {
-					unset( $this->locksHeld[$path] ); // no SH or EX locks left for key
+		$pathsToUnlock = array(); // (bucket => type => paths)
+		foreach ( $pathsByType as $type => $paths ) {
+			foreach ( $paths as $path ) {
+				if ( !isset( $this->locksHeld[$path][$type] ) ) {
+					$status->warning( 'lockmanager-notlocked', $path );
+				} else {
+					--$this->locksHeld[$path][$type];
+					// Reference count the locks held and release locks when zero
+					if ( $this->locksHeld[$path][$type] <= 0 ) {
+						unset( $this->locksHeld[$path][$type] );
+						$bucket = $this->getBucketFromPath( $path );
+						$pathsToUnlock[$bucket][$type][] = $path;
+					}
+					if ( !count( $this->locksHeld[$path] ) ) {
+						unset( $this->locksHeld[$path] ); // no SH or EX locks left for key
+					}
 				}
 			}
 		}
 
 		// Remove these specific locks if possible, or at least release
 		// all locks once this process is currently not holding any locks.
-		foreach ( $pathsToUnlock as $bucket => $paths ) {
-			$status->merge( $this->doUnlockingRequestBucket( $bucket, $paths, $type ) );
+		foreach ( $pathsToUnlock as $bucket => $pathsToUnlockByType ) {
+			$status->merge( $this->doUnlockingRequestBucket( $bucket, $pathsToUnlockByType ) );
 		}
 		if ( !count( $this->locksHeld ) ) {
 			$status->merge( $this->releaseAllLocks() );
+			$this->degradedBuckets = array(); // safe to retry the normal quorum
 		}
 
 		return $status;
@@ -116,11 +121,10 @@ abstract class QuorumLockManager extends LockManager {
 	 * This is all or nothing; if any key is locked then this totally fails.
 	 *
 	 * @param $bucket integer
-	 * @param array $paths List of resource keys to lock
-	 * @param $type integer LockManager::LOCK_EX or LockManager::LOCK_SH
+	 * @param array $pathsByType Map of LockManager::LOCK_* constants to lists of paths
 	 * @return Status
 	 */
-	final protected function doLockingRequestBucket( $bucket, array $paths, $type ) {
+	final protected function doLockingRequestBucket( $bucket, array $pathsByType ) {
 		$status = Status::newGood();
 
 		$yesVotes = 0; // locks made on trustable servers
@@ -131,10 +135,11 @@ abstract class QuorumLockManager extends LockManager {
 			if ( !$this->isServerUp( $lockSrv ) ) {
 				--$votesLeft;
 				$status->warning( 'lockmanager-fail-svr-acquire', $lockSrv );
+				$this->degradedBuckets[$bucket] = time();
 				continue; // server down?
 			}
 			// Attempt to acquire the lock on this peer
-			$status->merge( $this->getLocksOnServer( $lockSrv, $paths, $type ) );
+			$status->merge( $this->getLocksOnServer( $lockSrv, $pathsByType ) );
 			if ( !$status->isOK() ) {
 				return $status; // vetoed; resource locked
 			}
@@ -158,21 +163,33 @@ abstract class QuorumLockManager extends LockManager {
 	 * Attempt to release locks with the peers for a bucket
 	 *
 	 * @param $bucket integer
-	 * @param array $paths List of resource keys to lock
-	 * @param $type integer LockManager::LOCK_EX or LockManager::LOCK_SH
+	 * @param array $pathsByType Map of LockManager::LOCK_* constants to lists of paths
 	 * @return Status
 	 */
-	final protected function doUnlockingRequestBucket( $bucket, array $paths, $type ) {
+	final protected function doUnlockingRequestBucket( $bucket, array $pathsByType ) {
 		$status = Status::newGood();
 
+		$yesVotes = 0; // locks freed on trustable servers
+		$votesLeft = count( $this->srvsByBucket[$bucket] ); // remaining peers
+		$quorum = floor( $votesLeft / 2 + 1 ); // simple majority
+		$isDegraded = isset( $this->degradedBuckets[$bucket] ); // not the normal quorum?
 		foreach ( $this->srvsByBucket[$bucket] as $lockSrv ) {
 			if ( !$this->isServerUp( $lockSrv ) ) {
-				$status->fatal( 'lockmanager-fail-svr-release', $lockSrv );
+				$status->warning( 'lockmanager-fail-svr-release', $lockSrv );
 			// Attempt to release the lock on this peer
 			} else {
-				$status->merge( $this->freeLocksOnServer( $lockSrv, $paths, $type ) );
+				$status->merge( $this->freeLocksOnServer( $lockSrv, $pathsByType ) );
+				++$yesVotes; // success for this peer
+				// Normally the first peers form the quorum, and the others are ignored.
+				// Ignore them in this case, but not when an alternative quorum was used.
+				if ( $yesVotes >= $quorum && !$isDegraded ) {
+					break; // lock released
+				}
 			}
 		}
+		// Set a bad status if the quorum was not met.
+		// Assumes the same "up" servers as during the acquire step.
+		$status->setResult( $yesVotes >= $quorum );
 
 		return $status;
 	}
@@ -190,7 +207,8 @@ abstract class QuorumLockManager extends LockManager {
 	}
 
 	/**
-	 * Check if a lock server is up
+	 * Check if a lock server is up.
+	 * This should process cache results to reduce RTT.
 	 *
 	 * @param $lockSrv string
 	 * @return bool
@@ -198,14 +216,13 @@ abstract class QuorumLockManager extends LockManager {
 	abstract protected function isServerUp( $lockSrv );
 
 	/**
-	 * Get a connection to a lock server and acquire locks on $paths
+	 * Get a connection to a lock server and acquire locks
 	 *
 	 * @param $lockSrv string
-	 * @param $paths array
-	 * @param $type integer
+	 * @param array $pathsByType Map of LockManager::LOCK_* constants to lists of paths
 	 * @return Status
 	 */
-	abstract protected function getLocksOnServer( $lockSrv, array $paths, $type );
+	abstract protected function getLocksOnServer( $lockSrv, array $pathsByType );
 
 	/**
 	 * Get a connection to a lock server and release locks on $paths.
@@ -213,11 +230,10 @@ abstract class QuorumLockManager extends LockManager {
 	 * Subclasses must effectively implement this or releaseAllLocks().
 	 *
 	 * @param $lockSrv string
-	 * @param $paths array
-	 * @param $type integer
+	 * @param array $pathsByType Map of LockManager::LOCK_* constants to lists of paths
 	 * @return Status
 	 */
-	abstract protected function freeLocksOnServer( $lockSrv, array $paths, $type );
+	abstract protected function freeLocksOnServer( $lockSrv, array $pathsByType );
 
 	/**
 	 * Release all locks that this session is holding.
diff --git a/includes/filebackend/lockmanager/RedisLockManager.php b/includes/filebackend/lockmanager/RedisLockManager.php
index e8e5996692..43b0198a68 100644
--- a/includes/filebackend/lockmanager/RedisLockManager.php
+++ b/includes/filebackend/lockmanager/RedisLockManager.php
@@ -78,7 +78,40 @@ class RedisLockManager extends QuorumLockManager {
 		$this->session = wfRandomString( 32 );
 	}
 
-	protected function getLocksOnServer( $lockSrv, array $paths, $type ) {
+	// @TODO: change this code to work in one batch
+	protected function getLocksOnServer( $lockSrv, array $pathsByType ) {
+		$status = Status::newGood();
+
+		$lockedPaths = array();
+		foreach ( $pathsByType as $type => $paths ) {
+			$status->merge( $this->doGetLocksOnServer( $lockSrv, $paths, $type ) );
+			if ( $status->isOK() ) {
+				$lockedPaths[$type] = isset( $lockedPaths[$type] )
+					? array_merge( $lockedPaths[$type], $paths )
+					: $paths;
+			} else {
+				foreach ( $lockedPaths as $type => $paths ) {
+					$status->merge( $this->doFreeLocksOnServer( $lockSrv, $paths, $type ) );
+				}
+				break;
+			}
+		}
+
+		return $status;
+	}
+
+	// @TODO: change this code to work in one batch
+	protected function freeLocksOnServer( $lockSrv, array $pathsByType ) {
+		$status = Status::newGood();
+
+		foreach ( $pathsByType as $type => $paths ) {
+			$status->merge( $this->doFreeLocksOnServer( $lockSrv, $paths, $type ) );
+		}
+
+		return $status;
+	}
+
+	protected function doGetLocksOnServer( $lockSrv, array $paths, $type ) {
 		$status = Status::newGood();
 
 		$server = $this->lockServers[$lockSrv];
@@ -162,7 +195,7 @@ LUA;
 		return $status;
 	}
 
-	protected function freeLocksOnServer( $lockSrv, array $paths, $type ) {
+	protected function doFreeLocksOnServer( $lockSrv, array $paths, $type ) {
 		$status = Status::newGood();
 
 		$server = $this->lockServers[$lockSrv];