From 0bb939611dd80a2a4cb07d4fb526cc39304d9a1d Mon Sep 17 00:00:00 2001 From: Peter Dillinger Date: Mon, 1 Jul 2024 23:29:02 -0700 Subject: [PATCH] Avoid unnecessary work in internal calls to GetSortedWalFiles (#12831) Summary: We are seeing a number of crash test failures coming from checkpoint and backup code, likely from WalManager::GetSortedWalFiles -> ... -> WalManager::ReadFirstLine and this code path is not needed, because we don't need to know the sequence numbers of WAL files going into a checkpoint or backup. We can minimize the impact of whatever inconsistency is causing that problem by not relying on it where it's not needed. Similarly, when we only need a roughly accurate set of current WAL files, we don't need to query all the archived WAL files (and redundantly the live ones again). So this reduces filesystem queries and DB mutex acquires in creating backups and checkpoints. Needed follow-up: Figure out what is causing various failures with an apparent inconsistency where GetSortedWalFiles fails on reading a WAL file. If it's an injected failure, perhaps it's not propagating that injected failure appropriately. It might also be an inconsistency between what the DB knows is flushed and what WalManager reads from the filesystem (which we know is dubious and should be phased out, which this is arguably another step toward). Or completing that phase-out might solve the problem without a full diagnosis. Pull Request resolved: https://github.com/facebook/rocksdb/pull/12831 Test Plan: existing tests (easily caught when I went too far in initally developing this change) Update to BackupUsingDirectIO test so that there's a WAL file in what is backed up. (Was relying on some oddity.) Reviewed By: cbi42 Differential Revision: D59252649 Pulled By: pdillinger fbshipit-source-id: 7ad4187a1c70caa59a6d6c1c643ef95232b929f5 --- db/db_filesnapshot.cc | 26 +++++++++----- db/db_impl/db_impl.h | 2 ++ db/wal_manager.cc | 36 +++++++++++-------- db/wal_manager.h | 5 +-- .../performance_improvements/sorted_wals.md | 1 + utilities/backup/backup_engine_test.cc | 2 +- 6 files changed, 46 insertions(+), 26 deletions(-) create mode 100644 unreleased_history/performance_improvements/sorted_wals.md diff --git a/db/db_filesnapshot.cc b/db/db_filesnapshot.cc index ecb8f44405..782fe3856d 100644 --- a/db/db_filesnapshot.cc +++ b/db/db_filesnapshot.cc @@ -93,6 +93,11 @@ Status DBImpl::GetLiveFiles(std::vector& ret, } Status DBImpl::GetSortedWalFiles(VectorWalPtr& files) { + return GetSortedWalFilesImpl(files, + /*need_seqnos*/ true); +} + +Status DBImpl::GetSortedWalFilesImpl(VectorWalPtr& files, bool need_seqnos) { // Record tracked WALs as a (minimum) cross-check for directory scan std::vector required_by_manifest; @@ -118,7 +123,10 @@ Status DBImpl::GetSortedWalFiles(VectorWalPtr& files) { } } - Status s = wal_manager_.GetSortedWalFiles(files); + // NOTE: need to include archived WALs because needed WALs might have been + // archived since getting required_by_manifest set + Status s = wal_manager_.GetSortedWalFiles(files, need_seqnos, + /*include_archived*/ true); // DisableFileDeletions / EnableFileDeletions not supported in read-only DB if (deletions_disabled.ok()) { @@ -207,7 +215,12 @@ Status DBImpl::GetLiveFilesStorageInfo( } else if (opts.wal_size_for_flush > 0) { // FIXME: avoid querying the filesystem for current WAL state // If the outstanding WAL files are small, we skip the flush. - s = GetSortedWalFiles(live_wal_files); + // Don't take archived log size into account when calculating wal + // size for flush, and don't need to verify consistency with manifest + // here & now. + s = wal_manager_.GetSortedWalFiles(live_wal_files, + /* need_seqnos */ false, + /*include_archived*/ false); if (!s.ok()) { return s; @@ -218,11 +231,7 @@ Status DBImpl::GetLiveFilesStorageInfo( // We may be able to cover 2PC case too. uint64_t total_wal_size = 0; for (auto& wal : live_wal_files) { - // Don't take archived log size into account - // when calculating log size for flush - if (wal->Type() == kArchivedLogFile) { - continue; - } + assert(wal->Type() == kAliveLogFile); total_wal_size += wal->SizeFileBytes(); } if (total_wal_size < opts.wal_size_for_flush) { @@ -434,7 +443,8 @@ Status DBImpl::GetLiveFilesStorageInfo( // WAL files. if (s.ok()) { // FIXME: avoid querying the filesystem for current WAL state - s = GetSortedWalFiles(live_wal_files); + s = GetSortedWalFilesImpl(live_wal_files, + /* need_seqnos */ false); } if (!s.ok()) { return s; diff --git a/db/db_impl/db_impl.h b/db/db_impl/db_impl.h index 7ecf53b40a..319cad99e9 100644 --- a/db/db_impl/db_impl.h +++ b/db/db_impl/db_impl.h @@ -508,6 +508,8 @@ class DBImpl : public DB { Status GetLiveFiles(std::vector&, uint64_t* manifest_file_size, bool flush_memtable = true) override; Status GetSortedWalFiles(VectorWalPtr& files) override; + Status GetSortedWalFilesImpl(VectorWalPtr& files, bool need_seqnos); + // Get the known flushed sizes of WALs that might still be written to // or have pending sync. // NOTE: unlike alive_log_files_, this function includes WALs that might diff --git a/db/wal_manager.cc b/db/wal_manager.cc index f10e0b99d4..cad494c541 100644 --- a/db/wal_manager.cc +++ b/db/wal_manager.cc @@ -43,16 +43,17 @@ Status WalManager::DeleteFile(const std::string& fname, uint64_t number) { } return s; } - -Status WalManager::GetSortedWalFiles(VectorWalPtr& files) { +Status WalManager::GetSortedWalFiles(VectorWalPtr& files, bool need_seqnos, + bool include_archived) { // First get sorted files in db dir, then get sorted files from archived // dir, to avoid a race condition where a log file is moved to archived // dir in between. Status s; // list wal files in main db dir. VectorWalPtr logs; - s = GetSortedWalsOfType(wal_dir_, logs, kAliveLogFile); - if (!s.ok()) { + s = GetSortedWalsOfType(wal_dir_, logs, kAliveLogFile, need_seqnos); + + if (!include_archived || !s.ok()) { return s; } @@ -67,7 +68,7 @@ Status WalManager::GetSortedWalFiles(VectorWalPtr& files) { std::string archivedir = ArchivalDirectory(wal_dir_); Status exists = env_->FileExists(archivedir); if (exists.ok()) { - s = GetSortedWalsOfType(archivedir, files, kArchivedLogFile); + s = GetSortedWalsOfType(archivedir, files, kArchivedLogFile, need_seqnos); if (!s.ok()) { return s; } @@ -249,7 +250,8 @@ void WalManager::PurgeObsoleteWALFiles() { size_t files_del_num = log_files_num - files_keep_num; VectorWalPtr archived_logs; - s = GetSortedWalsOfType(archival_dir, archived_logs, kArchivedLogFile); + s = GetSortedWalsOfType(archival_dir, archived_logs, kArchivedLogFile, + /*need_seqno=*/false); if (!s.ok()) { ROCKS_LOG_WARN(db_options_.info_log, "Unable to get archived WALs from: %s: %s", @@ -294,7 +296,7 @@ void WalManager::ArchiveWALFile(const std::string& fname, uint64_t number) { Status WalManager::GetSortedWalsOfType(const std::string& path, VectorWalPtr& log_files, - WalFileType log_type) { + WalFileType log_type, bool need_seqnos) { std::vector all_files; const Status status = env_->GetChildren(path, &all_files); if (!status.ok()) { @@ -306,13 +308,17 @@ Status WalManager::GetSortedWalsOfType(const std::string& path, FileType type; if (ParseFileName(f, &number, &type) && type == kWalFile) { SequenceNumber sequence; - Status s = ReadFirstRecord(log_type, number, &sequence); - if (!s.ok()) { - return s; - } - if (sequence == 0) { - // empty file - continue; + if (need_seqnos) { + Status s = ReadFirstRecord(log_type, number, &sequence); + if (!s.ok()) { + return s; + } + if (sequence == 0) { + // empty file + continue; + } + } else { + sequence = 0; } // Reproduce the race condition where a log file is moved @@ -322,7 +328,7 @@ Status WalManager::GetSortedWalsOfType(const std::string& path, TEST_SYNC_POINT("WalManager::GetSortedWalsOfType:2"); uint64_t size_bytes; - s = env_->GetFileSize(LogFileName(path, number), &size_bytes); + Status s = env_->GetFileSize(LogFileName(path, number), &size_bytes); // re-try in case the alive log file has been moved to archive. if (!s.ok() && log_type == kAliveLogFile) { std::string archived_file = ArchivedLogFileName(path, number); diff --git a/db/wal_manager.h b/db/wal_manager.h index 64af8e6a04..6db4169603 100644 --- a/db/wal_manager.h +++ b/db/wal_manager.h @@ -49,7 +49,8 @@ class WalManager { wal_in_db_path_(db_options_.IsWalDirSameAsDBPath()), io_tracer_(io_tracer) {} - Status GetSortedWalFiles(VectorWalPtr& files); + Status GetSortedWalFiles(VectorWalPtr& files, bool need_seqnos = true, + bool include_archived = true); // Allow user to tail transaction log to find all recent changes to the // database that are newer than `seq_number`. @@ -78,7 +79,7 @@ class WalManager { private: Status GetSortedWalsOfType(const std::string& path, VectorWalPtr& log_files, - WalFileType type); + WalFileType type, bool need_seqnos); // Requires: all_logs should be sorted with earliest log file first // Retains all log files in all_logs which contain updates with seq no. // Greater Than or Equal to the requested SequenceNumber. diff --git a/unreleased_history/performance_improvements/sorted_wals.md b/unreleased_history/performance_improvements/sorted_wals.md new file mode 100644 index 0000000000..b585a9572b --- /dev/null +++ b/unreleased_history/performance_improvements/sorted_wals.md @@ -0,0 +1 @@ +* Reduce unnecessary filesystem queries and DB mutex acquires in creating backups and checkpoints. diff --git a/utilities/backup/backup_engine_test.cc b/utilities/backup/backup_engine_test.cc index 9882ea7daa..62dda66fcd 100644 --- a/utilities/backup/backup_engine_test.cc +++ b/utilities/backup/backup_engine_test.cc @@ -3962,7 +3962,7 @@ TEST_P(BackupEngineTestWithParam, BackupUsingDirectIO) { OpenDBAndBackupEngine(true /* destroy_old_data */); for (int i = 0; i < kNumBackups; ++i) { FillDB(db_.get(), i * kNumKeysPerBackup /* from */, - (i + 1) * kNumKeysPerBackup /* to */, kFlushAll); + (i + 1) * kNumKeysPerBackup /* to */); // Clear the file open counters and then do a bunch of backup engine ops. // For all ops, files should be opened in direct mode.