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

6 files changed, 46 insertions, 26 deletions

diff --git a/db/db_filesnapshot.cc b/db/db_filesnapshot.cc
index ecb8f4440..782fe3856 100644
--- a/db/db_filesnapshot.cc
+++ b/db/db_filesnapshot.cc
@@ -93,6 +93,11 @@ Status DBImpl::GetLiveFiles(std::vector<std::string>& 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<uint64_t> 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 7ecf53b40..319cad99e 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<std::string>&, 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 f10e0b99d..cad494c54 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<std::string> 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 64af8e6a0..6db416960 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 000000000..b585a9572
--- /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 9882ea7da..62dda66fc 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.