From 1f041ac54f43093e4f4df1caa630d491ff51c3f8 Mon Sep 17 00:00:00 2001 From: Andrew Brooks Date: Fri, 2 Sep 2022 18:32:35 -0500 Subject: [PATCH 1/3] Prevent tempdir from being GC-ed before addToStoreFromDump has renamed it This fixes issue 6823 by placing the tempdir used in LocalStore::addToStoreFromDump outside the Nix store, where automatic GC is no longer a concern. --- src/libstore/local-store.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index a272e4301..6abd52683 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1388,7 +1388,7 @@ StorePath LocalStore::addToStoreFromDump(Source & source0, std::string_view name StringSource dumpSource { dump }; ChainSource bothSource { dumpSource, source }; - auto tempDir = createTempDir(realStoreDir, "add"); + auto tempDir = createTempDir("", "add"); delTempDir = std::make_unique(tempDir); tempPath = tempDir + "/x"; From 84fe75a12a085c6b4b8d4ac65a048f569de1252b Mon Sep 17 00:00:00 2001 From: Andrew Brooks Date: Tue, 6 Sep 2022 17:48:00 -0500 Subject: [PATCH 2/3] Keep created temp dirs inside store, but protect from GC Implements the approach suggested by feedback on PR #6994, where tempdir paths are created in the store (now with an exclusive lock). As part of this work, the currently-broken and unused `createTempDirInStore` function is updated to create an exclusive lock on the temp directory in the store. The GC now makes a non-blocking attempt to lock any store directories that "look like" the temp directories created by this function, and if it can't acquire one, ignores the directory. --- src/libstore/gc.cc | 12 ++++++++++++ src/libstore/local-store.cc | 29 +++++++++++++++++++---------- src/libstore/local-store.hh | 2 +- 3 files changed, 32 insertions(+), 11 deletions(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 4c1a82279..6cd7efbc9 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -619,6 +619,18 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) Path path = storeDir + "/" + std::string(baseName); Path realPath = realStoreDir + "/" + std::string(baseName); + /* There may be temp directories in the store that are still in use + by another process. We need to be sure that we can acquire an + exclusive lock before deleting them. */ + AutoCloseFD tmpDirFd; + if (baseName.rfind("add-", 0) == 0) { + tmpDirFd = open(realPath.c_str(), O_RDONLY | O_DIRECTORY); + if (tmpDirFd.get() == -1 || !lockFile(tmpDirFd.get(), ltWrite, false)) { + debug("skipping locked tempdir '%s'", realPath); + return; + } + } + printInfo("deleting '%1%'", path); results.paths.insert(path); diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 6abd52683..5ee451da3 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1382,13 +1382,15 @@ StorePath LocalStore::addToStoreFromDump(Source & source0, std::string_view name std::unique_ptr delTempDir; Path tempPath; + Path tempDir; + AutoCloseFD tempDirFd; if (!inMemory) { /* Drain what we pulled so far, and then keep on pulling */ StringSource dumpSource { dump }; ChainSource bothSource { dumpSource, source }; - auto tempDir = createTempDir("", "add"); + std::tie(tempDir, tempDirFd) = createTempDirInStore(); delTempDir = std::make_unique(tempDir); tempPath = tempDir + "/x"; @@ -1431,6 +1433,7 @@ StorePath LocalStore::addToStoreFromDump(Source & source0, std::string_view name } else { /* Move the temporary path we restored above. */ moveFile(tempPath, realPath); + tempDirFd.close(); } /* For computing the nar hash. In recursive SHA-256 mode, this @@ -1507,18 +1510,24 @@ StorePath LocalStore::addTextToStore( /* Create a temporary directory in the store that won't be - garbage-collected. */ -Path LocalStore::createTempDirInStore() + garbage-collected until the returned FD is closed. */ +std::pair LocalStore::createTempDirInStore() { - Path tmpDir; + Path tmpDirFn; + AutoCloseFD tmpDirFd; + bool lockedByUs = false; do { /* There is a slight possibility that `tmpDir' gets deleted by - the GC between createTempDir() and addTempRoot(), so repeat - until `tmpDir' exists. */ - tmpDir = createTempDir(realStoreDir); - addTempRoot(parseStorePath(tmpDir)); - } while (!pathExists(tmpDir)); - return tmpDir; + the GC between createTempDir() and when we acquire a lock on it. + We'll repeat until 'tmpDir' exists and we've locked it. */ + tmpDirFn = createTempDir(realStoreDir, "add"); + tmpDirFd = open(tmpDirFn.c_str(), O_RDONLY | O_DIRECTORY); + if (tmpDirFd.get() < 0) { + continue; + } + lockedByUs = lockFile(tmpDirFd.get(), ltWrite, true); + } while (!pathExists(tmpDirFn) || !lockedByUs); + return {tmpDirFn, std::move(tmpDirFd)}; } diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 70d225be3..bd0ce1fe6 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -256,7 +256,7 @@ private: void findRuntimeRoots(Roots & roots, bool censor); - Path createTempDirInStore(); + std::pair createTempDirInStore(); void checkDerivationOutputs(const StorePath & drvPath, const Derivation & drv); From 565d888e0f6a2c66ee7b10f6fe6a97f79fa51732 Mon Sep 17 00:00:00 2001 From: Andrew Brooks Date: Mon, 12 Sep 2022 11:33:23 -0500 Subject: [PATCH 3/3] Address PR feedback on #6694 --- src/libstore/gc.cc | 5 ++--- src/libstore/local-store.cc | 3 +-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 6cd7efbc9..9ef8972f3 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -622,9 +622,8 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) /* There may be temp directories in the store that are still in use by another process. We need to be sure that we can acquire an exclusive lock before deleting them. */ - AutoCloseFD tmpDirFd; - if (baseName.rfind("add-", 0) == 0) { - tmpDirFd = open(realPath.c_str(), O_RDONLY | O_DIRECTORY); + if (baseName.find("tmp-", 0) == 0) { + AutoCloseFD tmpDirFd = open(realPath.c_str(), O_RDONLY | O_DIRECTORY); if (tmpDirFd.get() == -1 || !lockFile(tmpDirFd.get(), ltWrite, false)) { debug("skipping locked tempdir '%s'", realPath); return; diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 5ee451da3..0b07cde34 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1433,7 +1433,6 @@ StorePath LocalStore::addToStoreFromDump(Source & source0, std::string_view name } else { /* Move the temporary path we restored above. */ moveFile(tempPath, realPath); - tempDirFd.close(); } /* For computing the nar hash. In recursive SHA-256 mode, this @@ -1520,7 +1519,7 @@ std::pair LocalStore::createTempDirInStore() /* There is a slight possibility that `tmpDir' gets deleted by the GC between createTempDir() and when we acquire a lock on it. We'll repeat until 'tmpDir' exists and we've locked it. */ - tmpDirFn = createTempDir(realStoreDir, "add"); + tmpDirFn = createTempDir(realStoreDir, "tmp"); tmpDirFd = open(tmpDirFn.c_str(), O_RDONLY | O_DIRECTORY); if (tmpDirFd.get() < 0) { continue;