Avoid usage of ReopenWritableFile in db_stress (#9649)

Summary:
The UniqueIdVerifier constructor currently calls ReopenWritableFile on
the FileSystem, which might not be supported. Instead of relying on
reopening the unique IDs file for writing, create a new file and copy
the original contents.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/9649

Test Plan: Run db_stress

Reviewed By: pdillinger

Differential Revision: D34572307

Pulled By: anand1976

fbshipit-source-id: 3a777908582d79dae57488d4278bad126774f698

db_stress_tool/db_stress_listener.cc

@@ -7,6 +7,7 @@
 
 #include <cstdint>
 
+#include "file/file_util.h"
 #include "rocksdb/file_system.h"
 #include "util/coding_lean.h"
 
@@ -37,10 +38,31 @@ UniqueIdVerifier::UniqueIdVerifier(const std::string& db_name, Env* env)
     exit(1);
   }
 
+  // Avoid relying on ReopenWritableFile which is not supported by all
+  // file systems. Create a new file and copy the old file contents to it.
+  std::string tmp_path = path_ + ".tmp";
+  st = fs->FileExists(tmp_path, opts, /*dbg*/ nullptr);
+  if (st.IsNotFound()) {
+    st = fs->RenameFile(path_, tmp_path, opts, /*dbg*/ nullptr);
+    // Either it should succeed or fail because src path doesn't exist
+    assert(st.ok() || st.IsPathNotFound());
+  } else {
+    // If path_ and tmp_path both exist, retain tmp_path as its
+    // guaranteed to be more complete. The order of operations are -
+    // 1. Rename path_ to tmp_path
+    // 2. Parse tmp_path contents
+    // 3. Create path_
+    // 4. Copy tmp_path contents to path_
+    // 5. Delete tmp_path
+    st = fs->DeleteFile(path_, opts, /*dbg*/ nullptr);
+    assert(st.ok() || st.IsPathNotFound());
+  }
+
+  uint64_t size = 0;
   {
     std::unique_ptr<FSSequentialFile> reader;
-    Status s =
+    Status s = fs->NewSequentialFile(tmp_path, FileOptions(), &reader,
-        fs->NewSequentialFile(path_, FileOptions(), &reader, /*dbg*/ nullptr);
+                                     /*dbg*/ nullptr);
     if (s.ok()) {
       // Load from file
       std::string id(24U, '\0');
@@ -60,37 +82,57 @@ UniqueIdVerifier::UniqueIdVerifier(const std::string& db_name, Env* env)
             fprintf(stdout, "Warning: clearing corrupt unique id file\n");
             id_set_.clear();
             reader.reset();
-            s = fs->DeleteFile(path_, opts, /*dbg*/ nullptr);
+            s = fs->DeleteFile(tmp_path, opts, /*dbg*/ nullptr);
             assert(s.ok());
+            size = 0;
           }
           break;
         }
+        size += 24U;
         VerifyNoWrite(id);
       }
     } else {
       // Newly created is ok.
       // But FileSystem doesn't tell us whether non-existence was the cause of
       // the failure. (Issue #9021)
-      Status s2 = fs->FileExists(path_, opts, /*dbg*/ nullptr);
+      Status s2 = fs->FileExists(tmp_path, opts, /*dbg*/ nullptr);
       if (!s2.IsNotFound()) {
         fprintf(stderr, "Error opening unique id file: %s\n",
                 s.ToString().c_str());
         assert(false);
       }
+      size = 0;
     }
   }
   fprintf(stdout, "(Re-)verified %zu unique IDs\n", id_set_.size());
-  Status s = fs->ReopenWritableFile(path_, FileOptions(), &data_file_writer_,
+
-                                    /*dbg*/ nullptr);
+  std::unique_ptr<FSWritableFile> file_writer;
-  if (!s.ok()) {
+  st = fs->NewWritableFile(path_, FileOptions(), &file_writer, /*dbg*/ nullptr);
-    fprintf(stderr, "Error opening unique id file for append: %s\n",
+  if (!st.ok()) {
-            s.ToString().c_str());
+    fprintf(stderr, "Error creating the unique ids file: %s\n",
+            st.ToString().c_str());
+    assert(false);
+  }
+  data_file_writer_.reset(
+      new WritableFileWriter(std::move(file_writer), path_, FileOptions()));
+
+  if (size > 0) {
+    st = CopyFile(fs.get(), tmp_path, data_file_writer_, size,
+                  /*use_fsync*/ true, /*io_tracer*/ nullptr,
+                  /*temparature*/ Temperature::kHot);
+    if (!st.ok()) {
+      fprintf(stderr, "Error copying contents of old unique id file: %s\n",
+              st.ToString().c_str());
       assert(false);
     }
+  }
+  st = fs->DeleteFile(tmp_path, opts, /*dbg*/ nullptr);
+  assert(st.ok() || st.IsPathNotFound());
 }
 
 UniqueIdVerifier::~UniqueIdVerifier() {
-  data_file_writer_->Close(IOOptions(), /*dbg*/ nullptr);
+  IOStatus s = data_file_writer_->Close();
+  assert(s.ok());
 }
 
 void UniqueIdVerifier::VerifyNoWrite(const std::string& id) {
@@ -112,14 +154,13 @@ void UniqueIdVerifier::Verify(const std::string& id) {
   if (id_set_.size() >= 4294967) {
     return;
   }
-  IOStatus s =
+  IOStatus s = data_file_writer_->Append(Slice(id));
-      data_file_writer_->Append(Slice(id), IOOptions(), /*dbg*/ nullptr);
   if (!s.ok()) {
     fprintf(stderr, "Error writing to unique id file: %s\n",
             s.ToString().c_str());
     assert(false);
   }
-  s = data_file_writer_->Flush(IOOptions(), /*dbg*/ nullptr);
+  s = data_file_writer_->Flush();
   if (!s.ok()) {
     fprintf(stderr, "Error flushing unique id file: %s\n",
             s.ToString().c_str());

db_stress_tool/db_stress_listener.h

@@ -10,6 +10,7 @@
 #include <unordered_set>
 
 #include "file/filename.h"
+#include "file/writable_file_writer.h"
 #include "rocksdb/db.h"
 #include "rocksdb/env.h"
 #include "rocksdb/file_system.h"
@@ -39,7 +40,7 @@ class UniqueIdVerifier {
   std::mutex mutex_;
   // IDs persisted to a hidden file inside DB dir
   std::string path_;
-  std::unique_ptr<FSWritableFile> data_file_writer_;
+  std::unique_ptr<WritableFileWriter> data_file_writer_;
   // Starting byte for which 8 bytes to check in memory within 24 byte ID
   size_t offset_;
   // Working copy of the set of 8 byte pieces

file/file_util.cc

@@ -18,13 +18,13 @@ namespace ROCKSDB_NAMESPACE {
 
 // Utility function to copy a file up to a specified length
 IOStatus CopyFile(FileSystem* fs, const std::string& source,
-                  const std::string& destination, uint64_t size, bool use_fsync,
+                  std::unique_ptr<WritableFileWriter>& dest_writer,
+                  uint64_t size, bool use_fsync,
                   const std::shared_ptr<IOTracer>& io_tracer,
                   const Temperature temperature) {
   FileOptions soptions;
   IOStatus io_s;
   std::unique_ptr<SequentialFileReader> src_reader;
-  std::unique_ptr<WritableFileWriter> dest_writer;
 
   {
     soptions.temperature = temperature;
@@ -33,11 +33,6 @@ IOStatus CopyFile(FileSystem* fs, const std::string& source,
     if (!io_s.ok()) {
       return io_s;
     }
-    std::unique_ptr<FSWritableFile> destfile;
-    io_s = fs->NewWritableFile(destination, soptions, &destfile, nullptr);
-    if (!io_s.ok()) {
-      return io_s;
-    }
 
     if (size == 0) {
       // default argument means copy everything
@@ -48,8 +43,6 @@ IOStatus CopyFile(FileSystem* fs, const std::string& source,
     }
     src_reader.reset(
         new SequentialFileReader(std::move(srcfile), source, io_tracer));
-    dest_writer.reset(
-        new WritableFileWriter(std::move(destfile), destination, soptions));
   }
 
   char buffer[4096];
@@ -72,6 +65,30 @@ IOStatus CopyFile(FileSystem* fs, const std::string& source,
   return dest_writer->Sync(use_fsync);
 }
 
+IOStatus CopyFile(FileSystem* fs, const std::string& source,
+                  const std::string& destination, uint64_t size, bool use_fsync,
+                  const std::shared_ptr<IOTracer>& io_tracer,
+                  const Temperature temperature) {
+  FileOptions options;
+  IOStatus io_s;
+  std::unique_ptr<WritableFileWriter> dest_writer;
+
+  {
+    options.temperature = temperature;
+    std::unique_ptr<FSWritableFile> destfile;
+    io_s = fs->NewWritableFile(destination, options, &destfile, nullptr);
+    if (!io_s.ok()) {
+      return io_s;
+    }
+
+    dest_writer.reset(
+        new WritableFileWriter(std::move(destfile), destination, options));
+  }
+
+  return CopyFile(fs, source, dest_writer, size, use_fsync, io_tracer,
+                  temperature);
+}
+
 // Utility function to create a file with the provided contents
 IOStatus CreateFile(FileSystem* fs, const std::string& destination,
                     const std::string& contents, bool use_fsync) {

file/file_util.h

@@ -19,6 +19,11 @@
 namespace ROCKSDB_NAMESPACE {
 // use_fsync maps to options.use_fsync, which determines the way that
 // the file is synced after copying.
+extern IOStatus CopyFile(FileSystem* fs, const std::string& source,
+                         std::unique_ptr<WritableFileWriter>& dest_writer,
+                         uint64_t size, bool use_fsync,
+                         const std::shared_ptr<IOTracer>& io_tracer,
+                         const Temperature temperature);
 extern IOStatus CopyFile(FileSystem* fs, const std::string& source,
                          const std::string& destination, uint64_t size,
                          bool use_fsync,