Handle the result of posix_fallocate system call #15650

pull lucayepa wants to merge 1 commits into bitcoin:master from lucayepa:handle-posix-fallocate changing 1 files +4 −3
  1. lucayepa commented at 5:08 am on March 23, 2019: contributor

    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

  2. fanquake added the label Utils/log/libs on Mar 23, 2019
  3. gmaxwell commented at 5:13 am on March 23, 2019: contributor
    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.
  4. lucayepa commented at 5:37 am on March 23, 2019: contributor

    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:

    0#if defined(WIN32)
1...
2#elif defined(MAC_OSX)
3...
4#endif
5    // Linux and fallback version
6 (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.

  5. DrahtBot commented at 6:12 am on March 23, 2019: member

    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.

  6. gmaxwell commented at 7:28 am on March 23, 2019: contributor
    No but you can do #if win32 #elif mac #else #if linux #endif fallback #endif
  7. lucayepa force-pushed on Mar 23, 2019
  8. lucayepa commented at 1:12 pm on March 23, 2019: contributor

    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.

  9. gmaxwell commented at 1:25 pm on March 23, 2019: contributor
    Style ACK. :) (should get some testing on an impacted filesystem)
  10. MarcoFalke commented at 1:37 pm on March 23, 2019: member
    utACK eb18d9af0686d79fad9761aacc8cda76b9ebe9a3
  11. MarcoFalke added this to the milestone 0.19.0 on Mar 23, 2019
  12. MarcoFalke added the label Needs gitian build on Mar 23, 2019
  13. lucayepa force-pushed on Mar 23, 2019
  14. lucayepa commented at 6:48 pm on March 23, 2019: contributor

    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):

    0$ strace test/test_bitcoin -t flatfile_tests 2>&1 | grep -A 5 fallocate | tail -6
1fallocate(3, 0, 0, 100)                 = -1 EOPNOTSUPP (Operation not supported)
2fcntl(3, F_GETFL)                       = 0x8002 (flags O_RDWR|O_LARGEFILE)
3fstat(3, {st_mode=S_IFREG|0644, st_size=0, ...}) = 0
4fstatfs(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
5pwrite64(3, "\0", 1, 99)                = 1
6close(3)                                = 0

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

    0$ strace test/test_bitcoin -t flatfile_tests 2>&1 | grep -A 1 fallocate | tail -2
1fallocate(3, 0, 0, 100)                 = 0
2close(3)                                = 0
  15. MarcoFalke removed the label Needs gitian build on Mar 23, 2019
  16. in src/util/system.cpp:1096 in 78062599d5 outdated 
    1094+    #if defined(__linux__)
1095     // Version using posix_fallocate
1096     off_t nEndPos = (off_t)offset + length;
1097-    posix_fallocate(fileno(file), 0, nEndPos);
1098-#else
1099+    if (!posix_fallocate(fileno(file), 0, nEndPos)) return;
    MarcoFalke commented at 7:51 pm on March 23, 2019:
    Might be cleaner to just compare 0 == posix_fallocate(...
    lucayepa commented at 8:40 pm on March 23, 2019:
    Yes, I agree. 0 == … is more expressive and less error prone.
  17. Handle the result of posix_fallocate system call 5d35ae3326
  18. lucayepa force-pushed on Mar 23, 2019
  19. MarcoFalke added the label Needs gitian build on Mar 24, 2019
  20. DrahtBot commented at 6:45 pm on March 25, 2019: member

    Gitian builds for commit 7b13c646457980f44599412f243694fa682a1abf (master):

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

  21. DrahtBot removed the label Needs gitian build on Mar 25, 2019
  22. MarcoFalke commented at 7:06 pm on March 25, 2019: member
    utACK 5d35ae3326624da3fe5dcb4047c9a7cec6665cab
  23. sipa commented at 11:02 pm on March 25, 2019: member
    utACK 5d35ae3326624da3fe5dcb4047c9a7cec6665cab, though the Yoda condition is an uncommon style in this project.
  24. luke-jr approved
  25. luke-jr commented at 0:22 am on April 18, 2019: member

    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

  26. hebasto commented at 6:45 am on May 2, 2019: member
    utACK 5d35ae3326624da3fe5dcb4047c9a7cec6665cab
  27. practicalswift commented at 7:24 am on May 2, 2019: contributor
    utACK 5d35ae3326624da3fe5dcb4047c9a7cec6665cab
  28. MarcoFalke merged this on May 2, 2019
  29. MarcoFalke closed this on May 2, 2019
  30. MarcoFalke referenced this in commit c4560a7dfe on May 2, 2019
  31. sidhujag referenced this in commit b5fe08a951 on May 2, 2019
  32. luke-jr referenced this in commit 1fd11c523a on Aug 23, 2019
  33. fanquake referenced this in commit 39a0452721 on Aug 24, 2019
  34. fanquake referenced this in commit c52dd120fd on Sep 23, 2019
  35. laanwj referenced this in commit 29d70264fb on Nov 25, 2019
  36. PastaPastaPasta referenced this in commit b504998b77 on Jun 27, 2021
  37. PastaPastaPasta referenced this in commit 7d35d71797 on Jun 28, 2021
  38. PastaPastaPasta referenced this in commit a3d5e832a4 on Jun 29, 2021
  39. PastaPastaPasta referenced this in commit c29a551053 on Jul 1, 2021
  40. PastaPastaPasta referenced this in commit 02534d43d5 on Jul 1, 2021
  41. PastaPastaPasta referenced this in commit 43a027d45e on Jul 8, 2021
  42. PastaPastaPasta referenced this in commit 0199129fdc on Jul 10, 2021
  43. DrahtBot locked this on Dec 16, 2021


lucayepa gmaxwell DrahtBot MarcoFalke sipa luke-jr hebasto practicalswift

Labels
Utils/log/libs

Milestone
0.19.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-18 18:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me