Careful mutating the global worktodo.txt

Author: preda

Makefile

@@ -31,7 +31,7 @@ STRIP=-s
 
 endif
 
-SRCS1 = TuneEntry.cpp Primes.cpp tune.cpp CycleFile.cpp TrigBufCache.cpp Event.cpp Queue.cpp TimeInfo.cpp Profile.cpp bundle.cpp Saver.cpp KernelCompiler.cpp Kernel.cpp gpuid.cpp File.cpp Proof.cpp log.cpp Worktodo.cpp common.cpp main.cpp Gpu.cpp clwrap.cpp Task.cpp timeutil.cpp Args.cpp state.cpp Signal.cpp FFTConfig.cpp AllocTrac.cpp sha3.cpp md5.cpp version.cpp
+SRCS1 = fs.cpp TuneEntry.cpp Primes.cpp tune.cpp CycleFile.cpp TrigBufCache.cpp Event.cpp Queue.cpp TimeInfo.cpp Profile.cpp bundle.cpp Saver.cpp KernelCompiler.cpp Kernel.cpp gpuid.cpp File.cpp Proof.cpp log.cpp Worktodo.cpp common.cpp main.cpp Gpu.cpp clwrap.cpp Task.cpp timeutil.cpp Args.cpp state.cpp Signal.cpp FFTConfig.cpp AllocTrac.cpp sha3.cpp md5.cpp version.cpp
 
 SRCS2 = test.cpp
 

src/CMakeLists.txt

@@ -19,6 +19,7 @@ add_executable(gpuowl
   CycleFile.cpp
   tune.cpp
   TuneEntry.cpp
+  fs.cpp
   )
 
 target_link_libraries(gpuowl OpenCL)

src/CycleFile.cpp

@@ -1,37 +1,19 @@
 // Copyright (C) Mihai Preda
 
 #include "CycleFile.h"
-#include <filesystem>
+#include "fs.h"
 
 CycleFile::CycleFile(const fs::path& name, bool keepOld) :
   name{name},
   f{File::openWrite(name + ".new")},
   keepOld{keepOld}
 {}
 
+
 CycleFile::~CycleFile() {
   if (!f) { return; }
-
-  string old = name + ".bak";
-  string tmp = f->name;
-
   f.reset();
-
-  std::error_code dummy;
-  try {
-    if (keepOld) { fs::rename(name, old, dummy); }
-
-    // The normal behavior of rename() is to unlink a pre-existing destination name and to rename successfully
-    // See https://en.cppreference.com/w/cpp/filesystem/rename
-    // But this behavior is not obeyed by some Windows implementations (mingw/msys?)
-    fs::rename(tmp, name);
-  } catch (const fs::filesystem_error& e) {
-    // So if rename() throws, we attempt to remove the destination explicitly
-    fs::remove(old, dummy);
-    fs::rename(name, old, dummy);
-    fs::rename(tmp, name, dummy);
-    if (!keepOld) { fs::remove(old, dummy); }
-  }
+  fancyRename(name + ".new", name, keepOld);
 }
 
 File* CycleFile::operator->() { return f.operator->(); }

src/File.h

@@ -26,11 +26,6 @@
 
 namespace fs = std::filesystem;
 
-inline fs::path operator+(fs::path p, const std::string& tail) {
-  p += tail;
-  return p;
-}
-
 class File {
   FILE* f = nullptr;
   const bool readOnly;

src/Worktodo.cpp

@@ -2,16 +2,15 @@
 
 #include "Worktodo.h"
 
-#include "CycleFile.h"
 #include "Task.h"
 #include "File.h"
 #include "common.h"
 #include "Args.h"
+#include "fs.h"
 
 #include <cassert>
 #include <string>
 #include <optional>
-#include <mutex>
 #include <charconv>
 
 namespace {
@@ -68,28 +67,6 @@ std::optional<Task> parse(const std::string& line) {
   return {};
 }
 
-bool deleteLine(const fs::path& fileName, const std::string& targetLine) {
-  assert(!targetLine.empty());
-  bool lineDeleted = false;
-
-  CycleFile fo{fileName};
-  for (const string& line : File::openReadThrow(fileName)) {
-    // log("line '%s'\n", line.c_str());
-    if (!lineDeleted && line == targetLine) {
-      lineDeleted = true;
-    } else {
-      fo->write(line);
-    }
-  }
-
-  if (!lineDeleted) {
-    log("'%s': did not find the line '%s' to delete\n", fileName.string().c_str(), targetLine.c_str());
-    fo.reset();
-  }
-
-  return lineDeleted;
-}
-
 // Among the valid tasks from fileName, return the "best" which means with the smallest exponent
 static std::optional<Task> bestTask(const fs::path& fileName) {
   optional<Task> best;
@@ -100,34 +77,60 @@ static std::optional<Task> bestTask(const fs::path& fileName) {
   return best;
 }
 
-static string workName(i32 instance) { return "work-" + to_string(instance) + ".txt"; }
+string workName(i32 instance) { return "worktodo-" + to_string(instance) + ".txt"; }
 
 optional<Task> getWork(Args& args, i32 instance) {
-  static mutex mut;
-
- again:
-  // Try to get a task from the local work-<N> file.
-  // This only reads from the per-worker file, so we don't need to lock.
+  // Try to get a task from the local worktodo-<N> file.
   if (optional<Task> task = bestTask(workName(instance))) { return task; }
 
-  // Try in order: the local worktodo.txt, and the global worktodo.txt if set up.
-  vector<fs::path> worktodoFiles{"worktodo.txt"};
-  if (!args.masterDir.empty()) { worktodoFiles.push_back(args.masterDir / "worktodo.txt"); }
+  if (args.masterDir.empty()) { return {}; }
+
+  fs::path worktodo = args.masterDir / "worktodo.txt";
 
-  lock_guard lock(mut);
+  /*
+    We need to aquire a task from the global worktodo.txt, and "atomically"
+    add the task to the local worktodo-N.txt and remove it from worktodo.txt
 
-  for (fs::path& worktodo : worktodoFiles) {
-    if (optional<Task> task = bestTask(worktodo)) {
-      File::append(workName(instance), task->line);
-      deleteLine(worktodo, task->line);
-      goto again;
+    Below we call the global worktodo.txt "global worktodo", and worktodo-N.txt "local worktodo".
+
+    We want to avoid filesystem-based locking, so we approximate it this way:
+    1. read the file-size of the global worktodo
+    2. read one task from the global worktodo
+    3. append the task to the local worktodo
+    4. write the new content of the global worktodo without the task to a temporary file
+    5. compare the size of the global worktodo with its initial size (as an heuristic to detect modifications to it)
+    6a. if the size is not changed, rename the temporary file to global worktodo and done
+    6b. if the size is changed (i.e. global worktodo was modified in the meantime):
+       7. remove the task from the local worktodo (undo the local task add)
+       8. start again (from step 1)
+  */
+
+  for (int retry = 0; retry < 2; ++retry) {
+    u64 initialSize = fileSize(worktodo);
+    if (!initialSize) { return {}; }
+
+    optional<Task> task = bestTask(worktodo);
+    if (!task) { return {}; }
+
+    string workLine = task->line;
+    File::append(workName(instance), workLine);
+
+    if (deleteLine(worktodo, workLine, initialSize)) {
+      return task;
     }
+
+    // Undo add to local worktodo
+    [[maybe_unused]] bool found = deleteLine(workName(instance), workLine);
+    assert(found);
   }
 
-  return std::nullopt;
+  log("Could not extract a task from '%s'\n", worktodo.string().c_str());
+  // must be tough luck to be preempted twice while mutating the global worktodo
+  assert(false);
+  return {};
 }
 
-}
+} // namespace
 
 std::optional<Task> Worktodo::getTask(Args &args, i32 instance) {
   if (instance == 0) {

src/common.cpp

@@ -12,7 +12,6 @@
 #include <iomanip>
 #include <filesystem>
 
-
 string hex(u64 x) {
   ostringstream out{};
   out << setbase(16) << setfill('0') << setw(16) << x;

src/fs.cpp

@@ -0,0 +1,66 @@
+// Copyright (C) Mihai Preda
+
+#include "fs.h"
+#include "File.h"
+
+#include <thread>
+
+namespace {
+
+std::string toString(const std::thread::id& x) {
+  std::ostringstream os;
+  os << x;
+  return os.str();
+}
+
+bool sizeMatches(const fs::path& path, u64 initialSize) {
+  error_code dummy;
+  return !initialSize || (initialSize == fs::file_size(path, dummy));
+}
+
+} // namespace
+
+void fancyRename(const fs::path& src, const fs::path& dst, bool keepOld) {
+  try {
+    // The normal behavior of rename() is to unlink a pre-existing destination name and to rename successfully
+    // See https://en.cppreference.com/w/cpp/filesystem/rename
+    // But this behavior is not obeyed by some Windows implementations (mingw/msys?)
+    if (keepOld) { fs::rename(dst, dst + ".bak"); }
+    fs::rename(src, dst);
+  } catch (const fs::filesystem_error& e) {
+    // So if rename() throws, we attempt to remove the destination explicitly
+    std::error_code dummy;
+    fs::remove(dst + ".bak", dummy);
+    fs::rename(dst, dst + ".bak", dummy);
+    fs::rename(src, dst); // If this rename does not succeed, let it throw
+    if (!keepOld) { fs::remove(dst + ".bak", dummy); }
+  }
+}
+
+u64 fileSize(const fs::path& path) {
+  error_code dummy;
+  auto size = fs::file_size(path, dummy);
+  if (size == decltype(size)(-1)) { size = 0; }
+  return size;
+}
+
+bool deleteLine(const fs::path& path, const string& targetLine, u64 initialSize) {
+  fs::path tmp = path + ("-"s + toString(this_thread::get_id()));
+
+  File fi = File::openRead(path);
+  File fo = File::openWrite(tmp);
+
+  bool found = false;
+  for (const string& line : fi) {
+    if (!found && line == targetLine) {
+      found = true;
+    } else {
+      fo.write(line);
+    }
+  }
+
+  if (!found || !sizeMatches(path, initialSize)) { return false; }
+
+  fancyRename(tmp, path);
+  return true;
+}

src/fs.h

@@ -0,0 +1,16 @@
+// Copyright (C) Mihai Preda
+
+#include "common.h"
+
+#include <filesystem>
+
+inline fs::path operator+(fs::path p, const std::string& tail) {
+  p += tail;
+  return p;
+}
+
+u64 fileSize(const fs::path& path);
+
+void fancyRename(const fs::path& src, const fs::path& dst, bool keepOld = false);
+
+bool deleteLine(const fs::path& path, const string& targetLine, u64 initialSize = 0);