From 79f9c39e3630d8b503257f8f74bc035a90ca993c Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Mon, 3 Feb 2025 21:56:26 +0100 Subject: [PATCH] libstore: split tryLockFile lockFile lockFile is currently interruptible by signals like SIGCHLD. which happen a lot in daemons. now imagine that daemon enabled automatic garbage collection. observe that the local store does not actually check whether its lock operations have succeeded ... get the idea? Change-Id: Ibfd7ee786c4fee3add72d4456a7e95e73e09c73e --- lix/legacy/build-remote.cc | 6 ++-- lix/libstore/gc.cc | 6 ++-- lix/libstore/local-store.cc | 18 ++++++------ lix/libstore/lock.cc | 4 +-- lix/libstore/pathlocks.cc | 58 +++++++++++++++++++++---------------- lix/libstore/pathlocks.hh | 3 +- 6 files changed, 52 insertions(+), 43 deletions(-) diff --git a/lix/legacy/build-remote.cc b/lix/legacy/build-remote.cc index aa05899b1..f5e64d4da 100644 --- a/lix/legacy/build-remote.cc +++ b/lix/legacy/build-remote.cc @@ -137,7 +137,7 @@ static int main_build_remote(AsyncIoRoot & aio, std::string programName, Strings while (true) { bestSlotLock.reset(); AutoCloseFD lock = openLockFile(currentLoad + "/main-lock", true); - lockFile(lock.get(), ltWrite, true); + lockFile(lock.get(), ltWrite); bool rightType = false; @@ -156,7 +156,7 @@ static int main_build_remote(AsyncIoRoot & aio, std::string programName, Strings uint64_t load = 0; for (uint64_t slot = 0; slot < m.maxJobs; ++slot) { auto slotLock = openSlotLock(m, slot); - if (lockFile(slotLock.get(), ltWrite, false)) { + if (tryLockFile(slotLock.get(), ltWrite)) { if (!free) { free = std::move(slotLock); } @@ -281,7 +281,7 @@ connected: auto old = signal(SIGALRM, handleAlarm); alarm(15 * 60); - if (!lockFile(uploadLock.get(), ltWrite, true)) + if (!lockFile(uploadLock.get(), ltWrite)) printError("somebody is hogging the upload lock for '%s', continuing..."); alarm(0); signal(SIGALRM, old); diff --git a/lix/libstore/gc.cc b/lix/libstore/gc.cc index 0ef8a15de..f084b83c1 100644 --- a/lix/libstore/gc.cc +++ b/lix/libstore/gc.cc @@ -93,7 +93,7 @@ void LocalStore::createTempRootsFile() *fdTempRoots = openLockFile(fnTempRoots, true); debug("acquiring write lock on '%s'", fnTempRoots); - lockFile(fdTempRoots->get(), ltWrite, true); + lockFile(fdTempRoots->get(), ltWrite); /* Check whether the garbage collector didn't get in our way. */ @@ -214,7 +214,7 @@ void LocalStore::findTempRoots(Roots & tempRoots, bool censor) /* 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)) { + if (tryLockFile(fd.get(), ltWrite)) { printInfo("removing stale temporary roots file '%1%'", path); unlink(path.c_str()); writeFull(fd.get(), "d"); @@ -626,7 +626,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) exclusive lock before deleting them. */ if (baseName.find("tmp-", 0) == 0) { AutoCloseFD tmpDirFd{open(realPath.c_str(), O_RDONLY | O_DIRECTORY)}; - if (tmpDirFd.get() == -1 || !lockFile(tmpDirFd.get(), ltWrite, false)) { + if (tmpDirFd.get() == -1 || !tryLockFile(tmpDirFd.get(), ltWrite)) { debug("skipping locked tempdir '%s'", realPath); return; } diff --git a/lix/libstore/local-store.cc b/lix/libstore/local-store.cc index 2609d6632..f38af19eb 100644 --- a/lix/libstore/local-store.cc +++ b/lix/libstore/local-store.cc @@ -97,10 +97,10 @@ void migrateCASchema(SQLite& db, Path schemaPath, AutoCloseFD& lockFd) curCASchema, nixCASchemaVersion); } - if (!lockFile(lockFd.get(), ltWrite, false)) { + if (!tryLockFile(lockFd.get(), ltWrite)) { printInfo("waiting for exclusive access to the Nix store for ca drvs..."); unlockFile(lockFd.get()); // We have acquired a shared lock; release it to prevent deadlocks - lockFile(lockFd.get(), ltWrite, true); + lockFile(lockFd.get(), ltWrite); } if (curCASchema == 0) { @@ -168,7 +168,7 @@ void migrateCASchema(SQLite& db, Path schemaPath, AutoCloseFD& lockFd) } writeFile(schemaPath, fmt("%d", nixCASchemaVersion), 0666, true); - lockFile(lockFd.get(), ltRead, true); + lockFile(lockFd.get(), ltRead); } } @@ -278,9 +278,9 @@ LocalStore::LocalStore(LocalStoreConfig config) globalLock = openLockFile(globalLockPath.c_str(), true); } - if (!config_.readOnly && !lockFile(globalLock.get(), ltRead, false)) { + if (!config_.readOnly && !tryLockFile(globalLock.get(), ltRead)) { printInfo("waiting for the big Nix store lock..."); - lockFile(globalLock.get(), ltRead, true); + lockFile(globalLock.get(), ltRead); } /* Check the current database schema and if necessary do an @@ -317,10 +317,10 @@ LocalStore::LocalStore(LocalStoreConfig config) "which is no longer supported. To convert to the new format,\n" "please use the original Nix version 1.11 first."); - if (!lockFile(globalLock.get(), ltWrite, false)) { + if (!tryLockFile(globalLock.get(), ltWrite)) { printInfo("waiting for exclusive access to the Nix store..."); unlockFile(globalLock.get()); // We have acquired a shared lock; release it to prevent deadlocks - lockFile(globalLock.get(), ltWrite, true); + lockFile(globalLock.get(), ltWrite); } /* Get the schema version again, because another process may @@ -352,7 +352,7 @@ LocalStore::LocalStore(LocalStoreConfig config) writeFile(schemaPath, fmt("%1%", nixSchemaVersion), 0666, true); - lockFile(globalLock.get(), ltRead, true); + lockFile(globalLock.get(), ltRead); } else openDB(*state, false); @@ -1489,7 +1489,7 @@ std::pair LocalStore::createTempDirInStore() if (tmpDirFd.get() < 0) { continue; } - lockedByUs = lockFile(tmpDirFd.get(), ltWrite, true); + lockedByUs = lockFile(tmpDirFd.get(), ltWrite); } while (!pathExists(tmpDirFn) || !lockedByUs); return {tmpDirFn, std::move(tmpDirFd)}; } diff --git a/lix/libstore/lock.cc b/lix/libstore/lock.cc index 397199f57..b8a623cbb 100644 --- a/lix/libstore/lock.cc +++ b/lix/libstore/lock.cc @@ -90,7 +90,7 @@ struct SimpleUserLock : UserLock if (!fd) throw SysError("opening user lock '%s'", fnUserLock); - if (lockFile(fd.get(), ltWrite, false)) { + if (tryLockFile(fd.get(), ltWrite)) { auto lock = std::make_unique(); lock->fdUserLock = std::move(fd); @@ -162,7 +162,7 @@ struct AutoUserLock : UserLock if (!fd) throw SysError("opening user lock '%s'", fnUserLock); - if (lockFile(fd.get(), ltWrite, false)) { + if (tryLockFile(fd.get(), ltWrite)) { auto firstUid = settings.startId + i * maxIdsPerBuild; diff --git a/lix/libstore/pathlocks.cc b/lix/libstore/pathlocks.cc index 38d179f45..257155ebf 100644 --- a/lix/libstore/pathlocks.cc +++ b/lix/libstore/pathlocks.cc @@ -35,29 +35,37 @@ void deleteLockFile(const Path & path, int fd) file is an optimisation, not a necessity. */ } - -bool lockFile(int fd, LockType lockType, bool wait) +static int convertLockType(LockType lockType) { - int type; - if (lockType == ltRead) type = LOCK_SH; - else if (lockType == ltWrite) type = LOCK_EX; + if (lockType == ltRead) return LOCK_SH; + else if (lockType == ltWrite) return LOCK_EX; else abort(); +} - if (wait) { - while (flock(fd, type) != 0) { - checkInterrupt(); - if (errno != EINTR) - throw SysError("acquiring lock"); - else - return false; - } - } else { - while (flock(fd, type | LOCK_NB) != 0) { - checkInterrupt(); - if (errno == EWOULDBLOCK) return false; - if (errno != EINTR) - throw SysError("acquiring lock"); - } +bool lockFile(int fd, LockType lockType) +{ + int type = convertLockType(lockType); + + while (flock(fd, type) != 0) { + checkInterrupt(); + if (errno != EINTR) + throw SysError("acquiring lock"); + else + return false; + } + + return true; +} + +bool tryLockFile(int fd, LockType lockType) +{ + int type = convertLockType(lockType); + + while (flock(fd, type | LOCK_NB) != 0) { + checkInterrupt(); + if (errno == EWOULDBLOCK) return false; + if (errno != EINTR) + throw SysError("acquiring lock"); } return true; @@ -111,10 +119,10 @@ bool PathLocks::lockPaths(const PathSet & paths, fd = openLockFile(lockPath, true); /* Acquire an exclusive lock. */ - if (!lockFile(fd.get(), ltWrite, false)) { + if (!tryLockFile(fd.get(), ltWrite)) { if (wait) { if (waitMsg != "") printError(waitMsg); - lockFile(fd.get(), ltWrite, true); + lockFile(fd.get(), ltWrite); } else { /* Failed to lock this path; release all other locks. */ @@ -185,12 +193,12 @@ FdLock::FdLock(int fd, LockType lockType, bool wait, std::string_view waitMsg) : fd(fd) { if (wait) { - if (!lockFile(fd, lockType, false)) { + if (!tryLockFile(fd, lockType)) { printInfo("%s", waitMsg); - acquired = lockFile(fd, lockType, true); + acquired = lockFile(fd, lockType); } } else - acquired = lockFile(fd, lockType, false); + acquired = tryLockFile(fd, lockType); } diff --git a/lix/libstore/pathlocks.hh b/lix/libstore/pathlocks.hh index 3309cd687..d4b3ef7a2 100644 --- a/lix/libstore/pathlocks.hh +++ b/lix/libstore/pathlocks.hh @@ -20,7 +20,8 @@ void deleteLockFile(const Path & path, int fd); enum LockType { ltRead, ltWrite }; -bool lockFile(int fd, LockType lockType, bool wait); +bool lockFile(int fd, LockType lockType); +bool tryLockFile(int fd, LockType lockType); void unlockFile(int fd); class PathLocks