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

[RFC] Posix API support for Async Read and Poll APIs #9578

Closed
wants to merge 6 commits into from
Closed

[RFC] Posix API support for Async Read and Poll APIs #9578

wants to merge 6 commits into from

Conversation

akankshamahajan15
Copy link
Contributor

Summary: Provide support for Async Read and Poll in Posix file system using IOUring.

Test Plan: In progress

@akankshamahajan15 akankshamahajan15 marked this pull request as draft February 15, 2022 21:53
@akankshamahajan15 akankshamahajan15 changed the title [WIP] Posix API support for Async Read and Poll APIs [WIP] [Not ready for review] Posix API support for Async Read and Poll APIs Feb 16, 2022
siying
siying reviewed Feb 16, 2022
View reviewed changes
Copy link
Contributor

@siying siying left a comment

Choose a reason for hiding this comment

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

I'm excited in seeing progress on this! Thank you for working on it.

include/rocksdb/file_system.h Outdated
virtual IOStatus ReadAsync(const IOOptions& /*opts*/, IODebugContext* /*dbg*/,
FSReadRequest* /*req*/,
std::function<void(FSReadResponse* resp)> /*cb*/,
IOHandle* /*io_handle*/) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's interesting that we are supporting both callback and polling. Is it very hard to implement? Thinking about the data race between the two, it feels not easy.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't think about this, but its an interesting idea. I guess polling and async callback initially made sense because PosixEnv requires polling the IO uring, whereas warm storage can complete an IO asynchronously. But a warm storage Poll() implementation could handle the required synchronization by itself and just return the results.

env/io_posix.cc

#if defined(ROCKSDB_IOURING_PRESENT)
// Step 1: io_uring_queue_init
struct io_uring* iu = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

A uring is not thread safe. Do we expect the caller not to make ReadAsync() call in one thread and polling in another? It feels nicer if we can provide an interface that isn't prone to misuse.

Do we consider for a user to pass a async call context that contains an IO Uring?

Copy link
Contributor

Choose a reason for hiding this comment

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

They're supposed to be in the same thread. Do you mean a way to enforce that?The io_handle is context that the ReadAsync() implementation can allocate, and will be passed back to it in Poll(). PosixEnv can store a pointer to the IO uring in it, and compare against thread_local_io_urings_->Get() to detect if the call is being made on a different thread..

@akankshamahajan15 akankshamahajan15 changed the title [WIP] [Not ready for review] Posix API support for Async Read and Poll APIs [RFC] Posix API support for Async Read and Poll APIs Feb 23, 2022
anand1976
anand1976 reviewed Feb 24, 2022
View reviewed changes
env/io_posix.cc Outdated Show resolved Hide resolved
env/fs_posix.cc Outdated Show resolved Hide resolved
env/fs_posix.cc Outdated Show resolved Hide resolved
env/fs_posix.cc Outdated Show resolved Hide resolved
@akankshamahajan15 akankshamahajan15 marked this pull request as ready for review March 7, 2022 16:04
@akankshamahajan15 akankshamahajan15 force-pushed the posix_apis branch 2 times, most recently from e380387 to 35aee7e Compare March 7, 2022 17:42
@facebook-github-bot
Copy link
Contributor

@akankshamahajan15 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

anand1976
anand1976 reviewed Mar 7, 2022
View reviewed changes
env/fs_posix.cc Outdated Show resolved Hide resolved
env/fs_posix.cc Show resolved Hide resolved
env/io_posix.cc Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

anand1976
anand1976 reviewed Mar 9, 2022
View reviewed changes
env/io_posix.h Show resolved Hide resolved
env/io_posix.h Outdated Show resolved Hide resolved
env/io_posix.cc Outdated Show resolved Hide resolved
env/fs_posix.cc Outdated Show resolved Hide resolved
env/fs_posix.cc Outdated Show resolved Hide resolved
anand1976
anand1976 approved these changes Mar 10, 2022
View reviewed changes
Copy link
Contributor

@anand1976 anand1976 left a comment

Choose a reason for hiding this comment

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

LGTM. Left a few comments. They're not major, but hopefully we can address them.

env/io_posix.h Show resolved Hide resolved
env/fs_posix.cc Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@akankshamahajan15 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

akankshamahajan15 added a commit to akankshamahajan15/rocksdb that referenced this pull request Mar 23, 2022
@akankshamahajan15
Posix API support for Async Read and Poll APIs (facebook#9578)
26960b2 
Summary:
Provide support for Async Read and Poll in Posix file system using IOUring.

Pull Request resolved: facebook#9578

Test Plan: In progress

Reviewed By: anand1976

Differential Revision: D34690256

Pulled By: akankshamahajan15

fbshipit-source-id: 291cbd1380a3cb904b726c34c0560d1b2ce44a2e
pingyu pushed a commit to pingyu/rocksdb that referenced this pull request Oct 22, 2022
@akankshamahajan15 @pingyu
Posix API support for Async Read and Poll APIs (facebook#9578)
7a9e0e5 
Summary:
Provide support for Async Read and Poll in Posix file system using IOUring.

Pull Request resolved: facebook#9578

Test Plan: In progress

Reviewed By: anand1976

Differential Revision: D34690256

Pulled By: akankshamahajan15

fbshipit-source-id: 291cbd1380a3cb904b726c34c0560d1b2ce44a2e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@siying siying siying left review comments

@anand1976 anand1976 anand1976 approved these changes

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@akankshamahajan15 @facebook-github-bot @siying @anand1976