Try to use posix_fadvise with CBufferedFile

eklitzke commented at 5:53 pm on February 20, 2018: contributor

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:

When opening a block file for processing, it advises the kernel that the block will be read immediately, and that the read will be sequential.
When closing a block file, it advises the kernel that it can discard entries in the page cache associated with that block file, which potentially leaves a bit more page cache room for things like chainstate files when chainstate reindexing happens.

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

Try to use posix_fadvise with CBufferedFile

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.

5259c72a76

in src/util.cpp:844 in 5259c72a76

839+    if (file != nullptr) {
840+        off_t end;
841+        int fd = fileno(file);
842+        if (fd == -1)
843+            goto close;
844+        end = lseek(fd, 0, SEEK_END);

jamesob commented at 9:02 pm on February 20, 2018:

off_t end = ... and remove line 840?

jamesob approved

jamesob commented at 9:03 pm on February 20, 2018: member

utACK https://github.com/bitcoin/bitcoin/pull/12491/commits/5259c72a769eba32aabb4e61d77e9f6ef4ce9a0d

Cool! Seems like a good idea.

jamesob commented at 9:06 pm on February 20, 2018: member

Note for future reviewers: the only usage of CBufferedFile I can find is in LoadExternalBLockFile.

fanquake added the label Resource usage on Feb 21, 2018

theuni commented at 3:44 pm on February 21, 2018: member

Concept ACK on advising, but we’ll definitely want some data first.

in src/util.cpp:830 in 5259c72a76

825+    off_t end = lseek(fd, 0, SEEK_END);
826+    if (end != -1) {
827+        posix_fadvise(fd, start, end - start, POSIX_FADV_WILLNEED);
828+        posix_fadvise(fd, start, end - start, POSIX_FADV_SEQUENTIAL);
829+    }
830+    lseek(fd, start, SEEK_SET);

luke-jr commented at 3:51 pm on February 27, 2018:

If SEEK_END didn’t fail, we need to be sure this doesn’t fail either.

ryanofsky commented at 10:37 pm on March 5, 2018:

Agree this should be fixed. Presumably the seek shouldn’t fail, but if it did, the result could be confusing.

in src/util.cpp:828 in 5259c72a76

823+    if (start == -1)
824+        return file;
825+    off_t end = lseek(fd, 0, SEEK_END);
826+    if (end != -1) {
827+        posix_fadvise(fd, start, end - start, POSIX_FADV_WILLNEED);
828+        posix_fadvise(fd, start, end - start, POSIX_FADV_SEQUENTIAL);

luke-jr commented at 3:53 pm on February 27, 2018:

I think POSIX_FADV_NOREUSE may be useful here also (although my manpage says it is currently a no-op).

in src/util.cpp:850 in 5259c72a76

845+        if (end == -1)
846+            goto close;
847+        posix_fadvise(fd, 0, end, POSIX_FADV_DONTNEED);
848+    }
849+#endif
850+ close:

luke-jr commented at 3:54 pm on February 27, 2018:

Unused labels might be a warning in some compiler versions; move inside #if block?

in src/util.cpp:791 in 5259c72a76

787@@ -798,8 +788,9 @@ void AllocateFileRange(FILE *file, unsigned int offset, unsigned int length) {
788         fcntl(fileno(file), F_PREALLOCATE, &fst);
789     }
790     ftruncate(fileno(file), fst.fst_length);
791-#elif defined(__linux__)
792-    // Version using posix_fallocate
793+#elif _POSIX_C_SOURCE >= 200112L

ryanofsky commented at 10:46 pm on March 5, 2018:

I think @laanwj’s comment about _POSIX_C_SOURCE at #12495 (review) applies here as well, so this check is not right.

in src/util.h:179 in 5259c72a76

174@@ -175,6 +175,15 @@ void AllocateFileRange(FILE *file, unsigned int offset, unsigned int length);
175 bool RenameOver(fs::path src, fs::path dest);
176 bool LockDirectory(const fs::path& directory, const std::string lockfile_name, bool probe_only=false);
177 
178+//! Return the original FILE* unchanged. On POSIX systems that support it,
179+//! also advise the kernel that the file will be accessed sequentially.

ryanofsky commented at 10:56 pm on March 5, 2018:

Might want to mention also that this advises WILLNEED in addition to just SEQUENTIAL.

eklitzke closed this on Mar 10, 2018

laanwj commented at 4:27 pm on March 10, 2018: member

Why close?

ryanofsky commented at 7:07 pm on March 15, 2018: member

Why close?

I’m also curious.

jamesob commented at 1:10 pm on March 27, 2018: member

Also curious why this was closed.

MarcoFalke locked this on Sep 8, 2021

Try to use posix_fadvise with CBufferedFile #12491