tokio-rs / io-uring

The `io_uring` library for Rust
Apache License 2.0
1.16k stars 129 forks source link

Port to rustix #207

Closed jordanisaacs closed 1 year ago

jordanisaacs commented 1 year ago

I marked this as a draft because it could do with some code cleanup, but it works and runs the tests (almost successfully, see below). Also requires rustix to point to git until some missing io_uring types are merged & a new release is tagged.

Breaking changes: The API is the same except for function parameter types. It uses rustix types, and re-exports them out. Flags are now strongly typed. Another big user facing type change is user_data which uses io_uring_user_data for keeping pointer provenance.

Rustix, bindgen, and new operations: All the bindgen files and syscalls are removed. Bindgen is done through linux-raw-sys which is used by rustix. So updating would involve updating linux-raw-sys, then creating the type wrappers in rustix, and then exposing the operation in io-uring.

Why:

  1. Better safety: get to utilize all the work rustix has done with safety. Pointer provenance, flag types, etc.
  2. Separation of concerns. Use crates that already do the bindgen, types, and syscalls. Let the crate focus on the API.
  3. io_uring can now be compiled on no_std. Still requires alloc for probes, but can even remove that dependency if you want to go the mmap route like liburing.

Tests: Passes all the tests/examples except for a single socket test with the following config. Every test passed when commenting out the socket fd number assertion. Not sure what is wrong with it:

ring type: IoUring<squeue::Entry, cqueue::Entry32>
params: Parameters {
    is_setup_sqpoll: false,
    is_setup_iopoll: false,
    is_setup_single_issuer: false,
    is_feature_single_mmap: true,
    is_feature_nodrop: true,
    is_feature_submit_stable: true,
    is_feature_rw_cur_pos: true,
    is_feature_cur_personality: true,
    is_feature_poll_32bits: true,
    sq_entries: 8,
    cq_entries: 16,
}
test socket
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `12`,
 right: `11`', io-uring-test/src/tests/net.rs:1296:5
quininer commented 1 year ago

I would reject it.

This introduced a huge dependency and I didn't see any (real) benefits.

jordanisaacs commented 1 year ago

Understandable. I already am using rustix & no_std/no libc so this helps unify a lot of my dependencies and types. For those who are interested in this, I have a new library rustix-uring that I will publish to cargo once rustix releases a new version that contains all the necessary io_uring types. For now you can point to it with a git dependency.

You may want the rustix port:

  1. Already are using rustix in your library
  2. Want no_std support
  3. Want a ltitle more type safety (eg with flags).

Otherwise stick with this one as It does pull in a huge dependency, and is very much a breaking change for your existing code.

quininer commented 1 year ago

Let me explain a little bit why I don't think this is of real benefit to the io_uring crate.

  1. it's a huge dependency.
  2. This is fine if you use rustix. This sounds like a political benefit rather than a practical one.
  3. If anyone want no_std support, it can be done in this crate, just use own error type instead of io::Error. but I don't currently see anyone want it.
  4. This crate already provides type safety, it does a lot. I've intentionally kept a part of it, as its meaning may change in future.
  5. Using rustix made the development process a lot longer. As you have done, adding an opcode requires update linux-raw-sys and then rustix, and then rustix-uring.
  6. I need to keep bindgen, to support compiling on niche arch/system (eg loongarch, private distro). I wish to preserve ability for users to generate bindings using their own header files.

rustix is a beautiful project, and it's the most mature libc-free standard library out there. I want rustix to be successful, but at this time, I don't think it's a good fit for io_uring crate. Especially I use io_uring in production and don't want any surprises.

jordanisaacs commented 1 year ago

Thanks for explaining your reasoning! That makes a ton of sense.