NixOS · Aug 2, 2019 · Aug 2, 2019 · Aug 2, 2019 · Aug 2, 2019 · Aug 2, 2019
diff --git a/doc/manual/command-ref/conf-file.xml b/doc/manual/command-ref/conf-file.xml
@@ -483,8 +483,10 @@ builtins.fetchurl {
 
   <varlistentry xml:id="conf-max-free"><term><literal>max-free</literal></term>
 
-    <listitem><para>This option defines after how many free bytes to stop collecting
-    garbage once the <literal>min-free</literal> condition gets triggered.</para></listitem>
+    <listitem><para>When a garbage collection is triggered by the
+    <literal>min-free</literal> option, it stops as soon as
+    <literal>max-free</literal> bytes are available. The default is
+    infinity (i.e. delete all garbage).</para></listitem>
 
   </varlistentry>
 
@@ -528,9 +530,11 @@ builtins.fetchurl {
   <varlistentry xml:id="conf-min-free"><term><literal>min-free</literal></term>
 
     <listitem>
-      <para>When the disk reaches <literal>min-free</literal> bytes of free disk space during a build, nix
-        will start to garbage-collection until <literal>max-free</literal> bytes are available on the disk.
-        A value of <literal>0</literal> (the default) means that this feature is disabled.</para>
+      <para>When free disk space in <filename>/nix/store</filename>
+      drops below <literal>min-free</literal> during a build, Nix
+      performs a garbage-collection until <literal>max-free</literal>
+      bytes are available or there is no more garbage.  A value of
+      <literal>0</literal> (the default) disables this feature.</para>
     </listitem>
 
   </varlistentry>

diff --git a/src/libstore/build.cc b/src/libstore/build.cc
@@ -3984,17 +3984,6 @@ void SubstitutionGoal::tryToRun()
         return;
     }
 
-    /* If the store path is already locked (probably by a
-       DerivationGoal), then put this goal to sleep. Note: we don't
-       acquire a lock here since that breaks addToStore(), so below we
-       handle an AlreadyLocked exception from addToStore(). The check
-       here is just an optimisation to prevent having to redo a
-       download due to a locked path. */
-    if (pathIsLockedByMe(worker.store.toRealPath(storePath))) {
-        worker.waitForAWhile(shared_from_this());
-        return;
-    }
-
     maintainRunningSubstitutions = std::make_unique<MaintainCount<uint64_t>>(worker.runningSubstitutions);
     worker.updateProgress();
 
@@ -4034,12 +4023,6 @@ void SubstitutionGoal::finished()
 
     try {
         promise.get_future().get();
-    } catch (AlreadyLocked & e) {
-        /* Probably a DerivationGoal is already building this store
-           path. Sleep for a while and try again. */
-        state = &SubstitutionGoal::init;
-        worker.waitForAWhile(shared_from_this());
-        return;
     } catch (std::exception & e) {
         printError(e.what());
 

diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc
@@ -29,7 +29,7 @@ static string gcRootsDir = "gcroots";
    read.  To be precise: when they try to create a new temporary root
    file, they will block until the garbage collector has finished /
    yielded the GC lock. */
-int LocalStore::openGCLock(LockType lockType)
+AutoCloseFD LocalStore::openGCLock(LockType lockType)
 {
     Path fnGCLock = (format("%1%/%2%")
         % stateDir % gcLockName).str();
@@ -49,7 +49,7 @@ int LocalStore::openGCLock(LockType lockType)
        process that can open the file for reading can DoS the
        collector. */
 
-    return fdGCLock.release();
+    return fdGCLock;
 }
 
 
@@ -221,26 +221,22 @@ void LocalStore::findTempRoots(FDs & fds, Roots & tempRoots, bool censor)
         //FDPtr fd(new AutoCloseFD(openLockFile(path, false)));
         //if (*fd == -1) continue;
 
-        if (path != fnTempRoots) {
-
-            /* Try to acquire a write lock without blocking.  This can
-               only succeed if the owning process has died.  In that case
-               we don't care about its temporary roots. */
-            if (lockFile(fd->get(), ltWrite, false)) {
-                printError(format("removing stale temporary roots file '%1%'") % path);
-                unlink(path.c_str());
-                writeFull(fd->get(), "d");
-                continue;
-            }
-
-            /* Acquire a read lock.  This will prevent the owning process
-               from upgrading to a write lock, therefore it will block in
-               addTempRoot(). */
-            debug(format("waiting for read lock on '%1%'") % path);
-            lockFile(fd->get(), ltRead, true);
-
+        /* Try to acquire a write lock without blocking.  This can
+           only succeed if the owning process has died.  In that case
+           we don't care about its temporary roots. */
+        if (lockFile(fd->get(), ltWrite, false)) {
+            printError(format("removing stale temporary roots file '%1%'") % path);
+            unlink(path.c_str());
+            writeFull(fd->get(), "d");
+            continue;
         }
 
+        /* Acquire a read lock.  This will prevent the owning process
+           from upgrading to a write lock, therefore it will block in
+           addTempRoot(). */
+        debug(format("waiting for read lock on '%1%'") % path);
+        lockFile(fd->get(), ltRead, true);
+
         /* Read the entire file. */
         string contents = readFile(fd->get());
 
@@ -871,7 +867,12 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results)
 
 void LocalStore::autoGC(bool sync)
 {
-    auto getAvail = [this]() {
+    static auto fakeFreeSpaceFile = getEnv("_NIX_TEST_FREE_SPACE_FILE", "");
+
+    auto getAvail = [this]() -> uint64_t {
+        if (!fakeFreeSpaceFile.empty())
+            return std::stoll(readFile(fakeFreeSpaceFile));
+
         struct statvfs st;
         if (statvfs(realStoreDir.c_str(), &st))
             throw SysError("getting filesystem info about '%s'", realStoreDir);
@@ -892,7 +893,7 @@ void LocalStore::autoGC(bool sync)
 
         auto now = std::chrono::steady_clock::now();
 
-        if (now < state->lastGCCheck + std::chrono::seconds(5)) return;
+        if (now < state->lastGCCheck + std::chrono::seconds(settings.minFreeCheckInterval)) return;
 
         auto avail = getAvail();
 

diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh
@@ -342,6 +342,9 @@ public:
     Setting<uint64_t> maxFree{this, std::numeric_limits<uint64_t>::max(), "max-free",
         "Stop deleting garbage when free disk space is above the specified amount."};
 
+    Setting<uint64_t> minFreeCheckInterval{this, 5, "min-free-check-interval",
+        "Number of seconds between checking free disk space."};
+
     Setting<Paths> pluginFiles{this, {}, "plugin-files",
         "Plugins to dynamically load at nix initialization time."};
 };

diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc
@@ -1210,7 +1210,8 @@ bool LocalStore::verifyStore(bool checkContents, RepairFlag repair)
 
     bool errors = false;
 
-    /* Acquire the global GC lock to prevent a garbage collection. */
+    /* Acquire the global GC lock to get a consistent snapshot of
+       existing and valid paths. */
     AutoCloseFD fdGCLock = openGCLock(ltWrite);
 
     PathSet store;
@@ -1221,13 +1222,11 @@ bool LocalStore::verifyStore(bool checkContents, RepairFlag repair)
 
     PathSet validPaths2 = queryAllValidPaths(), validPaths, done;
 
+    fdGCLock = -1;
+
     for (auto & i : validPaths2)
         verifyPath(i, store, done, validPaths, repair, errors);
 
-    /* Release the GC lock so that checking content hashes (which can
-       take ages) doesn't block the GC or builds. */
-    fdGCLock = -1;
-
     /* Optionally, check the content hashes (slow). */
     if (checkContents) {
         printInfo("checking hashes...");

diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh
@@ -263,7 +263,7 @@ private:
     bool isActiveTempFile(const GCState & state,
         const Path & path, const string & suffix);
 
-    int openGCLock(LockType lockType);
+    AutoCloseFD openGCLock(LockType lockType);
 
     void findRoots(const Path & path, unsigned char type, Roots & roots);
 

diff --git a/src/libstore/pathlocks.cc b/src/libstore/pathlocks.cc
@@ -7,7 +7,7 @@
 
 #include <sys/types.h>
 #include <sys/stat.h>
-#include <fcntl.h>
+#include <sys/file.h>
 
 
 namespace nix {
@@ -40,27 +40,24 @@ void deleteLockFile(const Path & path, int fd)
 
 bool lockFile(int fd, LockType lockType, bool wait)
 {
-    struct flock lock;
-    if (lockType == ltRead) lock.l_type = F_RDLCK;
-    else if (lockType == ltWrite) lock.l_type = F_WRLCK;
-    else if (lockType == ltNone) lock.l_type = F_UNLCK;
+    int type;
+    if (lockType == ltRead) type = LOCK_SH;
+    else if (lockType == ltWrite) type = LOCK_EX;
+    else if (lockType == ltNone) type = LOCK_UN;
     else abort();
-    lock.l_whence = SEEK_SET;
-    lock.l_start = 0;
-    lock.l_len = 0; /* entire file */
 
     if (wait) {
-        while (fcntl(fd, F_SETLKW, &lock) != 0) {
+        while (flock(fd, type) != 0) {
             checkInterrupt();
             if (errno != EINTR)
                 throw SysError(format("acquiring/releasing lock"));
             else
                 return false;
         }
     } else {
-        while (fcntl(fd, F_SETLK, &lock) != 0) {
+        while (flock(fd, type | LOCK_NB) != 0) {
             checkInterrupt();
-            if (errno == EACCES || errno == EAGAIN) return false;
+            if (errno == EWOULDBLOCK) return false;
             if (errno != EINTR)
                 throw SysError(format("acquiring/releasing lock"));
         }
@@ -70,14 +67,6 @@ bool lockFile(int fd, LockType lockType, bool wait)
 }
 
 
-/* This enables us to check whether are not already holding a lock on
-   a file ourselves.  POSIX locks (fcntl) suck in this respect: if we
-   close a descriptor, the previous lock will be closed as well.  And
-   there is no way to query whether we already have a lock (F_GETLK
-   only works on locks held by other processes). */
-static Sync<StringSet> lockedPaths_;
-
-
 PathLocks::PathLocks()
     : deletePaths(false)
 {
@@ -91,83 +80,62 @@ PathLocks::PathLocks(const PathSet & paths, const string & waitMsg)
 }
 
 
-bool PathLocks::lockPaths(const PathSet & _paths,
+bool PathLocks::lockPaths(const PathSet & paths,
     const string & waitMsg, bool wait)
 {
     assert(fds.empty());
 
     /* Note that `fds' is built incrementally so that the destructor
        will only release those locks that we have already acquired. */
 
-    /* Sort the paths.  This assures that locks are always acquired in
-       the same order, thus preventing deadlocks. */
-    Paths paths(_paths.begin(), _paths.end());
-    paths.sort();
-
-    /* Acquire the lock for each path. */
+    /* Acquire the lock for each path in sorted order. This ensures
+       that locks are always acquired in the same order, thus
+       preventing deadlocks. */
     for (auto & path : paths) {
         checkInterrupt();
         Path lockPath = path + ".lock";
 
         debug(format("locking path '%1%'") % path);
 
-        {
-            auto lockedPaths(lockedPaths_.lock());
-            if (lockedPaths->count(lockPath)) {
-                if (!wait) return false;
-                throw AlreadyLocked("deadlock: trying to re-acquire self-held lock '%s'", lockPath);
-            }
-            lockedPaths->insert(lockPath);
-        }
-
-        try {
+        AutoCloseFD fd;
 
-            AutoCloseFD fd;
+        while (1) {
 
-            while (1) {
+            /* Open/create the lock file. */
+            fd = openLockFile(lockPath, true);
 
-                /* Open/create the lock file. */
-                fd = openLockFile(lockPath, true);
-
-                /* Acquire an exclusive lock. */
-                if (!lockFile(fd.get(), ltWrite, false)) {
-                    if (wait) {
-                        if (waitMsg != "") printError(waitMsg);
-                        lockFile(fd.get(), ltWrite, true);
-                    } else {
-                        /* Failed to lock this path; release all other
-                           locks. */
-                        unlock();
-                        lockedPaths_.lock()->erase(lockPath);
-                        return false;
-                    }
+            /* Acquire an exclusive lock. */
+            if (!lockFile(fd.get(), ltWrite, false)) {
+                if (wait) {
+                    if (waitMsg != "") printError(waitMsg);
+                    lockFile(fd.get(), ltWrite, true);
+                } else {
+                    /* Failed to lock this path; release all other
+                       locks. */
+                    unlock();
+                    return false;
                 }
-
-                debug(format("lock acquired on '%1%'") % lockPath);
-
-                /* Check that the lock file hasn't become stale (i.e.,
-                   hasn't been unlinked). */
-                struct stat st;
-                if (fstat(fd.get(), &st) == -1)
-                    throw SysError(format("statting lock file '%1%'") % lockPath);
-                if (st.st_size != 0)
-                    /* This lock file has been unlinked, so we're holding
-                       a lock on a deleted file.  This means that other
-                       processes may create and acquire a lock on
-                       `lockPath', and proceed.  So we must retry. */
-                    debug(format("open lock file '%1%' has become stale") % lockPath);
-                else
-                    break;
             }
 
-            /* Use borrow so that the descriptor isn't closed. */
-            fds.push_back(FDPair(fd.release(), lockPath));
-
-        } catch (...) {
-            lockedPaths_.lock()->erase(lockPath);
-            throw;
+            debug(format("lock acquired on '%1%'") % lockPath);
+
+            /* Check that the lock file hasn't become stale (i.e.,
+               hasn't been unlinked). */
+            struct stat st;
+            if (fstat(fd.get(), &st) == -1)
+                throw SysError(format("statting lock file '%1%'") % lockPath);
+            if (st.st_size != 0)
+                /* This lock file has been unlinked, so we're holding
+                   a lock on a deleted file.  This means that other
+                   processes may create and acquire a lock on
+                   `lockPath', and proceed.  So we must retry. */
+                debug(format("open lock file '%1%' has become stale") % lockPath);
+            else
+                break;
         }
 
+        /* Use borrow so that the descriptor isn't closed. */
+        fds.push_back(FDPair(fd.release(), lockPath));
     }
 
     return true;
@@ -189,8 +157,6 @@ void PathLocks::unlock()
     for (auto & i : fds) {
         if (deletePaths) deleteLockFile(i.second, i.first);
 
-        lockedPaths_.lock()->erase(i.second);
-
         if (close(i.first) == -1)
             printError(
                 format("error (ignored): cannot close lock file on '%1%'") % i.second);
@@ -208,11 +174,4 @@ void PathLocks::setDeletion(bool deletePaths)
 }
 
 
-bool pathIsLockedByMe(const Path & path)
-{
-    Path lockPath = path + ".lock";
-    return lockedPaths_.lock()->count(lockPath);
-}
-
-
 }