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: DeveloperEcosystemEngineering@apple.com
| caddr_t *addr; | ||
| uint64_t chunk_size = MMAP_CHUNK_SIZE; | ||
|
|
||
| printf("fsync'ing file, then invalidating UBC.\n"); |
| @@ -26,9 +29,68 @@ int sync_file_range(int fd, uint64_t offset, uint64_t nbytes, | |||
| } | |||
| #endif | |||
|
|
|||
| #ifdef CONFIG_MAC_FADVISE_DISCARD | |||
| /** | |||
| #ifndef CONFIG_POSIX_FADVISE | ||
| int posix_fadvise(int fd, off_t offset, off_t len, int advice) | ||
| { | ||
| return 0; | ||
| } | ||
| #endif | ||
| #endif //CONFIG_MAC_FADVISE_DISCARD |
There was a problem hiding this comment.
#endif /* CONFIG_MAC_FADVISE_DISCARD */
| printf("fsync'ing file, then invalidating UBC.\n"); | ||
| if (fsync(fd) < 0) { | ||
| fprintf(stderr, "Cannot fsync file\n"); | ||
| exit(1); |
There was a problem hiding this comment.
Needs to return an error here, not exit. It should just return errno, but before you overwrite it by calling fprintf.
| if (addr == MAP_FAILED) { | ||
| fprintf(stderr, "Failed to mmap (%s), offset = %llu, size = %llu\n", | ||
| strerror(errno), offset, mmap_size); | ||
| return -1; |
There was a problem hiding this comment.
You should return errno for the failure cases, not -1. For this one and other spots in this function.
| } | ||
|
|
||
| int posix_fadvise(int fd, off_t offset, off_t len, int advice) | ||
| { |
There was a problem hiding this comment.
I'd probably write this function as:
if (advice == POSIX_FADV_DONTNEED)
return discard_pages(fd, offset, len);
return 0;
and avoid needing 'ret' at all.
There was a problem hiding this comment.
Well and the return 0 should be return EINVAL or something like that, should not be pretending success for cases it's not handling.
|
Thank you for the fast review @axboe, will integrate the requested changes! |
|
|
||
| if test "$targetos" = "Darwin"; then | ||
| output_sym "CONFIG_MAC_FADVISE_DISCARD" | ||
| fi |
There was a problem hiding this comment.
Do we really need another symbol for this? Couldn't you just use if defined(__APPLE__) as is done over in https://github.com/axboe/fio/blob/d2a4910bbefcb3ddecf2868a955ef27ec71286fc/os/os.h#L49C4-L49C25 ?
| if (advice == POSIX_FADV_DONTNEED) | ||
| ret = discard_pages(fd, offset, len); | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
Should this be abstracted into os-mac.h? Technically os-mac.h could advertise the define CONFIG_POSIX_FADVISE and fake up a posix_fadvise function...
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