From 3362a730dc3d2cc4847b4608ba3866901a8b52bf Mon Sep 17 00:00:00 2001 From: anand76 Date: Fri, 4 Mar 2022 10:30:10 -0800 Subject: [PATCH] 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 | 67 ++++++++++++++++++++++------ db_stress_tool/db_stress_listener.h | 3 +- file/file_util.cc | 35 +++++++++++---- file/file_util.h | 5 +++ 4 files changed, 87 insertions(+), 23 deletions(-) diff --git a/db_stress_tool/db_stress_listener.cc b/db_stress_tool/db_stress_listener.cc index a58b2db2e..c8c13e92f 100644 --- a/db_stress_tool/db_stress_listener.cc +++ b/db_stress_tool/db_stress_listener.cc @@ -7,6 +7,7 @@ #include +#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 reader; - Status s = - fs->NewSequentialFile(path_, FileOptions(), &reader, /*dbg*/ nullptr); + Status s = fs->NewSequentialFile(tmp_path, FileOptions(), &reader, + /*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); - if (!s.ok()) { - fprintf(stderr, "Error opening unique id file for append: %s\n", - s.ToString().c_str()); + + std::unique_ptr file_writer; + st = fs->NewWritableFile(path_, FileOptions(), &file_writer, /*dbg*/ nullptr); + if (!st.ok()) { + 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 = - data_file_writer_->Append(Slice(id), IOOptions(), /*dbg*/ nullptr); + IOStatus s = data_file_writer_->Append(Slice(id)); 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()); diff --git a/db_stress_tool/db_stress_listener.h b/db_stress_tool/db_stress_listener.h index 62d3fb5c0..faced3172 100644 --- a/db_stress_tool/db_stress_listener.h +++ b/db_stress_tool/db_stress_listener.h @@ -10,6 +10,7 @@ #include #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 data_file_writer_; + std::unique_ptr 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 diff --git a/file/file_util.cc b/file/file_util.cc index c2392e9e8..011f12455 100644 --- a/file/file_util.cc +++ b/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& dest_writer, + uint64_t size, bool use_fsync, const std::shared_ptr& io_tracer, const Temperature temperature) { FileOptions soptions; IOStatus io_s; std::unique_ptr src_reader; - std::unique_ptr 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 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& io_tracer, + const Temperature temperature) { + FileOptions options; + IOStatus io_s; + std::unique_ptr dest_writer; + + { + options.temperature = temperature; + std::unique_ptr 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) { diff --git a/file/file_util.h b/file/file_util.h index 4351d2557..f7900ba40 100644 --- a/file/file_util.h +++ b/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& dest_writer, + uint64_t size, bool use_fsync, + const std::shared_ptr& io_tracer, + const Temperature temperature); extern IOStatus CopyFile(FileSystem* fs, const std::string& source, const std::string& destination, uint64_t size, bool use_fsync,