-
Notifications
You must be signed in to change notification settings - Fork 6.7k
Provide support for direct_reads with async_io #10197
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
Conversation
|
|
@akankshamahajan15 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
04f8d1b to
8f9303c
Compare
|
@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing. |
|
@akankshamahajan15 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
8f9303c to
dd39f48
Compare
|
@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing. |
dd39f48 to
e5a20f6
Compare
|
@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing. |
file/random_access_file_reader.cc
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e5a20f6 to
c5cd3e7
Compare
|
@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing. |
|
@akankshamahajan15 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
db_bench results for any regression for async_io = false:
|
c5cd3e7 to
62df44d
Compare
|
@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing. |
62df44d to
5dedccf
Compare
|
@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing. |
file/file_prefetch_buffer.cc
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to keep rate_limiter_priority. And, perhaps in another PR, ReadAsync should use the rate_limiter_priority and also pass it in IOOptions to the file system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am planning to create another PR supporting rate_limiter. I will pass the argument there.
5dedccf to
c9833c4
Compare
|
@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing. |
|
@akankshamahajan15 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
1 similar comment
|
@akankshamahajan15 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing. |
|
@akankshamahajan15 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
530afac to
9b570a6
Compare
|
@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing. |
|
@akankshamahajan15 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
anand1976
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Can we add a test (maybe in random_access_file_reader_test.cc) to verify ReadAsync called with an unaligned buffer?
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:
…supported. Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:
… clear. Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:
alignment and new scratch and use the scratch passed down by caller Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:
9b570a6 to
0133b34
Compare
|
@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing. |
|
@akankshamahajan15 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: Fix bug in O_DIRECT and io_uring when its EOF and bytes_read = 0 because of wrong check, it got added into incomplete list and gets stuck in an infinite loop as it will always return bytes_read = 0. The bug was introduced by PR #10197 and that PR is not released yet in any release branch. Pull Request resolved: #10368 Test Plan: Added new unit test Reviewed By: siying Differential Revision: D37885184 Pulled By: akankshamahajan15 fbshipit-source-id: 35b36a44b696d29b2f6f25301aa1b19547b4e03b
Summary: Fix bug in O_DIRECT and io_uring when its EOF and bytes_read = 0 because of wrong check, it got added into incomplete list and gets stuck in an infinite loop as it will always return bytes_read = 0. The bug was introduced by PR #10197 and that PR is not released yet in any release branch. Pull Request resolved: #10368 Test Plan: Added new unit test Reviewed By: siying Differential Revision: D37885184 Pulled By: akankshamahajan15 fbshipit-source-id: 35b36a44b696d29b2f6f25301aa1b19547b4e03b

Summary: Provide support for use_direct_reads with async_io.
TestPlan: