From c206aebd0bf77554c5f8d76ed7567cfccf0597ab Mon Sep 17 00:00:00 2001 From: anand76 Date: Thu, 15 Sep 2022 19:18:52 -0700 Subject: [PATCH] Fix a MultiGet crash (#10688) Summary: Fix a bug in the async IO/coroutine version of MultiGet that may cause a segfault or assertion failure due to accessing an invalid file index in a LevelFilesBrief. The bug is that when a MultiGetRange is split into two, we may re-process keys in the original range that were already marked to be skipped (in ```current_level_range_```) due to not overlapping the level. Pull Request resolved: https://github.com/facebook/rocksdb/pull/10688 Reviewed By: gitbw95 Differential Revision: D39556131 Pulled By: anand1976 fbshipit-source-id: 65e79438508a283cb19e64eca5c91d0714b81458 --- db/version_set.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/db/version_set.cc b/db/version_set.cc index f2f8d8fb7..b068d06c2 100644 --- a/db/version_set.cc +++ b/db/version_set.cc @@ -511,6 +511,7 @@ class FilePickerMultiGet { MultiGetRange& GetRange() { return range_; } void ReplaceRange(const MultiGetRange& other) { + assert(curr_level_ == 0 || !RemainingOverlapInLevel()); range_ = other; current_level_range_ = other; } @@ -2727,8 +2728,9 @@ Status Version::ProcessBatch( f = fp.GetNextFileInLevel(); } // Split the current batch only if some keys are likely in this level and - // some are not. - if (s.ok() && !leftover.empty() && !range.empty()) { + // some are not. Only split if we're done with this level, i.e f is null. + // Otherwise, it means there are more files in this level to look at. + if (s.ok() && !f && !leftover.empty() && !range.empty()) { fp.ReplaceRange(range); batches.emplace_back(&leftover, fp); to_process.emplace_back(batches.size() - 1);