Sort per-file blob read requests by offset #8953

riversand963 · 2021-09-23T22:41:16Z

RandomAccessFileReader::MultiRead() tries to merge requests in direct IO, assuming input IO requests are
sorted by offsets.

Add a test in direct IO mode.

Test plan:
make check

facebook-github-bot · 2021-09-24T22:06:38Z

@riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

ltamasi

Thanks so much for the fix @riversand963 !

ltamasi · 2021-09-24T22:37:53Z

db/version_set.cc

+        blobs_in_file.begin(), blobs_in_file.end(),
+        [](const BlobReadRequest& lhs, const BlobReadRequest& rhs) -> bool {
+          assert(lhs.first.file_number() == rhs.first.file_number());
+          return lhs.first.offset() <= rhs.first.offset();


Probably does not make any practical difference but I think it would be nice to use strictly less here

db/blob/db_blob_basic_test.cc

ltamasi · 2021-09-24T22:44:33Z

db/blob/db_blob_basic_test.cc

+  SyncPoint::GetInstance()->ClearAllCallBacks();
+  SyncPoint::GetInstance()->SetCallBack(
+      "RandomAccessFileReader::MultiRead:AlignedReqs", [&](void* arg) {
+        auto* aligned_reqs = reinterpret_cast<std::vector<FSReadRequest>*>(arg);


static_cast would be sufficient here

db/blob/db_blob_basic_test.cc

ltamasi · 2021-09-24T22:48:06Z

db/blob/db_blob_basic_test.cc

+  SyncPoint::GetInstance()->DisableProcessing();
+  SyncPoint::GetInstance()->ClearAllCallBacks();


Minor but it would be nice to move these to right after the MultiGet call so they are executed even in the case of assertion failures.

ltamasi · 2021-09-24T22:48:48Z

db/blob/db_blob_basic_test.cc

@@ -127,6 +127,189 @@ TEST_F(DBBlobBasicTest, MultiGetBlobs) {
  }
 }

+TEST_F(DBBlobBasicTest, MultiGetWithDirectIO) {


Thanks for this awesome unit test @riversand963 !

facebook-github-bot · 2021-09-24T23:46:31Z

@riversand963 has updated the pull request. You must reimport the pull request before landing.

riversand963 · 2021-09-24T23:47:34Z

Thanks @ltamasi for the review!

facebook-github-bot · 2021-09-24T23:47:58Z

@riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-09-25T00:09:22Z

@riversand963 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-09-25T00:09:43Z

@riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-09-25T03:25:29Z

@riversand963 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-09-25T03:26:38Z

@riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: `RandomAccessFileReader::MultiRead()` tries to merge requests in direct IO, assuming input IO requests are sorted by offsets. Add a test in direct IO mode. Pull Request resolved: #8953 Test Plan: make check Reviewed By: ltamasi Differential Revision: D31183546 Pulled By: riversand963 fbshipit-source-id: 5d043ec68e2daa47a3149066150afd41ee3d73e6

pdillinger · 2021-09-28T21:56:18Z

But you didn't add a comment on BlobFileReader::MultiGetBlob (nor MultiRead?) that they expect sorted inputs?

riversand963 · 2021-09-28T22:38:52Z

But you didn't add a comment on BlobFileReader::MultiGetBlob (nor MultiRead?) that they expect sorted inputs?

@pdillinger oops, we should have added comment and checks for MultiGetblob(). For the record, MultiRead() does have a comment about sorted inputs. Maybe we should add checks to emphasize it as well.

riversand963 · 2021-09-28T23:21:50Z

#8972

pdillinger · 2021-09-30T16:08:44Z

For the record, MultiRead() does have a comment about sorted inputs.

I was looking at FSRandomAccessFile::MultiRead, which still does not have such a comment. (When I approved #8972 I was not in a position to verify this claim.)

ltamasi · 2021-09-30T16:14:43Z

For the record, MultiRead() does have a comment about sorted inputs.

I was looking at FSRandomAccessFile::MultiRead, which still does not have such a comment. (When I approved #8972 I was not in a position to verify this claim.)

It's RandomAccessFileReader::MultiRead that relies on the requests being sorted (that's where the merging happens). I don't think FSRandomAccessFile itself has this precondition

pdillinger · 2021-09-30T16:26:58Z

Implementations of FSRandomAccessFile::MultiRead are provided by both RocksDB and advanced users. Evidently confusion ensues when it is not clear whether sorting is required for the caller or whether the callee must deal with unsorted requests; "ambiguity means the callee must sort it out" is evidently not good enough. (Sorry if my meta-point "sortedness requirements should be clear" was confused by suggesting we add a comment like "inputs must be sorted.")

I don't think ...

Seems you agree it's ambiguous.

pdillinger · 2021-09-30T16:43:56Z

... or whether the callee must deal with unsorted requests

Especially if we currently always provide sorted requests but make no promise to continue that in the future.

ltamasi · 2021-09-30T16:51:00Z

I don't think ...

Seems you agree it's ambiguous.

No, what I meant above is that I didn't bother to check :) I did now and neither FSRandomAccessFile::MultiRead nor PosixRandomAccessFile::MultiRead has such a requirement.

Personally, I'm perfectly fine with not having a comment for non-requirements. As a developer, I would assume that as long as I'm not told explicitly that requests have to be sorted, they don't have to be.

pdillinger · 2021-09-30T17:16:05Z

A non-requirement on the caller is a requirement for the callee. Many implicit requirements (e.g. nullability) are often obvious and relatively low-risk (crash). Sortedness is tricky and high-risk (corruption).

Our job is not to have a plausible claim of "someone else's problem" but to mitigate the risk of problems. :) We have a history of people depending on our implementation details (here "you always passed in a sorted list before") and things breaking when we change implementation details.

riversand963 · 2021-09-30T18:00:35Z

I am trying to understand where the confusion and risk is.

If you use FSRandomAccessFile, then calling FSRandomAccessFile::MultiRead() does not require sorted input.
If you use RandomAccessFileReader::MultiRead(), then you need to sort the input, as specified in the comment and asserted in debug mode.

Besides, RandomAccessFileReader is not a public API. RandomAccessFileReader is a user of and wrapper on top of FSRandomAccessFile. Sorting input is a requirement only at the layer of RandomAccessFileReader() to merge requests in direct IO. Therefore, we can add comment about sorted input here, but not for FSRandomAccessFile::MultiRead.

Summary: * Clarify that RocksDB is not exception safe on many of our callback and extension interfaces * Clarify FSRandomAccessFile::MultiRead implementations must accept non-sorted inputs (see #8953) * Clarify ConcurrentTaskLimiter and SstFileManager are not (currently) extensible interfaces * Mark WriteBufferManager as `final`, so it is then clearly not a callback interface, even though it smells like one * Clarify TablePropertiesCollector Status returns are mostly ignored Pull Request resolved: #9080 Test Plan: comments only (except WriteBufferManager final) Reviewed By: ajkr Differential Revision: D31968782 Pulled By: pdillinger fbshipit-source-id: 11b648ce3ce3c5e5bdc02d2eafc7ea4b864bd1d2

Summary: `RandomAccessFileReader::MultiRead()` tries to merge requests in direct IO, assuming input IO requests are sorted by offsets. Add a test in direct IO mode. Pull Request resolved: facebook/rocksdb#8953 Test Plan: make check Reviewed By: ltamasi Differential Revision: D31183546 Pulled By: riversand963 fbshipit-source-id: 5d043ec68e2daa47a3149066150afd41ee3d73e6

facebook-github-bot added the CLA Signed label Sep 23, 2021

riversand963 force-pushed the multiget-blob-sort-offset branch 2 times, most recently from 9e0da46 to 4c37b0e Compare September 24, 2021 22:03

riversand963 marked this pull request as ready for review September 24, 2021 22:06

riversand963 requested a review from ltamasi September 24, 2021 22:06

ltamasi approved these changes Sep 24, 2021

View reviewed changes

riversand963 added 7 commits September 24, 2021 20:25

Sort per-file blob read requests by offset

78b0dfc

try to repro

f3fe873

Remove prints

5e7df9d

Update HISTORY

4d80ad9

CI and review comments

2787610

Minor

abd54dd

Update comment

c891af6

riversand963 force-pushed the multiget-blob-sort-offset branch from c7e2ae1 to c891af6 Compare September 25, 2021 03:25

facebook-github-bot closed this in b92cef2 Sep 25, 2021

riversand963 deleted the multiget-blob-sort-offset branch September 25, 2021 15:33

pdillinger mentioned this pull request Oct 26, 2021

Some API clarifications #9080

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sort per-file blob read requests by offset #8953

Sort per-file blob read requests by offset #8953

riversand963 commented Sep 23, 2021

facebook-github-bot commented Sep 24, 2021

ltamasi left a comment

ltamasi Sep 24, 2021

ltamasi Sep 24, 2021

ltamasi Sep 24, 2021

ltamasi Sep 24, 2021

facebook-github-bot commented Sep 24, 2021

riversand963 commented Sep 24, 2021

facebook-github-bot commented Sep 24, 2021

facebook-github-bot commented Sep 25, 2021

facebook-github-bot commented Sep 25, 2021

facebook-github-bot commented Sep 25, 2021

facebook-github-bot commented Sep 25, 2021

pdillinger commented Sep 28, 2021

riversand963 commented Sep 28, 2021

riversand963 commented Sep 28, 2021

pdillinger commented Sep 30, 2021

ltamasi commented Sep 30, 2021 •

edited

Loading

pdillinger commented Sep 30, 2021

pdillinger commented Sep 30, 2021

ltamasi commented Sep 30, 2021

pdillinger commented Sep 30, 2021

riversand963 commented Sep 30, 2021

		SyncPoint::GetInstance()->DisableProcessing();
		SyncPoint::GetInstance()->ClearAllCallBacks();

Sort per-file blob read requests by offset #8953

Sort per-file blob read requests by offset #8953

Conversation

riversand963 commented Sep 23, 2021

facebook-github-bot commented Sep 24, 2021

ltamasi left a comment

Choose a reason for hiding this comment

ltamasi Sep 24, 2021

Choose a reason for hiding this comment

ltamasi Sep 24, 2021

Choose a reason for hiding this comment

ltamasi Sep 24, 2021

Choose a reason for hiding this comment

ltamasi Sep 24, 2021

Choose a reason for hiding this comment

facebook-github-bot commented Sep 24, 2021

riversand963 commented Sep 24, 2021

facebook-github-bot commented Sep 24, 2021

facebook-github-bot commented Sep 25, 2021

facebook-github-bot commented Sep 25, 2021

facebook-github-bot commented Sep 25, 2021

facebook-github-bot commented Sep 25, 2021

pdillinger commented Sep 28, 2021

riversand963 commented Sep 28, 2021

riversand963 commented Sep 28, 2021

pdillinger commented Sep 30, 2021

ltamasi commented Sep 30, 2021 • edited Loading

pdillinger commented Sep 30, 2021

pdillinger commented Sep 30, 2021

ltamasi commented Sep 30, 2021

pdillinger commented Sep 30, 2021

riversand963 commented Sep 30, 2021

ltamasi commented Sep 30, 2021 •

edited

Loading