From b932ea58ec610830ed3141bb14fbd812aa66b2c1 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 5 Sep 2017 20:39:57 +0200 Subject: [PATCH] GC: Don't delete own temproots file Since file locks are per-process rather than per-file-descriptor, the garbage collector would always acquire a lock on its own temproots file and conclude that it's stale. --- src/libstore/gc.cc | 57 ++++++++++++++++++------------------- src/libstore/local-store.cc | 8 ++++-- src/libstore/local-store.hh | 3 +- 3 files changed, 34 insertions(+), 34 deletions(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 5e3958ea5..534db8c6e 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -18,7 +18,6 @@ namespace nix { static string gcLockName = "gc.lock"; -static string tempRootsDir = "temproots"; static string gcRootsDir = "gcroots"; @@ -153,30 +152,25 @@ void LocalStore::addTempRoot(const Path & path) if (!state->fdTempRoots) { while (1) { - Path dir = (format("%1%/%2%") % stateDir % tempRootsDir).str(); - createDirs(dir); - - state->fnTempRoots = (format("%1%/%2%") % dir % getpid()).str(); - AutoCloseFD fdGCLock = openGCLock(ltRead); - if (pathExists(state->fnTempRoots)) + if (pathExists(fnTempRoots)) /* It *must* be stale, since there can be no two processes with the same pid. */ - unlink(state->fnTempRoots.c_str()); + unlink(fnTempRoots.c_str()); - state->fdTempRoots = openLockFile(state->fnTempRoots, true); + state->fdTempRoots = openLockFile(fnTempRoots, true); fdGCLock = -1; - debug(format("acquiring read lock on '%1%'") % state->fnTempRoots); + debug(format("acquiring read lock on '%1%'") % fnTempRoots); lockFile(state->fdTempRoots.get(), ltRead, true); /* Check whether the garbage collector didn't get in our way. */ struct stat st; if (fstat(state->fdTempRoots.get(), &st) == -1) - throw SysError(format("statting '%1%'") % state->fnTempRoots); + throw SysError(format("statting '%1%'") % fnTempRoots); if (st.st_size == 0) break; /* The garbage collector deleted this file before we could @@ -188,14 +182,14 @@ void LocalStore::addTempRoot(const Path & 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%'") % state->fnTempRoots); + debug(format("acquiring write lock on '%1%'") % fnTempRoots); lockFile(state->fdTempRoots.get(), ltWrite, true); string s = path + '\0'; writeFull(state->fdTempRoots.get(), s); /* Downgrade to a read lock. */ - debug(format("downgrading to read lock on '%1%'") % state->fnTempRoots); + debug(format("downgrading to read lock on '%1%'") % fnTempRoots); lockFile(state->fdTempRoots.get(), ltRead, true); } @@ -204,11 +198,10 @@ void LocalStore::readTempRoots(PathSet & tempRoots, FDs & fds) { /* Read the `temproots' directory for per-process temporary root files. */ - DirEntries tempRootFiles = readDirectory( - (format("%1%/%2%") % stateDir % tempRootsDir).str()); + DirEntries tempRootFiles = readDirectory(tempRootsDir); for (auto & i : tempRootFiles) { - Path path = (format("%1%/%2%/%3%") % stateDir % tempRootsDir % i.name).str(); + Path path = tempRootsDir + "/" + i.name; debug(format("reading temporary root file '%1%'") % path); FDPtr fd(new AutoCloseFD(open(path.c_str(), O_CLOEXEC | O_RDWR, 0666))); @@ -222,21 +215,25 @@ void LocalStore::readTempRoots(PathSet & tempRoots, FDs & fds) //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)) { - printError(format("removing stale temporary roots file '%1%'") % path); - unlink(path.c_str()); - writeFull(fd->get(), "d"); - continue; - } + if (path != fnTempRoots) { - /* Acquire a read lock. This will prevent the owning process - from upgrading to a write lock, therefore it will block in - addTempRoot(). */ - debug(format("waiting for read lock on '%1%'") % path); - lockFile(fd->get(), ltRead, true); + /* Try to acquire a write lock without blocking. This can + only succeed if the owning process has died. In that case + we don't care about its temporary roots. */ + if (lockFile(fd->get(), ltWrite, false)) { + printError(format("removing stale temporary roots file '%1%'") % path); + unlink(path.c_str()); + writeFull(fd->get(), "d"); + continue; + } + + /* Acquire a read lock. This will prevent the owning process + from upgrading to a write lock, therefore it will block in + addTempRoot(). */ + debug(format("waiting for read lock on '%1%'") % path); + lockFile(fd->get(), ltRead, true); + + } /* Read the entire file. */ string contents = readFile(fd->get()); diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 95b05f8af..5ca776099 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -51,6 +51,8 @@ LocalStore::LocalStore(const Params & params) , reservedPath(dbDir + "/reserved") , schemaPath(dbDir + "/schema") , trashDir(realStoreDir + "/trash") + , tempRootsDir(stateDir + "/temproots") + , fnTempRoots(fmt("%s/%d", tempRootsDir, getpid())) , publicKeys(getDefaultPublicKeys()) { auto state(_state.lock()); @@ -61,7 +63,7 @@ LocalStore::LocalStore(const Params & params) createDirs(linksDir); Path profilesDir = stateDir + "/profiles"; createDirs(profilesDir); - createDirs(stateDir + "/temproots"); + createDirs(tempRootsDir); createDirs(dbDir); Path gcRootsDir = stateDir + "/gcroots"; if (!pathExists(gcRootsDir)) { @@ -242,12 +244,12 @@ LocalStore::LocalStore(const Params & params) LocalStore::~LocalStore() { - auto state(_state.lock()); try { + auto state(_state.lock()); if (state->fdTempRoots) { state->fdTempRoots = -1; - unlink(state->fnTempRoots.c_str()); + unlink(fnTempRoots.c_str()); } } catch (...) { ignoreException(); diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 2af1bfbb3..04519bfca 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -59,7 +59,6 @@ private: SQLiteStmt stmtQueryValidPaths; /* The file to which we write our temporary roots. */ - Path fnTempRoots; AutoCloseFD fdTempRoots; }; @@ -75,6 +74,8 @@ public: const Path reservedPath; const Path schemaPath; const Path trashDir; + const Path tempRootsDir; + const Path fnTempRoots; private: