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

allow re-use of OpenOptions in other crates #1

Draft
wants to merge 1 commit into
base: neon
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/fs/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ pub use file::File;

mod open_options;
pub use open_options::OpenOptions;
pub use open_options::OpenOptionsIoUringExt;

mod statx;
pub use statx::is_dir_regfile;
Expand Down
30 changes: 30 additions & 0 deletions src/fs/open_options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -389,3 +389,33 @@ impl OpenOptionsExt for OpenOptions {
self
}
}

/// Extension trait to allow re-use of [`OpenOptions`] in other crates that
/// build on top of [`io_uring`].
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
///
///

/// # Safety
///
/// The returned SQE stores the provided `path` pointer.
/// The caller must ensure that it remains valid until either the returned
/// SQE is dropped without being submitted, or if the SQE is submitted until
/// the corresponding completion is observed.
unsafe fn as_openat_sqe(&self, path: *const u8) -> io::Result<io_uring::squeue::Entry>;
}

impl OpenOptionsIoUringExt for OpenOptions {
unsafe fn as_openat_sqe(&self, path: *const u8) -> io::Result<io_uring::squeue::Entry> {
use io_uring::{opcode, types};
let flags = libc::O_CLOEXEC
| self.access_mode()?
| self.creation_mode()?
| (self.custom_flags & !libc::O_ACCMODE);

Ok(opcode::OpenAt::new(types::Fd(libc::AT_FDCWD), path)
.flags(flags)
.mode(self.mode)
.build())
}
}
26 changes: 8 additions & 18 deletions src/io/open.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,33 +11,23 @@ use std::path::Path;
#[allow(dead_code)]
pub(crate) struct Open {
pub(crate) path: CString,
pub(crate) flags: libc::c_int,
}

impl Op<Open> {
/// Submit a request to open a file.
pub(crate) fn open(path: &Path, options: &OpenOptions) -> io::Result<Op<Open>> {
use io_uring::{opcode, types};
let path = super::util::cstr(path)?;
let flags = libc::O_CLOEXEC
| options.access_mode()?
| options.creation_mode()?
| (options.custom_flags & !libc::O_ACCMODE);

// 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.
Comment on lines +20 to +22
Copy link
Member

@koivunej koivunej Nov 21, 2023

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.

let sqe = unsafe {
let p_ref = path.as_c_str().as_ptr();
crate::fs::OpenOptionsIoUringExt::as_openat_sqe(options, p_ref)?
};
CONTEXT.with(|x| {
x.handle()
.expect("Not in a runtime context")
.submit_op(Open { path, flags }, |open| {
// 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.
let p_ref = open.path.as_c_str().as_ptr();

opcode::OpenAt::new(types::Fd(libc::AT_FDCWD), p_ref)
.flags(flags)
.mode(options.mode)
.build()
})
.submit_op(Open { path }, move |_| sqe)
})
}
}
Expand Down