Skip to content

Commit 7183422

Browse files
archang19facebook-github-bot
authored andcommitted
Check that NewWritableFile succeeded when copying over backup files (facebook#13734)
Summary: I am seeing crashes during backups. The stack trace points back to `WritableFileWriter` creation inside `BackupEngineImpl::CopyOrCreateFile`. I believe the issue is that we are calling `writable_file_->GetRequiredBufferAlignment()` with a `null` `writable_file`. https://github.com/facebook/rocksdb/blob/v10.2.1/utilities/backup/backup_engine.cc#L2396-L2397 https://github.com/facebook/rocksdb/blob/v10.2.1/file/writable_file_writer.h#L210 Here's how I think the flow is: ```cpp io_s = dst_env->GetFileSystem()->NewWritableFile(dst, dst_file_options, &dst_file, nullptr); // say there was some issue and dst_file is nullptr // evaluates to false if (io_s.ok() && !src.empty()) { // we don't go down this branch auto src_file_options = FileOptions(src_env_options); src_file_options.temperature = *src_temperature; io_s = src_env->GetFileSystem()->NewSequentialFile(src, src_file_options, &src_file, nullptr); } // say this evaluates to true if (io_s.IsPathNotFound() && *src_temperature != Temperature::kUnknown) { // Retry without temperature hint in case the FileSystem is strict with // non-kUnknown temperature option io_s = src_env->GetFileSystem()->NewSequentialFile( src, FileOptions(src_env_options), &src_file, nullptr); } // this is now from the NewSequentialFile call, not NewWritableFile if (!io_s.ok()) { return io_s; } // dst_file is still nullptr ``` If the first `NewWritableFile` fails and `IsPathNotFound Tests: existing unit tests Pull Request resolved: facebook#13734 Reviewed By: pdillinger Differential Revision: D77390694 Pulled By: archang19 fbshipit-source-id: 865a3a646079ae2349a3b6f25e53ae85df8e4985
1 parent ca24135 commit 7183422

1 file changed

Lines changed: 5 additions & 1 deletion

File tree

utilities/backup/backup_engine.cc

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2372,7 +2372,11 @@ IOStatus BackupEngineImpl::CopyOrCreateFile(
23722372

23732373
io_s = dst_env->GetFileSystem()->NewWritableFile(dst, dst_file_options,
23742374
&dst_file, nullptr);
2375-
if (io_s.ok() && !src.empty()) {
2375+
if (!io_s.ok()) {
2376+
return io_s;
2377+
}
2378+
2379+
if (!src.empty()) {
23762380
auto src_file_options = FileOptions(src_env_options);
23772381
src_file_options.temperature = *src_temperature;
23782382
io_s = src_env->GetFileSystem()->NewSequentialFile(src, src_file_options,

0 commit comments

Comments
 (0)