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.
This commit is contained in:
Eelco Dolstra 2017-09-05 20:39:57 +02:00
parent 8215b75d36
commit b932ea58ec
No known key found for this signature in database
GPG key ID: 8170B4726D7198DE
3 changed files with 34 additions and 34 deletions

View file

@ -18,7 +18,6 @@ namespace nix {
static string gcLockName = "gc.lock"; static string gcLockName = "gc.lock";
static string tempRootsDir = "temproots";
static string gcRootsDir = "gcroots"; static string gcRootsDir = "gcroots";
@ -153,30 +152,25 @@ void LocalStore::addTempRoot(const Path & path)
if (!state->fdTempRoots) { if (!state->fdTempRoots) {
while (1) { while (1) {
Path dir = (format("%1%/%2%") % stateDir % tempRootsDir).str();
createDirs(dir);
state->fnTempRoots = (format("%1%/%2%") % dir % getpid()).str();
AutoCloseFD fdGCLock = openGCLock(ltRead); AutoCloseFD fdGCLock = openGCLock(ltRead);
if (pathExists(state->fnTempRoots)) if (pathExists(fnTempRoots))
/* It *must* be stale, since there can be no two /* It *must* be stale, since there can be no two
processes with the same pid. */ 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; 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); lockFile(state->fdTempRoots.get(), ltRead, true);
/* Check whether the garbage collector didn't get in our /* Check whether the garbage collector didn't get in our
way. */ way. */
struct stat st; struct stat st;
if (fstat(state->fdTempRoots.get(), &st) == -1) 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; if (st.st_size == 0) break;
/* The garbage collector deleted this file before we could /* 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 /* Upgrade the lock to a write lock. This will cause us to block
if the garbage collector is holding our lock. */ 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); lockFile(state->fdTempRoots.get(), ltWrite, true);
string s = path + '\0'; string s = path + '\0';
writeFull(state->fdTempRoots.get(), s); writeFull(state->fdTempRoots.get(), s);
/* Downgrade to a read lock. */ /* 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); 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 /* Read the `temproots' directory for per-process temporary root
files. */ files. */
DirEntries tempRootFiles = readDirectory( DirEntries tempRootFiles = readDirectory(tempRootsDir);
(format("%1%/%2%") % stateDir % tempRootsDir).str());
for (auto & i : tempRootFiles) { 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); debug(format("reading temporary root file '%1%'") % path);
FDPtr fd(new AutoCloseFD(open(path.c_str(), O_CLOEXEC | O_RDWR, 0666))); 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))); //FDPtr fd(new AutoCloseFD(openLockFile(path, false)));
//if (*fd == -1) continue; //if (*fd == -1) continue;
/* Try to acquire a write lock without blocking. This can if (path != fnTempRoots) {
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 /* Try to acquire a write lock without blocking. This can
from upgrading to a write lock, therefore it will block in only succeed if the owning process has died. In that case
addTempRoot(). */ we don't care about its temporary roots. */
debug(format("waiting for read lock on '%1%'") % path); if (lockFile(fd->get(), ltWrite, false)) {
lockFile(fd->get(), ltRead, true); 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. */ /* Read the entire file. */
string contents = readFile(fd->get()); string contents = readFile(fd->get());

View file

@ -51,6 +51,8 @@ LocalStore::LocalStore(const Params & params)
, reservedPath(dbDir + "/reserved") , reservedPath(dbDir + "/reserved")
, schemaPath(dbDir + "/schema") , schemaPath(dbDir + "/schema")
, trashDir(realStoreDir + "/trash") , trashDir(realStoreDir + "/trash")
, tempRootsDir(stateDir + "/temproots")
, fnTempRoots(fmt("%s/%d", tempRootsDir, getpid()))
, publicKeys(getDefaultPublicKeys()) , publicKeys(getDefaultPublicKeys())
{ {
auto state(_state.lock()); auto state(_state.lock());
@ -61,7 +63,7 @@ LocalStore::LocalStore(const Params & params)
createDirs(linksDir); createDirs(linksDir);
Path profilesDir = stateDir + "/profiles"; Path profilesDir = stateDir + "/profiles";
createDirs(profilesDir); createDirs(profilesDir);
createDirs(stateDir + "/temproots"); createDirs(tempRootsDir);
createDirs(dbDir); createDirs(dbDir);
Path gcRootsDir = stateDir + "/gcroots"; Path gcRootsDir = stateDir + "/gcroots";
if (!pathExists(gcRootsDir)) { if (!pathExists(gcRootsDir)) {
@ -242,12 +244,12 @@ LocalStore::LocalStore(const Params & params)
LocalStore::~LocalStore() LocalStore::~LocalStore()
{ {
auto state(_state.lock());
try { try {
auto state(_state.lock());
if (state->fdTempRoots) { if (state->fdTempRoots) {
state->fdTempRoots = -1; state->fdTempRoots = -1;
unlink(state->fnTempRoots.c_str()); unlink(fnTempRoots.c_str());
} }
} catch (...) { } catch (...) {
ignoreException(); ignoreException();

View file

@ -59,7 +59,6 @@ private:
SQLiteStmt stmtQueryValidPaths; SQLiteStmt stmtQueryValidPaths;
/* The file to which we write our temporary roots. */ /* The file to which we write our temporary roots. */
Path fnTempRoots;
AutoCloseFD fdTempRoots; AutoCloseFD fdTempRoots;
}; };
@ -75,6 +74,8 @@ public:
const Path reservedPath; const Path reservedPath;
const Path schemaPath; const Path schemaPath;
const Path trashDir; const Path trashDir;
const Path tempRootsDir;
const Path fnTempRoots;
private: private: