Revision 1.12 of lockf.c fixed a "thundering herd" scenario when the

lock experienced contention a number of processes would race to acquire
lock when it was released.  This problem resulted in a lot of CPU
load as well as locks being picked up out of order.

Unfortunately, a regression snuck in which allowed multiple threads
to pickup the same lock when -k was not used.  This could occur when
multiple processes open a file descriptor to inode X (one process
will be blocked) and the file is unlinked on unlock (thereby removing
the directory entry allow another process to create a new directory
entry for the same file name and lock it).

This changes restores the old algorithm of: wait for the lock, then
acquire lock when we want to unlink the file on exit (specifically
when -k is not used) and keeps the new algorithm for when -k is used,
which yields fairness and improved performance.

Also, update the man page to inform users that if lockf(1) is being
used to facilitate concurrency between a number of processes, it
is recommended that -k be used to reduce CPU load and yeld
fairness with regard to lock ordering.

Collaborated with:	jdp
PR:		bin/114341
PR:		bin/116543
PR:		bin/111101
MFC after:	1 week

2 files changed, 72 insertions, 11 deletions

diff --git a/usr.bin/lockf/lockf.1 b/usr.bin/lockf/lockf.1
index c974e4f..86b9fbd 100644
--- a/usr.bin/lockf/lockf.1
+++ b/usr.bin/lockf/lockf.1
@@ -1,4 +1,4 @@
-.\"
+\"
 .\" Copyright (C) 1998 John D. Polstra.  All rights reserved.
 .\"
 .\" Redistribution and use in source and binary forms, with or without
@@ -66,6 +66,18 @@ the mere existence of the
 .Ar file
 is not considered to constitute a lock.
 .Pp
+If the
+.Nm
+utility is being used to facilitate concurrency between a number
+of processes, it is recommended that the
+.Fl k
+option be used. This will guarantee lock ordering, as well as implement
+a performance enhanced algorithm which minimizes CPU load associated
+with concurrent unlink, drop and re-acquire activity. It should be noted
+that if the
+.Fl k
+option is not used, then no guarantees around lock ordering can be made.
+.Pp
 The following options are supported:
 .Bl -tag -width ".Fl t Ar seconds"
 .It Fl k
diff --git a/usr.bin/lockf/lockf.c b/usr.bin/lockf/lockf.c
index 58a08a3..843f061 100644
--- a/usr.bin/lockf/lockf.c
+++ b/usr.bin/lockf/lockf.c
@@ -38,11 +38,12 @@ __FBSDID("$FreeBSD$");
 #include <sysexits.h>
 #include <unistd.h>
 
+static int acquire_lock(const char *name, int flags);
 static void cleanup(void);
 static void killed(int sig);
 static void timeout(int sig);
 static void usage(void);
-static int wait_for_lock(const char *name, int flags);
+static void wait_for_lock(const char *name);
 
 static const char *lockname;
 static int lockfd = -1;
@@ -95,9 +96,37 @@ main(int argc, char **argv)
 		sigaction(SIGALRM, &act, NULL);
 		alarm(waitsec);
 	}
-	lockfd = wait_for_lock(lockname, O_NONBLOCK);
-	while (lockfd == -1 && !timed_out && waitsec != 0)
-		lockfd = wait_for_lock(lockname, 0);
+	/*
+	 * If the "-k" option is not given, then we must not block when
+	 * acquiring the lock.  If we did, then the lock holder would
+	 * unlink the file upon releasing the lock, and we would acquire
+	 * a lock on a file with no directory entry.  Then another
+	 * process could come along and acquire the same lock.  To avoid
+	 * this problem, we separate out the actions of waiting for the
+	 * lock to be available and of actually acquiring the lock.
+	 *
+	 * That approach produces behavior that is technically correct;
+	 * however, it causes some performance & ordering problems for
+	 * locks that have a lot of contention.  First, it is unfair in
+	 * the sense that a released lock isn't necessarily granted to
+	 * the process that has been waiting the longest.  A waiter may
+	 * be starved out indefinitely.  Second, it creates a thundering
+	 * herd situation each time the lock is released.
+	 *
+	 * When the "-k" option is used, the unlink race no longer
+	 * exists.  In that case we can block while acquiring the lock,
+	 * avoiding the separate step of waiting for the lock.  This
+	 * yields fairness and improved performance.
+	 */
+	lockfd = acquire_lock(lockname, O_NONBLOCK);
+	while (lockfd == -1 && !timed_out && waitsec != 0) {
+		if (keep)
+			lockfd = acquire_lock(lockname, 0);
+		else {
+			wait_for_lock(lockname);
+			lockfd = acquire_lock(lockname, O_NONBLOCK);
+		}
+	}
 	if (waitsec > 0)
 		alarm(0);
 	if (lockfd == -1) {		/* We failed to acquire the lock. */
@@ -126,6 +155,25 @@ main(int argc, char **argv)
 }
 
 /*
+ * Try to acquire a lock on the given file, creating the file if
+ * necessary.  The flags argument is O_NONBLOCK or 0, depending on
+ * whether we should wait for the lock.  Returns an open file descriptor
+ * on success, or -1 on failure.
+ */
+static int
+acquire_lock(const char *name, int flags)
+{
+	int fd;
+
+	if ((fd = open(name, O_RDONLY|O_CREAT|O_EXLOCK|flags, 0666)) == -1) {
+		if (errno == EAGAIN || errno == EINTR)
+			return (-1);
+		err(EX_CANTCREAT, "cannot open %s", name);
+	}
+	return (fd);
+}
+
+/*
  * Remove the lock file.
  */
 static void
@@ -173,16 +221,17 @@ usage(void)
 
 /*
  * Wait until it might be possible to acquire a lock on the given file.
+ * If the file does not exist, return immediately without creating it.
  */
-static int
-wait_for_lock(const char *name, int flags)
+static void
+wait_for_lock(const char *name)
 {
 	int fd;
 
-	if ((fd = open(name, O_CREAT|O_RDONLY|O_EXLOCK|flags, 0666)) == -1) {
-		if (errno == EINTR || errno == EAGAIN)
-			return (-1);
+	if ((fd = open(name, O_RDONLY|O_EXLOCK, 0666)) == -1) {
+		if (errno == ENOENT || errno == EINTR)
+			return;
 		err(EX_CANTCREAT, "cannot open %s", name);
 	}
-	return (fd);
+	close(fd);
 }