[gentoo-catalyst] [PATCH 1/2] stagebase: simplify lock calls a bit

[gentoo-catalyst] [PATCH 1/2] stagebase: simplify lock calls a bit

[Mike Frysinger]

We create self.snapcache_lock to hold the lock, then assign it to
self.snapshot_lock_object, and then operate on self.snapshot_lock_object.
There's no need for this indirection, so operate on self.snapcache_lock
directly instead.
---
 catalyst/base/stagebase.py | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/catalyst/base/stagebase.py b/catalyst/base/stagebase.py
index 88d71ba..3ce7dba 100644
--- a/catalyst/base/stagebase.py
+++ b/catalyst/base/stagebase.py
@@ -150,7 +150,6 @@ class StageBase(TargetBase, ClearBase, GenBase):
 		self.set_source_subpath()
 
 		# Set paths
-		self.snapshot_lock_object = None
 		self.set_snapshot_path()
 		self.set_root_path()
 		self.set_source_path()
@@ -822,7 +821,6 @@ class StageBase(TargetBase, ClearBase, GenBase):
 				self.settings["snapshot_cache_path"]+\
 				" (This can take a long time)..."
 			cleanup_errmsg="Error removing existing snapshot cache directory."
-			self.snapshot_lock_object=self.snapcache_lock
 
 			if self.settings["snapshot_path_hash"]==snapshot_cache_hash:
 				print "Valid snapshot cache, skipping unpack of portage tree..."
@@ -846,7 +844,7 @@ class StageBase(TargetBase, ClearBase, GenBase):
 
 		if unpack:
 			if "snapcache" in self.settings["options"]:
-				self.snapshot_lock_object.write_lock()
+				self.snapcache_lock.write_lock()
 			if os.path.exists(target_portdir):
 				print cleanup_msg
 				cleanup_cmd = "rm -rf " + target_portdir
@@ -868,7 +866,7 @@ class StageBase(TargetBase, ClearBase, GenBase):
 					data=self.settings["snapshot_path_hash"])
 
 			if "snapcache" in self.settings["options"]:
-				self.snapshot_lock_object.unlock()
+				self.snapcache_lock.unlock()
 
 	def config_profile_link(self):
 		if "autoresume" in self.settings["options"] \
@@ -946,7 +944,7 @@ class StageBase(TargetBase, ClearBase, GenBase):
 			src=self.mountmap[x]
 			#print "bind(); src =", src
 			if "snapcache" in self.settings["options"] and x == "portdir":
-				self.snapshot_lock_object.read_lock()
+				self.snapcache_lock.read_lock()
 			if os.uname()[0] == "FreeBSD":
 				if src == "/dev":
 					_cmd = "mount -t devfs none " + target
@@ -998,7 +996,7 @@ class StageBase(TargetBase, ClearBase, GenBase):
 					# It's possible the snapshot lock object isn't created yet.
 					# This is because mount safety check calls unbind before the
 					# target is fully initialized
-					self.snapshot_lock_object.unlock()
+					self.snapcache_lock.unlock()
 				except Exception:
 					pass
 		if ouch:
-- 
2.5.2

[gentoo-catalyst] [PATCH 2/2] lock: gut & replace with snakeoil

[Mike Frysinger]

The hardlink logic is unused, so start by deleting all of that.
If someone wants that (multiple builds on NFS?), we can look at
restoring it.
---
 catalyst/lock.py | 467 ++-----------------------------------------------------
 1 file changed, 15 insertions(+), 452 deletions(-)

diff --git a/catalyst/lock.py b/catalyst/lock.py
index 8095a82..39926dd 100644
--- a/catalyst/lock.py
+++ b/catalyst/lock.py
@@ -1,467 +1,30 @@
 
-
 import os
-import fcntl
-import errno
-import sys
-import time
-from catalyst.support import CatalystError, normpath
 
-def writemsg(mystr):
-	sys.stderr.write(mystr)
-	sys.stderr.flush()
+from snakeoil import fileutils
+from snakeoil import osutils
 
 
-class LockInUse(Exception):
-	def __init__(self, message):
-		if message:
-			#(type,value)=sys.exc_info()[:2]
-			#if value!=None:
-			    #print
-			    #kprint traceback.print_exc(file=sys.stdout)
-			print
-			print "!!! catalyst lock file in use: "+message
-			print
+LockInUse = osutils.LockException
 
 
 class LockDir(object):
-	locking_method=fcntl.flock
-	lock_dirs_in_use=[]
-	die_on_failed_lock=True
-
-	def __del__(self):
-		#print "Lock.__del__() 1"
-		self.clean_my_hardlocks()
-		#print "Lock.__del__() 2"
-		self.delete_lock_from_path_list()
-		#print "Lock.__del__() 3"
-		if self.islocked():
-			#print "Lock.__del__() 4"
-			self.fcntl_unlock()
-		#print "Lock.__del__() finnished"
-
-	def __init__(self,lockdir):
-		self.locked=False
-		self.myfd=None
-		self.set_gid(250)
-		self.locking_method=LockDir.locking_method
-		self.set_lockdir(lockdir)
-		self.set_lockfilename(".catalyst_lock")
-		self.set_lockfile()
-
-		if LockDir.lock_dirs_in_use.count(lockdir)>0:
-			raise "This directory already associated with a lock object"
-		else:
-			LockDir.lock_dirs_in_use.append(lockdir)
-
-		self.myhardlock = None
-		self.hardlock_paths={}
-
-	def delete_lock_from_path_list(self):
-		try:
-			LockDir.lock_dirs_in_use.remove(self.lockdir)
-		except ValueError:
-			pass
-
-	def islocked(self):
-		if self.locked:
-			return True
-		else:
-			return False
+	"""An object that creates locks inside dirs"""
 
-	def set_gid(self,gid):
-		if not self.islocked():
-#			if self.settings["DEBUG"]:
-#				print "setting gid to", gid
-			self.gid=gid
-
-	def set_lockdir(self,lockdir):
-		if not os.path.exists(lockdir):
-			os.makedirs(lockdir)
-		if os.path.isdir(lockdir):
-			if not self.islocked():
-				if lockdir[-1] == "/":
-					lockdir=lockdir[:-1]
-				self.lockdir=normpath(lockdir)
-#				if self.settings["DEBUG"]:
-#					print "setting lockdir to", self.lockdir
-		else:
-			raise "the lock object needs a path to a dir"
-
-	def set_lockfilename(self,lockfilename):
-		if not self.islocked():
-			self.lockfilename=lockfilename
-#			if self.settings["DEBUG"]:
-#				print "setting lockfilename to", self.lockfilename
-
-	def set_lockfile(self):
-		if not self.islocked():
-			self.lockfile=normpath(self.lockdir+'/'+self.lockfilename)
-#			if self.settings["DEBUG"]:
-#				print "setting lockfile to", self.lockfile
+	def __init__(self, lockdir):
+		self.gid = 250
+		self.lockfile = os.path.join(lockdir, '.catalyst_lock')
+		osutils.ensure_dirs(lockdir)
+		fileutils.touch(self.lockfile, mode=0o664)
+		os.chown(self.lockfile, -1, self.gid)
+		self.lock = osutils.FsLock(self.lockfile)
 
 	def read_lock(self):
-		if not self.locking_method == "HARDLOCK":
-			self.fcntl_lock("read")
-		else:
-			print "HARDLOCKING doesnt support shared-read locks"
-			print "using exclusive write locks"
-			self.hard_lock()
+		self.lock.acquire_read_lock()
 
 	def write_lock(self):
-		if not self.locking_method == "HARDLOCK":
-			self.fcntl_lock("write")
-		else:
-			self.hard_lock()
+		self.lock.acquire_write_lock()
 
 	def unlock(self):
-		if not self.locking_method == "HARDLOCK":
-			self.fcntl_unlock()
-		else:
-			self.hard_unlock()
-
-	def fcntl_lock(self,locktype):
-		if self.myfd==None:
-			if not os.path.exists(os.path.dirname(self.lockdir)):
-				raise CatalystError("DirectoryNotFound: %s"
-					% os.path.dirname(self.lockdir), print_traceback=True)
-			if not os.path.exists(self.lockfile):
-				old_mask=os.umask(000)
-				self.myfd = os.open(self.lockfile, os.O_CREAT|os.O_RDWR,0660)
-				try:
-					if os.stat(self.lockfile).st_gid != self.gid:
-						os.chown(self.lockfile,os.getuid(),self.gid)
-				except OSError, e:
-					if e[0] == 2: #XXX: No such file or directory
-						return self.fcntl_locking(locktype)
-					else:
-						writemsg("Cannot chown a lockfile. This could cause inconvenience later.\n")
-
-				os.umask(old_mask)
-			else:
-				self.myfd = os.open(self.lockfile, os.O_CREAT|os.O_RDWR,0660)
-
-		try:
-			if locktype == "read":
-				self.locking_method(self.myfd,fcntl.LOCK_SH|fcntl.LOCK_NB)
-			else:
-				self.locking_method(self.myfd,fcntl.LOCK_EX|fcntl.LOCK_NB)
-		except IOError, e:
-			if "errno" not in dir(e):
-				raise
-			if e.errno == errno.EAGAIN:
-				if not LockDir.die_on_failed_lock:
-					# Resource temp unavailable; eg, someone beat us to the lock.
-					writemsg("waiting for lock on %s\n" % self.lockfile)
-
-					# Try for the exclusive or shared lock again.
-					if locktype == "read":
-						self.locking_method(self.myfd,fcntl.LOCK_SH)
-					else:
-						self.locking_method(self.myfd,fcntl.LOCK_EX)
-				else:
-					raise LockInUse,self.lockfile
-			elif e.errno == errno.ENOLCK:
-				pass
-			else:
-				raise
-		if not os.path.exists(self.lockfile):
-			os.close(self.myfd)
-			self.myfd=None
-			#writemsg("lockfile recurse\n")
-			self.fcntl_lock(locktype)
-		else:
-			self.locked=True
-			#writemsg("Lockfile obtained\n")
-
-	def fcntl_unlock(self):
-		unlinkfile = 1
-		if not os.path.exists(self.lockfile):
-			print "lockfile does not exist '%s'" % self.lockfile
-			#print "fcntl_unlock() , self.myfd:", self.myfd, type(self.myfd)
-			if self.myfd != None:
-				#print "fcntl_unlock() trying to close it "
-				try:
-					os.close(self.myfd)
-					self.myfd=None
-				except Exception:
-					pass
-				return False
-
-			try:
-				if self.myfd == None:
-					self.myfd = os.open(self.lockfile, os.O_WRONLY,0660)
-					unlinkfile = 1
-					self.locking_method(self.myfd,fcntl.LOCK_UN)
-			except Exception, e:
-				#if self.myfd is not None:
-					#print "fcntl_unlock() trying to close", self.myfd
-					#os.close(self.myfd)
-					#self.myfd=None
-				#raise IOError, "Failed to unlock file '%s'\n%s" % (self.lockfile, str(e))
-				try:
-					# This sleep call was added to allow other processes that are
-					# waiting for a lock to be able to grab it before it is deleted.
-					# lockfile() already accounts for this situation, however, and
-					# the sleep here adds more time than is saved overall, so am
-					# commenting until it is proved necessary.
-					#time.sleep(0.0001)
-					if unlinkfile:
-						InUse=False
-						try:
-							self.locking_method(self.myfd,fcntl.LOCK_EX|fcntl.LOCK_NB)
-						except Exception:
-							print "Read lock may be in effect. skipping lockfile delete..."
-							InUse=True
-							# We won the lock, so there isn't competition for it.
-							# We can safely delete the file.
-							#writemsg("Got the lockfile...\n")
-							#writemsg("Unlinking...\n")
-							self.locking_method(self.myfd,fcntl.LOCK_UN)
-					if not InUse:
-						os.unlink(self.lockfile)
-						os.close(self.myfd)
-						self.myfd=None
-#						if self.settings["DEBUG"]:
-#							print "Unlinked lockfile..."
-				except Exception, e:
-					# We really don't care... Someone else has the lock.
-					# So it is their problem now.
-					print "Failed to get lock... someone took it."
-					print str(e)
-
-					# Why test lockfilename?  Because we may have been handed an
-					# fd originally, and the caller might not like having their
-					# open fd closed automatically on them.
-					#if type(lockfilename) == types.StringType:
-					#        os.close(myfd)
-		#print "fcntl_unlock() trying a last ditch close", self.myfd
-		if self.myfd != None:
-			os.close(self.myfd)
-			self.myfd=None
-			self.locked=False
-			time.sleep(.0001)
-
-	def hard_lock(self,max_wait=14400):
-		"""Does the NFS, hardlink shuffle to ensure locking on the disk.
-		We create a PRIVATE lockfile, that is just a placeholder on the disk.
-		Then we HARDLINK the real lockfile to that private file.
-		If our file can 2 references, then we have the lock. :)
-		Otherwise we lather, rise, and repeat.
-		We default to a 4 hour timeout.
-		"""
-
-		self.myhardlock = self.hardlock_name(self.lockdir)
-
-		start_time = time.time()
-		reported_waiting = False
-
-		while time.time() < (start_time + max_wait):
-			# We only need it to exist.
-			self.myfd = os.open(self.myhardlock, os.O_CREAT|os.O_RDWR,0660)
-			os.close(self.myfd)
-
-			self.add_hardlock_file_to_cleanup()
-			if not os.path.exists(self.myhardlock):
-				raise CatalystError("FileNotFound: Created lockfile is missing: "
-					"%(filename)s" % {"filename":self.myhardlock},
-					print_traceback=True)
-			try:
-				os.link(self.myhardlock, self.lockfile)
-			except Exception:
-#				if self.settings["DEBUG"]:
-#					print "lockfile(): Hardlink: Link failed."
-#					print "Exception: ",e
-				pass
-
-			if self.hardlink_is_mine(self.myhardlock, self.lockfile):
-				# We have the lock.
-				if reported_waiting:
-					print
-				return True
-
-			if reported_waiting:
-				writemsg(".")
-			else:
-				reported_waiting = True
-				print
-				print "Waiting on (hardlink) lockfile: (one '.' per 3 seconds)"
-				print "Lockfile: " + self.lockfile
-			time.sleep(3)
-
-		os.unlink(self.myhardlock)
-		return False
-
-	def hard_unlock(self):
-		try:
-			if os.path.exists(self.myhardlock):
-				os.unlink(self.myhardlock)
-			if os.path.exists(self.lockfile):
-				os.unlink(self.lockfile)
-		except Exception:
-			writemsg("Something strange happened to our hardlink locks.\n")
-
-	def add_hardlock_file_to_cleanup(self):
-		#mypath = self.normpath(path)
-		if os.path.isdir(self.lockdir) and os.path.isfile(self.myhardlock):
-			self.hardlock_paths[self.lockdir]=self.myhardlock
-
-	def remove_hardlock_file_from_cleanup(self):
-		if self.lockdir in self.hardlock_paths:
-			del self.hardlock_paths[self.lockdir]
-			print self.hardlock_paths
-
-	@staticmethod
-	def hardlock_name(path):
-		mypath=path+"/.hardlock-"+os.uname()[1]+"-"+str(os.getpid())
-		newpath = os.path.normpath(mypath)
-		if len(newpath) > 1:
-			if newpath[1] == "/":
-				newpath = "/"+newpath.lstrip("/")
-		return newpath
-
-	@staticmethod
-	def hardlink_is_mine(link, lock):
-		import stat
-		try:
-			myhls = os.stat(link)
-			mylfs = os.stat(lock)
-		except Exception:
-			myhls = None
-			mylfs = None
-
-		if myhls:
-			if myhls[stat.ST_NLINK] == 2:
-				return True
-		if mylfs:
-			if mylfs[stat.ST_INO] == myhls[stat.ST_INO]:
-				return True
-		return False
-
-	@staticmethod
-	def hardlink_active(lock):
-		if not os.path.exists(lock):
-			return False
-
-	def clean_my_hardlocks(self):
-		try:
-			for x in self.hardlock_paths.keys():
-				self.hardlock_cleanup(x)
-		except AttributeError:
-			pass
-
-	def hardlock_cleanup(self,path):
-		#mypid  = str(os.getpid())
-		myhost = os.uname()[1]
-		mydl = os.listdir(path)
-		results = []
-		mycount = 0
-
-		mylist = {}
-		for x in mydl:
-			filepath=path+"/"+x
-			if os.path.isfile(filepath):
-				parts = filepath.split(".hardlock-")
-			if len(parts) == 2:
-				filename = parts[0]
-				hostpid  = parts[1].split("-")
-				host  = "-".join(hostpid[:-1])
-				pid   = hostpid[-1]
-			if filename not in mylist:
-				mylist[filename] = {}
-
-			if host not in mylist[filename]:
-				mylist[filename][host] = []
-				mylist[filename][host].append(pid)
-				mycount += 1
-			else:
-				mylist[filename][host].append(pid)
-				mycount += 1
-
-
-		results.append("Found %(count)s locks" % {"count":mycount})
-		for x in mylist.keys():
-			if myhost in mylist[x]:
-				mylockname = self.hardlock_name(x)
-				if self.hardlink_is_mine(mylockname, self.lockfile) or \
-					not os.path.exists(self.lockfile):
-					for y in mylist[x].keys():
-						for z in mylist[x][y]:
-							filename = x+".hardlock-"+y+"-"+z
-							if filename == mylockname:
-								self.hard_unlock()
-								continue
-							try:
-								# We're sweeping through, unlinking everyone's locks.
-								os.unlink(filename)
-								results.append("Unlinked: " + filename)
-							except Exception:
-								pass
-					try:
-						os.unlink(x)
-						results.append("Unlinked: " + x)
-						os.unlink(mylockname)
-						results.append("Unlinked: " + mylockname)
-					except Exception:
-						pass
-				else:
-					try:
-						os.unlink(mylockname)
-						results.append("Unlinked: " + mylockname)
-					except Exception:
-						pass
-		return results
-
-
-if __name__ == "__main__":
-
-	def lock_work():
-		print
-		for i in range(1,6):
-			print i,time.time()
-			time.sleep(1)
-		print
-
-	print "Lock 5 starting"
-	Lock1=LockDir("/tmp/lock_path")
-	Lock1.write_lock()
-	print "Lock1 write lock"
-
-	lock_work()
-
-	Lock1.unlock()
-	print "Lock1 unlock"
-
-	Lock1.read_lock()
-	print "Lock1 read lock"
-
-	lock_work()
-
-	Lock1.unlock()
-	print "Lock1 unlock"
-
-	Lock1.read_lock()
-	print "Lock1 read lock"
-
-	Lock1.write_lock()
-	print "Lock1 write lock"
-
-	lock_work()
-
-	Lock1.unlock()
-	print "Lock1 unlock"
-
-	Lock1.read_lock()
-	print "Lock1 read lock"
-
-	lock_work()
-
-	Lock1.unlock()
-	print "Lock1 unlock"
-
-#Lock1.write_lock()
-#time.sleep(2)
-#Lock1.unlock()
-    ##Lock1.write_lock()
-    #time.sleep(2)
-    #Lock1.unlock()
+		# Releasing a write lock is the same as a read lock.
+		self.lock.release_write_lock()
-- 
2.5.2

Re: [gentoo-catalyst] [PATCH 1/2] stagebase: simplify lock calls a bit

[Anthony G. Basile]

On 10/10/15 9:54 PM, Mike Frysinger wrote:
> We create self.snapcache_lock to hold the lock, then assign it to
> self.snapshot_lock_object, and then operate on self.snapshot_lock_object.
> There's no need for this indirection, so operate on self.snapcache_lock
> directly instead.
> ---
>   catalyst/base/stagebase.py | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)
>

This and the next patch look fine and simplify matters a lot.  This 
doesn't fix bug #519656 but it clears the way.  When running multiple 
instances of catalyst, if the second instance tries to acquire the lock 
on the snapcache while the first one has it, it throws and exception 
instead of waiting on the lock.  It would be nice for it to wait and 
timeout if it doesn't get the lock within some acceptable time limit.

On systems with lots-o-cores I run mutliple instances of catalyst to 
economize on throughput.

-- 
Anthony G. Basile, Ph.D.
Gentoo Linux Developer [Hardened]
E-Mail    : blueness@gentoo.org
GnuPG FP  : 1FED FAD9 D82C 52A5 3BAB  DC79 9384 FA6E F52D 4BBA
GnuPG ID  : F52D4BBA