From 38776198880d06dc6af0fbdfa7cb2c053000112a Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 21 Dec 2011 11:47:52 +0000 Subject: [PATCH 1/9] * Add some accidentally committed files. --- tests/lang/eval-okay-search-path.nix~ | 1 - tests/lang/eval-okay-search-path.out | 1 - 2 files changed, 2 deletions(-) delete mode 100644 tests/lang/eval-okay-search-path.nix~ delete mode 100644 tests/lang/eval-okay-search-path.out diff --git a/tests/lang/eval-okay-search-path.nix~ b/tests/lang/eval-okay-search-path.nix~ deleted file mode 100644 index da52a6d39..000000000 --- a/tests/lang/eval-okay-search-path.nix~ +++ /dev/null @@ -1 +0,0 @@ -(import ) \ No newline at end of file diff --git a/tests/lang/eval-okay-search-path.out b/tests/lang/eval-okay-search-path.out deleted file mode 100644 index d0bc8c5e8..000000000 --- a/tests/lang/eval-okay-search-path.out +++ /dev/null @@ -1 +0,0 @@ -"abcc" From 69d6f0936a59da5cc35040407f4b667437d61add Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 21 Dec 2011 18:59:25 +0000 Subject: [PATCH 2/9] * Use a lock to ensure that only one build-remote instance can copy a closure to a given machine at the same time. This prevents the case where multiple instances try to copy the same missing store path to the target machine, which is very wasteful. --- scripts/build-remote.pl.in | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/scripts/build-remote.pl.in b/scripts/build-remote.pl.in index c805d6740..6a6515756 100755 --- a/scripts/build-remote.pl.in +++ b/scripts/build-remote.pl.in @@ -225,8 +225,17 @@ sub removeRoots { } -# Copy the derivation and its dependencies to the build machine. +# Copy the derivation and its dependencies to the build machine. This +# is guarded by an exclusive lock per machine to prevent multiple +# build-remote instances from copying to a machine simultaneously. +# That's undesirable because we may end up with N instances uploading +# the same missing path simultaneously, causing the effective network +# bandwidth and target disk speed to be divided by N. +my $uploadLock = "$currentLoad/$hostName.upload-lock"; +open MAINLOCK, ">>$uploadLock" or die; +flock(MAINLOCK, LOCK_EX) or die; Nix::CopyClosure::copyTo($hostName, [ @sshOpts ], [ $drvPath, @inputs ], "", "", 0, 0, $maybeSign ne ""); +close MAINLOCK; # Perform the build. From 4d728bc3e60a6d07858f7a881221688ccdebb7fe Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 21 Dec 2011 19:11:58 +0000 Subject: [PATCH 3/9] * Security: make sure the lock files used by build-remote.pl are not readable to other users. Otherwise, any user can open the lock file for reading and lock it, thus DoSing the remote build mechanism. --- scripts/build-remote.pl.in | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/scripts/build-remote.pl.in b/scripts/build-remote.pl.in index 6a6515756..8bf77df04 100755 --- a/scripts/build-remote.pl.in +++ b/scripts/build-remote.pl.in @@ -1,6 +1,6 @@ #! @perl@ -w @perlFlags@ -use Fcntl ':flock'; +use Fcntl qw(:DEFAULT :flock); use English '-no_match_vars'; use IO::Handle; use Nix::Config; @@ -56,7 +56,7 @@ sub openSlotLock { my ($machine, $slot) = @_; my $slotLockFn = "$currentLoad/" . (join '+', @{$machine->{systemTypes}}) . "-" . $machine->{hostName} . "-$slot"; my $slotLock = new IO::Handle; - open $slotLock, ">>$slotLockFn" or die; + sysopen $slotLock, "$slotLockFn", O_RDWR|O_CREAT, 0600 or die; return $slotLock; } @@ -64,7 +64,7 @@ sub openSlotLock { # Read the list of machines. my @machines; if (defined $conf && -e $conf) { - open CONF, "< $conf" or die; + open CONF, "<$conf" or die; while () { chomp; s/\#.*$//g; @@ -104,7 +104,7 @@ REQ: while (1) { # Acquire the exclusive lock on $currentLoad/main-lock. mkdir $currentLoad, 0777 or die unless -d $currentLoad; my $mainLock = "$currentLoad/main-lock"; - open MAINLOCK, ">>$mainLock" or die; + sysopen MAINLOCK, "$mainLock", O_RDWR|O_CREAT, 0600 or die; flock(MAINLOCK, LOCK_EX) or die; @@ -232,7 +232,7 @@ sub removeRoots { # the same missing path simultaneously, causing the effective network # bandwidth and target disk speed to be divided by N. my $uploadLock = "$currentLoad/$hostName.upload-lock"; -open MAINLOCK, ">>$uploadLock" or die; +sysopen MAINLOCK, "$uploadLock", O_RDWR|O_CREAT, 0600 or die; flock(MAINLOCK, LOCK_EX) or die; Nix::CopyClosure::copyTo($hostName, [ @sshOpts ], [ $drvPath, @inputs ], "", "", 0, 0, $maybeSign ne ""); close MAINLOCK; From 2aac7cd0217ce3417b12574ca7f9930090da6c4c Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 21 Dec 2011 19:17:45 +0000 Subject: [PATCH 4/9] * Another case of lock file permissions being too liberal. --- src/libstore/pathlocks.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/pathlocks.cc b/src/libstore/pathlocks.cc index d8290815c..645f4cd67 100644 --- a/src/libstore/pathlocks.cc +++ b/src/libstore/pathlocks.cc @@ -16,7 +16,7 @@ int openLockFile(const Path & path, bool create) { AutoCloseFD fd; - fd = open(path.c_str(), O_RDWR | (create ? O_CREAT : 0), 0666); + fd = open(path.c_str(), O_RDWR | (create ? O_CREAT : 0), 0600); if (fd == -1 && (create || errno != ENOENT)) throw SysError(format("opening lock file `%1%'") % path); From 66c99b0cf50bb1d6290f55c209e9541b50ce41e8 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 22 Dec 2011 10:58:27 +0000 Subject: [PATCH 5/9] =?UTF-8?q?*=20=E2=80=98--disable-shared=E2=80=99=20is?= =?UTF-8?q?=20no=20longer=20supported.=20=20Fortunately=20it's=20not=20=20?= =?UTF-8?q?=20needed=20for=20the=20coverage=20analysis.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- release.nix | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/release.nix b/release.nix index 4f3e5fac0..91f3752d5 100644 --- a/release.nix +++ b/release.nix @@ -1,5 +1,5 @@ { nixpkgs ? , nixos ? -, nix ? { outPath = ../nix-export; rev = 1234; } +, nix ? { outPath = ./.; rev = 1234; } , officialRelease ? false }: @@ -98,7 +98,7 @@ let ]; configureFlags = '' - --disable-init-state --disable-shared + --disable-init-state --with-bzip2=${bzip2} --with-sqlite=${sqlite} --with-dbi=${perlPackages.DBI}/lib/perl5/site_perl --with-dbd-sqlite=${perlPackages.DBDSQLite}/lib/perl5/site_perl From 58d974336c733416756e5b396928602ea8ed8df2 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 22 Dec 2011 14:33:34 +0000 Subject: [PATCH 6/9] * Drop unnecessary call to canonPath() (nixStore is already canonical). --- src/libstore/gc.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index e79d93723..a99bb1a81 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -634,7 +634,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) try { foreach (vector::iterator, i, entries_) - tryToDelete(state, canonPath(nixStore + "/" + *i)); + tryToDelete(state, nixStore + "/" + *i); } catch (GCLimitReached & e) { } } From b33da599c5c1b06a32a3eeac58f95481d10f821d Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 22 Dec 2011 15:55:53 +0000 Subject: [PATCH 7/9] * In the garbage collector, delete invalid paths before deleting unreachable paths. This matters when using --max-freed etc.: unreachable paths could become reachable again, so it's nicer to keep them if there is "real" garbage to be deleted. Also, don't use readDirectory() but read the Nix store and delete invalid paths in parallel. This reduces GC latency on very large Nix stores. --- src/libstore/gc.cc | 46 ++++++++++++++++++++++++++++++++++----------- src/libutil/util.cc | 10 +++++++++- src/libutil/util.hh | 1 + 3 files changed, 45 insertions(+), 12 deletions(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index a99bb1a81..20f194e6e 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -617,27 +617,51 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) } else { - printMsg(lvlError, format("reading the Nix store...")); - Paths entries = readDirectory(nixStore); - - /* 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()); - random_shuffle(entries_.begin(), entries_.end()); - if (shouldDelete(state.options.action)) printMsg(lvlError, format("deleting garbage...")); else printMsg(lvlError, format("determining live/dead paths...")); try { + + AutoCloseDir dir = opendir(nixStore.c_str()); + if (!dir) throw SysError(format("opening directory `%1%'") % nixStore); + + /* 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. */ + Paths entries; + struct dirent * dirent; + while (errno = 0, dirent = readdir(dir)) { + checkInterrupt(); + string name = dirent->d_name; + if (name == "." || name == "..") continue; + Path path = nixStore + "/" + name; + if (isValidPath(path)) + entries.push_back(path); + else + tryToDelete(state, path); + } + + dir.close(); + + /* 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()); + random_shuffle(entries_.begin(), entries_.end()); + foreach (vector::iterator, i, entries_) tryToDelete(state, nixStore + "/" + *i); + } catch (GCLimitReached & e) { } - } + } } diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 9adaac40d..0352754f5 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -701,7 +701,7 @@ AutoCloseDir::AutoCloseDir(DIR * dir) AutoCloseDir::~AutoCloseDir() { - if (dir) closedir(dir); + close(); } @@ -717,6 +717,14 @@ AutoCloseDir::operator DIR *() } +void AutoCloseDir::close() +{ + if (dir) { + closedir(dir); + dir = 0; + } +} + ////////////////////////////////////////////////////////////////////// diff --git a/src/libutil/util.hh b/src/libutil/util.hh index f86290f31..a1cf68e69 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -223,6 +223,7 @@ public: ~AutoCloseDir(); void operator =(DIR * dir); operator DIR *(); + void close(); }; From 524fa8a4f11826fdf22005f3304366856f72ffa5 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 22 Dec 2011 16:27:03 +0000 Subject: [PATCH 8/9] * Oops. --- src/libstore/gc.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 20f194e6e..feaab573e 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -657,7 +657,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) random_shuffle(entries_.begin(), entries_.end()); foreach (vector::iterator, i, entries_) - tryToDelete(state, nixStore + "/" + *i); + tryToDelete(state, *i); } catch (GCLimitReached & e) { } From 8c42a8c8ff2986940a41d46b0bdaa1c2ff0f15ad Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Sun, 25 Dec 2011 16:38:37 +0000 Subject: [PATCH 9/9] * Make sure that lock files are cleaned up properly when building through the build hook. --- src/libstore/build.cc | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index a8ef9b23e..149cd8b09 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -1300,6 +1300,13 @@ void DerivationGoal::buildDone() being valid. */ computeClosure(); + /* It is now safe to delete the lock files, since all future + lockers will see that the output paths are valid; they will + not create new lock files with the same names as the old + (unlinked) lock files. */ + outputLocks.setDeletion(true); + outputLocks.unlock(); + } catch (BuildError & e) { printMsg(lvlError, e.msg()); outputLocks.unlock(); @@ -1987,13 +1994,6 @@ void DerivationGoal::computeClosure() infos.push_back(info); } worker.store.registerValidPaths(infos); - - /* It is now safe to delete the lock files, since all future - lockers will see that the output paths are valid; they will not - create new lock files with the same names as the old (unlinked) - lock files. */ - outputLocks.setDeletion(true); - outputLocks.unlock(); }