-
Notifications
You must be signed in to change notification settings - Fork 10
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
[1.0.2] non-atomic append fallback to random_access_file
for old kernels
#780
Conversation
#ifdef __linux__ | ||
wrote = pwritev2(fd, iov, i, 0, RWF_APPEND); //linux *not* opened with O_APPEND, appending requires special flag | ||
if(wrote == -1 && errno == EOPNOTSUPP) //fallback for kernels before 4.16 | ||
wrote = pwritev(fd, iov, i, size()); |
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.
pwritev2
ignores offset (except -1) when given RWF_APPEND
.
pwritev
you need size because you want to append. I agree it is cleaner and more concise. Is it actually more performant?
I wonder if pwritev
should just be used.
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.
Oh, never mind. It is more performant because you don't have to call size()
.
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.
When I saw the report about amznlinux2, I wondered if we shouldn't try to support it. This resolves #776 then. My vote would be to put it in 1.0.2.
random_access_file
for old kernelsrandom_access_file
for old kernels
Note:start |
random_access_file
, used for state_history in Spring, makes use of a kernel feature added in 4.16. I didn't really think much of that requirement because 4.16 was released well over 6 years (before EOS genesis!), and is well outside the scope of our officially supported platforms.Yet we've now seen two reports of users stumbling on this:
state_history doesn't actually need this atomic append, the implementation was just done this way for cleanliness, conciseness, and performance. So adding a fallback for old kernels here doesn't impact correctness for state_history. Still not really sure I want to add this additional chatter to the code though.
Footnotes
I regret a little not bumping the reproducible build's runtime requirement before 1.0.0 release so that it simply would not work on Ubuntu 18 -- other projects have made this bump recently, for example
https://github.com/bitcoin/bitcoin/pull/29987
↩AL2 is in its 11th hour of support. It was originally scheduled to be EOL July 2023 but they extended it to July 2025. ↩