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.

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

Merged
merged 1 commit into from
Sep 16, 2024

Conversation

spoonincode
Copy link
Member

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

  1. 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

  2. AL2 is in its 11th hour of support. It was originally scheduled to be EOL July 2023 but they extended it to July 2025.

#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());
Copy link
Member

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.

Copy link
Member

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().

Copy link
Member

@heifner heifner left a 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.

@spoonincode spoonincode changed the title [1.0.2/RFC] non-atomic append fallback to random_access_file for old kernels [1.0.2] non-atomic append fallback to random_access_file for old kernels Sep 16, 2024
@spoonincode spoonincode marked this pull request as ready for review September 16, 2024 21:07
@spoonincode spoonincode merged commit ea7fe7c into release/1.0 Sep 16, 2024
36 checks passed
@spoonincode spoonincode deleted the nonatomic_append_fallback branch September 16, 2024 22:46
@arhag arhag added this to the Spring v1.0.2 milestone Sep 17, 2024
@ericpassmore
Copy link
Contributor

Note:start
category: Other
component: Internal
summary: Support Linux Kernel 4.15, by falling back to random_access_file for the older kernel.
Note:end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants