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

BoundedBuf.slice() panics for empty bufs #46

Open
problame opened this issue Feb 7, 2024 · 0 comments
Open

BoundedBuf.slice() panics for empty bufs #46

problame opened this issue Feb 7, 2024 · 0 comments

Comments

@problame
Copy link
Collaborator

problame commented Feb 7, 2024

Problem

The

impl<T: IoBuf> BoundedBuf for T

panics here

assert!(begin < self.bytes_total());

if the T is a zero-capacity IoBuf (e.g., a Vec::new()).

This is unlike regular Rust slices, which are zero-sized:

let v = Vec::new();
&v[..];
&v[0..0];

Workaround

Initially, Pageserver integration PRs neondatabase/neon#6663 dealt with this by checking for empty bufs beforehand.

As of neondatabase/neon#8717, the Pageserver write path uses a newtype that, in addition to mitigating other deficiencies of the tokio_uring::Slice, mitigates this issue here:

 let s = if len == 0 {
    // `BoundedBuf::slice(0..len)` or `BoundedBuf::slice(..)` has an incorrect assertion,
    // causing a panic if len == 0.
    // The Slice::from_buf_bounds has the correct assertion (<= instead of <).
    // => https://github.com/neondatabase/tokio-epoll-uring/issues/46
    let slice = self.slice_full();
    let mut bounds: Range<_> = slice.bounds();
    bounds.end = bounds.start;
    Slice::from_buf_bounds(slice.into_inner(), bounds)
} else {
    self.slice(0..len)
};

https://github.com/neondatabase/neon/blob/a5c062c33619d19b58dbf89517d021199bfd772c/pageserver/src/virtual_file/owned_buffers_io/io_buf_ext.rs#L58-L69

problame added a commit to neondatabase/neon that referenced this issue Feb 12, 2024
This PR refactors the `blob_io` code away from using slices towards
taking owned buffers and return them after use.
Using owned buffers will eventually allow us to use io_uring for writes.

part of #6663

Depends on neondatabase/tokio-epoll-uring#43

The high level scheme is as follows:
- call writing functions with the `BoundedBuf`
- return the underlying `BoundedBuf::Buf` for potential reuse in the
caller

NB: Invoking `BoundedBuf::slice(..)` will return a slice that _includes
the uninitialized portion of `BoundedBuf`_.
I.e., the portion between `bytes_init()` and `bytes_total()`.
It's a safe API that actually permits access to uninitialized memory.
Not great.

Another wrinkle is that it panics if the range has length 0.

However, I don't want to switch away from the `BoundedBuf` API, since
it's what tokio-uring uses.
We can always weed this out later by replacing `BoundedBuf` with our own
type.
Created an issue so we don't forget:
neondatabase/tokio-epoll-uring#46
jcsp pushed a commit to neondatabase/neon that referenced this issue Feb 12, 2024
This PR refactors the `blob_io` code away from using slices towards
taking owned buffers and return them after use.
Using owned buffers will eventually allow us to use io_uring for writes.

part of #6663

Depends on neondatabase/tokio-epoll-uring#43

The high level scheme is as follows:
- call writing functions with the `BoundedBuf`
- return the underlying `BoundedBuf::Buf` for potential reuse in the
caller

NB: Invoking `BoundedBuf::slice(..)` will return a slice that _includes
the uninitialized portion of `BoundedBuf`_.
I.e., the portion between `bytes_init()` and `bytes_total()`.
It's a safe API that actually permits access to uninitialized memory.
Not great.

Another wrinkle is that it panics if the range has length 0.

However, I don't want to switch away from the `BoundedBuf` API, since
it's what tokio-uring uses.
We can always weed this out later by replacing `BoundedBuf` with our own
type.
Created an issue so we don't forget:
neondatabase/tokio-epoll-uring#46
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

No branches or pull requests

1 participant