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

Handle the result of posix_fallocate system call #15650

Merged
merged 1 commit into from May 2, 2019

Conversation

Projects
None yet
9 participants
@lucayepa
Copy link
Contributor

commented Mar 23, 2019

The system call posix_fallocate is not supported on some filesystems.

  • catches the result of posix_allocate and fall back to the default behaviour if the return value is different from 0 (success)

Fixes #15624

@gmaxwell

This comment has been minimized.

Copy link
Member

commented Mar 23, 2019

OR-ing the fallback version with linux might be clearer than sticking in a bunch of returns and leaving in a chunk of dead code that some static analysis tools may warn about.

@lucayepa

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2019

Do you think we can consider the Linux part in the fallback? I don't know if posix_allocate can be called on every platform. It will become something like:

#if defined(WIN32)
...
#elif defined(MAC_OSX)
...
#endif
    // Linux and fallback version
 (this calls posix_fallocate for every other platform)

This way the system call posix_fallocate would be called for every fallback platform. I don't know if this is a good idea.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #14485 (Try to use posix_fadvise with CBufferedFile by luke-jr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@gmaxwell

This comment has been minimized.

Copy link
Member

commented Mar 23, 2019

No but you can do
#if win32
#elif mac
#else
#if linux
#endif
fallback
#endif

@lucayepa lucayepa force-pushed the lucayepa:handle-posix-fallocate branch from 0e7e234 to eb18d9a Mar 23, 2019

@lucayepa

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2019

This sounds great. I indent the nested #if to be consistent with the style of the rest of the code.

I also add here a link for reference to the first introduction of the system call: #2229.

@gmaxwell

This comment has been minimized.

Copy link
Member

commented Mar 23, 2019

Style ACK. :) (should get some testing on an impacted filesystem)

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Mar 23, 2019

utACK eb18d9a

@MarcoFalke MarcoFalke added this to the 0.19.0 milestone Mar 23, 2019

@lucayepa lucayepa force-pushed the lucayepa:handle-posix-fallocate branch from eb18d9a to 7806259 Mar 23, 2019

@lucayepa

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2019

Just tested on an affected filesystem (unionf on Debian stable), using strace to check the system calls sent to the system. The success return value of posix_fallocate is obviously zero. Somehow in a previous version I had the '!', but then I pushed the wrong one, without it. I've just force-pushed the good version, I've just tested with strace.

Affected filesystem (fallocate result is EOPNOTSUPP, and the fallback code is activated):

$ strace test/test_bitcoin -t flatfile_tests 2>&1 | grep -A 5 fallocate | tail -6
fallocate(3, 0, 0, 100)                 = -1 EOPNOTSUPP (Operation not supported)
fcntl(3, F_GETFL)                       = 0x8002 (flags O_RDWR|O_LARGEFILE)
fstat(3, {st_mode=S_IFREG|0644, st_size=0, ...}) = 0
fstatfs(3, {f_type=FUSE_SUPER_MAGIC, f_bsize=4096, f_blocks=2047598, f_bfree=712591, f_bavail=603651, f_files=524288, f_ffree=411273, f_fsid={val=[0, 0]}, f_namelen=255, f_frsize=4096, f_flags=ST_VALID|ST_NOSUID|ST_NODEV|ST_RELATIME}) = 0
pwrite64(3, "\0", 1, 99)                = 1
close(3)                                = 0

Ext4 filesystem (fallocate result is zero, that means success, and no other code is activated):

$ strace test/test_bitcoin -t flatfile_tests 2>&1 | grep -A 1 fallocate | tail -2
fallocate(3, 0, 0, 100)                 = 0
close(3)                                = 0
Show resolved Hide resolved src/util/system.cpp Outdated

@lucayepa lucayepa force-pushed the lucayepa:handle-posix-fallocate branch from 7806259 to 5d35ae3 Mar 23, 2019

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

Gitian builds for commit 7b13c64 (master):

Gitian builds for commit a48eb1b (master and this pull):

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

utACK 5d35ae3

@sipa

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

utACK 5d35ae3, though the Yoda condition is an uncommon style in this project.

@luke-jr
Copy link
Member

left a comment

Probably would be better to have configure check for posix_fallocate availability instead of using __linux__, but since it's already that way, this is strictly an improvement.

utACK

@hebasto

This comment has been minimized.

Copy link
Member

commented May 2, 2019

utACK 5d35ae3

1 similar comment
@practicalswift

This comment has been minimized.

Copy link
Member

commented May 2, 2019

utACK 5d35ae3

@MarcoFalke MarcoFalke merged commit 5d35ae3 into bitcoin:master May 2, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request May 2, 2019

Merge #15650: Handle the result of posix_fallocate system call
5d35ae3 Handle the result of posix_fallocate system call (Luca Venturini)

Pull request description:

  The system call `posix_fallocate` is not supported on some filesystems.

  - catches the result of posix_allocate and fall back to the default behaviour if the return value is different from 0 (success)

  Fixes #15624

ACKs for commit 5d35ae:
  MarcoFalke:
    utACK 5d35ae3
  sipa:
    utACK 5d35ae3, though the Yoda condition is an uncommon style in this project.
  hebasto:
    utACK 5d35ae3
  practicalswift:
    utACK 5d35ae3

Tree-SHA512: 7ab3b35fb633926f28a58b2b07ffde8e31bb997c80a716b1b45ee716fe9ff4ddcef0a05810bd4423530e220cfc62f8925517d27a8b92b05a524272063e43f746

sidhujag added a commit to syscoin/syscoin that referenced this pull request May 2, 2019

Merge bitcoin#15650: Handle the result of posix_fallocate system call
5d35ae3 Handle the result of posix_fallocate system call (Luca Venturini)

Pull request description:

  The system call `posix_fallocate` is not supported on some filesystems.

  - catches the result of posix_allocate and fall back to the default behaviour if the return value is different from 0 (success)

  Fixes bitcoin#15624

ACKs for commit 5d35ae:
  MarcoFalke:
    utACK 5d35ae3
  sipa:
    utACK 5d35ae3, though the Yoda condition is an uncommon style in this project.
  hebasto:
    utACK 5d35ae3
  practicalswift:
    utACK 5d35ae3

Tree-SHA512: 7ab3b35fb633926f28a58b2b07ffde8e31bb997c80a716b1b45ee716fe9ff4ddcef0a05810bd4423530e220cfc62f8925517d27a8b92b05a524272063e43f746
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.