-
Notifications
You must be signed in to change notification settings - Fork 52
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
Use #ifdef
to guard O_DIRECT
usage
#404
Use #ifdef
to guard O_DIRECT
usage
#404
Conversation
@qkoziol could you please try this and let us know if that addresses the issue? |
AIUI |
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"); |
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.
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
11a2e52
to
08c8c5e
Compare
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.
08c8c5e
to
eb60f97
Compare
cpp/include/kvikio/file_handle.hpp
Outdated
// 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); } |
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.
Added similar logic for macOS. Would be good to check whether we need this
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.
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?
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.
Yes, or rather, cuFile does some magic.
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.
Since cufile is not supported on macos, we therefore do not want this part on macos.
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.
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
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.
Thanks @jakirkham, looks good.
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); |
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.
nit: Can you please update the docstring?
Is this superseded by PR ( #410 )? |
No, we still need to guard the |
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.
Thanks John. Non-blocking docstring nit.
…o use_ifdef_odirect
/merge |
Thanks Mads and Lawrence! 🙏 |
@@ -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) |
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.
Looks like a small typo snuck in here. Fixing in PR: #416
#if defined(O_DIRECTT) | |
#if defined(O_DIRECT) |
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
Fixes #383