From 8eac7dfad427acff412d2cd1a0de6e6e683aac0b Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 16 Aug 2021 13:52:19 +0200 Subject: [PATCH 01/25] Remove trash directory --- src/libstore/gc.cc | 62 ++++--------------------------------- src/libstore/local-store.cc | 1 - src/libstore/local-store.hh | 1 - 3 files changed, 6 insertions(+), 58 deletions(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 5a62c6529..5de09d8c2 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -465,11 +465,9 @@ struct LocalStore::GCState StorePathSet alive; bool gcKeepOutputs; bool gcKeepDerivations; - uint64_t bytesInvalidated; - bool moveToTrash = true; bool shouldDelete; GCState(const GCOptions & options, GCResults & results) - : options(options), results(results), bytesInvalidated(0) { } + : options(options), results(results) { } }; @@ -493,15 +491,12 @@ void LocalStore::deletePathRecursive(GCState & state, const Path & path) { checkInterrupt(); - uint64_t size = 0; - auto storePath = maybeParseStorePath(path); if (storePath && isValidPath(*storePath)) { StorePathSet referrers; queryReferrers(*storePath, referrers); for (auto & i : referrers) if (printStorePath(i) != path) deletePathRecursive(state, printStorePath(i)); - size = queryPathInfo(*storePath)->narSize; invalidatePathChecked(*storePath); } @@ -517,33 +512,10 @@ void LocalStore::deletePathRecursive(GCState & state, const Path & path) state.results.paths.insert(path); - /* If the path is not a regular file or symlink, move it to the - trash directory. The move is to ensure that later (when we're - not holding the global GC lock) we can delete the path without - being afraid that the path has become alive again. Otherwise - delete it right away. */ - if (state.moveToTrash && S_ISDIR(st.st_mode)) { - // Estimate the amount freed using the narSize field. FIXME: - // if the path was not valid, need to determine the actual - // size. - try { - if (chmod(realPath.c_str(), st.st_mode | S_IWUSR) == -1) - throw SysError("making '%1%' writable", realPath); - Path tmp = trashDir + "/" + std::string(baseNameOf(path)); - if (rename(realPath.c_str(), tmp.c_str())) - throw SysError("unable to rename '%1%' to '%2%'", realPath, tmp); - state.bytesInvalidated += size; - } catch (SysError & e) { - if (e.errNo == ENOSPC) { - printInfo(format("note: can't create move '%1%': %2%") % realPath % e.msg()); - deleteGarbage(state, realPath); - } - } - } else - deleteGarbage(state, realPath); + deleteGarbage(state, realPath); - if (state.results.bytesFreed + state.bytesInvalidated > state.options.maxFreed) { - printInfo(format("deleted or invalidated more than %1% bytes; stopping") % state.options.maxFreed); + if (state.results.bytesFreed > state.options.maxFreed) { + printInfo("deleted more than %d bytes; stopping", state.options.maxFreed); throw GCLimitReached(); } } @@ -607,7 +579,7 @@ void LocalStore::tryToDelete(GCState & state, const Path & path) checkInterrupt(); auto realPath = realStoreDir + "/" + std::string(baseNameOf(path)); - if (realPath == linksDir || realPath == trashDir) return; + if (realPath == linksDir) return; //Activity act(*logger, lvlDebug, format("considering whether to delete '%1%'") % path); @@ -710,9 +682,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) if (state.shouldDelete) deletePath(reservedPath); - /* Acquire the global GC root. This prevents - a) New roots from being added. - b) Processes from creating new temporary root files. */ + /* Acquire the global GC root. */ AutoCloseFD fdGCLock = openGCLock(ltWrite); /* Find the roots. Since we've grabbed the GC lock, the set of @@ -739,18 +709,6 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) increase, since we hold locks on everything. So everything that is not reachable from `roots' is garbage. */ - if (state.shouldDelete) { - if (pathExists(trashDir)) deleteGarbage(state, trashDir); - try { - createDirs(trashDir); - } catch (SysError & e) { - if (e.errNo == ENOSPC) { - printInfo("note: can't create trash directory: %s", e.msg()); - state.moveToTrash = false; - } - } - } - /* Now either delete all garbage paths, or just the specified paths (for gcDeleteSpecific). */ @@ -828,14 +786,6 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) return; } - /* Allow other processes to add to the store from here on. */ - fdGCLock = -1; - fds.clear(); - - /* Delete the trash directory. */ - printInfo(format("deleting '%1%'") % trashDir); - deleteGarbage(state, trashDir); - /* Clean up the links directory. */ if (options.action == GCOptions::gcDeleteDead || options.action == GCOptions::gcDeleteSpecific) { printInfo("deleting unused links..."); diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 5b2490472..3978f4eed 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -145,7 +145,6 @@ LocalStore::LocalStore(const Params & params) , linksDir(realStoreDir + "/.links") , reservedPath(dbDir + "/reserved") , schemaPath(dbDir + "/schema") - , trashDir(realStoreDir + "/trash") , tempRootsDir(stateDir + "/temproots") , fnTempRoots(fmt("%s/%d", tempRootsDir, getpid())) , locksHeld(tokenizeString(getEnv("NIX_HELD_LOCKS").value_or(""))) diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index a01d48c4b..5215c9f02 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -87,7 +87,6 @@ public: const Path linksDir; const Path reservedPath; const Path schemaPath; - const Path trashDir; const Path tempRootsDir; const Path fnTempRoots; From 9947f1646a26b339fff2e02b77798e9841fac7f0 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 17 Aug 2021 18:53:14 +0200 Subject: [PATCH 02/25] Remove syncWithGC() --- src/libstore/daemon.cc | 2 +- src/libstore/gc.cc | 17 ++++++----------- src/libstore/local-store.hh | 2 -- src/libstore/remote-store.cc | 9 --------- src/libstore/remote-store.hh | 2 -- src/libstore/store-api.hh | 20 -------------------- 6 files changed, 7 insertions(+), 45 deletions(-) diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index 164a9b2be..3df88ff03 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -624,9 +624,9 @@ static void performOp(TunnelLogger * logger, ref store, break; } + // Obsolete. case wopSyncWithGC: { logger->startWork(); - store->syncWithGC(); logger->stopWork(); to << 1; break; diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 5de09d8c2..18328467b 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -71,12 +71,6 @@ static void makeSymlink(const Path & link, const Path & target) } -void LocalStore::syncWithGC() -{ - AutoCloseFD fdGCLock = openGCLock(ltRead); -} - - void LocalStore::addIndirectRoot(const Path & path) { string hash = hashString(htSHA1, path).to_string(Base32, false); @@ -95,6 +89,12 @@ Path LocalFSStore::addPermRoot(const StorePath & storePath, const Path & _gcRoot "creating a garbage collector root (%1%) in the Nix store is forbidden " "(are you running nix-build inside the store?)", gcRoot); + /* Register this root with the garbage collector, if it's + running. This should be superfluous since the caller should + have registered this root yet, but let's be on the safe + side. */ + addTempRoot(storePath); + /* Don't clobber the link if it already exists and doesn't point to the Nix store. */ if (pathExists(gcRoot) && (!isLink(gcRoot) || !isInStore(readLink(gcRoot)))) @@ -102,11 +102,6 @@ Path LocalFSStore::addPermRoot(const StorePath & storePath, const Path & _gcRoot makeSymlink(gcRoot, printStorePath(storePath)); addIndirectRoot(gcRoot); - /* Grab the global GC root, causing us to block while a GC is in - progress. This prevents the set of permanent roots from - increasing while a GC is in progress. */ - syncWithGC(); - return gcRoot; } diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 5215c9f02..3b3f26162 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -148,8 +148,6 @@ public: void addIndirectRoot(const Path & path) override; - void syncWithGC() override; - private: typedef std::shared_ptr FDPtr; diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index fa5ea8af7..7decc059c 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -797,15 +797,6 @@ void RemoteStore::addIndirectRoot(const Path & path) } -void RemoteStore::syncWithGC() -{ - auto conn(getConnection()); - conn->to << wopSyncWithGC; - conn.processStderr(); - readInt(conn->from); -} - - Roots RemoteStore::findRoots(bool censor) { auto conn(getConnection()); diff --git a/src/libstore/remote-store.hh b/src/libstore/remote-store.hh index ac1eaa19e..a3036e6b0 100644 --- a/src/libstore/remote-store.hh +++ b/src/libstore/remote-store.hh @@ -101,8 +101,6 @@ public: void addIndirectRoot(const Path & path) override; - void syncWithGC() override; - Roots findRoots(bool censor) override; void collectGarbage(const GCOptions & options, GCResults & results) override; diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 54471bdf2..35461b76d 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -561,26 +561,6 @@ public: virtual void addIndirectRoot(const Path & path) { unsupported("addIndirectRoot"); } - /* Acquire the global GC lock, then immediately release it. This - function must be called after registering a new permanent root, - but before exiting. Otherwise, it is possible that a running - garbage collector doesn't see the new root and deletes the - stuff we've just built. By acquiring the lock briefly, we - ensure that either: - - - The collector is already running, and so we block until the - collector is finished. The collector will know about our - *temporary* locks, which should include whatever it is we - want to register as a permanent lock. - - - The collector isn't running, or it's just started but hasn't - acquired the GC lock yet. In that case we get and release - the lock right away, then exit. The collector scans the - permanent root and sees ours. - - In either case the permanent root is seen by the collector. */ - virtual void syncWithGC() { }; - /* Find the roots of the garbage collector. Each root is a pair (link, storepath) where `link' is the path of the symlink outside of the Nix store that point to `storePath'. If From 8614cf13344eca75074cd4af20fd90238571b0b6 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 16 Aug 2021 20:03:32 +0200 Subject: [PATCH 03/25] Non-blocking garbage collector The garbage collector no longer blocks other processes from adding/building store paths or adding GC roots. To prevent the collector from deleting store paths just added by another process, processes need to connect to the garbage collector via a Unix domain socket to register new temporary roots. --- src/libstore/build/local-derivation-goal.cc | 2 +- src/libstore/gc.cc | 524 +++++++++++--------- src/libstore/local-store.cc | 9 +- src/libstore/local-store.hh | 28 +- src/libstore/pathlocks.cc | 13 + src/libstore/pathlocks.hh | 14 + src/libstore/uds-remote-store.cc | 9 +- src/libutil/util.cc | 10 +- src/libutil/util.hh | 3 + tests/ca/build-with-garbage-path.sh | 2 +- tests/repair.sh | 2 +- 11 files changed, 344 insertions(+), 272 deletions(-) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 8d245f84a..bafa83a9d 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -1353,7 +1353,7 @@ void LocalDerivationGoal::startDaemon() AutoCloseFD remote = accept(daemonSocket.get(), (struct sockaddr *) &remoteAddr, &remoteAddrLen); if (!remote) { - if (errno == EINTR) continue; + if (errno == EINTR || errno == EAGAIN) continue; if (errno == EINVAL) break; throw SysError("accepting connection"); } diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 18328467b..2d7734729 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -10,48 +10,22 @@ #include #include -#include -#include -#include +#include #include #include +#include +#include +#include +#include +#include +#include #include -#include namespace nix { -static string gcLockName = "gc.lock"; -static string gcRootsDir = "gcroots"; - - -/* Acquire the global GC lock. This is used to prevent new Nix - processes from starting after the temporary root files have been - 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. */ -AutoCloseFD LocalStore::openGCLock(LockType lockType) -{ - Path fnGCLock = (format("%1%/%2%") - % stateDir % gcLockName).str(); - - debug(format("acquiring global GC lock '%1%'") % fnGCLock); - - AutoCloseFD fdGCLock = open(fnGCLock.c_str(), O_RDWR | O_CREAT | O_CLOEXEC, 0600); - if (!fdGCLock) - throw SysError("opening global GC lock '%1%'", fnGCLock); - - if (!lockFile(fdGCLock.get(), lockType, false)) { - printInfo("waiting for the big garbage collector lock..."); - lockFile(fdGCLock.get(), lockType, true); - } - - /* !!! Restrict read permission on the GC root. Otherwise any - process that can open the file for reading can DoS the - collector. */ - - return fdGCLock; -} +static std::string gcSocketPath = "/gc-socket/socket"; +static std::string gcRootsDir = "gcroots"; static void makeSymlink(const Path & link, const Path & target) @@ -114,8 +88,6 @@ void LocalStore::addTempRoot(const StorePath & path) if (!state->fdTempRoots) { while (1) { - AutoCloseFD fdGCLock = openGCLock(ltRead); - if (pathExists(fnTempRoots)) /* It *must* be stale, since there can be no two processes with the same pid. */ @@ -123,10 +95,8 @@ void LocalStore::addTempRoot(const StorePath & path) state->fdTempRoots = openLockFile(fnTempRoots, true); - fdGCLock = -1; - - debug(format("acquiring read lock on '%1%'") % fnTempRoots); - lockFile(state->fdTempRoots.get(), ltRead, true); + debug("acquiring write lock on '%s'", fnTempRoots); + lockFile(state->fdTempRoots.get(), ltWrite, true); /* Check whether the garbage collector didn't get in our way. */ @@ -142,24 +112,62 @@ void LocalStore::addTempRoot(const StorePath & path) } - /* Upgrade the lock to a write lock. This will cause us to block - if the garbage collector is holding our lock. */ - debug(format("acquiring write lock on '%1%'") % fnTempRoots); - lockFile(state->fdTempRoots.get(), ltWrite, true); + restart: + FdLock gcLock(state->fdGCLock.get(), ltRead, false, ""); + if (!gcLock.acquired) { + /* We couldn't get a shared global GC lock, so the garbage + collector is running. So we have to connect to the garbage + collector and inform it about our root. */ + if (!state->fdRootsSocket) { + state->fdRootsSocket = createUnixDomainSocket(); + + auto socketPath = stateDir.get() + gcSocketPath; + + debug("connecting to '%s'", socketPath); + + struct sockaddr_un addr; + addr.sun_family = AF_UNIX; + if (socketPath.size() + 1 >= sizeof(addr.sun_path)) + throw Error("socket path '%s' is too long", socketPath); + strcpy(addr.sun_path, socketPath.c_str()); + + if (::connect(state->fdRootsSocket.get(), (struct sockaddr *) &addr, sizeof(addr)) == -1) + throw SysError("cannot connect to garbage collector at '%s'", socketPath); + } + + try { + debug("sending GC root '%s'", printStorePath(path)); + writeFull(state->fdRootsSocket.get(), printStorePath(path) + "\n", false); + char c; + readFull(state->fdRootsSocket.get(), &c, 1); + assert(c == '1'); + debug("got ack for GC root '%s'", printStorePath(path)); + } catch (SysError & e) { + /* The garbage collector may have exited, so we need to + restart. */ + if (e.errNo == EPIPE) { + debug("GC socket disconnected"); + state->fdRootsSocket.close(); + goto restart; + } + } catch (EndOfFile & e) { + debug("GC socket disconnected"); + state->fdRootsSocket.close(); + goto restart; + } + } + + /* Append the store path to the temporary roots file. */ string s = printStorePath(path) + '\0'; writeFull(state->fdTempRoots.get(), s); - - /* Downgrade to a read lock. */ - debug(format("downgrading to read lock on '%1%'") % fnTempRoots); - lockFile(state->fdTempRoots.get(), ltRead, true); } static std::string censored = "{censored}"; -void LocalStore::findTempRoots(FDs & fds, Roots & tempRoots, bool censor) +void LocalStore::findTempRoots(Roots & tempRoots, bool censor) { /* Read the `temproots' directory for per-process temporary root files. */ @@ -174,35 +182,25 @@ void LocalStore::findTempRoots(FDs & fds, Roots & tempRoots, bool censor) pid_t pid = std::stoi(i.name); debug(format("reading temporary root file '%1%'") % path); - FDPtr fd(new AutoCloseFD(open(path.c_str(), O_CLOEXEC | O_RDWR, 0666))); - if (!*fd) { + AutoCloseFD fd(open(path.c_str(), O_CLOEXEC | O_RDWR, 0666)); + if (!fd) { /* It's okay if the file has disappeared. */ if (errno == ENOENT) continue; throw SysError("opening temporary roots file '%1%'", path); } - /* This should work, but doesn't, for some reason. */ - //FDPtr fd(new AutoCloseFD(openLockFile(path, false))); - //if (*fd == -1) continue; - /* 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 (lockFile(fd.get(), ltWrite, false)) { printInfo("removing stale temporary roots file '%1%'", path); unlink(path.c_str()); - writeFull(fd->get(), "d"); + 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()); + string contents = readFile(fd.get()); /* Extract the roots. */ string::size_type pos = 0, end; @@ -213,8 +211,6 @@ void LocalStore::findTempRoots(FDs & fds, Roots & tempRoots, bool censor) tempRoots[parseStorePath(root)].emplace(censor ? censored : fmt("{temp:%d}", pid)); pos = end + 1; } - - fds.push_back(fd); /* keep open */ } } @@ -299,8 +295,7 @@ Roots LocalStore::findRoots(bool censor) Roots roots; findRootsNoTemp(roots, censor); - FDs fds; - findTempRoots(fds, roots, censor); + findTempRoots(roots, censor); return roots; } @@ -455,160 +450,141 @@ struct LocalStore::GCState const GCOptions & options; GCResults & results; StorePathSet roots; - StorePathSet tempRoots; StorePathSet dead; StorePathSet alive; bool gcKeepOutputs; bool gcKeepDerivations; bool shouldDelete; + + struct Shared + { + // The temp roots only store the hash part to make it easier to + // ignore suffixes like '.lock', '.chroot' and '.check'. + std::unordered_set tempRoots; + + // Hash part of the store path currently being deleted, if + // any. + std::optional pending; + }; + + Sync shared; + + std::condition_variable wakeup; + GCState(const GCOptions & options, GCResults & results) : options(options), results(results) { } }; -bool LocalStore::isActiveTempFile(const GCState & state, - const Path & path, const string & suffix) -{ - return hasSuffix(path, suffix) - && state.tempRoots.count(parseStorePath(string(path, 0, path.size() - suffix.size()))); -} - - -void LocalStore::deleteGarbage(GCState & state, const Path & path) -{ - uint64_t bytesFreed; - deletePath(path, bytesFreed); - state.results.bytesFreed += bytesFreed; -} - - -void LocalStore::deletePathRecursive(GCState & state, const Path & path) -{ - checkInterrupt(); - - auto storePath = maybeParseStorePath(path); - if (storePath && isValidPath(*storePath)) { - StorePathSet referrers; - queryReferrers(*storePath, referrers); - for (auto & i : referrers) - if (printStorePath(i) != path) deletePathRecursive(state, printStorePath(i)); - invalidatePathChecked(*storePath); - } - - Path realPath = realStoreDir + "/" + std::string(baseNameOf(path)); - - struct stat st; - if (lstat(realPath.c_str(), &st)) { - if (errno == ENOENT) return; - throw SysError("getting status of %1%", realPath); - } - - printInfo(format("deleting '%1%'") % path); - - state.results.paths.insert(path); - - deleteGarbage(state, realPath); - - if (state.results.bytesFreed > state.options.maxFreed) { - printInfo("deleted more than %d bytes; stopping", state.options.maxFreed); - throw GCLimitReached(); - } -} - - -bool LocalStore::canReachRoot(GCState & state, StorePathSet & visited, const StorePath & path) -{ - if (visited.count(path)) return false; - - if (state.alive.count(path)) return true; - - if (state.dead.count(path)) return false; - - if (state.roots.count(path)) { - debug("cannot delete '%1%' because it's a root", printStorePath(path)); - state.alive.insert(path); - return true; - } - - visited.insert(path); - - if (!isValidPath(path)) return false; - - StorePathSet incoming; - - /* Don't delete this path if any of its referrers are alive. */ - queryReferrers(path, incoming); - - /* If keep-derivations is set and this is a derivation, then - don't delete the derivation if any of the outputs are alive. */ - if (state.gcKeepDerivations && path.isDerivation()) { - for (auto & [name, maybeOutPath] : queryPartialDerivationOutputMap(path)) - if (maybeOutPath && - isValidPath(*maybeOutPath) && - queryPathInfo(*maybeOutPath)->deriver == path - ) - incoming.insert(*maybeOutPath); - } - - /* If keep-outputs is set, then don't delete this path if there - are derivers of this path that are not garbage. */ - if (state.gcKeepOutputs) { - auto derivers = queryValidDerivers(path); - for (auto & i : derivers) - incoming.insert(i); - } - - for (auto & i : incoming) - if (i != path) - if (canReachRoot(state, visited, i)) { - state.alive.insert(path); - return true; - } - - return false; -} - - -void LocalStore::tryToDelete(GCState & state, const Path & path) +bool LocalStore::tryToDelete( + GCState & state, + StorePathSet & visited, + const Path & path, + bool recursive) { checkInterrupt(); auto realPath = realStoreDir + "/" + std::string(baseNameOf(path)); - if (realPath == linksDir) return; + if (realPath == linksDir) return false; //Activity act(*logger, lvlDebug, format("considering whether to delete '%1%'") % path); auto storePath = maybeParseStorePath(path); - if (!storePath || !isValidPath(*storePath)) { - /* A lock file belonging to a path that we're building right - now isn't garbage. */ - if (isActiveTempFile(state, path, ".lock")) return; + /* Wake up any client waiting for deletion of this path to + finish. */ + Finally releasePending([&]() { + auto shared(state.shared.lock()); + shared->pending.reset(); + state.wakeup.notify_all(); + }); - /* Don't delete .chroot directories for derivations that are - currently being built. */ - if (isActiveTempFile(state, path, ".chroot")) return; + if (storePath) { - /* Don't delete .check directories for derivations that are - currently being built, because we may need to run - diff-hook. */ - if (isActiveTempFile(state, path, ".check")) return; - } + if (!visited.insert(*storePath).second) return true; - StorePathSet visited; + if (state.alive.count(*storePath)) return false; + + if (state.dead.count(*storePath)) return true; + + if (state.roots.count(*storePath)) { + debug("cannot delete '%s' because it's a root", path); + state.alive.insert(*storePath); + return false; + } + + if (isValidPath(*storePath)) { + StorePathSet incoming; + + /* Don't delete this path if any of its referrers are alive. */ + queryReferrers(*storePath, incoming); + + /* If keep-derivations is set and this is a derivation, then + don't delete the derivation if any of the outputs are alive. */ + if (state.gcKeepDerivations && storePath->isDerivation()) { + for (auto & [name, maybeOutPath] : queryPartialDerivationOutputMap(*storePath)) + if (maybeOutPath && + isValidPath(*maybeOutPath) && + queryPathInfo(*maybeOutPath)->deriver == *storePath) + incoming.insert(*maybeOutPath); + } + + /* If keep-outputs is set, then don't delete this path if there + are derivers of this path that are not garbage. */ + if (state.gcKeepOutputs) { + auto derivers = queryValidDerivers(*storePath); + for (auto & i : derivers) + incoming.insert(i); + } + + for (auto & i : incoming) + if (i != *storePath + && (recursive || state.options.pathsToDelete.count(i)) + && !tryToDelete(state, visited, printStorePath(i), recursive)) + { + state.alive.insert(*storePath); + return false; + } + } + + { + auto hashPart = std::string(storePath->hashPart()); + auto shared(state.shared.lock()); + if (shared->tempRoots.count(hashPart)) + return false; + shared->pending = hashPart; + } + + state.dead.insert(*storePath); - if (storePath && canReachRoot(state, visited, *storePath)) { - debug("cannot delete '%s' because it's still reachable", path); - } else { - /* No path we visited was a root, so everything is garbage. - But we only delete ‘path’ and its referrers here so that - ‘nix-store --delete’ doesn't have the unexpected effect of - recursing into derivations and outputs. */ - for (auto & i : visited) - state.dead.insert(i); if (state.shouldDelete) - deletePathRecursive(state, path); + invalidatePathChecked(*storePath); } + + if (state.shouldDelete) { + Path realPath = realStoreDir + "/" + std::string(baseNameOf(path)); + + struct stat st; + if (lstat(realPath.c_str(), &st)) { + if (errno == ENOENT) return true; + throw SysError("getting status of %1%", realPath); + } + + printInfo("deleting '%1%'", path); + + state.results.paths.insert(path); + + uint64_t bytesFreed; + deletePath(realPath, bytesFreed); + state.results.bytesFreed += bytesFreed; + + if (state.results.bytesFreed > state.options.maxFreed) { + printInfo("deleted more than %d bytes; stopping", state.options.maxFreed); + throw GCLimitReached(); + } + } + + return true; } @@ -678,7 +654,104 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) deletePath(reservedPath); /* Acquire the global GC root. */ - AutoCloseFD fdGCLock = openGCLock(ltWrite); + FdLock gcLock(_state.lock()->fdGCLock.get(), ltWrite, true, "waiting for the big garbage collector lock..."); + + /* Start the server for receiving new roots. */ + auto socketPath = stateDir.get() + gcSocketPath; + createDirs(dirOf(socketPath)); + auto fdServer = createUnixDomainSocket(socketPath, 0666); + + if (fcntl(fdServer.get(), F_SETFL, fcntl(fdServer.get(), F_GETFL) | O_NONBLOCK) == -1) + throw SysError("making socket '%1%' non-blocking", socketPath); + + Pipe shutdownPipe; + shutdownPipe.create(); + + std::thread serverThread([&]() { + std::map, std::string>> fdClients; + bool quit = false; + + while (!quit) { + std::vector fds; + fds.push_back({.fd = shutdownPipe.readSide.get(), .events = POLLIN}); + fds.push_back({.fd = fdServer.get(), .events = POLLIN}); + for (auto & i : fdClients) + fds.push_back({.fd = i.first, .events = POLLIN}); + auto count = poll(fds.data(), fds.size(), -1); + assert(count != -1); + + for (auto & fd : fds) { + if (!fd.revents) continue; + if (fd.fd == shutdownPipe.readSide.get()) + /* Parent is asking us to quit. */ + quit = true; + else if (fd.fd == fdServer.get()) { + /* Accept a new connection. */ + assert(fd.revents & POLLIN); + auto fdClient = std::make_unique(accept(fdServer.get(), nullptr, nullptr)); + if (*fdClient) { + auto fd = fdClient->get(); + fdClients.insert({fd, std::make_pair(std::move(fdClient), "")}); + } + } + else { + /* Receive data from a client. */ + auto fdClient = fdClients.find(fd.fd); + assert(fdClient != fdClients.end()); + if (fd.revents & POLLIN) { + char buf[16384]; + auto n = read(fd.fd, buf, sizeof(buf)); + if (n > 0) { + fdClient->second.second.append(buf, n); + /* Split the input into lines. */ + while (true) { + auto p = fdClient->second.second.find('\n'); + if (p == std::string::npos) break; + /* We got a full line. Send ack back + to the client. */ + auto path = fdClient->second.second.substr(0, p); + fdClient->second.second = fdClient->second.second.substr(p + 1); + auto storePath = maybeParseStorePath(path); + if (storePath) { + debug("got new GC root '%s'", path); + auto hashPart = std::string(storePath->hashPart()); + auto shared(state.shared.lock()); + shared->tempRoots.insert(hashPart); + /* If this path is currently being + deleted, then we have to wait + until deletion is finished to + ensure that the client doesn't + start re-creating it before + we're done. FIXME: ideally we + would use a FD for this so we + don't block the poll loop. */ + while (shared->pending == hashPart) { + debug("synchronising with deletion of path '%s'", path); + shared.wait(state.wakeup); + } + } else + printError("received garbage instead of a root from client"); + // This could block, but meh. + try { + writeFull(fd.fd, "1", false); + } catch (SysError &) { } + } + } else if (n == 0) + fdClients.erase(fdClient); + } else + fdClients.erase(fdClient); + } + } + } + + debug("GC roots server shut down"); + }); + + Finally stopServer([&]() { + writeFull(shutdownPipe.writeSide.get(), "x", false); + state.wakeup.notify_all(); + if (serverThread.joinable()) serverThread.join(); + }); /* Find the roots. Since we've grabbed the GC lock, the set of permanent roots cannot increase now. */ @@ -689,32 +762,26 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) for (auto & i : rootMap) state.roots.insert(i.first); - /* Read the temporary roots. This acquires read locks on all - per-process temporary root files. So after this point no paths - can be added to the set of temporary roots. */ - FDs fds; + /* Read the temporary roots created before we acquired the global + GC root. Any new roots will be sent to our socket. */ Roots tempRoots; - findTempRoots(fds, tempRoots, true); + findTempRoots(tempRoots, true); for (auto & root : tempRoots) { - state.tempRoots.insert(root.first); + state.shared.lock()->tempRoots.insert(std::string(root.first.hashPart())); state.roots.insert(root.first); } - /* After this point the set of roots or temporary roots cannot - increase, since we hold locks on everything. So everything - that is not reachable from `roots' is garbage. */ - /* Now either delete all garbage paths, or just the specified paths (for gcDeleteSpecific). */ if (options.action == GCOptions::gcDeleteSpecific) { for (auto & i : options.pathsToDelete) { - tryToDelete(state, printStorePath(i)); - if (state.dead.find(i) == state.dead.end()) + StorePathSet visited; + if (!tryToDelete(state, visited, printStorePath(i), false)) throw Error( "cannot delete path '%1%' since it is still alive. " - "To find out why use: " + "To find out why, use: " "nix-store --query --roots", printStorePath(i)); } @@ -727,44 +794,21 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) printInfo("determining live/dead paths..."); try { - AutoCloseDir dir(opendir(realStoreDir.get().c_str())); if (!dir) throw SysError("opening directory '%1%'", realStoreDir); - /* Read the store and immediately delete all paths that - aren't valid. When using --max-freed etc., deleting - invalid paths is preferred over deleting unreachable - paths, since unreachable paths could become reachable - again. We don't use readDirectory() here so that GCing - can start faster. */ + /* Read the store and delete all paths that are invalid or + unreachable. We don't use readDirectory() here so that + GCing can start faster. */ Paths entries; struct dirent * dirent; while (errno = 0, dirent = readdir(dir.get())) { checkInterrupt(); string name = dirent->d_name; if (name == "." || name == "..") continue; - Path path = storeDir + "/" + name; - auto storePath = maybeParseStorePath(path); - if (storePath && isValidPath(*storePath)) - entries.push_back(path); - else - tryToDelete(state, path); + StorePathSet visited; + tryToDelete(state, visited, storeDir + "/" + name, true); } - - dir.reset(); - - /* Now delete the unreachable valid paths. Randomise the - order in which we delete entries to make the collector - less biased towards deleting paths that come - alphabetically first (e.g. /nix/store/000...). This - matters when using --max-freed etc. */ - vector entries_(entries.begin(), entries.end()); - std::mt19937 gen(1); - std::shuffle(entries_.begin(), entries_.end(), gen); - - for (auto & i : entries_) - tryToDelete(state, i); - } catch (GCLimitReached & e) { } } diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 3978f4eed..faecdb00b 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -382,6 +382,11 @@ LocalStore::LocalStore(const Params & params) (select id from Realisations where drvPath = ? and outputName = ?)); )"); } + + Path fnGCLock = stateDir + "/gc.lock"; + state->fdGCLock = open(fnGCLock.c_str(), O_RDWR | O_CREAT | O_CLOEXEC, 0600); + if (!state->fdGCLock) + throw SysError("opening global GC lock '%1%'", fnGCLock); } @@ -1504,7 +1509,7 @@ bool LocalStore::verifyStore(bool checkContents, RepairFlag repair) /* Acquire the global GC lock to get a consistent snapshot of existing and valid paths. */ - AutoCloseFD fdGCLock = openGCLock(ltWrite); + FdLock gcLock(_state.lock()->fdGCLock.get(), ltWrite, true, "waiting for the big garbage collector lock..."); StringSet store; for (auto & i : readDirectory(realStoreDir)) store.insert(i.name); @@ -1515,8 +1520,6 @@ bool LocalStore::verifyStore(bool checkContents, RepairFlag repair) StorePathSet validPaths; PathSet done; - fdGCLock = -1; - for (auto & i : queryAllValidPaths()) verifyPath(printStorePath(i), store, done, validPaths, repair, errors); diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 3b3f26162..85c6c4495 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -58,9 +58,15 @@ private: struct Stmts; std::unique_ptr stmts; + /* The global GC lock */ + AutoCloseFD fdGCLock; + /* The file to which we write our temporary roots. */ AutoCloseFD fdTempRoots; + /* Connection to the garbage collector. */ + AutoCloseFD fdRootsSocket; + /* The last time we checked whether to do an auto-GC, or an auto-GC finished. */ std::chrono::time_point lastGCCheck; @@ -150,10 +156,7 @@ public: private: - typedef std::shared_ptr FDPtr; - typedef list FDs; - - void findTempRoots(FDs & fds, Roots & roots, bool censor); + void findTempRoots(Roots & roots, bool censor); public: @@ -235,18 +238,11 @@ private: struct GCState; - void deleteGarbage(GCState & state, const Path & path); - - void tryToDelete(GCState & state, const Path & path); - - bool canReachRoot(GCState & state, StorePathSet & visited, const StorePath & path); - - void deletePathRecursive(GCState & state, const Path & path); - - bool isActiveTempFile(const GCState & state, - const Path & path, const string & suffix); - - AutoCloseFD openGCLock(LockType lockType); + bool tryToDelete( + GCState & state, + StorePathSet & visited, + const Path & path, + bool recursive); void findRoots(const Path & path, unsigned char type, Roots & roots); diff --git a/src/libstore/pathlocks.cc b/src/libstore/pathlocks.cc index 926f4ea1e..2da74e262 100644 --- a/src/libstore/pathlocks.cc +++ b/src/libstore/pathlocks.cc @@ -176,4 +176,17 @@ void PathLocks::setDeletion(bool deletePaths) } +FdLock::FdLock(int fd, LockType lockType, bool wait, std::string_view waitMsg) + : fd(fd) +{ + if (wait) { + if (!lockFile(fd, lockType, false)) { + printInfo("%s", waitMsg); + acquired = lockFile(fd, lockType, true); + } + } else + acquired = lockFile(fd, lockType, false); +} + + } diff --git a/src/libstore/pathlocks.hh b/src/libstore/pathlocks.hh index 411da0222..919c8904c 100644 --- a/src/libstore/pathlocks.hh +++ b/src/libstore/pathlocks.hh @@ -35,4 +35,18 @@ public: void setDeletion(bool deletePaths); }; +struct FdLock +{ + int fd; + bool acquired = false; + + FdLock(int fd, LockType lockType, bool wait, std::string_view waitMsg); + + ~FdLock() + { + if (acquired) + lockFile(fd, ltNone, false); + } +}; + } diff --git a/src/libstore/uds-remote-store.cc b/src/libstore/uds-remote-store.cc index 02e81b022..5c38323cd 100644 --- a/src/libstore/uds-remote-store.cc +++ b/src/libstore/uds-remote-store.cc @@ -56,14 +56,7 @@ ref UDSRemoteStore::openConnection() auto conn = make_ref(); /* Connect to a daemon that does the privileged work for us. */ - conn->fd = socket(PF_UNIX, SOCK_STREAM - #ifdef SOCK_CLOEXEC - | SOCK_CLOEXEC - #endif - , 0); - if (!conn->fd) - throw SysError("cannot create Unix domain socket"); - closeOnExec(conn->fd.get()); + conn->fd = createUnixDomainSocket(); nix::connect(conn->fd.get(), path ? *path : settings.nixDaemonSocketFile); diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 563a72c12..5945571b9 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -1670,7 +1670,7 @@ std::unique_ptr createInterruptCallback(std::function } -AutoCloseFD createUnixDomainSocket(const Path & path, mode_t mode) +AutoCloseFD createUnixDomainSocket() { AutoCloseFD fdSocket = socket(PF_UNIX, SOCK_STREAM #ifdef SOCK_CLOEXEC @@ -1679,8 +1679,14 @@ AutoCloseFD createUnixDomainSocket(const Path & path, mode_t mode) , 0); if (!fdSocket) throw SysError("cannot create Unix domain socket"); - closeOnExec(fdSocket.get()); + return fdSocket; +} + + +AutoCloseFD createUnixDomainSocket(const Path & path, mode_t mode) +{ + auto fdSocket = nix::createUnixDomainSocket(); bind(fdSocket.get(), path); diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 29232453f..528d113db 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -571,6 +571,9 @@ extern PathFilter defaultPathFilter; /* Common initialisation performed in child processes. */ void commonChildInit(Pipe & logPipe); +/* Create a Unix domain socket. */ +AutoCloseFD createUnixDomainSocket(); + /* Create a Unix domain socket in listen mode. */ AutoCloseFD createUnixDomainSocket(const Path & path, mode_t mode); diff --git a/tests/ca/build-with-garbage-path.sh b/tests/ca/build-with-garbage-path.sh index 9aa08a899..884cd2802 100755 --- a/tests/ca/build-with-garbage-path.sh +++ b/tests/ca/build-with-garbage-path.sh @@ -8,7 +8,7 @@ requireDaemonNewerThan "2.4pre20210621" # Get the output path of `rootCA`, and put some garbage instead outPath="$(nix-build ./content-addressed.nix -A rootCA --no-out-link)" -nix-store --delete "$outPath" +nix-store --delete $(nix-store -q --referrers-closure "$outPath") touch "$outPath" # The build should correctly remove the garbage and put the expected path instead diff --git a/tests/repair.sh b/tests/repair.sh index 12dcde8ea..1105b446b 100644 --- a/tests/repair.sh +++ b/tests/repair.sh @@ -30,7 +30,7 @@ nix-store --verify-path $path2 chmod u+w $path2 touch $path2/bad -nix-store --delete $(nix-store -qd $path2) +nix-store --delete $(nix-store -q --referrers-closure $(nix-store -qd $path2)) (! nix-store --verify --check-contents --repair) From ff453b06f94b4305694beac8255d9ff51bed1a63 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 19 Aug 2021 22:43:43 +0200 Subject: [PATCH 04/25] Fix auto-gc --- src/libstore/gc.cc | 10 ++++++++-- src/libstore/local-store.cc | 12 +++++++++--- src/libstore/local-store.hh | 2 ++ 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 2d7734729..ff66c3938 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -112,6 +112,9 @@ void LocalStore::addTempRoot(const StorePath & path) } + if (!state->fdGCLock) + state->fdGCLock = openGCLock(); + restart: FdLock gcLock(state->fdGCLock.get(), ltRead, false, ""); @@ -653,8 +656,11 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) if (state.shouldDelete) deletePath(reservedPath); - /* Acquire the global GC root. */ - FdLock gcLock(_state.lock()->fdGCLock.get(), ltWrite, true, "waiting for the big garbage collector lock..."); + /* Acquire the global GC root. Note: we don't use state->fdGCLock + here because then in auto-gc mode, another thread could + downgrade our exclusive lock. */ + auto fdGCLock = openGCLock(); + FdLock gcLock(fdGCLock.get(), ltWrite, true, "waiting for the big garbage collector lock..."); /* Start the server for receiving new roots. */ auto socketPath = stateDir.get() + gcSocketPath; diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index faecdb00b..0e6556e11 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -382,11 +382,16 @@ LocalStore::LocalStore(const Params & params) (select id from Realisations where drvPath = ? and outputName = ?)); )"); } +} + +AutoCloseFD LocalStore::openGCLock() +{ Path fnGCLock = stateDir + "/gc.lock"; - state->fdGCLock = open(fnGCLock.c_str(), O_RDWR | O_CREAT | O_CLOEXEC, 0600); - if (!state->fdGCLock) + auto fdGCLock = open(fnGCLock.c_str(), O_RDWR | O_CREAT | O_CLOEXEC, 0600); + if (!fdGCLock) throw SysError("opening global GC lock '%1%'", fnGCLock); + return fdGCLock; } @@ -1509,7 +1514,8 @@ bool LocalStore::verifyStore(bool checkContents, RepairFlag repair) /* Acquire the global GC lock to get a consistent snapshot of existing and valid paths. */ - FdLock gcLock(_state.lock()->fdGCLock.get(), ltWrite, true, "waiting for the big garbage collector lock..."); + auto fdGCLock = openGCLock(); + FdLock gcLock(fdGCLock.get(), ltRead, true, "waiting for the big garbage collector lock..."); StringSet store; for (auto & i : readDirectory(realStoreDir)) store.insert(i.name); diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 85c6c4495..12c057ec3 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -158,6 +158,8 @@ private: void findTempRoots(Roots & roots, bool censor); + AutoCloseFD openGCLock(); + public: Roots findRoots(bool censor) override; From 262520fcfe2544a7278b6b5967d0d8b605fd89d9 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 20 Aug 2021 11:18:35 +0200 Subject: [PATCH 05/25] Use a thread per connection --- src/libstore/gc.cc | 127 +++++++++++++++++++++----------------------- src/libutil/util.hh | 12 +++++ 2 files changed, 72 insertions(+), 67 deletions(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index ff66c3938..bb76ee084 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -636,6 +636,7 @@ void LocalStore::removeUnusedLinks(const GCState & state) } + void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) { GCState state(options, results); @@ -674,83 +675,75 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) shutdownPipe.create(); std::thread serverThread([&]() { - std::map, std::string>> fdClients; - bool quit = false; + Sync> connections; + std::atomic_bool quit = false; - while (!quit) { + Finally cleanup([&]() { + debug("GC roots server shutting down"); + while (true) { + auto item = remove_begin(*connections.lock()); + if (!item) break; + auto & [fd, thread] = *item; + shutdown(fd, SHUT_RDWR); + thread.join(); + } + }); + + while (true) { std::vector fds; fds.push_back({.fd = shutdownPipe.readSide.get(), .events = POLLIN}); fds.push_back({.fd = fdServer.get(), .events = POLLIN}); - for (auto & i : fdClients) - fds.push_back({.fd = i.first, .events = POLLIN}); auto count = poll(fds.data(), fds.size(), -1); assert(count != -1); - for (auto & fd : fds) { - if (!fd.revents) continue; - if (fd.fd == shutdownPipe.readSide.get()) - /* Parent is asking us to quit. */ - quit = true; - else if (fd.fd == fdServer.get()) { - /* Accept a new connection. */ - assert(fd.revents & POLLIN); - auto fdClient = std::make_unique(accept(fdServer.get(), nullptr, nullptr)); - if (*fdClient) { - auto fd = fdClient->get(); - fdClients.insert({fd, std::make_pair(std::move(fdClient), "")}); + if (fds[0].revents) + /* Parent is asking us to quit. */ + break; + + if (fds[1].revents) { + /* Accept a new connection. */ + assert(fds[1].revents & POLLIN); + AutoCloseFD fdClient = accept(fdServer.get(), nullptr, nullptr); + if (!fdClient) continue; + + /* Process the connection in a separate thread. */ + auto fdClient_ = fdClient.get(); + std::thread clientThread([&, fdClient = std::move(fdClient)]() { + Finally cleanup([&]() { + connections.lock()->erase(fdClient.get()); + }); + + while (true) { + try { + auto path = readLine(fdClient.get()); + auto storePath = maybeParseStorePath(path); + if (storePath) { + debug("got new GC root '%s'", path); + auto hashPart = std::string(storePath->hashPart()); + auto shared(state.shared.lock()); + shared->tempRoots.insert(hashPart); + /* If this path is currently being + deleted, then we have to wait until + deletion is finished to ensure that + the client doesn't start + re-creating it before we're + done. FIXME: ideally we would use a + FD for this so we don't block the + poll loop. */ + while (shared->pending == hashPart) { + debug("synchronising with deletion of path '%s'", path); + shared.wait(state.wakeup); + } + } else + printError("received garbage instead of a root from client"); + writeFull(fdClient.get(), "1", false); + } catch (Error &) { break; } } - } - else { - /* Receive data from a client. */ - auto fdClient = fdClients.find(fd.fd); - assert(fdClient != fdClients.end()); - if (fd.revents & POLLIN) { - char buf[16384]; - auto n = read(fd.fd, buf, sizeof(buf)); - if (n > 0) { - fdClient->second.second.append(buf, n); - /* Split the input into lines. */ - while (true) { - auto p = fdClient->second.second.find('\n'); - if (p == std::string::npos) break; - /* We got a full line. Send ack back - to the client. */ - auto path = fdClient->second.second.substr(0, p); - fdClient->second.second = fdClient->second.second.substr(p + 1); - auto storePath = maybeParseStorePath(path); - if (storePath) { - debug("got new GC root '%s'", path); - auto hashPart = std::string(storePath->hashPart()); - auto shared(state.shared.lock()); - shared->tempRoots.insert(hashPart); - /* If this path is currently being - deleted, then we have to wait - until deletion is finished to - ensure that the client doesn't - start re-creating it before - we're done. FIXME: ideally we - would use a FD for this so we - don't block the poll loop. */ - while (shared->pending == hashPart) { - debug("synchronising with deletion of path '%s'", path); - shared.wait(state.wakeup); - } - } else - printError("received garbage instead of a root from client"); - // This could block, but meh. - try { - writeFull(fd.fd, "1", false); - } catch (SysError &) { } - } - } else if (n == 0) - fdClients.erase(fdClient); - } else - fdClients.erase(fdClient); - } + }); + + connections.lock()->insert({fdClient_, std::move(clientThread)}); } } - - debug("GC roots server shut down"); }); Finally stopServer([&]() { diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 528d113db..76f80f7a4 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -511,6 +511,18 @@ std::optional get(const T & map, const typename T::key_ } +/* Remove and return the first item from a container. */ +template +std::optional remove_begin(T & c) +{ + auto i = c.begin(); + if (i == c.end()) return {}; + auto v = std::move(*i); + c.erase(i); + return v; +} + + template class Callback; From c24b9d68c564713309044c7c15983926556dc234 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 25 Aug 2021 14:05:25 +0200 Subject: [PATCH 06/25] tests/multiple-outputs.sh: Assert empty store --- tests/multiple-outputs.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/multiple-outputs.sh b/tests/multiple-outputs.sh index 0bca12b42..0d45ad35b 100644 --- a/tests/multiple-outputs.sh +++ b/tests/multiple-outputs.sh @@ -76,7 +76,10 @@ if nix-build multiple-outputs.nix -A cyclic --no-out-link; then exit 1 fi +# Do a GC. This should leave an empty store. echo "collecting garbage..." rm $TEST_ROOT/result* nix-store --gc --keep-derivations --keep-outputs nix-store --gc --print-roots +rm -rf $NIX_STORE_DIR/.links +rmdir $NIX_STORE_DIR From dced45f146e5562b30ad8fb83260019a6f270ae5 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 17 Sep 2021 09:10:36 +0200 Subject: [PATCH 07/25] strcpy -> memcpy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Jörg Thalheim --- src/libutil/util.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 5945571b9..e99baf1fc 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -1715,7 +1715,7 @@ void bind(int fd, const std::string & path) std::string base(baseNameOf(path)); if (base.size() + 1 >= sizeof(addr.sun_path)) throw Error("socket path '%s' is too long", base); - strcpy(addr.sun_path, base.c_str()); + memcpy(addr.sun_path, base.c_str(), base.size() + 1); if (bind(fd, (struct sockaddr *) &addr, sizeof(addr)) == -1) throw SysError("cannot bind to socket '%s'", path); _exit(0); @@ -1724,7 +1724,7 @@ void bind(int fd, const std::string & path) if (status != 0) throw Error("cannot bind to socket '%s'", path); } else { - strcpy(addr.sun_path, path.c_str()); + memcpy(addr.sun_path, path.c_str(), path.size() + 1); if (bind(fd, (struct sockaddr *) &addr, sizeof(addr)) == -1) throw SysError("cannot bind to socket '%s'", path); } @@ -1744,7 +1744,7 @@ void connect(int fd, const std::string & path) std::string base(baseNameOf(path)); if (base.size() + 1 >= sizeof(addr.sun_path)) throw Error("socket path '%s' is too long", base); - strcpy(addr.sun_path, base.c_str()); + memcpy(addr.sun_path, base.c_str(), base.size() + 1); if (connect(fd, (struct sockaddr *) &addr, sizeof(addr)) == -1) throw SysError("cannot connect to socket at '%s'", path); _exit(0); @@ -1753,7 +1753,7 @@ void connect(int fd, const std::string & path) if (status != 0) throw Error("cannot connect to socket at '%s'", path); } else { - strcpy(addr.sun_path, path.c_str()); + memcpy(addr.sun_path, path.c_str(), path.size() + 1); if (connect(fd, (struct sockaddr *) &addr, sizeof(addr)) == -1) throw SysError("cannot connect to socket at '%s'", path); } From 1785ba2980c9dd8274dd4bec5c0e46be02544b7e Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 8 Oct 2021 14:58:35 +0200 Subject: [PATCH 08/25] Simplify --- src/libstore/gc.cc | 172 ++++++++++++++++++------------------ src/libstore/local-store.hh | 4 +- 2 files changed, 88 insertions(+), 88 deletions(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index bb76ee084..969066092 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -479,21 +479,36 @@ struct LocalStore::GCState }; +void LocalStore::deleteFromStore(GCState & state, std::string_view baseName) +{ + Path path = storeDir + "/" + std::string(baseName); + Path realPath = realStoreDir + "/" + std::string(baseName); + + printInfo("deleting '%1%'", path); + + state.results.paths.insert(path); + + uint64_t bytesFreed; + deletePath(realPath, bytesFreed); + state.results.bytesFreed += bytesFreed; + + if (state.results.bytesFreed > state.options.maxFreed) { + printInfo("deleted more than %d bytes; stopping", state.options.maxFreed); + throw GCLimitReached(); + } +} + + bool LocalStore::tryToDelete( GCState & state, StorePathSet & visited, - const Path & path, + const StorePath & path, bool recursive) { checkInterrupt(); - auto realPath = realStoreDir + "/" + std::string(baseNameOf(path)); - if (realPath == linksDir) return false; - //Activity act(*logger, lvlDebug, format("considering whether to delete '%1%'") % path); - auto storePath = maybeParseStorePath(path); - /* Wake up any client waiting for deletion of this path to finish. */ Finally releasePending([&]() { @@ -502,89 +517,66 @@ bool LocalStore::tryToDelete( state.wakeup.notify_all(); }); - if (storePath) { + if (!visited.insert(path).second) return true; - if (!visited.insert(*storePath).second) return true; + if (state.alive.count(path)) return false; - if (state.alive.count(*storePath)) return false; + if (state.dead.count(path)) return true; - if (state.dead.count(*storePath)) return true; - - if (state.roots.count(*storePath)) { - debug("cannot delete '%s' because it's a root", path); - state.alive.insert(*storePath); - return false; - } - - if (isValidPath(*storePath)) { - StorePathSet incoming; - - /* Don't delete this path if any of its referrers are alive. */ - queryReferrers(*storePath, incoming); - - /* If keep-derivations is set and this is a derivation, then - don't delete the derivation if any of the outputs are alive. */ - if (state.gcKeepDerivations && storePath->isDerivation()) { - for (auto & [name, maybeOutPath] : queryPartialDerivationOutputMap(*storePath)) - if (maybeOutPath && - isValidPath(*maybeOutPath) && - queryPathInfo(*maybeOutPath)->deriver == *storePath) - incoming.insert(*maybeOutPath); - } - - /* If keep-outputs is set, then don't delete this path if there - are derivers of this path that are not garbage. */ - if (state.gcKeepOutputs) { - auto derivers = queryValidDerivers(*storePath); - for (auto & i : derivers) - incoming.insert(i); - } - - for (auto & i : incoming) - if (i != *storePath - && (recursive || state.options.pathsToDelete.count(i)) - && !tryToDelete(state, visited, printStorePath(i), recursive)) - { - state.alive.insert(*storePath); - return false; - } - } - - { - auto hashPart = std::string(storePath->hashPart()); - auto shared(state.shared.lock()); - if (shared->tempRoots.count(hashPart)) - return false; - shared->pending = hashPart; - } - - state.dead.insert(*storePath); - - if (state.shouldDelete) - invalidatePathChecked(*storePath); + if (state.roots.count(path)) { + debug("cannot delete '%s' because it's a root", printStorePath(path)); + state.alive.insert(path); + return false; } + if (isValidPath(path)) { + StorePathSet incoming; + + /* Don't delete this path if any of its referrers are alive. */ + queryReferrers(path, incoming); + + /* If keep-derivations is set and this is a derivation, then + don't delete the derivation if any of the outputs are + alive. */ + if (state.gcKeepDerivations && path.isDerivation()) { + for (auto & [name, maybeOutPath] : queryPartialDerivationOutputMap(path)) + if (maybeOutPath && + isValidPath(*maybeOutPath) && + queryPathInfo(*maybeOutPath)->deriver == path) + incoming.insert(*maybeOutPath); + } + + /* If keep-outputs is set, then don't delete this path if + there are derivers of this path that are not garbage. */ + if (state.gcKeepOutputs) { + auto derivers = queryValidDerivers(path); + for (auto & i : derivers) + incoming.insert(i); + } + + for (auto & i : incoming) + if (i != path + && (recursive || state.options.pathsToDelete.count(i)) + && !tryToDelete(state, visited, i, recursive)) + { + state.alive.insert(path); + return false; + } + } + + { + auto hashPart = std::string(path.hashPart()); + auto shared(state.shared.lock()); + if (shared->tempRoots.count(hashPart)) + return false; + shared->pending = hashPart; + } + + state.dead.insert(path); + if (state.shouldDelete) { - Path realPath = realStoreDir + "/" + std::string(baseNameOf(path)); - - struct stat st; - if (lstat(realPath.c_str(), &st)) { - if (errno == ENOENT) return true; - throw SysError("getting status of %1%", realPath); - } - - printInfo("deleting '%1%'", path); - - state.results.paths.insert(path); - - uint64_t bytesFreed; - deletePath(realPath, bytesFreed); - state.results.bytesFreed += bytesFreed; - - if (state.results.bytesFreed > state.options.maxFreed) { - printInfo("deleted more than %d bytes; stopping", state.options.maxFreed); - throw GCLimitReached(); - } + invalidatePathChecked(path); + deleteFromStore(state, path.to_string()); } return true; @@ -777,7 +769,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) for (auto & i : options.pathsToDelete) { StorePathSet visited; - if (!tryToDelete(state, visited, printStorePath(i), false)) + if (!tryToDelete(state, visited, i, false)) throw Error( "cannot delete path '%1%' since it is still alive. " "To find out why, use: " @@ -799,14 +791,20 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) /* Read the store and delete all paths that are invalid or unreachable. We don't use readDirectory() here so that GCing can start faster. */ + auto linksName = baseNameOf(linksDir); Paths entries; struct dirent * dirent; while (errno = 0, dirent = readdir(dir.get())) { checkInterrupt(); string name = dirent->d_name; - if (name == "." || name == "..") continue; - StorePathSet visited; - tryToDelete(state, visited, storeDir + "/" + name, true); + if (name == "." || name == ".." || name == linksName) continue; + + if (auto storePath = maybeParseStorePath(storeDir + "/" + name)) { + StorePathSet visited; + tryToDelete(state, visited, *storePath, true); + } else + deleteFromStore(state, name); + } } catch (GCLimitReached & e) { } diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 12c057ec3..08bbc1a7d 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -243,9 +243,11 @@ private: bool tryToDelete( GCState & state, StorePathSet & visited, - const Path & path, + const StorePath & path, bool recursive); + void deleteFromStore(GCState & state, std::string_view baseName); + void findRoots(const Path & path, unsigned char type, Roots & roots); void findRootsNoTemp(Roots & roots, bool censor); From e31a48366f19d5fa452df6273e817a4ea5516e50 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 8 Oct 2021 16:57:25 +0200 Subject: [PATCH 09/25] Fix referrers test This test broke the assumption that the hash parts of store paths are unique. --- tests/referrers.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/referrers.sh b/tests/referrers.sh index 81323c280..7ede707b8 100644 --- a/tests/referrers.sh +++ b/tests/referrers.sh @@ -14,9 +14,9 @@ echo "making registration..." set +x for ((n = 0; n < $max; n++)); do - storePath=$NIX_STORE_DIR/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-$n + storePath=$NIX_STORE_DIR/$(printf "%05d\n" $n)aaaaaaaaaaaaaaaaaaaaaaaaaaa-$n echo -n > $storePath - ref2=$NIX_STORE_DIR/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-$((n+1)) + ref2=$NIX_STORE_DIR/$(printf "%05d\n" $((n+1)))aaaaaaaaaaaaaaaaaaaaaaaaaaa-$((n+1)) if test $((n+1)) = $max; then ref2=$reference fi From 35c98a59c5f09d8ca376ac809e87d14ff2fcbde1 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 8 Oct 2021 16:58:19 +0200 Subject: [PATCH 10/25] Fix GC when there are cycles in the referrers graph (where "referrers" includes the reverse of derivation outputs and derivers). Now we do a full traversal to look if we can reach any root. If not, all paths reached can be deleted. --- src/libstore/gc.cc | 74 +++++++++++++++++++++---------------- src/libstore/local-store.hh | 5 +-- 2 files changed, 45 insertions(+), 34 deletions(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 969066092..4123a90be 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -499,34 +499,29 @@ void LocalStore::deleteFromStore(GCState & state, std::string_view baseName) } -bool LocalStore::tryToDelete( +bool LocalStore::canReachRoot( GCState & state, StorePathSet & visited, - const StorePath & path, - bool recursive) + const StorePath & path) { checkInterrupt(); //Activity act(*logger, lvlDebug, format("considering whether to delete '%1%'") % path); - /* Wake up any client waiting for deletion of this path to - finish. */ - Finally releasePending([&]() { - auto shared(state.shared.lock()); - shared->pending.reset(); - state.wakeup.notify_all(); - }); + if (state.options.action == GCOptions::gcDeleteSpecific + && !state.options.pathsToDelete.count(path)) + return true; - if (!visited.insert(path).second) return true; + if (!visited.insert(path).second) return false; - if (state.alive.count(path)) return false; + if (state.alive.count(path)) return true; - if (state.dead.count(path)) return true; + if (state.dead.count(path)) return false; if (state.roots.count(path)) { debug("cannot delete '%s' because it's a root", printStorePath(path)); state.alive.insert(path); - return false; + return true; } if (isValidPath(path)) { @@ -555,12 +550,9 @@ bool LocalStore::tryToDelete( } for (auto & i : incoming) - if (i != path - && (recursive || state.options.pathsToDelete.count(i)) - && !tryToDelete(state, visited, i, recursive)) - { + if (i != path && canReachRoot(state, visited, i)) { state.alive.insert(path); - return false; + return true; } } @@ -568,18 +560,11 @@ bool LocalStore::tryToDelete( auto hashPart = std::string(path.hashPart()); auto shared(state.shared.lock()); if (shared->tempRoots.count(hashPart)) - return false; + return true; shared->pending = hashPart; } - state.dead.insert(path); - - if (state.shouldDelete) { - invalidatePathChecked(path); - deleteFromStore(state, path.to_string()); - } - - return true; + return false; } @@ -628,7 +613,6 @@ void LocalStore::removeUnusedLinks(const GCState & state) } - void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) { GCState state(options, results); @@ -769,12 +753,21 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) for (auto & i : options.pathsToDelete) { StorePathSet visited; - if (!tryToDelete(state, visited, i, false)) + + if (canReachRoot(state, visited, i)) throw Error( "cannot delete path '%1%' since it is still alive. " "To find out why, use: " "nix-store --query --roots", printStorePath(i)); + + auto sorted = topoSortPaths(visited); + for (auto & path : sorted) { + if (state.dead.count(path)) continue; + invalidatePathChecked(path); + deleteFromStore(state, path.to_string()); + state.dead.insert(path); + } } } else if (options.maxFreed > 0) { @@ -801,7 +794,26 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) if (auto storePath = maybeParseStorePath(storeDir + "/" + name)) { StorePathSet visited; - tryToDelete(state, visited, *storePath, true); + + /* Wake up any GC client waiting for deletion of + the paths in 'visited' to finish. */ + Finally releasePending([&]() { + auto shared(state.shared.lock()); + shared->pending.reset(); + state.wakeup.notify_all(); + }); + + if (!canReachRoot(state, visited, *storePath)) { + auto sorted = topoSortPaths(visited); + for (auto & path : sorted) { + if (state.dead.count(path)) continue; + if (state.shouldDelete) { + invalidatePathChecked(path); + deleteFromStore(state, path.to_string()); + } + state.dead.insert(path); + } + } } else deleteFromStore(state, name); diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 08bbc1a7d..8629e1528 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -240,11 +240,10 @@ private: struct GCState; - bool tryToDelete( + bool canReachRoot( GCState & state, StorePathSet & visited, - const StorePath & path, - bool recursive); + const StorePath & path); void deleteFromStore(GCState & state, std::string_view baseName); From 09b14ea97a7883114baa5878da163d9e403396d6 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 14 Oct 2021 10:04:13 +0200 Subject: [PATCH 11/25] Cleanup --- src/libstore/misc.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/libstore/misc.cc b/src/libstore/misc.cc index f184dd857..32786e963 100644 --- a/src/libstore/misc.cc +++ b/src/libstore/misc.cc @@ -239,12 +239,11 @@ StorePaths Store::topoSortPaths(const StorePathSet & paths) { return topoSort(paths, {[&](const StorePath & path) { - StorePathSet references; try { - references = queryPathInfo(path)->references; + return queryPathInfo(path)->references; } catch (InvalidPath &) { + return StorePathSet(); } - return references; }}, {[&](const StorePath & path, const StorePath & parent) { return BuildError( From eab934cb2a23595a7ac7c8a72373cd8096b606a9 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 14 Oct 2021 12:31:21 +0200 Subject: [PATCH 12/25] Make the canReachRoots() traversal non-recursive --- src/libstore/gc.cc | 210 ++++++++++++++++++------------------ src/libstore/local-store.hh | 5 - src/libutil/util.hh | 11 ++ 3 files changed, 115 insertions(+), 111 deletions(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 4123a90be..8a34f0c9b 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -499,75 +499,6 @@ void LocalStore::deleteFromStore(GCState & state, std::string_view baseName) } -bool LocalStore::canReachRoot( - GCState & state, - StorePathSet & visited, - const StorePath & path) -{ - checkInterrupt(); - - //Activity act(*logger, lvlDebug, format("considering whether to delete '%1%'") % path); - - if (state.options.action == GCOptions::gcDeleteSpecific - && !state.options.pathsToDelete.count(path)) - return true; - - if (!visited.insert(path).second) return false; - - if (state.alive.count(path)) return true; - - if (state.dead.count(path)) return false; - - if (state.roots.count(path)) { - debug("cannot delete '%s' because it's a root", printStorePath(path)); - state.alive.insert(path); - return true; - } - - if (isValidPath(path)) { - StorePathSet incoming; - - /* Don't delete this path if any of its referrers are alive. */ - queryReferrers(path, incoming); - - /* If keep-derivations is set and this is a derivation, then - don't delete the derivation if any of the outputs are - alive. */ - if (state.gcKeepDerivations && path.isDerivation()) { - for (auto & [name, maybeOutPath] : queryPartialDerivationOutputMap(path)) - if (maybeOutPath && - isValidPath(*maybeOutPath) && - queryPathInfo(*maybeOutPath)->deriver == path) - incoming.insert(*maybeOutPath); - } - - /* If keep-outputs is set, then don't delete this path if - there are derivers of this path that are not garbage. */ - if (state.gcKeepOutputs) { - auto derivers = queryValidDerivers(path); - for (auto & i : derivers) - incoming.insert(i); - } - - for (auto & i : incoming) - if (i != path && canReachRoot(state, visited, i)) { - state.alive.insert(path); - return true; - } - } - - { - auto hashPart = std::string(path.hashPart()); - auto shared(state.shared.lock()); - if (shared->tempRoots.count(hashPart)) - return true; - shared->pending = hashPart; - } - - return false; -} - - /* Unlink all files in /nix/store/.links that have a link count of 1, which indicates that there are no other links and so they can be safely deleted. FIXME: race condition with optimisePath(): we @@ -746,28 +677,115 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) state.roots.insert(root.first); } - /* Now either delete all garbage paths, or just the specified - paths (for gcDeleteSpecific). */ + /* Helper function that visits all paths reachable from `start` + via the referrers edges and optionally derivers and derivation + output edges. If any of those paths is a root, then we cannot + delete this path. */ + auto deleteReferrersClosure = [&](const StorePath & start) { + StorePathSet visited; + std::queue todo; + /* Wake up any GC client waiting for deletion of the paths in + 'visited' to finish. */ + Finally releasePending([&]() { + auto shared(state.shared.lock()); + shared->pending.reset(); + state.wakeup.notify_all(); + }); + + auto enqueue = [&](const StorePath & path) { + if (visited.insert(path).second) + todo.push(path); + }; + + enqueue(start); + + while (auto path = pop(todo)) { + checkInterrupt(); + + /* Bail out if we've previously discovered that this path + is alive. */ + if (state.alive.count(*path)) { + state.alive.insert(start); + return; + } + + /* If we've previously deleted this path, we don't have to + handle it again. */ + if (state.dead.count(*path)) continue; + + /* If this is a root, bail out. */ + if (state.roots.count(*path)) { + debug("cannot delete '%s' because it's a root", printStorePath(*path)); + state.alive.insert(*path); + state.alive.insert(start); + return; + } + + if (state.options.action == GCOptions::gcDeleteSpecific + && !state.options.pathsToDelete.count(*path)) + return; + + { + auto hashPart = std::string(path->hashPart()); + auto shared(state.shared.lock()); + if (shared->tempRoots.count(hashPart)) { + debug("cannot delete '%s' because it's a temporary root", printStorePath(*path)); + state.alive.insert(*path); + state.alive.insert(start); + return; + } + shared->pending = hashPart; + } + + if (isValidPath(*path)) { + + /* Visit the referrers of this path. */ + StorePathSet referrers; + queryReferrers(*path, referrers); + for (auto & p : referrers) + enqueue(p); + + /* If keep-derivations is set and this is a + derivation, then visit the derivation outputs. */ + if (state.gcKeepDerivations && path->isDerivation()) { + for (auto & [name, maybeOutPath] : queryPartialDerivationOutputMap(*path)) + if (maybeOutPath && + isValidPath(*maybeOutPath) && + queryPathInfo(*maybeOutPath)->deriver == *path) + enqueue(*maybeOutPath); + } + + /* If keep-outputs is set, then visit the derivers. */ + if (state.gcKeepOutputs) { + auto derivers = queryValidDerivers(*path); + for (auto & i : derivers) + enqueue(i); + } + } + } + + for (auto & path : topoSortPaths(visited)) { + if (!state.dead.insert(path).second) continue; + if (state.shouldDelete) { + invalidatePathChecked(path); + deleteFromStore(state, path.to_string()); + } + } + }; + + /* Either delete all garbage paths, or just the specified + paths (for gcDeleteSpecific). */ if (options.action == GCOptions::gcDeleteSpecific) { for (auto & i : options.pathsToDelete) { - StorePathSet visited; - - if (canReachRoot(state, visited, i)) + deleteReferrersClosure(i); + if (!state.dead.count(i)) throw Error( - "cannot delete path '%1%' since it is still alive. " + "Cannot delete path '%1%' since it is still alive. " "To find out why, use: " "nix-store --query --roots", printStorePath(i)); - - auto sorted = topoSortPaths(visited); - for (auto & path : sorted) { - if (state.dead.count(path)) continue; - invalidatePathChecked(path); - deleteFromStore(state, path.to_string()); - state.dead.insert(path); - } } } else if (options.maxFreed > 0) { @@ -792,29 +810,9 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) string name = dirent->d_name; if (name == "." || name == ".." || name == linksName) continue; - if (auto storePath = maybeParseStorePath(storeDir + "/" + name)) { - StorePathSet visited; - - /* Wake up any GC client waiting for deletion of - the paths in 'visited' to finish. */ - Finally releasePending([&]() { - auto shared(state.shared.lock()); - shared->pending.reset(); - state.wakeup.notify_all(); - }); - - if (!canReachRoot(state, visited, *storePath)) { - auto sorted = topoSortPaths(visited); - for (auto & path : sorted) { - if (state.dead.count(path)) continue; - if (state.shouldDelete) { - invalidatePathChecked(path); - deleteFromStore(state, path.to_string()); - } - state.dead.insert(path); - } - } - } else + if (auto storePath = maybeParseStorePath(storeDir + "/" + name)) + deleteReferrersClosure(*storePath); + else deleteFromStore(state, name); } diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 8629e1528..21a4e60cf 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -240,11 +240,6 @@ private: struct GCState; - bool canReachRoot( - GCState & state, - StorePathSet & visited, - const StorePath & path); - void deleteFromStore(GCState & state, std::string_view baseName); void findRoots(const Path & path, unsigned char type, Roots & roots); diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 76f80f7a4..485ff4153 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -523,6 +523,17 @@ std::optional remove_begin(T & c) } +/* Remove and return the first item from a container. */ +template +std::optional pop(T & c) +{ + if (c.empty()) return {}; + auto v = std::move(c.front()); + c.pop(); + return v; +} + + template class Callback; From 0be8cc1466f317e33977590510bac4b18471f0ce Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 14 Oct 2021 13:28:22 +0200 Subject: [PATCH 13/25] pathInfoCache: Use the entire base name as the cache key This fixes a bug in the garbage collector where if a path /nix/store/abcd-foo is valid, but we do a isValidPath("/nix/store/abcd-foo.lock") first, then a negative entry for /nix/store/abcd is added to pathInfoCache, so /nix/store/abcd-foo is subsequently considered invalid and deleted. --- src/libstore/binary-cache-store.cc | 8 ++++---- src/libstore/local-store.cc | 4 ++-- src/libstore/store-api.cc | 26 ++++++++++---------------- src/libstore/store-api.hh | 1 - tests/gc.sh | 11 +++++++++++ 5 files changed, 27 insertions(+), 23 deletions(-) diff --git a/src/libstore/binary-cache-store.cc b/src/libstore/binary-cache-store.cc index 8fce94264..943132754 100644 --- a/src/libstore/binary-cache-store.cc +++ b/src/libstore/binary-cache-store.cc @@ -111,15 +111,15 @@ void BinaryCacheStore::writeNarInfo(ref narInfo) upsertFile(narInfoFile, narInfo->to_string(*this), "text/x-nix-narinfo"); - std::string hashPart(narInfo->path.hashPart()); - { auto state_(state.lock()); - state_->pathInfoCache.upsert(hashPart, PathInfoCacheValue { .value = std::shared_ptr(narInfo) }); + state_->pathInfoCache.upsert( + std::string(narInfo->path.to_string()), + PathInfoCacheValue { .value = std::shared_ptr(narInfo) }); } if (diskCache) - diskCache->upsertNarInfo(getUri(), hashPart, std::shared_ptr(narInfo)); + diskCache->upsertNarInfo(getUri(), std::string(narInfo->path.hashPart()), std::shared_ptr(narInfo)); } AutoCloseFD openFile(const Path & path) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 0e6556e11..c853e9a9c 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -834,7 +834,7 @@ uint64_t LocalStore::addValidPath(State & state, { auto state_(Store::state.lock()); - state_->pathInfoCache.upsert(std::string(info.path.hashPart()), + state_->pathInfoCache.upsert(std::string(info.path.to_string()), PathInfoCacheValue{ .value = std::make_shared(info) }); } @@ -1207,7 +1207,7 @@ void LocalStore::invalidatePath(State & state, const StorePath & path) { auto state_(Store::state.lock()); - state_->pathInfoCache.erase(std::string(path.hashPart())); + state_->pathInfoCache.erase(std::string(path.to_string())); } } diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index b5ff3dccf..10aa1035e 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -414,11 +414,9 @@ StorePathSet Store::queryDerivationOutputs(const StorePath & path) bool Store::isValidPath(const StorePath & storePath) { - std::string hashPart(storePath.hashPart()); - { auto state_(state.lock()); - auto res = state_->pathInfoCache.get(hashPart); + auto res = state_->pathInfoCache.get(std::string(storePath.to_string())); if (res && res->isKnownNow()) { stats.narInfoReadAverted++; return res->didExist(); @@ -426,11 +424,11 @@ bool Store::isValidPath(const StorePath & storePath) } if (diskCache) { - auto res = diskCache->lookupNarInfo(getUri(), hashPart); + auto res = diskCache->lookupNarInfo(getUri(), std::string(storePath.hashPart())); if (res.first != NarInfoDiskCache::oUnknown) { stats.narInfoReadAverted++; auto state_(state.lock()); - state_->pathInfoCache.upsert(hashPart, + state_->pathInfoCache.upsert(std::string(storePath.to_string()), res.first == NarInfoDiskCache::oInvalid ? PathInfoCacheValue{} : PathInfoCacheValue { .value = res.second }); return res.first == NarInfoDiskCache::oValid; } @@ -440,7 +438,7 @@ bool Store::isValidPath(const StorePath & storePath) if (diskCache && !valid) // FIXME: handle valid = true case. - diskCache->upsertNarInfo(getUri(), hashPart, 0); + diskCache->upsertNarInfo(getUri(), std::string(storePath.hashPart()), 0); return valid; } @@ -487,13 +485,11 @@ static bool goodStorePath(const StorePath & expected, const StorePath & actual) void Store::queryPathInfo(const StorePath & storePath, Callback> callback) noexcept { - std::string hashPart; + auto hashPart = std::string(storePath.hashPart()); try { - hashPart = storePath.hashPart(); - { - auto res = state.lock()->pathInfoCache.get(hashPart); + auto res = state.lock()->pathInfoCache.get(std::string(storePath.to_string())); if (res && res->isKnownNow()) { stats.narInfoReadAverted++; if (!res->didExist()) @@ -508,7 +504,7 @@ void Store::queryPathInfo(const StorePath & storePath, stats.narInfoReadAverted++; { auto state_(state.lock()); - state_->pathInfoCache.upsert(hashPart, + state_->pathInfoCache.upsert(std::string(storePath.to_string()), res.first == NarInfoDiskCache::oInvalid ? PathInfoCacheValue{} : PathInfoCacheValue{ .value = res.second }); if (res.first == NarInfoDiskCache::oInvalid || !goodStorePath(storePath, res.second->path)) @@ -523,7 +519,7 @@ void Store::queryPathInfo(const StorePath & storePath, auto callbackPtr = std::make_shared(std::move(callback)); queryPathInfoUncached(storePath, - {[this, storePathS{printStorePath(storePath)}, hashPart, callbackPtr](std::future> fut) { + {[this, storePath, hashPart, callbackPtr](std::future> fut) { try { auto info = fut.get(); @@ -533,14 +529,12 @@ void Store::queryPathInfo(const StorePath & storePath, { auto state_(state.lock()); - state_->pathInfoCache.upsert(hashPart, PathInfoCacheValue { .value = info }); + state_->pathInfoCache.upsert(std::string(storePath.to_string()), PathInfoCacheValue { .value = info }); } - auto storePath = parseStorePath(storePathS); - if (!info || !goodStorePath(storePath, info->path)) { stats.narInfoMissing++; - throw InvalidPath("path '%s' is not valid", storePathS); + throw InvalidPath("path '%s' is not valid", printStorePath(storePath)); } (*callbackPtr)(ref(info)); diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 35461b76d..7d02340df 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -232,7 +232,6 @@ protected: struct State { - // FIXME: fix key LRUCache pathInfoCache; }; diff --git a/tests/gc.sh b/tests/gc.sh index cf0e2c32d..33d627dae 100644 --- a/tests/gc.sh +++ b/tests/gc.sh @@ -1,5 +1,7 @@ source common.sh +clearStore + drvPath=$(nix-instantiate dependencies.nix) outPath=$(nix-store -rvv "$drvPath") @@ -23,6 +25,11 @@ test -e $inUse if nix-store --delete $outPath; then false; fi test -e $outPath +for i in $NIX_STORE_DIR/*; do + touch $i.lock + touch $i.chroot +done + nix-collect-garbage # Check that the root and its dependencies haven't been deleted. @@ -38,3 +45,7 @@ nix-collect-garbage # Check that the output has been GC'd. if test -e $outPath/foobar; then false; fi + +# Check that the store is empty. +rmdir $NIX_STORE_DIR/.links +rmdir $NIX_STORE_DIR From 0317ffdad3b2137d35e501b38155437e2ec5d0cd Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 14 Oct 2021 13:34:48 +0200 Subject: [PATCH 14/25] Move deleteFromStore() --- src/libstore/gc.cc | 49 +++++++++++++++++++------------------ src/libstore/local-store.hh | 2 -- 2 files changed, 25 insertions(+), 26 deletions(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 8a34f0c9b..aa0087118 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -479,26 +479,6 @@ struct LocalStore::GCState }; -void LocalStore::deleteFromStore(GCState & state, std::string_view baseName) -{ - Path path = storeDir + "/" + std::string(baseName); - Path realPath = realStoreDir + "/" + std::string(baseName); - - printInfo("deleting '%1%'", path); - - state.results.paths.insert(path); - - uint64_t bytesFreed; - deletePath(realPath, bytesFreed); - state.results.bytesFreed += bytesFreed; - - if (state.results.bytesFreed > state.options.maxFreed) { - printInfo("deleted more than %d bytes; stopping", state.options.maxFreed); - throw GCLimitReached(); - } -} - - /* Unlink all files in /nix/store/.links that have a link count of 1, which indicates that there are no other links and so they can be safely deleted. FIXME: race condition with optimisePath(): we @@ -677,10 +657,31 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) state.roots.insert(root.first); } + /* Helper function that deletes a path from the store and throws + GCLimitReached if we've deleted enough garbage. */ + auto deleteFromStore = [&](std::string_view baseName) + { + Path path = storeDir + "/" + std::string(baseName); + Path realPath = realStoreDir + "/" + std::string(baseName); + + printInfo("deleting '%1%'", path); + + state.results.paths.insert(path); + + uint64_t bytesFreed; + deletePath(realPath, bytesFreed); + state.results.bytesFreed += bytesFreed; + + if (state.results.bytesFreed > state.options.maxFreed) { + printInfo("deleted more than %d bytes; stopping", state.options.maxFreed); + throw GCLimitReached(); + } + }; + /* Helper function that visits all paths reachable from `start` via the referrers edges and optionally derivers and derivation - output edges. If any of those paths is a root, then we cannot - delete this path. */ + output edges. If none of those paths are roots, then all + visited paths are garbage and are deleted. */ auto deleteReferrersClosure = [&](const StorePath & start) { StorePathSet visited; std::queue todo; @@ -769,7 +770,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) if (!state.dead.insert(path).second) continue; if (state.shouldDelete) { invalidatePathChecked(path); - deleteFromStore(state, path.to_string()); + deleteFromStore(path.to_string()); } } }; @@ -813,7 +814,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) if (auto storePath = maybeParseStorePath(storeDir + "/" + name)) deleteReferrersClosure(*storePath); else - deleteFromStore(state, name); + deleteFromStore(name); } } catch (GCLimitReached & e) { diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 21a4e60cf..15475f402 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -240,8 +240,6 @@ private: struct GCState; - void deleteFromStore(GCState & state, std::string_view baseName); - void findRoots(const Path & path, unsigned char type, Roots & roots); void findRootsNoTemp(Roots & roots, bool censor); From 0154fa30cf2a6da63e69f7488cec9e289f57cc15 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 14 Oct 2021 13:52:49 +0200 Subject: [PATCH 15/25] Remove GCState --- src/libstore/gc.cc | 196 ++++++++++++++++-------------------- src/libstore/local-store.hh | 4 - 2 files changed, 87 insertions(+), 113 deletions(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index aa0087118..800a3fd19 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -448,16 +448,13 @@ void LocalStore::findRuntimeRoots(Roots & roots, bool censor) struct GCLimitReached { }; -struct LocalStore::GCState +void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) { - const GCOptions & options; - GCResults & results; - StorePathSet roots; - StorePathSet dead; - StorePathSet alive; - bool gcKeepOutputs; - bool gcKeepDerivations; - bool shouldDelete; + bool shouldDelete = options.action == GCOptions::gcDeleteDead || options.action == GCOptions::gcDeleteSpecific; + bool gcKeepOutputs = settings.gcKeepOutputs; + bool gcKeepDerivations = settings.gcKeepDerivations; + + StorePathSet roots, dead, alive; struct Shared { @@ -470,81 +467,23 @@ struct LocalStore::GCState std::optional pending; }; - Sync shared; + Sync _shared; std::condition_variable wakeup; - GCState(const GCOptions & options, GCResults & results) - : options(options), results(results) { } -}; - - -/* Unlink all files in /nix/store/.links that have a link count of 1, - which indicates that there are no other links and so they can be - safely deleted. FIXME: race condition with optimisePath(): we - might see a link count of 1 just before optimisePath() increases - the link count. */ -void LocalStore::removeUnusedLinks(const GCState & state) -{ - AutoCloseDir dir(opendir(linksDir.c_str())); - if (!dir) throw SysError("opening directory '%1%'", linksDir); - - int64_t actualSize = 0, unsharedSize = 0; - - struct dirent * dirent; - while (errno = 0, dirent = readdir(dir.get())) { - checkInterrupt(); - string name = dirent->d_name; - if (name == "." || name == "..") continue; - Path path = linksDir + "/" + name; - - auto st = lstat(path); - - if (st.st_nlink != 1) { - actualSize += st.st_size; - unsharedSize += (st.st_nlink - 1) * st.st_size; - continue; - } - - printMsg(lvlTalkative, format("deleting unused link '%1%'") % path); - - if (unlink(path.c_str()) == -1) - throw SysError("deleting '%1%'", path); - - state.results.bytesFreed += st.st_size; - } - - struct stat st; - if (stat(linksDir.c_str(), &st) == -1) - throw SysError("statting '%1%'", linksDir); - int64_t overhead = st.st_blocks * 512ULL; - - printInfo("note: currently hard linking saves %.2f MiB", - ((unsharedSize - actualSize - overhead) / (1024.0 * 1024.0))); -} - - -void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) -{ - GCState state(options, results); - state.gcKeepOutputs = settings.gcKeepOutputs; - state.gcKeepDerivations = settings.gcKeepDerivations; - /* Using `--ignore-liveness' with `--delete' can have unintended consequences if `keep-outputs' or `keep-derivations' are true (the garbage collector will recurse into deleting the outputs or derivers, respectively). So disable them. */ if (options.action == GCOptions::gcDeleteSpecific && options.ignoreLiveness) { - state.gcKeepOutputs = false; - state.gcKeepDerivations = false; + gcKeepOutputs = false; + gcKeepDerivations = false; } - state.shouldDelete = options.action == GCOptions::gcDeleteDead || options.action == GCOptions::gcDeleteSpecific; - - if (state.shouldDelete) + if (shouldDelete) deletePath(reservedPath); - /* Acquire the global GC root. Note: we don't use state->fdGCLock + /* Acquire the global GC root. Note: we don't use fdGCLock here because then in auto-gc mode, another thread could downgrade our exclusive lock. */ auto fdGCLock = openGCLock(); @@ -607,7 +546,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) if (storePath) { debug("got new GC root '%s'", path); auto hashPart = std::string(storePath->hashPart()); - auto shared(state.shared.lock()); + auto shared(_shared.lock()); shared->tempRoots.insert(hashPart); /* If this path is currently being deleted, then we have to wait until @@ -619,7 +558,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) poll loop. */ while (shared->pending == hashPart) { debug("synchronising with deletion of path '%s'", path); - shared.wait(state.wakeup); + shared.wait(wakeup); } } else printError("received garbage instead of a root from client"); @@ -635,7 +574,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) Finally stopServer([&]() { writeFull(shutdownPipe.writeSide.get(), "x", false); - state.wakeup.notify_all(); + wakeup.notify_all(); if (serverThread.joinable()) serverThread.join(); }); @@ -646,15 +585,15 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) if (!options.ignoreLiveness) findRootsNoTemp(rootMap, true); - for (auto & i : rootMap) state.roots.insert(i.first); + for (auto & i : rootMap) roots.insert(i.first); /* Read the temporary roots created before we acquired the global GC root. Any new roots will be sent to our socket. */ Roots tempRoots; findTempRoots(tempRoots, true); for (auto & root : tempRoots) { - state.shared.lock()->tempRoots.insert(std::string(root.first.hashPart())); - state.roots.insert(root.first); + _shared.lock()->tempRoots.insert(std::string(root.first.hashPart())); + roots.insert(root.first); } /* Helper function that deletes a path from the store and throws @@ -666,14 +605,14 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) printInfo("deleting '%1%'", path); - state.results.paths.insert(path); + results.paths.insert(path); uint64_t bytesFreed; deletePath(realPath, bytesFreed); - state.results.bytesFreed += bytesFreed; + results.bytesFreed += bytesFreed; - if (state.results.bytesFreed > state.options.maxFreed) { - printInfo("deleted more than %d bytes; stopping", state.options.maxFreed); + if (results.bytesFreed > options.maxFreed) { + printInfo("deleted more than %d bytes; stopping", options.maxFreed); throw GCLimitReached(); } }; @@ -689,9 +628,9 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) /* Wake up any GC client waiting for deletion of the paths in 'visited' to finish. */ Finally releasePending([&]() { - auto shared(state.shared.lock()); + auto shared(_shared.lock()); shared->pending.reset(); - state.wakeup.notify_all(); + wakeup.notify_all(); }); auto enqueue = [&](const StorePath & path) { @@ -706,34 +645,34 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) /* Bail out if we've previously discovered that this path is alive. */ - if (state.alive.count(*path)) { - state.alive.insert(start); + if (alive.count(*path)) { + alive.insert(start); return; } /* If we've previously deleted this path, we don't have to handle it again. */ - if (state.dead.count(*path)) continue; + if (dead.count(*path)) continue; /* If this is a root, bail out. */ - if (state.roots.count(*path)) { + if (roots.count(*path)) { debug("cannot delete '%s' because it's a root", printStorePath(*path)); - state.alive.insert(*path); - state.alive.insert(start); + alive.insert(*path); + alive.insert(start); return; } - if (state.options.action == GCOptions::gcDeleteSpecific - && !state.options.pathsToDelete.count(*path)) + if (options.action == GCOptions::gcDeleteSpecific + && !options.pathsToDelete.count(*path)) return; { auto hashPart = std::string(path->hashPart()); - auto shared(state.shared.lock()); + auto shared(_shared.lock()); if (shared->tempRoots.count(hashPart)) { debug("cannot delete '%s' because it's a temporary root", printStorePath(*path)); - state.alive.insert(*path); - state.alive.insert(start); + alive.insert(*path); + alive.insert(start); return; } shared->pending = hashPart; @@ -749,7 +688,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) /* If keep-derivations is set and this is a derivation, then visit the derivation outputs. */ - if (state.gcKeepDerivations && path->isDerivation()) { + if (gcKeepDerivations && path->isDerivation()) { for (auto & [name, maybeOutPath] : queryPartialDerivationOutputMap(*path)) if (maybeOutPath && isValidPath(*maybeOutPath) && @@ -758,7 +697,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) } /* If keep-outputs is set, then visit the derivers. */ - if (state.gcKeepOutputs) { + if (gcKeepOutputs) { auto derivers = queryValidDerivers(*path); for (auto & i : derivers) enqueue(i); @@ -767,8 +706,8 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) } for (auto & path : topoSortPaths(visited)) { - if (!state.dead.insert(path).second) continue; - if (state.shouldDelete) { + if (!dead.insert(path).second) continue; + if (shouldDelete) { invalidatePathChecked(path); deleteFromStore(path.to_string()); } @@ -781,7 +720,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) for (auto & i : options.pathsToDelete) { deleteReferrersClosure(i); - if (!state.dead.count(i)) + if (!dead.count(i)) throw Error( "Cannot delete path '%1%' since it is still alive. " "To find out why, use: " @@ -791,7 +730,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) } else if (options.maxFreed > 0) { - if (state.shouldDelete) + if (shouldDelete) printInfo("deleting garbage..."); else printInfo("determining live/dead paths..."); @@ -821,22 +760,61 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) } } - if (state.options.action == GCOptions::gcReturnLive) { - for (auto & i : state.alive) - state.results.paths.insert(printStorePath(i)); + if (options.action == GCOptions::gcReturnLive) { + for (auto & i : alive) + results.paths.insert(printStorePath(i)); return; } - if (state.options.action == GCOptions::gcReturnDead) { - for (auto & i : state.dead) - state.results.paths.insert(printStorePath(i)); + if (options.action == GCOptions::gcReturnDead) { + for (auto & i : dead) + results.paths.insert(printStorePath(i)); return; } - /* Clean up the links directory. */ + /* Unlink all files in /nix/store/.links that have a link count of 1, + which indicates that there are no other links and so they can be + safely deleted. FIXME: race condition with optimisePath(): we + might see a link count of 1 just before optimisePath() increases + the link count. */ if (options.action == GCOptions::gcDeleteDead || options.action == GCOptions::gcDeleteSpecific) { printInfo("deleting unused links..."); - removeUnusedLinks(state); + + AutoCloseDir dir(opendir(linksDir.c_str())); + if (!dir) throw SysError("opening directory '%1%'", linksDir); + + int64_t actualSize = 0, unsharedSize = 0; + + struct dirent * dirent; + while (errno = 0, dirent = readdir(dir.get())) { + checkInterrupt(); + string name = dirent->d_name; + if (name == "." || name == "..") continue; + Path path = linksDir + "/" + name; + + auto st = lstat(path); + + if (st.st_nlink != 1) { + actualSize += st.st_size; + unsharedSize += (st.st_nlink - 1) * st.st_size; + continue; + } + + printMsg(lvlTalkative, format("deleting unused link '%1%'") % path); + + if (unlink(path.c_str()) == -1) + throw SysError("deleting '%1%'", path); + + results.bytesFreed += st.st_size; + } + + struct stat st; + if (stat(linksDir.c_str(), &st) == -1) + throw SysError("statting '%1%'", linksDir); + int64_t overhead = st.st_blocks * 512ULL; + + printInfo("note: currently hard linking saves %.2f MiB", + ((unsharedSize - actualSize - overhead) / (1024.0 * 1024.0))); } /* While we're at it, vacuum the database. */ diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 15475f402..301425eb1 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -238,16 +238,12 @@ private: PathSet queryValidPathsOld(); ValidPathInfo queryPathInfoOld(const Path & path); - struct GCState; - void findRoots(const Path & path, unsigned char type, Roots & roots); void findRootsNoTemp(Roots & roots, bool censor); void findRuntimeRoots(Roots & roots, bool censor); - void removeUnusedLinks(const GCState & state); - Path createTempDirInStore(); void checkDerivationOutputs(const StorePath & drvPath, const Derivation & drv); From 17e6ebcc90b6c7d5db8588f1d2b914c98543560b Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 14 Oct 2021 14:13:57 +0200 Subject: [PATCH 16/25] Speed up GC by marking entire closures as live --- src/libstore/gc.cc | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 800a3fd19..03d44ebf8 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -654,12 +654,22 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) handle it again. */ if (dead.count(*path)) continue; + auto markAlive = [&]() + { + alive.insert(*path); + alive.insert(start); + try { + StorePathSet closure; + computeFSClosure(*path, closure); + for (auto & p : closure) + alive.insert(p); + } catch (InvalidPath &) { } + }; + /* If this is a root, bail out. */ if (roots.count(*path)) { debug("cannot delete '%s' because it's a root", printStorePath(*path)); - alive.insert(*path); - alive.insert(start); - return; + return markAlive(); } if (options.action == GCOptions::gcDeleteSpecific @@ -671,9 +681,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) auto shared(_shared.lock()); if (shared->tempRoots.count(hashPart)) { debug("cannot delete '%s' because it's a temporary root", printStorePath(*path)); - alive.insert(*path); - alive.insert(start); - return; + return markAlive(); } shared->pending = hashPart; } From 304180d0ded6bf166b051f2b2ce5c33c18c2bbfe Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 15 Oct 2021 12:20:23 +0200 Subject: [PATCH 17/25] Memoize queryReferrers() --- src/libstore/gc.cc | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 03d44ebf8..ed792a777 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -617,6 +617,8 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) } }; + std::map referrersCache; + /* Helper function that visits all paths reachable from `start` via the referrers edges and optionally derivers and derivation output edges. If none of those paths are roots, then all @@ -689,9 +691,14 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) if (isValidPath(*path)) { /* Visit the referrers of this path. */ - StorePathSet referrers; - queryReferrers(*path, referrers); - for (auto & p : referrers) + auto i = referrersCache.find(*path); + if (i == referrersCache.end()) { + StorePathSet referrers; + queryReferrers(*path, referrers); + referrersCache.emplace(*path, std::move(referrers)); + i = referrersCache.find(*path); + } + for (auto & p : i->second) enqueue(p); /* If keep-derivations is set and this is a @@ -718,6 +725,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) if (shouldDelete) { invalidatePathChecked(path); deleteFromStore(path.to_string()); + referrersCache.erase(path); } } }; From be35569a6e488563f00caee521eba451d9b63680 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 15 Oct 2021 12:36:29 +0200 Subject: [PATCH 18/25] Run installTests on Hydra --- flake.nix | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/flake.nix b/flake.nix index 2d0c44889..cad6b22f8 100644 --- a/flake.nix +++ b/flake.nix @@ -488,12 +488,7 @@ ''; */ - }; - - checks = forAllSystems (system: { - binaryTarball = self.hydraJobs.binaryTarball.${system}; - perlBindings = self.hydraJobs.perlBindings.${system}; - installTests = + installTests = forAllSystems (system: let pkgs = nixpkgsFor.${system}; in pkgs.runCommand "install-tests" { againstSelf = testNixVersions pkgs pkgs.nix pkgs.pkgs.nix; @@ -505,7 +500,14 @@ # Disabled because the latest stable version doesn't handle # `NIX_DAEMON_SOCKET_PATH` which is required for the tests to work # againstLatestStable = testNixVersions pkgs pkgs.nix pkgs.nixStable; - } "touch $out"; + } "touch $out"); + + }; + + checks = forAllSystems (system: { + binaryTarball = self.hydraJobs.binaryTarball.${system}; + perlBindings = self.hydraJobs.perlBindings.${system}; + installTests = self.hydraJobs.installTests.${system}; }); packages = forAllSystems (system: { From 4d014221d4aaf9c2e7a323be002f058ebb4d79e2 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 15 Oct 2021 12:41:27 +0200 Subject: [PATCH 19/25] Fix test against old daemon --- tests/gc.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/gc.sh b/tests/gc.sh index 33d627dae..a736b63db 100644 --- a/tests/gc.sh +++ b/tests/gc.sh @@ -26,6 +26,7 @@ if nix-store --delete $outPath; then false; fi test -e $outPath for i in $NIX_STORE_DIR/*; do + if [[ $i =~ /trash ]]; then continue; fi # compat with old daemon touch $i.lock touch $i.chroot done From e0936ae38f110db68870b1d80d6c628172d12b4c Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 15 Oct 2021 16:12:21 +0200 Subject: [PATCH 20/25] Fix crash when a GC client disconnects The client thread can't just delete its own thread object from connections, it has to detach it. --- src/libstore/gc.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index ed792a777..220441927 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -536,7 +536,10 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) auto fdClient_ = fdClient.get(); std::thread clientThread([&, fdClient = std::move(fdClient)]() { Finally cleanup([&]() { - connections.lock()->erase(fdClient.get()); + auto conn(connections.lock()); + auto i = conn->find(fdClient.get()); + i->second.detach(); + conn->erase(i); }); while (true) { From ac54c6faa6b73c3da180dec666fadd6e8d7d18e3 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 15 Oct 2021 16:36:48 +0200 Subject: [PATCH 21/25] Fix main GC thread exiting --- src/libstore/gc.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 220441927..648295684 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -538,8 +538,10 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) Finally cleanup([&]() { auto conn(connections.lock()); auto i = conn->find(fdClient.get()); - i->second.detach(); - conn->erase(i); + if (i != conn->end()) { + i->second.detach(); + conn->erase(i); + } }); while (true) { From 10f9a8e77d504354deafbcf7351a6d46d4528542 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 15 Oct 2021 16:52:37 +0200 Subject: [PATCH 22/25] Add a test for the non-blocking GC --- src/libstore/gc.cc | 4 ++++ tests/gc-non-blocking.sh | 33 +++++++++++++++++++++++++++++++++ tests/local.mk | 1 + 3 files changed, 38 insertions(+) create mode 100644 tests/gc-non-blocking.sh diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 648295684..5e8cbf8fc 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -735,6 +735,10 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) } }; + /* Synchronisation point for testing, see tests/gc-concurrent.sh. */ + if (auto p = getEnv("_NIX_TEST_GC_SYNC")) + readFile(*p); + /* Either delete all garbage paths, or just the specified paths (for gcDeleteSpecific). */ if (options.action == GCOptions::gcDeleteSpecific) { diff --git a/tests/gc-non-blocking.sh b/tests/gc-non-blocking.sh new file mode 100644 index 000000000..8b21c6f1c --- /dev/null +++ b/tests/gc-non-blocking.sh @@ -0,0 +1,33 @@ +# Test whether the collector is non-blocking, i.e. a build can run in +# parallel with it. +source common.sh + +needLocalStore "the GC test needs a synchronisation point" + +clearStore + +fifo=$TEST_ROOT/test.fifo +mkfifo "$fifo" + +dummy=$(nix store add-path ./simple.nix) + +running=$TEST_ROOT/running +touch $running + +(_NIX_TEST_GC_SYNC=$fifo nix-store --gc -vvvvv; rm $running) & +pid=$! + +sleep 2 + +outPath=$(nix-build -o "$TEST_ROOT/result" -E " + with import ./config.nix; + mkDerivation { + name = \"non-blocking\"; + buildCommand = \"set -x; test -e $running; mkdir \$out; echo > $fifo\"; + }") + +wait $pid + +(! test -e $running) +(! test -e $dummy) +test -e $outPath diff --git a/tests/local.mk b/tests/local.mk index d88c3a875..8d454ee19 100644 --- a/tests/local.mk +++ b/tests/local.mk @@ -4,6 +4,7 @@ nix_tests = \ gc.sh \ ca/gc.sh \ gc-concurrent.sh \ + gc-non-blocking.sh \ gc-auto.sh \ referrers.sh user-envs.sh logging.sh nix-build.sh misc.sh fixed.sh \ gc-runtime.sh check-refs.sh filter-source.sh \ From a594d1afd5172abba85561f04fd9976582f1b6c3 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 15 Oct 2021 16:58:21 +0200 Subject: [PATCH 23/25] Revert "Fix referrers test" This reverts commit e31a48366f19d5fa452df6273e817a4ea5516e50. Unnecessary after 0be8cc1466f317e33977590510bac4b18471f0ce. --- tests/referrers.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/referrers.sh b/tests/referrers.sh index 7ede707b8..81323c280 100644 --- a/tests/referrers.sh +++ b/tests/referrers.sh @@ -14,9 +14,9 @@ echo "making registration..." set +x for ((n = 0; n < $max; n++)); do - storePath=$NIX_STORE_DIR/$(printf "%05d\n" $n)aaaaaaaaaaaaaaaaaaaaaaaaaaa-$n + storePath=$NIX_STORE_DIR/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-$n echo -n > $storePath - ref2=$NIX_STORE_DIR/$(printf "%05d\n" $((n+1)))aaaaaaaaaaaaaaaaaaaaaaaaaaa-$((n+1)) + ref2=$NIX_STORE_DIR/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-$((n+1)) if test $((n+1)) = $max; then ref2=$reference fi From 22c35ea5b86d4aa450c3517506576ed377437574 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 28 Oct 2021 20:57:36 +0200 Subject: [PATCH 24/25] Remove unused variable --- src/libstore/gc.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 5e8cbf8fc..13412e504 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -502,7 +502,6 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) std::thread serverThread([&]() { Sync> connections; - std::atomic_bool quit = false; Finally cleanup([&]() { debug("GC roots server shutting down"); From 33d04e8a8d9ab8f1736b7fe725e8851fbc41e1e7 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 28 Oct 2021 22:14:47 +0200 Subject: [PATCH 25/25] Use nix::connect() to connect to the garbage collector --- src/libstore/gc.cc | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 13412e504..8030c84f5 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -123,20 +123,10 @@ void LocalStore::addTempRoot(const StorePath & path) collector is running. So we have to connect to the garbage collector and inform it about our root. */ if (!state->fdRootsSocket) { - state->fdRootsSocket = createUnixDomainSocket(); - auto socketPath = stateDir.get() + gcSocketPath; - debug("connecting to '%s'", socketPath); - - struct sockaddr_un addr; - addr.sun_family = AF_UNIX; - if (socketPath.size() + 1 >= sizeof(addr.sun_path)) - throw Error("socket path '%s' is too long", socketPath); - strcpy(addr.sun_path, socketPath.c_str()); - - if (::connect(state->fdRootsSocket.get(), (struct sockaddr *) &addr, sizeof(addr)) == -1) - throw SysError("cannot connect to garbage collector at '%s'", socketPath); + state->fdRootsSocket = createUnixDomainSocket(); + nix::connect(state->fdRootsSocket.get(), socketPath); } try {