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.

Sign up for GitHub

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

Jump to bottom

allow re-use of OpenOptions in other crates #1

Draft
wants to merge 1 commit into
base: neon
Choose a base branch
from
Draft

allow re-use of OpenOptions in other crates #1

wants to merge 1 commit into from

Conversation

problame
Copy link
Collaborator

The OpenOptions struct's functionality of mapping the builder pattern APIs to libc-level flags flags is useful outside of tokio-uring.

Our particular use case is an alternative approach to tokio + io_uring which will be open-sourced soon. We are already re-using the IoBuf trait there, so, it makes sense to also re-use OpenOptions.

So, this PR adds an extension trait for OpenOptions that allows producing an io_uring::squeue::Entry.

@problame
allow re-use of OpenOptions in other crates
84d6019 
The `OpenOptions` struct's functionality of mapping the builder pattern APIs to
libc-level flags flags is useful outside of `tokio-uring`.

Our particular use case is an alternative approach to tokio + io_uring
which will be open-sourced soon. We are already re-using the `IoBuf`
trait there, so, it makes sense to also re-use `OpenOptions`.

So, this PR adds an extension trait for `OpenOptions` that allows producing
an `io_uring::squeue::Entry`.
problame added a commit that referenced this pull request Nov 20, 2023
@problame
allow re-use of OpenOptions in other crates
012722a 
The `OpenOptions` struct's functionality of mapping the builder pattern APIs to
libc-level flags flags is useful outside of `tokio-uring`.

Our particular use case is an alternative approach to tokio + io_uring
which will be open-sourced soon. We are already re-using the `IoBuf`
trait there, so, it makes sense to also re-use `OpenOptions`.

So, this PR adds an extension trait for `OpenOptions` that allows producing
an `io_uring::squeue::Entry`.

(This is a back-port of #1
to tokio-uring v0.4.0, the latest official release).
problame added a commit to neondatabase/tokio-epoll-uring that referenced this pull request Nov 20, 2023
@problame
vendor (and add ext trait for) IoBuf/IoBufMut and OpenOptions from `t…
37497b6 
…okio-uring`

For [`open_at`
support](#21) we
need an `OpenOptions` struct. Sadly we can't re-use the one from
`tokio-uring` because it doesn't have a public API for the conversion of
OpenOptions into the relevant libc flags.

We created [a PR asking for such an
API](neondatabase/tokio-uring#1), but, in the
meantime, let's unblock ourselves by vendoring the pieces of
`tokio-uring` that we need, and cusotmize them as needed.

The files that reproduce the `tokio-uring` LICENSE text at the top
are copied from `tokio-uring.git:d5e90539bd6d1c518e848298564a098c300866bc`.

Files without it were written by myself.
problame added a commit to neondatabase/tokio-epoll-uring that referenced this pull request Nov 20, 2023
@problame
vendor (and add ext trait for) IoBuf/IoBufMut from tokio-uring
6feb89e 
For [`open_at`
support](#21) we
need an `OpenOptions` struct. Sadly we can't re-use the one from
`tokio-uring` because it doesn't have a public API for the conversion of
OpenOptions into the relevant libc flags.

We created [a PR asking for such an
API](neondatabase/tokio-uring#1), but, in the
meantime, let's unblock ourselves by vendoring the pieces of
`tokio-uring` that we need, and cusotmize them as needed.

This PR starts that effort by vendoring the `IoBuf`/`IoBufMut` traits.
`OpenOptions` will follow in a later PR.

The files that reproduce the `tokio-uring` LICENSE text at the top
are copied from `tokio-uring.git:d5e90539bd6d1c518e848298564a098c300866bc`.

Files without it were written by myself.
problame added a commit to neondatabase/tokio-epoll-uring that referenced this pull request Nov 20, 2023
@problame
vendor (and add ext trait for) IoBuf/IoBufMut from tokio-uring
16d09a9 
For [`open_at`
support](#21) we
need an `OpenOptions` struct. Sadly we can't re-use the one from
`tokio-uring` because it doesn't have a public API for the conversion of
OpenOptions into the relevant libc flags.

We created [a PR asking for such an
API](neondatabase/tokio-uring#1), but, in the
meantime, let's unblock ourselves by vendoring the pieces of
`tokio-uring` that we need, and cusotmize them as needed.

This PR starts that effort by vendoring the `IoBuf`/`IoBufMut` traits.
`OpenOptions` will follow in a later PR.

The files that reproduce the `tokio-uring` LICENSE text at the top
are copied from `tokio-uring.git:d5e90539bd6d1c518e848298564a098c300866bc`.

Files without it were written by myself.
This was referenced Nov 20, 2023
Merged
Closed
@problame problame marked this pull request as draft November 20, 2023 11:28
@problame
Copy link
Collaborator Author

I didn't pursue this approach further, rationale see neondatabase/tokio-epoll-uring#21 (comment)

problame added a commit to neondatabase/tokio-epoll-uring that referenced this pull request Nov 20, 2023
@problame
vendor (and add ext trait for) IoBuf/IoBufMut from tokio-uring
04cb1bd 
For [`open_at`
support](#21) we
need an `OpenOptions` struct. Sadly we can't re-use the one from
`tokio-uring` because it doesn't have a public API for the conversion of
OpenOptions into the relevant libc flags.

We created [a PR asking for such an
API](neondatabase/tokio-uring#1), but, in the
meantime, let's unblock ourselves by vendoring the pieces of
`tokio-uring` that we need, and cusotmize them as needed.

This PR starts that effort by vendoring the `IoBuf`/`IoBufMut` traits.
`OpenOptions` will follow in a later PR.

The files that reproduce the `tokio-uring` LICENSE text at the top
are copied from `tokio-uring.git:d5e90539bd6d1c518e848298564a098c300866bc`.

Files without it were written by myself.
problame added a commit to neondatabase/tokio-epoll-uring that referenced this pull request Nov 20, 2023
@problame
vendor (and add ext trait for) IoBuf/IoBufMut from tokio-uring
716ef11 
For [`open_at`
support](#21) we
need an `OpenOptions` struct. Sadly we can't re-use the one from
`tokio-uring` because it doesn't have a public API for the conversion of
OpenOptions into the relevant libc flags.

We created [a PR asking for such an
API](neondatabase/tokio-uring#1), but, in the
meantime, let's unblock ourselves by vendoring the pieces of
`tokio-uring` that we need, and cusotmize them as needed.

This PR starts that effort by vendoring the `IoBuf`/`IoBufMut` traits.
`OpenOptions` will follow in a later PR.

The files that reproduce the `tokio-uring` LICENSE text at the top
are copied from `tokio-uring.git:d5e90539bd6d1c518e848298564a098c300866bc`.

Files without it were written by myself.
koivunej
koivunej reviewed Nov 21, 2023
View reviewed changes
src/fs/open_options.rs
pub trait OpenOptionsIoUringExt {
/// Turn `self` into an [`::io_uring::opcode::OpenAt`] SQE for the given `path`.
///
///
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
///
///

koivunej
koivunej reviewed Nov 21, 2023
View reviewed changes
src/io/open.rs
Comment on lines +20 to +22
// Get a reference to the memory. The string will be held by the
// operation state and will not be accessed again until the operation
// completes.
Copy link
Member

@koivunej koivunej Nov 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Get a reference to the memory. The string will be held by the
// operation state and will not be accessed again until the operation
// completes.
// TODO: SAFETY:
// how exactly is the safety requirement fulfilled here?

Turn formatting to https://rust-lang.github.io/rust-clippy/master/#/undocumented as I could imagine that is expected as the lint has been enabled in pageserver as well. Without re-reading the tokio-uring I am unable to answer this.

Regardless I don't think it makes sense to include "get a reference to the memory" or anything like reading the code aloud, instead the comment should say how are the requirements fulfilled.

Accessing or not accessing the Open::path: CString does not seem relevant here.

problame added a commit to neondatabase/tokio-epoll-uring that referenced this pull request Nov 22, 2023
@problame
vendor IoBuf/IoBufMut from tokio-uring (#24)
91d67e2 
In the draft PR for [`open_at`
support](#21) we
need an `OpenOptions` struct. Sadly we can't re-use the one from
`tokio-uring` because it doesn't have a public API for the conversion of
OpenOptions into the relevant libc flags.

We created [a PR asking for such an
API](neondatabase/tokio-uring#1), but, in the
meantime, let's unblock ourselves by vendoring the pieces of
`tokio-uring` that we need, and cusotmize them as needed.

This PR starts that effort by vendoring the `IoBuf`/`IoBufMut` traits as
well as `tokio-uring`'s approach to support slice-like operations.
Support for `OpenOptions` will follow as part of [the PR that adds
`open_at` support on top of this
PR](#25).

The files that reproduce the `tokio-uring` LICENSE text at the top are
copied from `tokio-uring.git:d5e90539bd6d1c518e848298564a098c300866bc`.
Files without it were written by myself.

To make `cargo test` pass, I had to remove the examples in the doc
comments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@koivunej koivunej koivunej left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@problame @koivunej