Read test fails in release mode

[twissel]

Running cargo +nightly test --release --test read produces

Error: Os { code: 14, kind: Other, message: "Bad address" }
thread 'read_test' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `0`: the test returned a termination value with a non-zero status code (1) which indicates a failure', src/libtest/lib.rs:196:5

[michael-grunder]

Perhaps where this points on the stack is destroyed when prep returns?https://github.com/withoutboats/iou/blob/c783dec60587bd21fcd7cee13380d6931dfe5de7/tests/read.rs#L54

This patch seems to fix the problem.

diff --git tests/read.rs tests/read.rs
index 5f24538..0b9029a 100644
--- tests/read.rs
+++ tests/read.rs
@@ -3,8 +3,8 @@ extern crate test;

 use std::fs::File;
 use std::io;
-use std::path::PathBuf;
 use std::os::unix::io::{AsRawFd, RawFd};
+use std::path::PathBuf;

 const TEXT: &[u8] = b"I really wanna stop
 But I just gotta taste for it
@@ -28,8 +28,10 @@ fn read_test() -> io::Result<()> {
     path.push("text.txt");
     let file = File::open(&path)?;
     let mut buf1 = [0; 4096];
+    let mut slice = [io::IoSliceMut::new(&mut buf1)];
+
     unsafe {
-        prep(&mut io_uring, &mut buf1, file.as_raw_fd())?;
+        prep(&mut io_uring, &mut slice, file.as_raw_fd())?;
     }

     let dirt = dirty_stack();
@@ -48,11 +50,10 @@ fn read_test() -> io::Result<()> {
 }

 #[inline(never)]
-unsafe fn prep(ring: &mut iou::IoUring, buf: &mut [u8], fd: RawFd) -> io::Result<()> {
+unsafe fn prep(ring: &mut iou::IoUring, bufs: &mut [io::IoSliceMut], fd: RawFd) -> io::Result<()> {
     let mut sq = ring.sq();
     let mut sqe = sq.next_sqe().unwrap();
-    let mut bufs = [io::IoSliceMut::new(buf)];
-    sqe.prep_read_vectored(fd, &mut bufs, 0);
+    sqe.prep_read_vectored(fd, bufs, 0);
     sqe.set_user_data(0xDEADBEEF);
     sq.submit()?;
     Ok(())

[twissel]

Yes, I did exactly the same to fix it

[withoutboats]

Does moving it into the unsafe block also work?

I want to confirm that the bufs intermediate variable needs to outlive the submit call, but does not need to live until completion.

[michael-grunder]

Looks like it does. Also now I understand why some unrelated code I have using the library is broken. 🤣

[twissel]

@withoutboats Why do you think that buffers does not need to live until completion?

[withoutboats]

From testing I think the buffers (the IoSlice/IoSliceMut values) need to live to completion but I don't think the intermediate slice of slices needs to live to completion. But I've not read the code in the kernel and I don't have any assurance from axboe that this is intended.

I hope that this is true because it makes writing a convenience wrapper for doing a single read without an intermediate slice of slices easier to implement. If not I think we'd want to try pushing axboe to implement an unvectored read and write.

To put it in terms of code, I think/hope:

&'submit [IoSlice<'complete>]

// The submission must occur during 'submit
// The completion must occur during 'complete

The problem with the current sample is that with NLL 'submit ends after the last use (the prep command, before submit happens) and the optimizer takes advantage of that fact.

[twissel]

Well for unvectored read and write we have prep_read_fixed prep_write_fixed

[withoutboats]

My understanding is that the _fixed commands only work if you have pre-registered the buffer you're using with the kernel through the io_uring_register interface.

[twissel]

Yes, you are correct, but I really think that there is only 'complete lifetime in your code

[twissel]

Btw: why we have &self not &mut self in https://docs.rs/iou/0.2.0/iou/struct.Registrar.html?

[withoutboats]

Yes, you are correct, but I really think that there is only 'complete lifetime in your code

You're saying that you think the completion needs to occur in the lifetime I've tagged 'submit? It's possible that's true - I've tried to prove that it isn't, that's why the test dirties the stack and so on, but probably the best thing is to talk to upstream about their guarantees.

Btw: why we have &self not &mut self in https://docs.rs/iou/0.2.0/iou/struct.Registrar.html?

At some point I convinced myself that simultaneous calls to io_uring_register from multiple threads is safe. This may be wrong, if so they should be &mut.

As this thread reveals: a lot of the early state of iou was me trying to guess at precisely what io_uring requires/allows in terms of Rust's type system. As a part of maturation we need to review these and become confident that the typing is correct.

[twissel]

I think buffers should to live until completion &'complete [IoSlice<'complete>]

[withoutboats]

The byte buffers themselves need to live to completion, no doubt about it, but what's unclear is if the intermediate array of iovecs needs to live to completion, or if thats part of the data that is copied out during submit?

[twissel]

Imaging if https://docs.rs/iou/0.2.0/iou/struct.SetupFlags.html#associatedconstant.SQPOLL is enabled, then all what happens on submit is sq tail++, when sq thread will see that sqe, buffers(temporary array) may already be invalid

[withoutboats]

Touché! Probably what is happening right now is that the io completes before the stack gets dirtied.

[withoutboats]

Check out axboe/liburing#44. In future kernel versions io_uring will support unvectored read/write and this won't be a thorny issue for us anymore.

Opened #29 to track documenting the safety requirements of the prep methods. Closing this issue now.