Improve flushing pagecache on darwin #1883
Conversation
The current behavior of flushing the pagecache via posix_fadvise(POSIX_FADV_DONTNEED) is not a valid operation on macOS. This can result adverse outcomes when measuring performance. The change will instead flush via mmap and msync, which better reflects the desired behavior on macOS Signed-off-by: [email protected]
|
Thank you for the fast review @axboe, will integrate the requested changes! |
Incorporating the PR discussion - Remove debug print - Make all comments a single * - Use /* */ instead of // - Return instead of exit - Ensure errno is populated - Direct return instead of using a placeholder - Move to __APPLE__ - Migrate code to os/os-mac.h since it is about macOS
|
The latest commit should address all the open feedback, thank you @sitsofe much and @axboe for the feedback. Let us know if there is anything else necessary or desired!
|
|
You should fold it into a single patch and force push it again, it doesn't make a lot of sense to have a broken state in the upstream tree. |
Not a problem, taking a peek at that failure real quick. Once everyone is satisfied, will squash and force push. |
There was a problem hiding this comment.
Just out of interest do you see a difference running the following job with and without your change?
./fio --filename=fio.tmp --stonewall --thread --size=1G --filename=fio.tmp --bs=4k \
--name=create --rw=write \
--name=cached --rw=read --loops=2 --invalidate=0 \
--name=invalidated --rw=read --loops=2 --invalidate=1
| @@ -117,3 +122,60 @@ static inline bool os_cpu_has(cpu_features feature) | |||
| } | |||
|
|
|||
| #endif | |||
|
|
|||
| #if defined (__APPLE__) | |||
There was a problem hiding this comment.
You won't need this guard here as os-mac.h will only be included on macOS (see
Line 49 in 58818df
|
|
||
| if (fsync(fd) < 0) { | ||
| int __err = errno; | ||
| fprintf(stderr, "Cannot fsync file\n"); |
There was a problem hiding this comment.
I think we would be better off using log_err(). Doing a quick grep there isn't a clear standard for how log_err's string should start but perhaps a prefix of "discard_pages: " would make it clear where the error bubbled up from?
| @@ -26,9 +26,19 @@ int sync_file_range(int fd, uint64_t offset, uint64_t nbytes, | |||
| } | |||
| #endif | |||
|
|
|||
| #if defined (__APPLE__) | |||
There was a problem hiding this comment.
You might be able to move this into os/os-mac.h by doing #define CONFIG_POSIX_FADVISE over there and providing an implementation in that file...
The current behavior of flushing the pagecache via
posix_fadvise(POSIX_FADV_DONTNEED)is not a valid operation on macOS.This can result in adverse outcomes when measuring performance. The change will instead flush via
mmapandmsync, which better reflects the desired behavior on macOS