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

feat(pageserver): newtype for dio-aligned buffer allocation #8730

Closed
wants to merge 13 commits into from

Conversation

yliang412
Copy link
Contributor

@yliang412 yliang412 commented Aug 15, 2024

Problem

We need to use aligned buffer to enable Direct IO. Currently there is no primitive for constructing such buffer.

Summary of changes

  • Introduce IoBufferMut, new-type around ManuallyDrop<Vec<u8>> with custom allocation and deallocation.
  • implement bytes::BufMut trait for IoBufferMut
  • implement tokio_epoll_uring::IoBuf, tokio_epoll_uring::IoBufMut trait for IoBufferMut
  • Marker trait IoBufAlignedMut to indicate alignment.

Run miri against the unit tests, passed.

The only error emitted was unsupported operation: can't call foreign function _rjem_malloc on OS linux, this is due to miri's own limitations to support jemalloc without the unprefixed_malloc_on_supported_platforms feature on. Otherwise,

cargo +nightly miri test --target x86_64-unknown-linux-gnu
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.03s
     Running unittests src/main.rs (target/miri/x86_64-unknown-linux-gnu/debug/deps/mem_layout_test-972dfe32100b3945)

running 4 tests
test dio::buffer_aligned::tests::test_bytes_put ... ok
test dio::buffer_aligned::tests::test_bytes_put_panic - should panic ... ok
test dio::buffer_aligned::tests::test_io_buf_put_slice ... ok
test dio::buffer_aligned::tests::test_with_capacity_aligned ... ok

test result: ok. 4 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.51s

Checklist before requesting a review

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough tests.
  • Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
  • If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

  • Do not forget to reformat commit message to not include the above checklist

@github-actions github-actions bot added the external A PR or Issue is created by an external user label Aug 15, 2024
Signed-off-by: Yuchen Liang <[email protected]>
@yliang412 yliang412 changed the title feat(pageserver): newtype aligned-vec as aligned buffer allocation feat(pageserver): newtype for dio-aligned buffer allocation Aug 15, 2024
Copy link

github-actions bot commented Aug 15, 2024

No tests were run or test report is not available

Test coverage report is not available

The comment gets automatically updated with the latest test results
eb16aa9 at 2024-09-29T21:12:55.472Z :recycle:

@bayandin bayandin removed the external A PR or Issue is created by an external user label Aug 15, 2024
@yliang412 yliang412 requested a review from problame August 16, 2024 18:14
@yliang412 yliang412 self-assigned this Aug 16, 2024
@yliang412 yliang412 marked this pull request as ready for review August 19, 2024 13:29
@yliang412 yliang412 requested a review from a team as a code owner August 19, 2024 13:29
Copy link
Contributor

@problame problame left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would feel better without having the Vec underneath. Seems like the only convenience we get from it is the capacity() / len() / set_len() method. I think we can implement those ourselves.

I think in an earlier sync meeting, I asked about whether you have some WIP stacked atop this PR that demonstrates how it will be used?

pageserver/src/virtual_file/dio.rs Outdated Show resolved Hide resolved
pageserver/src/virtual_file/dio.rs Outdated Show resolved Hide resolved
pageserver/src/virtual_file/dio.rs Outdated Show resolved Hide resolved
yliang412 added a commit that referenced this pull request Sep 24, 2024
Part of #8130.

## Problem

Currently, decompression is performed within the `read_blobs`
implementation and the decompressed blob will be appended to the end of
the `BytesMut` buffer. We will lose this flexibility of extending the
buffer when we switch to using our own dio-aligned buffer (WIP in
#8730). To facilitate the
adoption of aligned buffer, we need to refactor the code to perform
decompression outside `read_blobs`.

## Summary of changes

- `VectoredBlobReader::read_blobs` will return `VectoredBlob` without
performing decompression and appending decompressed blob. It becomes the
caller's responsibility to decompress the buffer.
- Added a new `BufView` type that functions as `Cow<Bytes, &[u8]>`.
- Perform decompression within `VectoredBlob::read` so that people don't
have to explicitly thinking about compression when using the reader
interface.

Signed-off-by: Yuchen Liang <[email protected]>
@yliang412 yliang412 force-pushed the yuchen/direct-io-aligned-alloc branch from 6b8a292 to 87fab6b Compare September 26, 2024 14:10
@yliang412 yliang412 force-pushed the yuchen/direct-io-aligned-alloc branch from 87fab6b to f6d0ed6 Compare September 26, 2024 14:13
@yliang412 yliang412 closed this Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants