From d9688ba70881f1735cf36161244a9a6b2acf4d48 Mon Sep 17 00:00:00 2001 From: Ben Radford Date: Wed, 26 Jul 2023 11:31:26 +0100 Subject: [PATCH 01/10] Add new remount-hook store parameter. --- src/libstore/local-overlay-store.hh | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/libstore/local-overlay-store.hh b/src/libstore/local-overlay-store.hh index 64e2ef488..f489c6ed6 100644 --- a/src/libstore/local-overlay-store.hh +++ b/src/libstore/local-overlay-store.hh @@ -41,6 +41,13 @@ struct LocalOverlayStoreConfig : virtual LocalStoreConfig default, but can be disabled if needed. )"}; + const PathSetting remountHook{(StoreConfig*) this, "", "remount-hook", + R"( + Script or program to run when overlay filesystem needs remounting. + + TODO: Document this in more detail. + )"}; + const std::string name() override { return "Experimental Local Overlay Store"; } std::string doc() override From cc6f8aa91a1716ed1801f51fb3a6a1fa468f338f Mon Sep 17 00:00:00 2001 From: Ben Radford Date: Wed, 26 Jul 2023 12:33:26 +0100 Subject: [PATCH 02/10] Test that delete works for duplicate file edge case. --- tests/overlay-local-store/delete-inner.sh | 38 +++++++++++++++++++++++ tests/overlay-local-store/delete.sh | 5 +++ tests/overlay-local-store/local.mk | 3 +- 3 files changed, 45 insertions(+), 1 deletion(-) create mode 100644 tests/overlay-local-store/delete-inner.sh create mode 100644 tests/overlay-local-store/delete.sh diff --git a/tests/overlay-local-store/delete-inner.sh b/tests/overlay-local-store/delete-inner.sh new file mode 100644 index 000000000..20b39c7fa --- /dev/null +++ b/tests/overlay-local-store/delete-inner.sh @@ -0,0 +1,38 @@ +#!/usr/bin/env bash + +set -eu -o pipefail + +set -x + +source common.sh + +# Avoid store dir being inside sandbox build-dir +unset NIX_STORE_DIR +unset NIX_STATE_DIR + +storeDirs + +initLowerStore + +mountOverlayfs + +# Add to overlay before lower to ensure file is duplicated +upperPath=$(nix-store --store "$storeB" --add delete.sh) +lowerPath=$(nix-store --store "$storeA" --add delete.sh) +[[ "$upperPath" = "$lowerPath" ]] + +# Check there really are two files with different inodes +upperInode=$(stat -c %i "$storeBRoot/$upperPath") +lowerInode=$(stat -c %i "$storeA/$lowerPath") +[[ "$upperInode" != "$lowerInode" ]] + +# Now delete file via the overlay store +nix-store --store "$storeB" --delete "$upperPath" + +# Check there is no longer a file in upper layer +expect 1 stat "$storeBTop/${upperPath##/nix/store/}" + +# Check that overlay file is now the one in lower layer +upperInode=$(stat -c %i "$storeBRoot/$upperPath") +lowerInode=$(stat -c %i "$storeA/$lowerPath") +[[ "$upperInode" = "$lowerInode" ]] diff --git a/tests/overlay-local-store/delete.sh b/tests/overlay-local-store/delete.sh new file mode 100644 index 000000000..79d67da71 --- /dev/null +++ b/tests/overlay-local-store/delete.sh @@ -0,0 +1,5 @@ +source common.sh + +requireEnvironment +setupConfig +execUnshare ./delete-inner.sh diff --git a/tests/overlay-local-store/local.mk b/tests/overlay-local-store/local.mk index 1f8de8f27..9233462a3 100644 --- a/tests/overlay-local-store/local.mk +++ b/tests/overlay-local-store/local.mk @@ -6,6 +6,7 @@ overlay-local-store-tests := \ $(d)/add-lower.sh \ $(d)/gc.sh \ $(d)/verify.sh \ - $(d)/optimise.sh + $(d)/optimise.sh \ + $(d)/delete.sh install-tests-groups += overlay-local-store From 11c493f8fa88a81645318844077392020618887f Mon Sep 17 00:00:00 2001 From: Ben Radford Date: Wed, 26 Jul 2023 13:21:38 +0100 Subject: [PATCH 03/10] Avoid creating whiteout for duplicate store paths. --- src/libstore/local-overlay-store.cc | 22 ++++++++++++++++++++-- src/libstore/local-overlay-store.hh | 2 ++ tests/overlay-local-store/delete-inner.sh | 1 + 3 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/libstore/local-overlay-store.cc b/src/libstore/local-overlay-store.cc index 47d09dc75..38c40fbad 100644 --- a/src/libstore/local-overlay-store.cc +++ b/src/libstore/local-overlay-store.cc @@ -183,11 +183,27 @@ void LocalOverlayStore::deleteGCPath(const Path & path, uint64_t & bytesFreed) warn("local-overlay: unexpected gc path '%s' ", path); return; } - if (pathExists(toUpperPath({path.substr(mergedDir.length())}))) { - LocalStore::deleteGCPath(path, bytesFreed); + + StorePath storePath = {path.substr(mergedDir.length())}; + auto upperPath = toUpperPath(storePath); + + if (pathExists(upperPath)) { + std::cerr << " upper exists" << std::endl; + if (lowerStore->isValidPath(storePath)) { + std::cerr << " lower exists" << std::endl; + // Path also exists in lower store. + // We must delete via upper layer to avoid creating a whiteout. + deletePath(upperPath, bytesFreed); + _remountRequired = true; + } else { + // Path does not exist in lower store. + // So we can delete via overlayfs and not need to remount. + LocalStore::deleteGCPath(path, bytesFreed); + } } } + void LocalOverlayStore::optimiseStore() { Activity act(*logger, actOptimiseStore); @@ -209,6 +225,7 @@ void LocalOverlayStore::optimiseStore() } } + Path LocalOverlayStore::toRealPathForHardLink(const StorePath & path) { return lowerStore->isValidPath(path) @@ -216,6 +233,7 @@ Path LocalOverlayStore::toRealPathForHardLink(const StorePath & path) : Store::toRealPath(path); } + static RegisterStoreImplementation regLocalOverlayStore; } diff --git a/src/libstore/local-overlay-store.hh b/src/libstore/local-overlay-store.hh index f489c6ed6..ffb310b7b 100644 --- a/src/libstore/local-overlay-store.hh +++ b/src/libstore/local-overlay-store.hh @@ -123,6 +123,8 @@ private: void optimiseStore() override; Path toRealPathForHardLink(const StorePath & storePath) override; + + bool _remountRequired = false; }; } diff --git a/tests/overlay-local-store/delete-inner.sh b/tests/overlay-local-store/delete-inner.sh index 20b39c7fa..051b59d5f 100644 --- a/tests/overlay-local-store/delete-inner.sh +++ b/tests/overlay-local-store/delete-inner.sh @@ -28,6 +28,7 @@ lowerInode=$(stat -c %i "$storeA/$lowerPath") # Now delete file via the overlay store nix-store --store "$storeB" --delete "$upperPath" +remountOverlayfs # Check there is no longer a file in upper layer expect 1 stat "$storeBTop/${upperPath##/nix/store/}" From 33ebae75ca4a7fd81585928b36a1cc18995f8212 Mon Sep 17 00:00:00 2001 From: Ben Radford Date: Wed, 26 Jul 2023 13:29:31 +0100 Subject: [PATCH 04/10] Reuse deletion logic for optimiseStore and rename method. --- src/libstore/gc.cc | 2 +- src/libstore/local-overlay-store.cc | 7 ++++--- src/libstore/local-overlay-store.hh | 2 +- src/libstore/local-store.cc | 2 +- src/libstore/local-store.hh | 2 +- 5 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index ea2868c58..a909d063b 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -653,7 +653,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) results.paths.insert(path); uint64_t bytesFreed; - deleteGCPath(realPath, bytesFreed); + deleteStorePath(realPath, bytesFreed); results.bytesFreed += bytesFreed; diff --git a/src/libstore/local-overlay-store.cc b/src/libstore/local-overlay-store.cc index 38c40fbad..8b89f68bc 100644 --- a/src/libstore/local-overlay-store.cc +++ b/src/libstore/local-overlay-store.cc @@ -176,7 +176,7 @@ void LocalOverlayStore::registerValidPaths(const ValidPathInfos & infos) } -void LocalOverlayStore::deleteGCPath(const Path & path, uint64_t & bytesFreed) +void LocalOverlayStore::deleteStorePath(const Path & path, uint64_t & bytesFreed) { auto mergedDir = realStoreDir.get() + "/"; if (path.substr(0, mergedDir.length()) != mergedDir) { @@ -198,7 +198,7 @@ void LocalOverlayStore::deleteGCPath(const Path & path, uint64_t & bytesFreed) } else { // Path does not exist in lower store. // So we can delete via overlayfs and not need to remount. - LocalStore::deleteGCPath(path, bytesFreed); + LocalStore::deleteStorePath(path, bytesFreed); } } } @@ -217,8 +217,9 @@ void LocalOverlayStore::optimiseStore() for (auto & path : paths) { if (lowerStore->isValidPath(path)) { + uint64_t bytesFreed = 0; // Deduplicate store path - deletePath(toUpperPath(path)); + deleteStorePath(Store::toRealPath(path), bytesFreed); } done++; act.progress(done, paths.size()); diff --git a/src/libstore/local-overlay-store.hh b/src/libstore/local-overlay-store.hh index ffb310b7b..d45f6bfc5 100644 --- a/src/libstore/local-overlay-store.hh +++ b/src/libstore/local-overlay-store.hh @@ -118,7 +118,7 @@ private: void queryRealisationUncached(const DrvOutput&, Callback> callback) noexcept override; - void deleteGCPath(const Path & path, uint64_t & bytesFreed) override; + void deleteStorePath(const Path & path, uint64_t & bytesFreed) override; void optimiseStore() override; diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 21acb3c38..2c18867f5 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -457,7 +457,7 @@ AutoCloseFD LocalStore::openGCLock() } -void LocalStore::deleteGCPath(const Path & path, uint64_t & bytesFreed) +void LocalStore::deleteStorePath(const Path & path, uint64_t & bytesFreed) { deletePath(path, bytesFreed); } diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 9a44722d4..9146f27a5 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -235,7 +235,7 @@ public: * The default implementation simply calls `deletePath`, but it can be * overridden by stores that wish to provide their own deletion behaviour. */ - virtual void deleteGCPath(const Path & path, uint64_t & bytesFreed); + virtual void deleteStorePath(const Path & path, uint64_t & bytesFreed); /** * Optimise the disk space usage of the Nix store by hard-linking From ed1428692409f94cd5553b66c9f5e6aa3c048e86 Mon Sep 17 00:00:00 2001 From: Ben Radford Date: Wed, 26 Jul 2023 14:05:54 +0100 Subject: [PATCH 05/10] Invoke remount-hook program when necessary. --- src/libstore/local-overlay-store.cc | 22 ++++++++++++++++++++++ src/libstore/local-overlay-store.hh | 4 ++++ tests/overlay-local-store/delete-inner.sh | 3 +-- tests/overlay-local-store/remount.sh | 2 ++ 4 files changed, 29 insertions(+), 2 deletions(-) create mode 100755 tests/overlay-local-store/remount.sh diff --git a/src/libstore/local-overlay-store.cc b/src/libstore/local-overlay-store.cc index 8b89f68bc..76c3adf5f 100644 --- a/src/libstore/local-overlay-store.cc +++ b/src/libstore/local-overlay-store.cc @@ -176,6 +176,14 @@ void LocalOverlayStore::registerValidPaths(const ValidPathInfos & infos) } +void LocalOverlayStore::collectGarbage(const GCOptions & options, GCResults & results) +{ + LocalStore::collectGarbage(options, results); + + remountIfNecessary(); +} + + void LocalOverlayStore::deleteStorePath(const Path & path, uint64_t & bytesFreed) { auto mergedDir = realStoreDir.get() + "/"; @@ -224,6 +232,8 @@ void LocalOverlayStore::optimiseStore() done++; act.progress(done, paths.size()); } + + remountIfNecessary(); } @@ -235,6 +245,18 @@ Path LocalOverlayStore::toRealPathForHardLink(const StorePath & path) } +void LocalOverlayStore::remountIfNecessary() +{ + if (remountHook.get().empty()) { + warn("'%s' needs remounting, set remount-hook to do this automatically", realStoreDir.get()); + } else { + runProgram(remountHook, false, {realStoreDir}); + } + + _remountRequired = false; +} + + static RegisterStoreImplementation regLocalOverlayStore; } diff --git a/src/libstore/local-overlay-store.hh b/src/libstore/local-overlay-store.hh index d45f6bfc5..a26392523 100644 --- a/src/libstore/local-overlay-store.hh +++ b/src/libstore/local-overlay-store.hh @@ -118,12 +118,16 @@ private: void queryRealisationUncached(const DrvOutput&, Callback> callback) noexcept override; + void collectGarbage(const GCOptions & options, GCResults & results) override; + void deleteStorePath(const Path & path, uint64_t & bytesFreed) override; void optimiseStore() override; Path toRealPathForHardLink(const StorePath & storePath) override; + void remountIfNecessary(); + bool _remountRequired = false; }; diff --git a/tests/overlay-local-store/delete-inner.sh b/tests/overlay-local-store/delete-inner.sh index 051b59d5f..f3878f657 100644 --- a/tests/overlay-local-store/delete-inner.sh +++ b/tests/overlay-local-store/delete-inner.sh @@ -27,8 +27,7 @@ lowerInode=$(stat -c %i "$storeA/$lowerPath") [[ "$upperInode" != "$lowerInode" ]] # Now delete file via the overlay store -nix-store --store "$storeB" --delete "$upperPath" -remountOverlayfs +nix-store --store "$storeB&remount-hook=$PWD/remount.sh" --delete "$upperPath" # Check there is no longer a file in upper layer expect 1 stat "$storeBTop/${upperPath##/nix/store/}" diff --git a/tests/overlay-local-store/remount.sh b/tests/overlay-local-store/remount.sh new file mode 100755 index 000000000..ee91c6b43 --- /dev/null +++ b/tests/overlay-local-store/remount.sh @@ -0,0 +1,2 @@ +#!/usr/bin/env bash +mount -o remount "$1" From 6da05c0a11365c8cd138c15600ec966e9c2b5940 Mon Sep 17 00:00:00 2001 From: Ben Radford Date: Wed, 26 Jul 2023 14:29:36 +0100 Subject: [PATCH 06/10] Rename test to delete-duplicate. --- .../{delete-inner.sh => delete-duplicate-inner.sh} | 4 ++-- tests/overlay-local-store/{delete.sh => delete-duplicate.sh} | 2 +- tests/overlay-local-store/local.mk | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) rename tests/overlay-local-store/{delete-inner.sh => delete-duplicate-inner.sh} (87%) rename tests/overlay-local-store/{delete.sh => delete-duplicate.sh} (55%) diff --git a/tests/overlay-local-store/delete-inner.sh b/tests/overlay-local-store/delete-duplicate-inner.sh similarity index 87% rename from tests/overlay-local-store/delete-inner.sh rename to tests/overlay-local-store/delete-duplicate-inner.sh index f3878f657..6053451d8 100644 --- a/tests/overlay-local-store/delete-inner.sh +++ b/tests/overlay-local-store/delete-duplicate-inner.sh @@ -17,8 +17,8 @@ initLowerStore mountOverlayfs # Add to overlay before lower to ensure file is duplicated -upperPath=$(nix-store --store "$storeB" --add delete.sh) -lowerPath=$(nix-store --store "$storeA" --add delete.sh) +upperPath=$(nix-store --store "$storeB" --add delete-duplicate.sh) +lowerPath=$(nix-store --store "$storeA" --add delete-duplicate.sh) [[ "$upperPath" = "$lowerPath" ]] # Check there really are two files with different inodes diff --git a/tests/overlay-local-store/delete.sh b/tests/overlay-local-store/delete-duplicate.sh similarity index 55% rename from tests/overlay-local-store/delete.sh rename to tests/overlay-local-store/delete-duplicate.sh index 79d67da71..0c0b1a3b2 100644 --- a/tests/overlay-local-store/delete.sh +++ b/tests/overlay-local-store/delete-duplicate.sh @@ -2,4 +2,4 @@ source common.sh requireEnvironment setupConfig -execUnshare ./delete-inner.sh +execUnshare ./delete-duplicate-inner.sh diff --git a/tests/overlay-local-store/local.mk b/tests/overlay-local-store/local.mk index 9233462a3..7de46ab02 100644 --- a/tests/overlay-local-store/local.mk +++ b/tests/overlay-local-store/local.mk @@ -7,6 +7,6 @@ overlay-local-store-tests := \ $(d)/gc.sh \ $(d)/verify.sh \ $(d)/optimise.sh \ - $(d)/delete.sh + $(d)/delete-duplicate.sh install-tests-groups += overlay-local-store From 5744a500d66214f6d833100924a2f7362b7b82a7 Mon Sep 17 00:00:00 2001 From: Ben Radford Date: Wed, 26 Jul 2023 14:33:47 +0100 Subject: [PATCH 07/10] Use debug instead of writing directly to stderr. --- src/libstore/local-overlay-store.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libstore/local-overlay-store.cc b/src/libstore/local-overlay-store.cc index 76c3adf5f..dfd2f0d34 100644 --- a/src/libstore/local-overlay-store.cc +++ b/src/libstore/local-overlay-store.cc @@ -196,9 +196,9 @@ void LocalOverlayStore::deleteStorePath(const Path & path, uint64_t & bytesFreed auto upperPath = toUpperPath(storePath); if (pathExists(upperPath)) { - std::cerr << " upper exists" << std::endl; + debug("upper exists: %s", path); if (lowerStore->isValidPath(storePath)) { - std::cerr << " lower exists" << std::endl; + debug("lower exists: %s", storePath.to_string()); // Path also exists in lower store. // We must delete via upper layer to avoid creating a whiteout. deletePath(upperPath, bytesFreed); From ca1a108dad93f00f18929c02bf29bbe5347035ac Mon Sep 17 00:00:00 2001 From: Ben Radford <104896700+benradf@users.noreply.github.com> Date: Wed, 26 Jul 2023 14:37:35 +0100 Subject: [PATCH 08/10] Update tests/overlay-local-store/remount.sh Co-authored-by: John Ericson --- tests/overlay-local-store/remount.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/overlay-local-store/remount.sh b/tests/overlay-local-store/remount.sh index ee91c6b43..0b06debb5 100755 --- a/tests/overlay-local-store/remount.sh +++ b/tests/overlay-local-store/remount.sh @@ -1,2 +1,2 @@ -#!/usr/bin/env bash +#!/bin/sh mount -o remount "$1" From 3a9fe1a085edc8db3e3cd35531829ddab51bcd81 Mon Sep 17 00:00:00 2001 From: Ben Radford Date: Wed, 26 Jul 2023 14:44:14 +0100 Subject: [PATCH 09/10] Made remountRequired atomic to avoid concurrency issues. --- src/libstore/local-overlay-store.hh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/local-overlay-store.hh b/src/libstore/local-overlay-store.hh index a26392523..66a43d84a 100644 --- a/src/libstore/local-overlay-store.hh +++ b/src/libstore/local-overlay-store.hh @@ -128,7 +128,7 @@ private: void remountIfNecessary(); - bool _remountRequired = false; + std::atomic_bool _remountRequired = false; }; } From c2d54496a0c8611a7008181e2dc4fc57d520752f Mon Sep 17 00:00:00 2001 From: Ben Radford Date: Wed, 26 Jul 2023 14:47:44 +0100 Subject: [PATCH 10/10] Forgot to check flag and early out. --- src/libstore/local-overlay-store.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libstore/local-overlay-store.cc b/src/libstore/local-overlay-store.cc index dfd2f0d34..04a1717cd 100644 --- a/src/libstore/local-overlay-store.cc +++ b/src/libstore/local-overlay-store.cc @@ -247,6 +247,8 @@ Path LocalOverlayStore::toRealPathForHardLink(const StorePath & path) void LocalOverlayStore::remountIfNecessary() { + if (!_remountRequired) return; + if (remountHook.get().empty()) { warn("'%s' needs remounting, set remount-hook to do this automatically", realStoreDir.get()); } else {