Skip to content

Commit 6ea50e6

Browse files
anand1976anand1976
committed
Revert "Create a new API FileSystem::SyncFile for file sync (facebook#13762)"
This reverts commit 961880b.
1 parent 17f7a62 commit 6ea50e6

14 files changed

Lines changed: 35 additions & 175 deletions

‎db/external_sst_file_basic_test.cc‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1311,7 +1311,7 @@ TEST_F(ExternalSSTFileBasicTest, SyncFailure) {
13111311
});
13121312
if (i == 0) {
13131313
SyncPoint::GetInstance()->SetCallBack(
1314-
"ExternalSstFileIngestionJob::CheckSyncReturnCode", [&](void* s) {
1314+
"ExternalSstFileIngestionJob::Prepare:Reopen", [&](void* s) {
13151315
Status* status = static_cast<Status*>(s);
13161316
if (status->IsNotSupported()) {
13171317
no_sync = true;
@@ -1372,11 +1372,11 @@ TEST_F(ExternalSSTFileBasicTest, ReopenNotSupported) {
13721372
options.create_if_missing = true;
13731373
options.env = env_;
13741374

1375-
SyncPoint::GetInstance()->SetCallBack("FileSystem::SyncFile:Open",
1376-
[&](void* arg) {
1377-
Status* s = static_cast<Status*>(arg);
1378-
*s = Status::NotSupported();
1379-
});
1375+
SyncPoint::GetInstance()->SetCallBack(
1376+
"ExternalSstFileIngestionJob::Prepare:Reopen", [&](void* arg) {
1377+
Status* s = static_cast<Status*>(arg);
1378+
*s = Status::NotSupported();
1379+
});
13801380
SyncPoint::GetInstance()->EnableProcessing();
13811381

13821382
DestroyAndReopen(options);

‎db/external_sst_file_ingestion_job.cc‎

Lines changed: 29 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -160,26 +160,35 @@ Status ExternalSstFileIngestionJob::Prepare(
160160
// It is unsafe to assume application had sync the file and file
161161
// directory before ingest the file. For integrity of RocksDB we need
162162
// to sync the file.
163-
TEST_SYNC_POINT("ExternalSstFileIngestionJob::BeforeSyncIngestedFile");
164-
auto s = fs_->SyncFile(path_inside_db, env_options_, IOOptions(),
165-
db_options_.use_fsync, nullptr);
166-
TEST_SYNC_POINT("ExternalSstFileIngestionJob::AfterSyncIngestedFile");
167-
TEST_SYNC_POINT_CALLBACK(
168-
"ExternalSstFileIngestionJob::CheckSyncReturnCode", &s);
169-
if (!s.ok()) {
170-
if (s.IsNotSupported()) {
171-
// Some file systems (especially remote/distributed) don't support
172-
// SyncFile API. Ignore the NotSupported error in that case.
173-
ROCKS_LOG_WARN(db_options_.info_log,
174-
"After link the file, SyncFile API is not supported "
175-
"for file %s: %s",
176-
path_inside_db.c_str(), status.ToString().c_str());
177-
} else {
178-
// for other errors, propagate the error
179-
status = s;
180-
ROCKS_LOG_WARN(db_options_.info_log,
181-
"Failed to sync ingested file %s: %s",
182-
path_inside_db.c_str(), status.ToString().c_str());
163+
164+
// TODO(xingbo), We should in general be moving away from production
165+
// uses of ReuseWritableFile (except explicitly for WAL recycling),
166+
// ReopenWritableFile, and NewRandomRWFile. We should create a
167+
// FileSystem::SyncFile/FsyncFile API that by default does the
168+
// re-open+sync+close combo but can (a) be reused easily, and (b) be
169+
// overridden to do that more cleanly, e.g. in EncryptedEnv.
170+
// https://github.com/facebook/rocksdb/issues/13741
171+
std::unique_ptr<FSWritableFile> file_to_sync;
172+
Status s = fs_->ReopenWritableFile(path_inside_db, env_options_,
173+
&file_to_sync, nullptr);
174+
TEST_SYNC_POINT_CALLBACK("ExternalSstFileIngestionJob::Prepare:Reopen",
175+
&s);
176+
// Some file systems (especially remote/distributed) don't support
177+
// reopening a file for writing and don't require reopening and
178+
// syncing the file. Ignore the NotSupported error in that case.
179+
if (!s.IsNotSupported()) {
180+
status = s;
181+
if (status.ok()) {
182+
TEST_SYNC_POINT(
183+
"ExternalSstFileIngestionJob::BeforeSyncIngestedFile");
184+
status = SyncIngestedFile(file_to_sync.get());
185+
TEST_SYNC_POINT(
186+
"ExternalSstFileIngestionJob::AfterSyncIngestedFile");
187+
if (!status.ok()) {
188+
ROCKS_LOG_WARN(db_options_.info_log,
189+
"Failed to sync ingested file %s: %s",
190+
path_inside_db.c_str(), status.ToString().c_str());
191+
}
183192
}
184193
}
185194
} else if (status.IsNotSupported() &&

‎env/composite_env_wrapper.h‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -142,12 +142,6 @@ class CompositeEnv : public Env {
142142
return file_system_->LinkFile(s, t, io_opts, &dbg);
143143
}
144144

145-
Status SyncFile(const std::string& fname, const EnvOptions& env_options,
146-
bool use_fsync) override {
147-
return file_system_->SyncFile(fname, env_options, IOOptions(), use_fsync,
148-
nullptr);
149-
}
150-
151145
Status NumFileLinks(const std::string& fname, uint64_t* count) override {
152146
IOOptions io_opts;
153147
IODebugContext dbg;

‎env/env.cc‎

Lines changed: 0 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -528,13 +528,6 @@ class LegacyFileSystemWrapper : public FileSystem {
528528
return status_to_io_status(target_->LinkFile(s, t));
529529
}
530530

531-
IOStatus SyncFile(const std::string& fname, const FileOptions& file_options,
532-
const IOOptions& /*io_options*/, bool use_fsync,
533-
IODebugContext* /*dbg*/) override {
534-
return status_to_io_status(
535-
target_->SyncFile(fname, file_options, use_fsync));
536-
}
537-
538531
IOStatus NumFileLinks(const std::string& fname, const IOOptions& /*options*/,
539532
uint64_t* count, IODebugContext* /*dbg*/) override {
540533
return status_to_io_status(target_->NumFileLinks(fname, count));
@@ -866,43 +859,6 @@ std::string Env::GenerateUniqueId() {
866859
return result;
867860
}
868861

869-
// This API Env::SyncFile is used for testing for 2 reasons:
870-
//
871-
// 1. The default implementation of SyncFile API is essentially a wrapper of
872-
// other FileSystem APIs. FaultInjectionTestEnv uses this default
873-
// implementation to call other FileSystem APIs defined at
874-
// FaultInjectionTestEnv class to inject failurses. See
875-
// FaultInjectionTestEnv::SyncFile for more details
876-
//
877-
// 2. Some of old tests are using LegacyFileSystemWrapper.
878-
// LegacyFileSystemWrapper forwards the API call to EnvWrapper, which forwards
879-
// to CompositeEnv, and then forwards to the actual FileSystem implemention.
880-
// Without this API in Env, LegacyFileSystemWrapper will not be able to
881-
// forward the API call to EnvWrapper, causing the default FileSystem API to
882-
// be called.
883-
//
884-
// Due to the above reason, adding a new API in FileSystem, would very likely
885-
// require the same API to be added to Env.
886-
//
887-
// TODO xingbo. Getting rid of FileSystem functions from Env.
888-
// We need to simplify the relationship between Env and FileSystem. At least
889-
// for internal test, we should stop using Env and switch to FileSystem, if
890-
// possible. Related github issue #9274
891-
Status Env::SyncFile(const std::string& fname, const EnvOptions& env_options,
892-
bool use_fsync) {
893-
std::unique_ptr<WritableFile> file_to_sync;
894-
auto status = ReopenWritableFile(fname, &file_to_sync, env_options);
895-
TEST_SYNC_POINT_CALLBACK("FileSystem::SyncFile:Open", &status);
896-
if (status.ok()) {
897-
if (use_fsync) {
898-
status = file_to_sync->Fsync();
899-
} else {
900-
status = file_to_sync->Sync();
901-
}
902-
}
903-
return status;
904-
}
905-
906862
SequentialFile::~SequentialFile() = default;
907863

908864
RandomAccessFile::~RandomAccessFile() = default;

‎env/env_encryption.cc‎

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -664,8 +664,6 @@ class EncryptedFileSystemImpl : public EncryptedFileSystem {
664664
const FileOptions& options,
665665
std::unique_ptr<FSWritableFile>* result,
666666
IODebugContext* dbg) override {
667-
// TODO xingbo Add unit test for the new implementation of
668-
// EncryptedFileSysmteImpl::ReopenWritableFile.
669667
result->reset();
670668
if (options.use_mmap_reads || options.use_mmap_writes) {
671669
return IOStatus::InvalidArgument();
@@ -816,15 +814,6 @@ class EncryptedFileSystemImpl : public EncryptedFileSystem {
816814
return status;
817815
}
818816

819-
IOStatus SyncFile(const std::string& fname, const FileOptions& file_options,
820-
const IOOptions& io_options, bool use_fsync,
821-
IODebugContext* dbg) override {
822-
// Use the underlying file system to sync the file, as we don't need to
823-
// read/write the file.
824-
return FileSystemWrapper::SyncFile(fname, file_options, io_options,
825-
use_fsync, dbg);
826-
}
827-
828817
private:
829818
std::shared_ptr<EncryptionProvider> provider_;
830819
};

‎env/file_system.cc‎

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -107,23 +107,6 @@ IOStatus FileSystem::ReuseWritableFile(const std::string& fname,
107107
return NewWritableFile(fname, opts, result, dbg);
108108
}
109109

110-
IOStatus FileSystem::SyncFile(const std::string& fname,
111-
const FileOptions& file_options,
112-
const IOOptions& io_options, bool use_fsync,
113-
IODebugContext* dbg) {
114-
std::unique_ptr<FSWritableFile> file_to_sync;
115-
auto status = ReopenWritableFile(fname, file_options, &file_to_sync, dbg);
116-
TEST_SYNC_POINT_CALLBACK("FileSystem::SyncFile:Open", &status);
117-
if (status.ok()) {
118-
if (use_fsync) {
119-
status = file_to_sync->Fsync(io_options, dbg);
120-
} else {
121-
status = file_to_sync->Sync(io_options, dbg);
122-
}
123-
}
124-
return status;
125-
}
126-
127110
IOStatus FileSystem::NewLogger(const std::string& fname,
128111
const IOOptions& io_opts,
129112
std::shared_ptr<Logger>* result,

‎env/mock_env.cc‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -957,14 +957,6 @@ IOStatus MockFileSystem::LinkFile(const std::string& src,
957957
return IOStatus::OK();
958958
}
959959

960-
IOStatus MockFileSystem::SyncFile(const std::string& /*fname*/,
961-
const FileOptions& /*file_options*/,
962-
const IOOptions& /*io_options*/,
963-
bool /*use_fsync*/, IODebugContext* /*dbg*/) {
964-
// Noop
965-
return IOStatus::OK();
966-
}
967-
968960
IOStatus MockFileSystem::NewLogger(const std::string& fname,
969961
const IOOptions& io_opts,
970962
std::shared_ptr<Logger>* result,

‎env/mock_env.h‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -86,10 +86,6 @@ class MockFileSystem : public FileSystem {
8686
IOStatus LinkFile(const std::string& /*src*/, const std::string& /*target*/,
8787
const IOOptions& /*options*/,
8888
IODebugContext* /*dbg*/) override;
89-
IOStatus SyncFile(const std::string& /*fname*/,
90-
const FileOptions& /*file_options*/,
91-
const IOOptions& /*io_options*/, bool /*use_fsync*/,
92-
IODebugContext* /*dbg*/) override;
9389
IOStatus LockFile(const std::string& fname, const IOOptions& options,
9490
FileLock** lock, IODebugContext* dbg) override;
9591
IOStatus UnlockFile(FileLock* lock, const IOOptions& options,

‎include/rocksdb/env.h‎

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -385,13 +385,6 @@ class Env : public Customizable {
385385
return Status::NotSupported("LinkFile is not supported for this Env");
386386
}
387387

388-
// Sync the file content to file system.
389-
// This API is only used for testing.
390-
// See FileSystem::SyncFile comment for details
391-
virtual Status SyncFile(const std::string& /*fname*/,
392-
const EnvOptions& /*env_options*/,
393-
bool /*use_fsync*/);
394-
395388
virtual Status NumFileLinks(const std::string& /*fname*/,
396389
uint64_t* /*count*/) {
397390
return Status::NotSupported(
@@ -1550,11 +1543,6 @@ class EnvWrapper : public Env {
15501543
return target_.env->LinkFile(s, t);
15511544
}
15521545

1553-
Status SyncFile(const std::string& fname, const EnvOptions& env_options,
1554-
bool use_fsync) override {
1555-
return target_.env->SyncFile(fname, env_options, use_fsync);
1556-
}
1557-
15581546
Status NumFileLinks(const std::string& fname, uint64_t* count) override {
15591547
return target_.env->NumFileLinks(fname, count);
15601548
}

‎include/rocksdb/file_system.h‎

Lines changed: 0 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -606,18 +606,6 @@ class FileSystem : public Customizable {
606606
"LinkFile is not supported for this FileSystem");
607607
}
608608

609-
// Sync the file content to file system.
610-
// The default implementation would open, sync and close the file.
611-
// This function could be overridden with no-op, if the file system
612-
// automatically sync the data when file is closed.
613-
// This is used when a user-provided file, probably unsynced, is pulled into a
614-
// context where power-outage-proof persistence is required (e.g.
615-
// IngestExternalFile without copy).
616-
virtual IOStatus SyncFile(const std::string& fname,
617-
const FileOptions& file_options,
618-
const IOOptions& io_options, bool use_fsync,
619-
IODebugContext* dbg);
620-
621609
virtual IOStatus NumFileLinks(const std::string& /*fname*/,
622610
const IOOptions& /*options*/,
623611
uint64_t* /*count*/, IODebugContext* /*dbg*/) {
@@ -1604,12 +1592,6 @@ class FileSystemWrapper : public FileSystem {
16041592
return target_->LinkFile(s, t, options, dbg);
16051593
}
16061594

1607-
IOStatus SyncFile(const std::string& fname, const FileOptions& file_options,
1608-
const IOOptions& io_options, bool use_fsync,
1609-
IODebugContext* dbg) override {
1610-
return target_->SyncFile(fname, file_options, io_options, use_fsync, dbg);
1611-
}
1612-
16131595
IOStatus NumFileLinks(const std::string& fname, const IOOptions& options,
16141596
uint64_t* count, IODebugContext* dbg) override {
16151597
return target_->NumFileLinks(fname, options, count, dbg);

0 commit comments

Comments
 (0)