On 10/21/19 7:12 PM, Alexandre Oliva wrote:
On Oct 21, 2019, Jason Merrill <ja...@redhat.com> wrote:
On Sat, Oct 19, 2019 at 12:08 AM Alexandre Oliva <ol...@gnu.org> wrote:
We might also wish to use different lock-breaking logic for that case,
too, e.g. checking that the timestamp of the dir didn't change by
comparing `ls -ld $lockdir` with what we got 30 seconds before. If it
changed or the output is now empty, we just lost the race again.
It's unlikely that the dir would remain unchanged for several seconds
without the pid file, so if we get the same timestamp after 30 seconds,
it's likely that something went wrong with the lock holder,
This patch uses stamps in the lock directory so that they are automatically
cleared when the lock changes hands.
Nice!
though it's
not impossible to imagine a scenario in which the lock program that just
succeeded in creating the dir got stopped (^Z) or killed-9 just before
creating the PID file.
One kind of problem is a left-over lock after a kill -9. That
doesn't provide a chance for the lock cleanup code to run in the
terminated process. But the current code will eventually steal it
(maybe too soon) and proceed.
Another kind of problem could arise if a lock-needing program got
stopped at an unfortunate time that enabled the lock to be taken from it
(before creating the pid file), and then woke back up believing it still
has the lock. If it were to carry out anything that really required
mutual exclusion, Bad Things (TM) could happen.
Some systems adopt a STOPITH (that's short for Shoot The Other Process
In The Head) to avoid that scenario, but we don't have that choice:
either we're stealing from a PID that's gone (safe-ish, assuming all
users on a single host, without a shared FS across separate PID
namespaces, but still potentially racy, see below), or we don't know who
we're stealing from (very likely racy, except after a full-system
crash).
Even then, maybe breaking the lock is not such a great idea in
general...
It seems to me that the downside of stealing the lock (greater resource
contention) is less than the downside of erroring out (build fails, user
has to intervene manually, possibly many hours later when they notice).
Oh, if that's indeed the only consequence, I tend to agree. I was
worried about scenarios that actually required mutual exclusion. For
that, stealing locks opens another can of worms since multiple processes
might decide to steal the lock at the same time, and then each one might
end up stealing the lock from others who'd just stole the lock, thinking
they're stealing from the dead or stopped process, with very
impredictable results.
Sure, that could (very rarely) happen if one process managed to break
the lock and recreate it in between these two lines in another process:
if test -f $lockdir/lock-$1.$$; then
rm -rf $lockdir
We should probably have comments as to the intended use, to express a
preference for serialization, so that nobody ends up using it in cases
that actually depend on mutual exclusion (e.g. to avoid corrupting some
file by concurrently changing it) without disabling the racy lock
stealing machinery:
# Shell-based mutex using mkdir. This script is used to prefer
# serialized execution to avoid consuming too much RAM. If reusing it,
# bear in mind that the lock-breaking logic is not race-free, so disable
# it in err() if concurrent execution could cause more serious problems.
Ok with that change or somesuch.
I mostly adopted that comment, thanks.
(Shell functions used to be non-portable, but I think even super
portable autoconf-generated configure scripts use them now)
Thanks!
Here's what I've committed:
commit f71e3aa2a5d614934ecda16987e12202a6b8007d
Author: jason <jason@138bc75d-0d04-0410-961f-82ee72b054a4>
Date: Tue Oct 22 03:09:41 2019 +0000
* lock-and-run.sh: Check for process existence rather than timeout.
Matthias Klose noted that on less powerful targets, a link might take more
than 5 minutes; he mentions a figure of 3 hours for an LTO link. So this
patch changes the timeout to a check for whether the locking process still
exists. If the lock exists in an erroneous state (no pid file or can't
signal the pid) for 30 sec, steal it.
git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@277277 138bc75d-0d04-0410-961f-82ee72b054a4
diff --git a/gcc/lock-and-run.sh b/gcc/lock-and-run.sh
index 3a6a84c253a..22bc436565c 100644
--- a/gcc/lock-and-run.sh
+++ b/gcc/lock-and-run.sh
@@ -1,33 +1,49 @@
#! /bin/sh
-# Shell-based mutex using mkdir.
+# Shell-based mutex using mkdir. This script is used in make to prefer
+# serialized execution to avoid consuming too much RAM. If reusing it,
+# bear in mind that the lock-breaking logic is not race-free, so disable
+# it in err() if concurrent execution could cause more serious problems.
+self=`basename $0`
lockdir="$1" prog="$2"; shift 2 || exit 1
# Remember when we started trying to acquire the lock.
count=0
-touch lock-stamp.$$
-trap 'rm -r "$lockdir" lock-stamp.$$' 0
+err () {
+ if test -f $lockdir/lock-$1.$$; then
+ echo "$self: *** (PID $$) removing stale $lockdir" >&2
+ rm -rf $lockdir
+ # Possible variant for uses where races are more problematic:
+ #echo "$self: *** (PID $$) giving up, maybe rm -r $lockdir" >&2
+ #exit 42
+ else
+ touch $lockdir/lock-$1.$$
+ fi
+}
until mkdir "$lockdir" 2>/dev/null; do
# Say something periodically so the user knows what's up.
if [ `expr $count % 30` = 0 ]; then
- # Reset if the lock has been renewed.
- if [ -n "`find \"$lockdir\" -newer lock-stamp.$$`" ]; then
- touch lock-stamp.$$
- count=1
- # Steal the lock after 5 minutes.
- elif [ $count = 300 ]; then
- echo removing stale $lockdir >&2
- rm -r "$lockdir"
+ # Check for valid lock.
+ if pid=`cat $lockdir/pid 2>/dev/null` && kill -0 $pid 2>/dev/null; then
+ echo "$self: (PID $$) waiting $count sec to acquire $lockdir from PID $pid" >&2
+ elif test -z "$pid"; then
+ echo "$self: (PID $$) cannot read $lockdir/pid" >&2
+ err nopid
else
- echo waiting to acquire $lockdir >&2
+ echo "$self: (PID $$) cannot signal $lockdir owner PID $pid" >&2
+ err dead
fi
fi
sleep 1
count=`expr $count + 1`
done
+trap 'rm -rf "$lockdir"' 0
+echo $$ > $lockdir/pidT && mv $lockdir/pidT $lockdir/pid
+echo "$self: (PID $$) acquired $lockdir after $count seconds" >&2
+
echo $prog "$@"
$prog "$@"
