Add explicit registered file type

[mxxo]

This PR reduces end user complexity for properly setting up fixed file IO in the kernel.

The fixed file interface was a little confusing for me (as opposed to fixed buffers, an entirely different thing as I just learned). After registering your RawFd's, you had to pass the index of that fd as registered in the kernel fileset (not the fd itself) into prep methods. Then, you had to set SubmissionFlags::FIXED_FILE after the prep call.

This PR introduces a new type representing a member of the kernel fileset called RegisteredFd. After a successful call to Registrar::register_files(&fds), you can access the currently registered files using a new Registrar member function fileset(). This returns a slice of registered file references owned by the registrar. The only way to get RegisteredFds is through this method (not counting placeholders, see further on for more info).

The RegisteredFds can then be passed to prep methods just like RawFd. I've added a private helper method set_fixed_flag so the users no longer have to set the flag themselves.

There is also the possibility of "sparse" registered filesets where one reserves a large fileset filled with -1s to be filled in later using the new update_registered_files method. I tried to implement these, but was unable to test them because my kernel is too old to support updates (see the ignored test tests/fileset-placeholder.rs). Regular fixed files seem to work OK (tests/fixed-file-write.rs)

I went off on a lark trying to implement a reserve method for the registrar but got stuck in temporary lifetime hell where adding a few print statements changed all my test failures to passes.

The end result is users must construct a buffer of placeholder values outside the register call, to ensure they remain valid until the fileset is updated.

Right now it looks like this

use iou::RegisteredFd; 
let placeholders = [RegisteredFd::placeholder().as_fd(); 4096]; 
registrar.register_files(&placeholders); 

// later 

offset = 0; 
registrar.update_registered_files(offset, &actual_fds); 

which isn't as nice as

registrar.reserve(4096);
registrar.update_registered_files(offset, &actual_fds); 

but it is similar to how users have to handle other kinds of input buffers, and maybe that kind of high-level interface isn't what we're trying to build here anyway.

Please let me know if you'd like any changes. The enum checking in each prep method is a little verbose but I thought it best to keep it more explicit at the prep call sites.

[withoutboats]

I'm sorry I never reviewed this! Between the pandemic, being unemployed, etc.

I think this is very cool and a good direction for pre-registering files. I like the RegisteredFd type, and changing the prep methods to take either a RegisteredFd or a RawFd. All of that makes sense and seems great.

I'm not as convinced about storing a fileset with the Registrar. iou is a super low level interface; I think instead we should make RegisteredFd::new a public API and make tracking their fileset the user's responsibiliy.

[withoutboats]

Instead of (or in addition to) making RegisteredFd::new public, we could make Registrar::register_files (and the update method as well) return impl Iterator<Item = RegisteredFd>, so users can write things like:

let registered_fds: Vec<RegisteredFd> = registrar.register_files(fds).collect();

[mxxo]

There is no problem at all, I really believe that people submitting PRs are indebted to the maintainer and not the other way around.

we could make Registrar::register_files (and the update method as well) return impl Iterator

I really like this idea. I can rebase the necessary changes onto #47 when it lands.

[withoutboats]

If you have the time, I'd rather land this sooner and rebase #47 off of it, since #47 is larger and will take longer to land.

[withoutboats]

Also, to save you time, the signature will have to be:

pub fn register_files<'a>(&self, files: &'a [RawFd]) -> io::Result<impl Iterator<Item = RegisteredFd> + 'a>

[mxxo]

You got it, will try and get it out today.

[mxxo]

In ede3190 I changed the Registrar to only track fileset size. I think tracking the size may be necessary to keep update_registered_files a safe method since the user provides the slice and offset, which might be out of bounds. In my tests io_uring errors with EBADF but I am unsure if it overruns a buffer somewhere. I thought it would be better to err on the safe side.

Tracking the fileset size also lets us provide better error messages for other cases, e.g. double registration. Before, this code would fail with EBUSY

let ring = IoUring::new(1).
let mut reg = ring.registrar();
let _ = reg.register_files(&[1]).unwrap();
let _ = reg.register_files(&[1]).unwrap();

but now, it errors with "there is a preexisting registered fileset".

[withoutboats]

I think tracking the size may be necessary to keep update_registered_files a safe method since the user provides the slice and offset, which might be out of bounds.

If the offset is past the end of the array being held in the kernel, I'm sure that the kernel errors and does not allow an out of bounds write. I often get tripped up by this, but remember that the interface cannot allow users to do anything unsafe in the kernel's memory, or else it would be totally insecure and exploitable (for example, a malicious user could pass an offset that lets them write into memory associated with other processes). Of course it lets you do whatever you want to the memory shared between your process and the kernel, which is why we have to deal with all the data race concerns around that.

So it's not necessary for safety! Since the registrar is not designed to necessarily be used permanently (you can drop the registrar and then get a new registrar from your IoUring) it can't hold state, any state would have to be held by IoUring. I'd rather make IoUring just a narrow wrapper around liburing's io_uring, so even though the error messages are worse lets not track anything about what's been registered before.

[mxxo]

That makes sense, I've removed all the size checks in 1e8620c.

After doing so, I ran into a strange issue that I think was masked by the time taken by the checks in update_registered_files. There is an error if you update right after the initial register call, which luckily is not very likely to occur in real code. Maybe it is specific to my setup? (Linux 5.8.1, liburing 2.0.0)

If you change this passing test

#[test]
fn valid_fd_update() {
    let ring = IoUring::new(1).unwrap();

    let file = std::fs::File::create("tmp.txt").unwrap();
    let _ = ring.registrar().register_files(&[file.as_raw_fd()]).unwrap();

    let new_file = std::fs::File::create("new_tmp.txt").unwrap();
    let _ = ring.registrar().update_registered_files(0, &[new_file.as_raw_fd()]).unwrap();

    let _ = std::fs::remove_file("tmp.txt");
    let _ = std::fs::remove_file("new_tmp.txt");
}

to update the fileset immediately after registration it fails with EINVAL invalid argument.

#[test]
fn valid_fd_update() {
    let ring = IoUring::new(1).unwrap();

    let file = std::fs::File::create("tmp.txt").unwrap();
    let new_file = std::fs::File::create("new_tmp.txt").unwrap();    
    let _ = ring.registrar().register_files(&[file.as_raw_fd()]).unwrap();
    let _ = ring.registrar().update_registered_files(0, &[new_file.as_raw_fd()]).unwrap(); // fails 

    let _ = std::fs::remove_file("tmp.txt");
    let _ = std::fs::remove_file("new_tmp.txt");
}

test registrar::tests::empty_update_err ... ok
test registrar::tests::empty_unregister_err ... ok
test registrar::tests::placeholder_submit ... ok
test registrar::tests::register_bad_fd ... ok
test registrar::tests::placeholder_update ... ok
test registrar::tests::offset_out_of_bounds_update ... ok
test registrar::tests::double_register ... ok
test registrar::tests::slice_len_out_of_bounds_update ... ok
test registrar::tests::register_empty_slice ... ok
test registrar::tests::valid_fd_update ... FAILED
test tests::test_resultify ... ok

failures:

---- registrar::tests::valid_fd_update stdout ----
thread 'registrar::tests::valid_fd_update' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 22, kind: InvalidInput, message: "Invalid argument" }', src/registrar.rs:342:17

All tests in the commit pass on my machine, it's just if the update happens immediately after registration that this happens. I thought I would mention in case this is my error.

[withoutboats]

Thanks for the info, must be some kind of race condition in the kernel. This looks good to merge and rebase off of! Thanks