Try to use posix_fadvise with CBufferedFile#12491
Try to use posix_fadvise with CBufferedFile#12491eklitzke wants to merge 1 commit intobitcoin:masterfrom
Conversation
This primarily affects blocks when bitcoin is launched with -reindex, as that causes the block files to be loaded as CBufferedFile objects one at a time as the reindex progresses.
| int fd = fileno(file); | ||
| if (fd == -1) | ||
| goto close; | ||
| end = lseek(fd, 0, SEEK_END); |
There was a problem hiding this comment.
off_t end = ... and remove line 840?
|
Note for future reviewers: the only usage of |
|
Concept ACK on advising, but we'll definitely want some data first. |
| posix_fadvise(fd, start, end - start, POSIX_FADV_WILLNEED); | ||
| posix_fadvise(fd, start, end - start, POSIX_FADV_SEQUENTIAL); | ||
| } | ||
| lseek(fd, start, SEEK_SET); |
There was a problem hiding this comment.
If SEEK_END didn't fail, we need to be sure this doesn't fail either.
There was a problem hiding this comment.
Agree this should be fixed. Presumably the seek shouldn't fail, but if it did, the result could be confusing.
| off_t end = lseek(fd, 0, SEEK_END); | ||
| if (end != -1) { | ||
| posix_fadvise(fd, start, end - start, POSIX_FADV_WILLNEED); | ||
| posix_fadvise(fd, start, end - start, POSIX_FADV_SEQUENTIAL); |
There was a problem hiding this comment.
I think POSIX_FADV_NOREUSE may be useful here also (although my manpage says it is currently a no-op).
| posix_fadvise(fd, 0, end, POSIX_FADV_DONTNEED); | ||
| } | ||
| #endif | ||
| close: |
There was a problem hiding this comment.
Unused labels might be a warning in some compiler versions; move inside #if block?
| posix_fadvise(fd, start, end - start, POSIX_FADV_WILLNEED); | ||
| posix_fadvise(fd, start, end - start, POSIX_FADV_SEQUENTIAL); | ||
| } | ||
| lseek(fd, start, SEEK_SET); |
There was a problem hiding this comment.
Agree this should be fixed. Presumably the seek shouldn't fail, but if it did, the result could be confusing.
| ftruncate(fileno(file), fst.fst_length); | ||
| #elif defined(__linux__) | ||
| // Version using posix_fallocate | ||
| #elif _POSIX_C_SOURCE >= 200112L |
There was a problem hiding this comment.
I think @laanwj's comment about _POSIX_C_SOURCE at #12495 (comment) applies here as well, so this check is not right.
| bool LockDirectory(const fs::path& directory, const std::string lockfile_name, bool probe_only=false); | ||
|
|
||
| //! Return the original FILE* unchanged. On POSIX systems that support it, | ||
| //! also advise the kernel that the file will be accessed sequentially. |
There was a problem hiding this comment.
Might want to mention also that this advises WILLNEED in addition to just SEQUENTIAL.
|
Why close? |
I'm also curious. |
|
Also curious why this was closed. |
7 participants
I've been working in another branch on speeding up IBD/reindexing, and have been looking at how to improve page cache hits on Linux. This is a minor (but safe) improvement I've discovered during that work.
The change here uses
posix_fadvise()on systems that support it (Linux, BSD, macOS) such that:There's also a minor/pedantic fix to related to an existing
posix_fallocate()call to make sure it doesn't redefine_POSIX_C_SOURCE(which could potentially affect the behavior of header files included after the redefinition).I don't expect this to be a huge performance win, but it's safe and well contained. The new util functions are potentially useful elsewhere. The speedup here is potentially bigger by using these methods selectively on CAutoFile, as that's how block files are loaded during the chainstate reindexing phase. I'm working on that as well, but that's more delicate since CAutoFile is used all over the place (compared to CBufferedFile which is just used when reindexing block files).