diff options
| -rw-r--r-- | src/libstore/build.cc | 17 | ||||
| -rw-r--r-- | src/libstore/gc.cc | 4 | ||||
| -rw-r--r-- | src/libstore/local-store.hh | 2 | ||||
| -rw-r--r-- | src/libstore/pathlocks.cc | 127 | ||||
| -rw-r--r-- | src/libstore/pathlocks.hh | 4 |
5 files changed, 46 insertions, 108 deletions
diff --git a/src/libstore/build.cc b/src/libstore/build.cc index cf6428e12467..a28619ab907d 100644 --- 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 index eeb2378393ae..2c791efbe67e 100644 --- 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; } diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 6b655647b031..af8b84bf5d73 100644 --- 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 index 08d1efdbeb01..b6d8547c70e6 100644 --- 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,17 +40,14 @@ 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")); @@ -58,9 +55,9 @@ bool lockFile(int fd, LockType lockType, bool wait) 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,7 +80,7 @@ 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()); @@ -99,75 +88,54 @@ bool PathLocks::lockPaths(const PathSet & _paths, /* 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); -} - - } diff --git a/src/libstore/pathlocks.hh b/src/libstore/pathlocks.hh index db51f950a320..411da022295d 100644 --- a/src/libstore/pathlocks.hh +++ b/src/libstore/pathlocks.hh @@ -16,8 +16,6 @@ enum LockType { ltRead, ltWrite, ltNone }; bool lockFile(int fd, LockType lockType, bool wait); -MakeError(AlreadyLocked, Error); - class PathLocks { private: @@ -37,6 +35,4 @@ public: void setDeletion(bool deletePaths); }; -bool pathIsLockedByMe(const Path & path); - } |