Merge pull request #7126 from squalus/fsync-store-paths

Add fsync-store-paths option

doc/manual/rl-next/fsync-store-paths.md

@@ -0,0 +1,9 @@
+---
+synopsis: Add setting `fsync-store-paths`
+issues: [1218]
+prs: [7126]
+---
+
+Nix now has a setting `fsync-store-paths` that ensures that new store paths are durably written to disk before they are registered as "valid" in Nix's database. This can prevent Nix store corruption if the system crashes or there is a power loss. This setting defaults to `false`.
+
+Author: [**@squalus**](https://github.com/squalus)

src/libstore/globals.hh

@@ -399,10 +399,18 @@ public:
           default is `true`.
         )"};
 
+    Setting<bool> fsyncStorePaths{this, false, "fsync-store-paths",
+        R"(
+          Whether to call `fsync()` on store paths before registering them, to
+          flush them to disk. This improves robustness in case of system crashes,
+          but reduces performance. The default is `false`.
+        )"};
+
     Setting<bool> useSQLiteWAL{this, !isWSL1(), "use-sqlite-wal",
         "Whether SQLite should use WAL mode."};
 
 #ifndef _WIN32
+    // FIXME: remove this option, `fsync-store-paths` is faster.
     Setting<bool> syncBeforeRegistering{this, false, "sync-before-registering",
         "Whether to call `sync()` before registering a path as valid."};
 #endif

src/libstore/local-store.cc

@@ -1137,7 +1137,7 @@ void LocalStore::addToStore(const ValidPathInfo & info, Source & source,
             TeeSource wrapperSource { source, hashSink };
 
             narRead = true;
-            restorePath(realPath, wrapperSource);
+            restorePath(realPath, wrapperSource, settings.fsyncStorePaths);
 
             auto hashResult = hashSink.finish();
 
@@ -1191,6 +1191,11 @@ void LocalStore::addToStore(const ValidPathInfo & info, Source & source,
 
             optimisePath(realPath, repair); // FIXME: combine with hashPath()
 
+            if (settings.fsyncStorePaths) {
+                recursiveSync(realPath);
+                syncParent(realPath);
+            }
+
             registerValidPath(info);
         }
 
@@ -1271,7 +1276,7 @@ StorePath LocalStore::addToStoreFromDump(
         delTempDir = std::make_unique<AutoDelete>(tempDir);
         tempPath = tempDir / "x";
 
-        restorePath(tempPath.string(), bothSource, dumpMethod);
+        restorePath(tempPath.string(), bothSource, dumpMethod, settings.fsyncStorePaths);
 
         dumpBuffer.reset();
         dump = {};
@@ -1318,7 +1323,7 @@ StorePath LocalStore::addToStoreFromDump(
                 switch (fim) {
                 case FileIngestionMethod::Flat:
                 case FileIngestionMethod::NixArchive:
-                    restorePath(realPath, dumpSource, (FileSerialisationMethod) fim);
+                    restorePath(realPath, dumpSource, (FileSerialisationMethod) fim, settings.fsyncStorePaths);
                     break;
                 case FileIngestionMethod::Git:
                     // doesn't correspond to serialization method, so
@@ -1343,6 +1348,11 @@ StorePath LocalStore::addToStoreFromDump(
 
             optimisePath(realPath, repair);
 
+            if (settings.fsyncStorePaths) {
+                recursiveSync(realPath);
+                syncParent(realPath);
+            }
+
             ValidPathInfo info {
                 *this,
                 name,

src/libutil/archive.cc

@@ -294,9 +294,9 @@ void parseDump(FileSystemObjectSink & sink, Source & source)
 }
 
 
-void restorePath(const std::filesystem::path & path, Source & source)
+void restorePath(const std::filesystem::path & path, Source & source, bool startFsync)
 {
-    RestoreSink sink;
+    RestoreSink sink{startFsync};
     sink.dstPath = path;
     parseDump(sink, source);
 }

src/libutil/archive.hh

@@ -75,7 +75,7 @@ void dumpString(std::string_view s, Sink & sink);
 
 void parseDump(FileSystemObjectSink & sink, Source & source);
 
-void restorePath(const std::filesystem::path & path, Source & source);
+void restorePath(const std::filesystem::path & path, Source & source, bool startFsync = false);
 
 /**
  * Read a NAR from 'source' and write it to 'sink'.

src/libutil/file-content-address.cc

@@ -88,14 +88,15 @@ void dumpPath(
 void restorePath(
     const Path & path,
     Source & source,
-    FileSerialisationMethod method)
+    FileSerialisationMethod method,
+    bool startFsync)
 {
     switch (method) {
     case FileSerialisationMethod::Flat:
-        writeFile(path, source);
+        writeFile(path, source, 0666, startFsync);
         break;
     case FileSerialisationMethod::NixArchive:
-        restorePath(path, source);
+        restorePath(path, source, startFsync);
         break;
     }
 }

src/libutil/file-content-address.hh

@@ -70,7 +70,8 @@ void dumpPath(
 void restorePath(
     const Path & path,
     Source & source,
-    FileSerialisationMethod method);
+    FileSerialisationMethod method,
+    bool startFsync = false);
 
 
 /**

src/libutil/file-descriptor.cc

@@ -92,7 +92,7 @@ void AutoCloseFD::close()
     }
 }
 
-void AutoCloseFD::fsync()
+void AutoCloseFD::fsync() const
 {
     if (fd != INVALID_DESCRIPTOR) {
         int result;
@@ -111,6 +111,18 @@ void AutoCloseFD::fsync()
 }
 
 
+
+void AutoCloseFD::startFsync() const
+{
+#if __linux__
+        if (fd != -1) {
+            /* Ignore failure, since fsync must be run later anyway. This is just a performance optimization. */
+            ::sync_file_range(fd, 0, 0, SYNC_FILE_RANGE_WRITE);
+        }
+#endif
+}
+
+
 AutoCloseFD::operator bool() const
 {
     return fd != INVALID_DESCRIPTOR;

src/libutil/file-descriptor.hh

@@ -128,7 +128,18 @@ public:
     explicit operator bool() const;
     Descriptor release();
     void close();
-    void fsync();
+
+    /**
+     * Perform a blocking fsync operation.
+     */
+    void fsync() const;
+
+    /**
+     * Asynchronously flush to disk without blocking, if available on
+     * the platform. This is just a performance optimization, and
+     * fsync must be run later even if this is called.
+     */
+    void startFsync() const;
 };
 
 class Pipe

src/libutil/file-system.cc

@@ -11,6 +11,7 @@
 #include <climits>
 #include <cstdio>
 #include <cstdlib>
+#include <deque>
 #include <sstream>
 #include <filesystem>
 
@@ -318,6 +319,50 @@ void syncParent(const Path & path)
 }
 
 
+void recursiveSync(const Path & path)
+{
+    /* If it's a file, just fsync and return. */
+    auto st = lstat(path);
+    if (S_ISREG(st.st_mode)) {
+        AutoCloseFD fd = open(path.c_str(), O_RDONLY, 0);
+        if (!fd)
+            throw SysError("opening file '%1%'", path);
+        fd.fsync();
+        return;
+    }
+
+    /* Otherwise, perform a depth-first traversal of the directory and
+       fsync all the files. */
+    std::deque<fs::path> dirsToEnumerate;
+    dirsToEnumerate.push_back(path);
+    std::vector<fs::path> dirsToFsync;
+    while (!dirsToEnumerate.empty()) {
+        auto currentDir = dirsToEnumerate.back();
+        dirsToEnumerate.pop_back();
+        for (auto & entry : std::filesystem::directory_iterator(currentDir)) {
+            auto st = entry.symlink_status();
+            if (fs::is_directory(st)) {
+                dirsToEnumerate.emplace_back(entry.path());
+            } else if (fs::is_regular_file(st)) {
+                AutoCloseFD fd = open(entry.path().c_str(), O_RDONLY, 0);
+                if (!fd)
+                    throw SysError("opening file '%1%'", entry.path());
+                fd.fsync();
+            }
+        }
+        dirsToFsync.emplace_back(std::move(currentDir));
+    }
+
+    /* Fsync all the directories. */
+    for (auto dir = dirsToFsync.rbegin(); dir != dirsToFsync.rend(); ++dir) {
+        AutoCloseFD fd = open(dir->c_str(), O_RDONLY, 0);
+        if (!fd)
+            throw SysError("opening directory '%1%'", *dir);
+        fd.fsync();
+    }
+}
+
+
 static void _deletePath(Descriptor parentfd, const fs::path & path, uint64_t & bytesFreed)
 {
 #ifndef _WIN32

src/libutil/file-system.hh

@@ -134,10 +134,15 @@ void writeFile(const Path & path, std::string_view s, mode_t mode = 0666, bool s
 void writeFile(const Path & path, Source & source, mode_t mode = 0666, bool sync = false);
 
 /**
- * Flush a file's parent directory to disk
+ * Flush a path's parent directory to disk.
  */
 void syncParent(const Path & path);
 
+/**
+ * Flush a file or entire directory tree to disk.
+ */
+void recursiveSync(const Path & path);
+
 /**
  * Delete a path; i.e., in the case of a directory, it is deleted
  * recursively. It's not an error if the path does not exist. The

src/libutil/fs-sink.cc

@@ -76,6 +76,17 @@ void RestoreSink::createDirectory(const CanonPath & path)
 
 struct RestoreRegularFile : CreateRegularFileSink {
     AutoCloseFD fd;
+    bool startFsync = false;
+
+    ~RestoreRegularFile()
+    {
+        /* Initiate an fsync operation without waiting for the
+           result. The real fsync should be run before registering a
+           store path, but this is a performance optimization to allow
+           the disk write to start early. */
+        if (fd && startFsync)
+            fd.startFsync();
+    }
 
     void operator () (std::string_view data) override;
     void isExecutable() override;
@@ -95,6 +106,7 @@ void RestoreSink::createRegularFile(const CanonPath & path, std::function<void(C
     auto p = append(dstPath, path);
 
     RestoreRegularFile crf;
+    crf.startFsync = startFsync;
     crf.fd =
 #ifdef _WIN32
         CreateFileW(p.c_str(), GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL)

src/libutil/fs-sink.hh

@@ -78,6 +78,11 @@ struct NullFileSystemObjectSink : FileSystemObjectSink
 struct RestoreSink : FileSystemObjectSink
 {
     std::filesystem::path dstPath;
+    bool startFsync = false;
+
+    explicit RestoreSink(bool startFsync)
+        : startFsync{startFsync}
+    { }
 
     void createDirectory(const CanonPath & path) override;
 

tests/nixos/default.nix

@@ -155,4 +155,6 @@ in
   user-sandboxing = runNixOSTestFor "x86_64-linux" ./user-sandboxing;
 
   s3-binary-cache-store = runNixOSTestFor "x86_64-linux" ./s3-binary-cache-store.nix;
+
+  fsync = runNixOSTestFor "x86_64-linux" ./fsync.nix;
 }

tests/nixos/fsync.nix

@@ -0,0 +1,39 @@
+{ lib, config, nixpkgs, pkgs, ... }:
+
+let
+  pkg1 = pkgs.go;
+in
+
+{
+  name = "fsync";
+
+  nodes.machine =
+    { config, lib, pkgs, ... }:
+    { virtualisation.emptyDiskImages = [ 1024 ];
+      environment.systemPackages = [ pkg1 ];
+      nix.settings.experimental-features = [ "nix-command" ];
+      nix.settings.fsync-store-paths = true;
+      nix.settings.require-sigs = false;
+      boot.supportedFilesystems = [ "ext4" "btrfs" "xfs" ];
+    };
+
+  testScript = { nodes }: ''
+    # fmt: off
+    for fs in ("ext4", "btrfs", "xfs"):
+      machine.succeed("mkfs.{} {} /dev/vdb".format(fs, "-F" if fs == "ext4" else "-f"))
+      machine.succeed("mkdir -p /mnt")
+      machine.succeed("mount /dev/vdb /mnt")
+      machine.succeed("sync")
+      machine.succeed("nix copy --offline ${pkg1} --to /mnt")
+      machine.crash()
+
+      machine.start()
+      machine.wait_for_unit("multi-user.target")
+      machine.succeed("mkdir -p /mnt")
+      machine.succeed("mount /dev/vdb /mnt")
+      machine.succeed("nix path-info --offline --store /mnt ${pkg1}")
+      machine.succeed("nix store verify --all --store /mnt --no-trust")
+
+      machine.succeed("umount /dev/vdb")
+  '';
+}