Skip to content

Allow sync_file_range2 on supported architectures. #30971

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
Merged
justincormack merged 1 commit into from
Feb 15, 2017
Merged

Allow sync_file_range2 on supported architectures. #30971

justincormack merged 1 commit into from
Feb 15, 2017

Conversation

@dargor
Copy link
Contributor

@dargor dargor commented Feb 13, 2017

Using docker 1.13.1 on a ppc64le host, running postgres (tried with 9.4 to 9.6) gives the following warning when trying to flush data to disks (which happens very frequently) :
WARNING: could not flush dirty data: Operation not permitted.

A quick dig in postgres source code indicate it uses sync_file_range(2) to flush data; which on ppe64le and arm64 is translated to sync_file_range2(2) for alignements reasons.

But docker does not allow sync_file_range2(2), making postgres sad because it can not flush its buffers. arm_sync_file_range(2) is an ancient alias to sync_file_range2(2), the syscall was renamed in Linux 2.6.22 when the same syscall was added for PowerPC.

This diff make postgres, docker and ppc64le great again.

@GordonTheTurtle GordonTheTurtle added dco/no Automatically set by a bot when one of the commits lacks proper signature status/0-triage labels Feb 13, 2017
@dargor dargor force-pushed the allow_sync_file_range2 branch from f10b99e to ddcfcc8 Compare February 13, 2017 13:35
@GordonTheTurtle GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Feb 13, 2017
@cpuguy83
Copy link
Member

cpuguy83 commented Feb 13, 2017 

13:51:26 The result of go generate ./profiles/seccomp/ differs
13:51:26 
13:51:26  M profiles/seccomp/default.json
13:51:26 
13:51:26 Please re-run go generate ./profiles/seccomp/

ping @justincormack

@justincormack
Copy link
Contributor

justincormack commented Feb 13, 2017 •
edited
Loading

Yes, you need to regenerate the JSON version with go generate... it is picky, it wants the final line ending the same as the script...

@justincormack
Copy link
Contributor

justincormack commented Feb 13, 2017

If you just remove the final line feed on the json perl -pi -e 'chomp if eof' profiles/seccomp/default.json should do it...

@justincormack
Copy link
Contributor

justincormack commented Feb 13, 2017

Other than that LGTM

@justincormack justincormack added this to the 1.13.2 milestone Feb 13, 2017
@cpuguy83
Copy link
Member

cpuguy83 commented Feb 13, 2017

Please make sure to squash commits, thanks!

@dargor dargor force-pushed the allow_sync_file_range2 branch from 7084bb0 to 3d926d8 Compare February 13, 2017 16:09
@justincormack
Copy link
Contributor

justincormack commented Feb 13, 2017

Sorry, the CI is obstinately refusing to rebuild even though I poked it.

@justincormack
Copy link
Contributor

justincormack commented Feb 13, 2017

Ok, its rebuilding now should be good.

@tonistiigi
Copy link
Member

tonistiigi commented Feb 13, 2017

LGTM

@justincormack
Copy link
Contributor

justincormack commented Feb 14, 2017

Hmm, Janky still thinks that go generate ./profiles/seccomp/ needs to be run...

tonistiigi
tonistiigi reviewed Feb 14, 2017
View reviewed changes
profiles/seccomp/default.json Outdated
Copy link
Member

@tonistiigi tonistiigi Feb 14, 2017

Choose a reason for hiding this comment

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

Extra commas here and line 642 seems to cause the validation issue.

@dargor dargor force-pushed the allow_sync_file_range2 branch from 3d926d8 to 52d8f58 Compare February 14, 2017 20:30
@justincormack
Copy link
Contributor

justincormack commented Feb 15, 2017

Thanks!

@justincormack justincormack merged commit 3b5ed56 into moby:master Feb 15, 2017
@thaJeztah
Copy link
Member

thaJeztah commented Feb 18, 2017

@justincormack I tried cherry-picking this into the 1.13.x branch (for 17.03), but it didn't apply cleanly. If you want this into that branch, could you open a PR?

@justincormack justincormack mentioned this pull request Feb 18, 2017
Merged
@thaJeztah thaJeztah mentioned this pull request Aug 24, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tonistiigi tonistiigi tonistiigi left review comments

+1 more reviewer

@justincormack justincormack justincormack approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

process/cherry-picked status/2-code-review

Projects

None yet

Milestone

17.03.0

Development

Successfully merging this pull request may close these issues.

7 participants

@dargor @cpuguy83 @justincormack @tonistiigi @thaJeztah @LK4D4 @GordonTheTurtle