mirror of
https://github.com/neondatabase/neon.git
synced 2026-05-29 19:10:38 +00:00
part of https://github.com/neondatabase/neon/issues/7418 I reviewed how the VirtualFile API's `read` methods look like and came to the conclusion that we've been using `IoBufMut` / `BoundedBufMut` / `Slice` wrong. This patch rectifies the situation. # Change 1: take `tokio_epoll_uring::Slice` in the read APIs Before, we took an `IoBufMut`, which is too low of a primitive and while it _seems_ convenient to be able to pass in a `Vec<u8>` without any fuzz, it's actually very unclear at the callsite that we're going to fill up that `Vec` up to its `capacity()`, because that's what `IoBuf::bytes_total()` returns and that's what `VirtualFile::read_exact_at` fills. By passing a `Slice` instead, a caller that "just wants to read into a `Vec`" is forced to be explicit about it, adding either `slice_full()` or `slice(x..y)`, and these methods panic if the read is outside of the bounds of the `Vec::capacity()`. Last, passing slices is more similar to what the `std::io` APIs look like. # Change 2: fix UB in `virtual_file_io_engine=std-fs` While reviewing call sites, I noticed that the `io_engine::IoEngine::read_at` method for `StdFs` mode has been constructing an `&mut[u8]` from raw parts that were uninitialized. We then used `std::fs::File::read_exact` to initialize that memory, but, IIUC we must not even be constructing an `&mut[u8]` where some of the memory isn't initialized. So, stop doing that and add a helper ext trait on `Slice` to do the zero-initialization. # Change 3: eliminate `read_exact_at_n` The `read_exact_at_n` doesn't make sense because the caller can just 1. `slice = buf.slice()` the exact memory it wants to fill 2. `slice = read_exact_at(slice)` 3. `buf = slice.into_inner()` Again, the `std::io` APIs specify the length of the read via the Rust slice length. We should do the same for the owned buffers IO APIs, i.e., via `Slice::bytes_total()`. # Change 4: simplify filling of `PageWriteGuard` The `PageWriteGuardBuf::init_up_to` was never necessary. Remove it. See changes to doc comment for more details. --- Reviewers should probably look at the added test case first, it illustrates my case a bit.
122 lines
4.1 KiB
Rust
122 lines
4.1 KiB
Rust
use tokio_epoll_uring::BoundedBuf;
|
|
use tokio_epoll_uring::BoundedBufMut;
|
|
use tokio_epoll_uring::IoBufMut;
|
|
use tokio_epoll_uring::Slice;
|
|
|
|
pub(crate) trait SliceExt {
|
|
/// Get a `&mut[0..self.bytes_total()`] slice, for when you need to do borrow-based IO.
|
|
///
|
|
/// See the test case `test_slice_full_zeroed` for the difference to just doing `&slice[..]`
|
|
fn as_mut_rust_slice_full_zeroed(&mut self) -> &mut [u8];
|
|
}
|
|
|
|
impl<B> SliceExt for Slice<B>
|
|
where
|
|
B: IoBufMut,
|
|
{
|
|
#[inline(always)]
|
|
fn as_mut_rust_slice_full_zeroed(&mut self) -> &mut [u8] {
|
|
// zero-initialize the uninitialized parts of the buffer so we can create a Rust slice
|
|
//
|
|
// SAFETY: we own `slice`, don't write outside the bounds
|
|
unsafe {
|
|
let to_init = self.bytes_total() - self.bytes_init();
|
|
self.stable_mut_ptr()
|
|
.add(self.bytes_init())
|
|
.write_bytes(0, to_init);
|
|
self.set_init(self.bytes_total());
|
|
};
|
|
let bytes_total = self.bytes_total();
|
|
&mut self[0..bytes_total]
|
|
}
|
|
}
|
|
|
|
#[cfg(test)]
|
|
mod tests {
|
|
use std::io::Read;
|
|
|
|
use super::*;
|
|
use bytes::Buf;
|
|
use tokio_epoll_uring::Slice;
|
|
|
|
#[test]
|
|
fn test_slice_full_zeroed() {
|
|
let make_fake_file = || bytes::BytesMut::from(&b"12345"[..]).reader();
|
|
|
|
// before we start the test, let's make sure we have a shared understanding of what slice_full does
|
|
{
|
|
let buf = Vec::with_capacity(3);
|
|
let slice: Slice<_> = buf.slice_full();
|
|
assert_eq!(slice.bytes_init(), 0);
|
|
assert_eq!(slice.bytes_total(), 3);
|
|
let rust_slice = &slice[..];
|
|
assert_eq!(
|
|
rust_slice.len(),
|
|
0,
|
|
"Slice only derefs to a &[u8] of the initialized part"
|
|
);
|
|
}
|
|
|
|
// and also let's establish a shared understanding of .slice()
|
|
{
|
|
let buf = Vec::with_capacity(3);
|
|
let slice: Slice<_> = buf.slice(0..2);
|
|
assert_eq!(slice.bytes_init(), 0);
|
|
assert_eq!(slice.bytes_total(), 2);
|
|
let rust_slice = &slice[..];
|
|
assert_eq!(
|
|
rust_slice.len(),
|
|
0,
|
|
"Slice only derefs to a &[u8] of the initialized part"
|
|
);
|
|
}
|
|
|
|
// the above leads to the easy mistake of using slice[..] for borrow-based IO like so:
|
|
{
|
|
let buf = Vec::with_capacity(3);
|
|
let mut slice: Slice<_> = buf.slice_full();
|
|
assert_eq!(slice[..].len(), 0);
|
|
let mut file = make_fake_file();
|
|
file.read_exact(&mut slice[..]).unwrap(); // one might think this reads 3 bytes but it reads 0
|
|
assert_eq!(&slice[..] as &[u8], &[][..] as &[u8]);
|
|
}
|
|
|
|
// With owned buffers IO like with VirtualFilem, you could totally
|
|
// pass in a `Slice` with bytes_init()=0 but bytes_total()=5
|
|
// and it will read 5 bytes into the slice, and return a slice that has bytes_init()=5.
|
|
{
|
|
// TODO: demo
|
|
}
|
|
|
|
//
|
|
// Ok, now that we have a shared understanding let's demo how to use the extension trait.
|
|
//
|
|
|
|
// slice_full()
|
|
{
|
|
let buf = Vec::with_capacity(3);
|
|
let mut slice: Slice<_> = buf.slice_full();
|
|
let rust_slice = slice.as_mut_rust_slice_full_zeroed();
|
|
assert_eq!(rust_slice.len(), 3);
|
|
assert_eq!(rust_slice, &[0, 0, 0]);
|
|
let mut file = make_fake_file();
|
|
file.read_exact(rust_slice).unwrap();
|
|
assert_eq!(rust_slice, b"123");
|
|
assert_eq!(&slice[..], b"123");
|
|
}
|
|
|
|
// .slice(..)
|
|
{
|
|
let buf = Vec::with_capacity(3);
|
|
let mut slice: Slice<_> = buf.slice(0..2);
|
|
let rust_slice = slice.as_mut_rust_slice_full_zeroed();
|
|
assert_eq!(rust_slice.len(), 2);
|
|
assert_eq!(rust_slice, &[0, 0]);
|
|
let mut file = make_fake_file();
|
|
file.read_exact(rust_slice).unwrap();
|
|
assert_eq!(rust_slice, b"12");
|
|
assert_eq!(&slice[..], b"12");
|
|
}
|
|
}
|
|
}
|