Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add optional_len to FSReadRequest #13350

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

archang19
Copy link
Contributor

@archang19 archang19 commented Jan 29, 2025

Summary

Context: We want to enable background compaction read sizes to be tuned to the particular storage backend. In this case, we want as many reads as possible to be aligned to the file system block boundaries.

Compaction prefetches go through FilePrefetchBuffer. If FilePrefetchBuffer had access to the exact block boundaries within the file, it could easily issue optimally sized reads.

Indeed, you could expose a GetBlockSize or GetNextBlockOffset API inside RandomAccessFileReader (and all other child classes). If all block sizes were fixed, GetBlockSize would give FilePrefetchBuffer all the information it needs. If there was some variation, GetNextBlockOffset would suffice.

Those solutions would not be too complicated, but I think we can simplify the solution even further. I think all we really need is an optional_read_size field in FSReadRequest.

What does this give us?

  1. The RocksDB file system-related APIs remain almost entirely unaltered. RandomAccessFileReader and its descendants do not need to implement new API methods.
  2. Backwards compatibility is maintained. The default of 0 is natural. If you don't want this functionality, you don't need to change any code. Ignoring it is fine because FSReadRequest::len is a valid result size.
  3. We can be very precise when we enable the use of optional_read_size. We can enable it only for compaction reads with non-direct IO.
  4. An implementation that has a low risk of introducing new bugs.

Potential concerns:

  1. What if some downstream class modifies offset or len, leaving it in an inconsistent state with optional_read_size? I was worried about this, and I thought about the scenario where the request got broken up into smaller pieces, but optional_read_size was left maintained. This would be terrible obviously because then optional_read_size could exceed the individual smaller request sizes. I think we are OK because FSReadRequest has its copy constructor implicitly deleted because of fs_scratch. Thus, any downstream use cases would have initialized FSReadRequest and set its parameters from scratch. It is OK for downstream classes to ignore optional_read_size. We are mainly concerned about optional_read_size getting copied over by accident, and luckily we are already protected against that.

Test Plan

Will need to update unit tests inside FilePrefetchBuffer. Will create a mock test file system implementation for a file system that does support optional_read_size.

@archang19 archang19 force-pushed the optional-read-size branch 2 times, most recently from bc31cba to 5949ea8 Compare January 29, 2025 23:45
@archang19 archang19 changed the title [work in progress] Optional read size in IOOptions Add optional read size in IOOptions Jan 29, 2025
@archang19 archang19 changed the title Add optional read size in IOOptions Add optional_read_size to IOOptions Jan 29, 2025
@archang19 archang19 changed the title Add optional_read_size to IOOptions Add optional_read_size Jan 30, 2025
@archang19 archang19 force-pushed the optional-read-size branch 3 times, most recently from 4fbeef7 to 6aa94a1 Compare January 30, 2025 20:23
@archang19 archang19 changed the title Add optional_read_size Add optional_read_size to FSReadRequest Jan 30, 2025
@archang19 archang19 force-pushed the optional-read-size branch 3 times, most recently from 9e677bd to ffc03b6 Compare January 30, 2025 22:01
@archang19 archang19 changed the title Add optional_read_size to FSReadRequest Add optional_len to FSReadRequest Jan 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants