From 72326c404487bbf8dc6ee069930c6c2a0319857e Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Wed, 5 Feb 2025 02:12:57 +0100 Subject: [PATCH] libstore: always delete PathLocks lockfiles they are not being created with O_EXCL and their existence is not checked for using the PathLocks api, so we will boldy assume that *nothing* checks for lock file existence. deleting lockfiles when we're done with them in all cases makes the code cleaner, and any failing builds no longer litter their store with stale lockfiles. Change-Id: Iffb588d29b00e6aca32fcb6776980455238ac2bd --- lix/libstore/build/derivation-goal.cc | 2 -- lix/libstore/build/local-derivation-goal.cc | 1 - lix/libstore/local-store.cc | 6 ---- lix/libstore/pathlocks.cc | 37 ++++++--------------- lix/libstore/pathlocks.hh | 7 ---- lix/libstore/profiles.cc | 1 - 6 files changed, 11 insertions(+), 43 deletions(-) diff --git a/lix/libstore/build/derivation-goal.cc b/lix/libstore/build/derivation-goal.cc index 12eac38cf..2f90df95b 100644 --- a/lix/libstore/build/derivation-goal.cc +++ b/lix/libstore/build/derivation-goal.cc @@ -756,7 +756,6 @@ retry: if (buildMode != bmCheck && allValid) { debug("skipping build of derivation '%s', someone beat us to it", worker.store.printStorePath(drvPath)); - outputLocks.setDeletion(true); co_return done(BuildResult::AlreadyValid, std::move(validOutputs)); } @@ -1087,7 +1086,6 @@ try { lockers will see that the output paths are valid; they will not create new lock files with the same names as the old (unlinked) lock files. */ - outputLocks.setDeletion(true); outputLocks.unlock(); co_return done(BuildResult::Built, std::move(builtOutputs)); diff --git a/lix/libstore/build/local-derivation-goal.cc b/lix/libstore/build/local-derivation-goal.cc index 7ae6474a3..554edfc5b 100644 --- a/lix/libstore/build/local-derivation-goal.cc +++ b/lix/libstore/build/local-derivation-goal.cc @@ -2296,7 +2296,6 @@ try { floating CA derivations and hash-mismatching fixed-output derivations. */ PathLocks dynamicOutputLock; - dynamicOutputLock.setDeletion(true); auto optFixedPath = output->path(worker.store, drv->name, outputName); if (!optFixedPath || worker.store.printStorePath(*optFixedPath) != finalDestPath) diff --git a/lix/libstore/local-store.cc b/lix/libstore/local-store.cc index 1fd23af41..92c4c6e07 100644 --- a/lix/libstore/local-store.cc +++ b/lix/libstore/local-store.cc @@ -1272,8 +1272,6 @@ void LocalStore::addToStore(const ValidPathInfo & info, Source & source, registerValidPath(info); } - - outputLock.setDeletion(true); } } @@ -1414,8 +1412,6 @@ StorePath LocalStore::addToStoreFromDump(Source & source0, std::string_view name info.narSize = narHash.second; registerValidPath(info); } - - outputLock.setDeletion(true); } return dstPath; @@ -1466,8 +1462,6 @@ StorePath LocalStore::addTextToStore( }; registerValidPath(info); } - - outputLock.setDeletion(true); } return dstPath; diff --git a/lix/libstore/pathlocks.cc b/lix/libstore/pathlocks.cc index 0d4cc1a81..28e995d5b 100644 --- a/lix/libstore/pathlocks.cc +++ b/lix/libstore/pathlocks.cc @@ -26,18 +26,6 @@ AutoCloseFD openLockFile(const Path & path, bool create) } -void deleteLockFile(const Path & path, int fd) -{ - /* Get rid of the lock file. Have to be careful not to introduce - races. Write a (meaningless) token to the file to indicate to - other processes waiting on this lock that the lock is stale - (deleted). */ - unlink(path.c_str()); - writeFull(fd, "d"); - /* Note that the result of unlink() is ignored; removing the lock - file is an optimisation, not a necessity. */ -} - static int convertLockType(LockType lockType) { if (lockType == ltRead) return LOCK_SH; @@ -144,13 +132,11 @@ void unlockFile(int fd) PathLocks::PathLocks() - : deletePaths(false) { } PathLocks::PathLocks(const PathSet & paths, const std::string & waitMsg) - : deletePaths(false) { lockPaths(paths, waitMsg); } @@ -200,7 +186,7 @@ bool PathLocks::lockPathsImpl(const PathSet & paths, struct stat st; if (fstat(fd.get(), &st) == -1) throw SysError("statting lock file '%1%'", lockPath); - if (st.st_size != 0) + if (st.st_nlink == 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 @@ -231,12 +217,17 @@ PathLocks::~PathLocks() void PathLocks::unlock() { for (auto & i : fds) { - if (deletePaths) deleteLockFile(i.second, i.first); + // delete the file. if another file descriptor is used to acquire a lock on + // this file it will figure out that the file is stale once it calls stat() + // and inspects the link count. if unlink fails we merely leave around some + // stale lock file paths that can be reused or cleaned up by other threads. + unlink(i.second.c_str()); + // clobber file contents for compatibility wither other nix implementations + writeFull(i.first, "d"); - if (close(i.first) == -1) - printError( - "error (ignored): cannot close lock file on '%1%'", - i.second); + if (close(i.first) == -1) { + printError("error (ignored): cannot close lock file on '%1%'", i.second); + } debug("lock released on '%1%'", i.second); } @@ -245,12 +236,6 @@ void PathLocks::unlock() } -void PathLocks::setDeletion(bool deletePaths) -{ - this->deletePaths = deletePaths; -} - - FdLock::FdLock(AutoCloseFD & fd, LockType lockType, DontWait) { if (tryLockFile(fd.get(), lockType)) { diff --git a/lix/libstore/pathlocks.hh b/lix/libstore/pathlocks.hh index 51928ee10..0220858d8 100644 --- a/lix/libstore/pathlocks.hh +++ b/lix/libstore/pathlocks.hh @@ -17,11 +17,6 @@ namespace nix { */ AutoCloseFD openLockFile(const Path & path, bool create); -/** - * Delete an open lock file. - */ -void deleteLockFile(const Path & path, int fd); - enum LockType { ltRead, ltWrite }; void lockFile(int fd, LockType lockType); @@ -41,7 +36,6 @@ class PathLocks private: typedef std::pair FDPair; std::list fds; - bool deletePaths; bool lockPathsImpl(const PathSet & _paths, const std::string & waitMsg, bool wait); @@ -58,7 +52,6 @@ public: } ~PathLocks(); void unlock(); - void setDeletion(bool deletePaths); }; class FdLock diff --git a/lix/libstore/profiles.cc b/lix/libstore/profiles.cc index 50c0dc47e..fd29eee78 100644 --- a/lix/libstore/profiles.cc +++ b/lix/libstore/profiles.cc @@ -294,7 +294,6 @@ void switchGeneration( void lockProfile(PathLocks & lock, const Path & profile) { lock.lockPaths({profile}, fmt("waiting for lock on profile '%1%'", profile)); - lock.setDeletion(true); }