Do not erase aborted StoreMap entries that are still being read (#1344)

The old `!s.lock.readers` precondition for calling freeChain() in
StoreMap::abortWriting() was not broad enough: It covered "old" readers
that have incremented ReadWriteLock::readers already while missing those
lockShared() callers that have fully qualified for becoming a reader but
have not incremented the `readers` counter yet. A correct test is now
implemented by ReadWriteLock::stopAppendingAndRestoreExclusive().

This code was broken since appending mode was introduced in 2013 commit
ce49546. Evidently, the mishandled race condition is short-lived enough
and abortWriting() calls are rare enough, that there are no (known)
relevant bug reports. This bug was accidentally discovered while reading
the code to explain another SMP bug.

Controller::markedForDeletionAndAbandoned() similarly misjudges
readers(), but that bug does not lead to serious problems, and we would
like to find a fix that identifies such entries without false positives
(that looking at ReadWriteLock::readLevel instead of readers creates).

Also polished Ipc::StoreMap::abortWriting() debugging a little.

diff --git a/src/ipc/ReadWriteLock.cc b/src/ipc/ReadWriteLock.cc
index 16afc8aa5fbab52893532b2670dd023256d1f58a..2e0f57fc23f115d78bd24bff4a8403833fb1664a 100644 (file)
--- a/src/ipc/ReadWriteLock.cc
+++ b/src/ipc/ReadWriteLock.cc
@@ -127,6 +127,23 @@ Ipc::ReadWriteLock::startAppending()
     appending = true;
 }
 
+bool
+Ipc::ReadWriteLock::stopAppendingAndRestoreExclusive()
+{
+    assert(writing);
+    assert(appending);
+
+    appending = false;
+
+    // Checking `readers` here would mishandle a lockShared() call that started
+    // before we banned appending above, saw still true `appending`, got on a
+    // "success" code path, but had not incremented the `readers` counter yet.
+    // Checking `readLevel` mishandles lockShared() that saw false `appending`,
+    // got on a "failure" code path, but had not decremented `readLevel` yet.
+    // Our callers prefer the wrong "false" to the wrong "true" result.
+    return !readLevel;
+}
+
 void
 Ipc::ReadWriteLock::updateStats(ReadWriteLockStats &stats) const
 {

diff --git a/src/ipc/ReadWriteLock.h b/src/ipc/ReadWriteLock.h
index 36254f3d2a3044a9cf04ad86a9a9a142b298ab24..187d72163a511138810d79a9503a67378e518734 100644 (file)
--- a/src/ipc/ReadWriteLock.h
+++ b/src/ipc/ReadWriteLock.h
@@ -42,6 +42,11 @@ public:
 
     void startAppending(); ///< writer keeps its lock but also allows reading
 
+    /// writer keeps its lock and disallows future readers
+    /// \returns whether access became exclusive (i.e. no readers)
+    /// \prec appending is true
+    bool stopAppendingAndRestoreExclusive();
+
     /// adds approximate current stats to the supplied ones
     void updateStats(ReadWriteLockStats &stats) const;
 

diff --git a/src/ipc/StoreMap.cc b/src/ipc/StoreMap.cc
index 50bcbb9960cf5402c89a1159d73588e2d3e1f86b..cd18ce2c00edd08f8b721bc6cd6423d78d46ea1b 100644 (file)
--- a/src/ipc/StoreMap.cc
+++ b/src/ipc/StoreMap.cc
@@ -253,15 +253,14 @@ Ipc::StoreMap::abortWriting(const sfileno fileno)
     debugs(54, 5, "aborting entry " << fileno << " for writing " << path);
     Anchor &s = anchorAt(fileno);
     assert(s.writing());
-    s.lock.appending = false; // locks out any new readers
-    if (!s.lock.readers) {
+    if (!s.lock.appending || s.lock.stopAppendingAndRestoreExclusive()) {
         freeChain(fileno, s, false);
-        debugs(54, 5, "closed clean entry " << fileno << " for writing " << path);
+        debugs(54, 5, "closed idle entry " << fileno << " for writing " << path);
     } else {
         s.waitingToBeFreed = true;
         s.writerHalted = true;
         s.lock.unlockExclusive();
-        debugs(54, 5, "closed dirty entry " << fileno << " for writing " << path);
+        debugs(54, 5, "closed busy entry " << fileno << " for writing " << path);
     }
 }