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

Add UnsubmittedRead and Link API #294

Closed
wants to merge 6 commits into from
Closed

Conversation

ileixe
Copy link

@ileixe ileixe commented Jan 30, 2024

This PR introduces two API changes:

  1. Add UnsubmittedRead in read part,
  2. Introduce Submit and Link struct.

(1) is basically following suggested API change in: #244 and (2) is for linked operations.

User now can link their operations like below

let read1 = file
    .read_at(buf1, 0);
let read2 = file.read_at(buf2, HELLO.len() as u64).;
let write = file.write(HELLO);

let (res1, read2_future) = read1.link(read2).link(write).submit().await;
let (res2, write_future) = read2_future.await;
let res3 = write_future.await;

Closes: #289

User can specify submission entry flags (e.g. FIXED_FILE, IO_LINK,
IO_HARDLINK) via new set_flags().
@ollie-etl
Copy link
Contributor

THere are some API issue to address here.

Opening up the flags to the user is presents the user with the ability to break the tokio-uring lifecycle managment. Setting IOSQE_CQE_SKIP_SUCCESS is catastrophic.

The link API is also flawed. If a IOSQE_IO_LINK is observed in the final SQE at submission, it is ignored. The current API doesn't protect the user from that, or provide any way of knowing if the ring has been submitted in between the ops

Finally, what stops the user experiencing concurrency errors, if await is called between read1.submit and read2.submit? Another task could submit to the ring.

@ileixe
Copy link
Author

ileixe commented Jan 30, 2024

@ollie-etl Thanks for the review, you're absolutely right for possible edge cases. I myself tried to provide additional API while hiding the set_flags() first but I found it's a bit blurry to me which is the usable API. So here I'm compromising to put some constraints to users while providing the very least primitives which enables their want including us first. (e.g. No yield between links submitting "valid" sequence of operations)

If possible (and viable), I'd like to discuss some of API options in this PR so complete this functionality in here.

Let me leave some approches to discuss that direction in advance, I added link!() and hardlink!() macros before and it looks like,

let read1 = file.read();
let write1 = file.write();

let (res1, res2) = link!(read1, write1);

This will expand as you can easily expect

let read1 = read1.submit().set_flags(Flags::IO_LINK);
let write1 = write1.submit();

(read1.await, write1.await)

With those API, we can hide uring primitive to users and prevent the mistakes.

But I felt it's not very flexible and restricted because user cannot use futures directly. (e.g. what if I want to await only last one?, what if I just want to use existed API like join_all?). To claim value of such API, I felt I should provide wider variants of APIs (e.g. link!() may accept iterables)

(For other directions, I failed all the non-macro approaches as link!() requires basically different output types with different length of inputs.)

Any suggestion would be appreicated.

Currently, there's no unsubmitted API change in read part yet. Add the
new API following write part.

Add simple link operation test cases to use read/write submittable
operations.
@ollie-etl
Copy link
Contributor

My thinking on this is that Link becomes a struct, and that one of the Op traits (maybe OneshotOutputTransform ) gains a link method. This would consume the unsubmitted Op, and return a link Op. This would expose the submit method, which would either succeed in performing an atomic submission, or fail. It would also be responsible for setting the link flags.

I guess there is a question around Futures behaviour. Should the Link struct return a single future on submission? There are options here - do you only return once the chain completes, or do you provide futres for all the ops in the link chain? Both are varid, so we probably want to support both.

@ileixe
Copy link
Author

ileixe commented Jan 31, 2024

I also tried to introduce somehow nested struct Link (I saw your previous PR which is the only one that I can find for the reference to implement.)

But the problem about Link implementation that I encountered is that it's hard to imagine what the future which Link op submitted looks like. The future should return different types of outputs and I felt it's almost impossible in Rust. (Here, I assume the future should return every results)

Still, I want to try the direction as it's far cleaner API so leave here my trials briefly hoping better idea.

struct UnsubmittedOneshot<D> {
     fn link<D2>(self, other: UnsubmittedOneShot<D2>) -> Link<D, D2> {
        // returns Link struct which have two different OneShot(s). It's gonna happen recursively.
     }
}

struct Link<D1, D2> {
    // Somehow awesome nested struct which have all operations.

    fn submit(&self) -> MultiInflightOneshot<D1, D2> {
        // Return new InflightOneshot which have several OPs.
    }
}

impl Future for MultiInflightOneshot<D1, D2> {
    type Output = ??? # This is dynamically decided (D1, D2, D3, D4... etc)
}

@ollie-etl
Copy link
Contributor

struct UnsubmittedOneshot<D> {
     fn link<D2>(self, other: UnsubmittedOneShot<D2>) -> Link<D, D2> {
        // returns Link struct which have two different OneShot(s). It's gonna happen recursively.
     }
}

struct Link<D1, D2> {
    // Somehow awesome nested struct which have all operations.

    fn submit(&self) -> InflightOneshot<Link<D1,D2> {
        // Use the existing Inflight oneshot
    }
}

impl Future for InflightOneshot<Link<D1, D2>> {
    // Link Future returns a tuple of the output of the first operation, and a future for subsequent linked ops
    type Output = (<D1 as Future>::Output, D2)
}

@ileixe
Copy link
Author

ileixe commented Feb 2, 2024

let links = file.read().chain(file.write());

let (read_output, write_future) = links.await;
let (write_output, next_future) = write_future.await;

You are suggesting such API above, right? It may be possible but not sure with the current API (InflightOnshot has unbounded generic D, so I cannot implement Future again and we need to submit all the operations first to make valid links, so here we still need to know all the types before).

Let me try with some more modification.

@ollie-etl
Copy link
Contributor

@ileixe yes, thats what I had in mind, although I haven't fully thought through all the implications / issues

@ileixe
Copy link
Author

ileixe commented Feb 4, 2024

@ollie-etl I added Link struct and introduce new API: Submit(). Can you take a look whether it's viable?

I don't like introducing Submit() trait as it makes breaking change, but could not find a clean way integrating with current traits.

@ileixe ileixe force-pushed the link-ops branch 2 times, most recently from 048cd07 to ff8e180 Compare February 4, 2024 08:45
@ileixe ileixe changed the title Add UnsubmittedRead to follow new API and set_flags(). Add UnsubmittedRead and Link API Feb 4, 2024
@ollie-etl
Copy link
Contributor

@ileixe I'd like the opinion of @Noah-Kennedy, as this is building on a lot of his work

@ileixe ileixe closed this by deleting the head repository Sep 30, 2024
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

Successfully merging this pull request may close these issues.

Support for IO_LINK feature
3 participants