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

Use #ifdef to guard O_DIRECT usage #404

Merged
merged 7 commits into from
Jul 24, 2024
Merged

Use #ifdef to guard O_DIRECT usage #404

merged 7 commits into from
Jul 24, 2024

Conversation

jakirkham
Copy link
Member

Fixes #383

@jakirkham jakirkham requested a review from a team as a code owner July 15, 2024 22:10
@jakirkham
Copy link
Member Author

@qkoziol could you please try this and let us know if that addresses the issue?

@jakirkham jakirkham added bug Something isn't working non-breaking Introduces a non-breaking change labels Jul 15, 2024
@jakirkham
Copy link
Member Author

AIUI O_DIRECT is only needed with GDS. However if we do need equivalent behavior on other platforms, there are other options, but it may involve reworking the code here more substantially

jakirkham
jakirkham commented Jul 15, 2024
View reviewed changes
cpp/include/kvikio/file_handle.hpp Outdated
#ifdef O_DIRECT
file_flags |= O_DIRECT;
#else
throw std::invalid_argument("'O_DIRECT' flag unsupported on this platform");
Copy link
Member Author

@jakirkham jakirkham Jul 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Currently this errors if o_direct is set and O_DIRECT is unavailable. Though other options would be to warn or simply silently suppress it

@jakirkham jakirkham force-pushed the use_ifdef_odirect branch 2 times, most recently from 11a2e52 to 08c8c5e Compare July 15, 2024 23:50
@jakirkham
Add o_direct support for macOS & Windows
eb60f97 
If the `o_direct` argument is passed with `true` on a platform other
than Linux, try to provide some equivalent behavior on common platforms
(like macOS & Windows). In the event no such option can be provided,
raise an error.
@jakirkham jakirkham force-pushed the use_ifdef_odirect branch from 08c8c5e to eb60f97 Compare July 15, 2024 23:52
jakirkham
jakirkham commented Jul 16, 2024
View reviewed changes
cpp/include/kvikio/file_handle.hpp Outdated
Comment on lines 100 to 102
// On macOS, pass `F_NOCACHE` to `fcntl` after opening file
fd = ::open(file_path.c_str(), open_fd_parse_flags(flags, false), mode);
if (fd != -1) { fcntl(fd, F_NOCACHE, 1); }
Copy link
Member Author

Choose a reason for hiding this comment

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

Added similar logic for macOS. Would be good to check whether we need this

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps @madsbk can explain what we want O_DIRECT for? Are we doing something magic in this scenario that means we don't want the kernel filesystem layer handling caching?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, or rather, cuFile does some magic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since cufile is not supported on macos, we therefore do not want this part on macos.

Copy link
Member

Choose a reason for hiding this comment

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

Good point, instead of this, we could have detail::open_fd() fail if O_DIRECT isn't available and then fall back to compatibility mode:
https://github.com/rapidsai/kvikio/blob/branch-24.08/cpp/include/kvikio/file_handle.hpp#L167-L171

@jakirkham jakirkham requested a review from madsbk July 16, 2024 01:31
madsbk
madsbk approved these changes Jul 22, 2024
View reviewed changes
Copy link
Member

@madsbk madsbk left a comment

Choose a reason for hiding this comment

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

Thanks @jakirkham, looks good.

wence-
wence- reviewed Jul 22, 2024
View reviewed changes
cpp/include/kvikio/file_handle.hpp Outdated
@@ -84,7 +91,21 @@ inline int open_fd(const std::string& file_path,
mode_t mode)
{
// NOLINTNEXTLINE(cppcoreguidelines-pro-type-vararg)
int fd = ::open(file_path.c_str(), open_fd_parse_flags(flags, o_direct), mode);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Can you please update the docstring?

@jakirkham
Copy link
Member Author

Is this superseded by PR ( #410 )?

@madsbk
Copy link
Member

madsbk commented Jul 24, 2024

Is this superseded by PR ( #410 )?

No, we still need to guard the O_DIRECT usage.
I pushed a change to enable compat mode when open_fd() fails

@madsbk madsbk requested a review from wence- July 24, 2024 08:02
wence-
wence- approved these changes Jul 24, 2024
View reviewed changes
Copy link
Contributor

@wence- wence- left a comment

Choose a reason for hiding this comment

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

Thanks John. Non-blocking docstring nit.

cpp/include/kvikio/file_handle.hpp Show resolved Hide resolved
@madsbk
Copy link
Member

madsbk commented Jul 24, 2024

/merge

@rapids-bot rapids-bot bot merged commit 89a3343 into rapidsai:branch-24.08 Jul 24, 2024
60 checks passed
@jakirkham jakirkham deleted the use_ifdef_odirect branch July 24, 2024 16:37
@jakirkham
Copy link
Member Author

Thanks Mads and Lawrence! 🙏

jakirkham
jakirkham commented Jul 24, 2024
View reviewed changes
cpp/include/kvikio/file_handle.hpp
@@ -66,7 +70,13 @@ inline int open_fd_parse_flags(const std::string& flags, bool o_direct)
default: throw std::invalid_argument("Unknown file open flag");
}
file_flags |= O_CLOEXEC;
if (o_direct) { file_flags |= O_DIRECT; }
if (o_direct) {
#if defined(O_DIRECTT)
Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like a small typo snuck in here. Fixing in PR: #416

Suggested change
#if defined(O_DIRECTT)
#if defined(O_DIRECT)

@jakirkham jakirkham mentioned this pull request Jul 24, 2024
Merged
rapids-bot bot pushed a commit that referenced this pull request Jul 24, 2024
@jakirkham
Fix O_DIRECT in defined (#416)
ec5e568 
There was a minor typo here. This fixes that

xref: #404 (comment)

Authors:
  - https://github.com/jakirkham

Approvers:
  - Mads R. B. Kristensen (https://github.com/madsbk)

URL: #416
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@madsbk madsbk madsbk approved these changes

@wence- wence- wence- approved these changes

Assignees
No one assigned
Labels
bug Something isn't working non-breaking Introduces a non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

MacOS doesn't have O_DIRECT, so I commented out this line in cpp/include/kvikio/file_handle.hpp:
3 participants
@jakirkham @madsbk @wence-