Add wrappers around posix_fadvise and use them in places we were calling posix_fadvise directly before. Also in virt-builder we were doing this (and ignoring the result): posix_fadvise (fd, 0, 0, POSIX_FADV_RANDOM|POSIX_FADV_DONTNEED); However the POSIX_FADV_* flags are _not_ bitmasks! In fact POSIX_FADV_RANDOM|POSIX_FADV_DONTNEED == POSIX_FADV_NOREUSE so we were giving a completely different hint from what we thought we were giving. --- builder/pxzcat-c.c | 5 +-- src/guestfs-internal-frontend.h | 5 +++ src/proto.c | 17 +------- src/utils.c | 87 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 95 insertions(+), 19 deletions(-) diff --git a/builder/pxzcat-c.c b/builder/pxzcat-c.c index 1f5ceeb..44722bc 100644 --- a/builder/pxzcat-c.c +++ b/builder/pxzcat-c.c @@ -214,10 +214,7 @@ pxzcat (value filenamev, value outputfilev, unsigned nr_threads) unix_error (err, (char *) "ftruncate", outputfilev); } -#if defined HAVE_POSIX_FADVISE - /* Tell the kernel we won't read the output file. */ - ignore_value (posix_fadvise (fd, 0, 0, POSIX_FADV_RANDOM|POSIX_FADV_DONTNEED)); -#endif + guestfs_int_fadvise_noreuse (fd); /* Iterate over blocks. */ iter_blocks (idx, nr_threads, filenamev, fd, outputfilev, ofd); diff --git a/src/guestfs-internal-frontend.h b/src/guestfs-internal-frontend.h index 7f10906..1305105 100644 --- a/src/guestfs-internal-frontend.h +++ b/src/guestfs-internal-frontend.h @@ -87,6 +87,11 @@ extern int guestfs_int_is_true (const char *str); extern const char *guestfs_int_ovmf_i386_firmware[]; extern const char *guestfs_int_ovmf_x86_64_firmware[]; extern const char *guestfs_int_aavmf_firmware[]; +extern void guestfs_int_fadvise_sequential (int fd); +extern void guestfs_int_fadvise_random (int fd); +extern void guestfs_int_fadvise_noreuse (int fd); +extern void guestfs_int_fadvise_dontneed (int fd); +extern void guestfs_int_fadvise_willneed (int fd); /* These functions are used internally by the CLEANUP_* macros. * Don't call them directly. diff --git a/src/proto.c b/src/proto.c index 722ac9f..cbf8680 100644 --- a/src/proto.c +++ b/src/proto.c @@ -333,19 +333,6 @@ guestfs_int_send (guestfs_h *g, int proc_nr, return serial; } -static void -fadvise_sequential (int fd) -{ -#if defined(HAVE_POSIX_FADVISE) && defined(POSIX_FADV_SEQUENTIAL) - /* Since the fd might be a non-file, eg. /dev/stdout, just ignore - * this when it fails. It's not clear from the man page, but the - * 'advice' parameter is NOT a bitmask. You can only pass one - * parameter with each call. - */ - ignore_value (posix_fadvise (fd, 0, 0, POSIX_FADV_SEQUENTIAL)); -#endif -} - static int send_file_chunk (guestfs_h *g, int cancel, const char *buf, size_t len); static int send_file_data (guestfs_h *g, const char *buf, size_t len); static int send_file_cancellation (guestfs_h *g); @@ -372,7 +359,7 @@ guestfs_int_send_file (guestfs_h *g, const char *filename) return -1; } - fadvise_sequential (fd); + guestfs_int_fadvise_sequential (fd); /* Send file in chunked encoding. */ while (!g->user_cancel) { @@ -829,7 +816,7 @@ guestfs_int_recv_file (guestfs_h *g, const char *filename) goto cancel; } - fadvise_sequential (fd); + guestfs_int_fadvise_sequential (fd); /* Receive the file in chunked encoding. */ while ((r = receive_file_data (g, &buf)) > 0) { diff --git a/src/utils.c b/src/utils.c index a0e8f98..bbb2d12 100644 --- a/src/utils.c +++ b/src/utils.c @@ -35,6 +35,8 @@ #include <sys/wait.h> #include <libintl.h> +#include "ignore-value.h" + /* NB: MUST NOT include "guestfs-internal.h" or gnulib headers. */ #include "guestfs.h" #include "guestfs-internal-frontend.h" @@ -385,3 +387,88 @@ guestfs_int_aavmf_firmware[] = { NULL }; + +/** + * Hint that we will read or write the file descriptor sequentially. + * + * It's OK to call this on a non-file since we ignore failure as it is + * only a hint. + */ +void +guestfs_int_fadvise_sequential (int fd) +{ +#if defined(HAVE_POSIX_FADVISE) && defined(POSIX_FADV_SEQUENTIAL) + /* It's not clear from the man page, but the 'advice' parameter is + * NOT a bitmask. You can only pass one parameter with each call. + */ + ignore_value (posix_fadvise (fd, 0, 0, POSIX_FADV_SEQUENTIAL)); +#endif +} + +/** + * Hint that we will read or write the file descriptor randomly. + * + * It's OK to call this on a non-file since we ignore failure as it is + * only a hint. + */ +void +guestfs_int_fadvise_random (int fd) +{ +#if defined(HAVE_POSIX_FADVISE) && defined(POSIX_FADV_RANDOM) + /* It's not clear from the man page, but the 'advice' parameter is + * NOT a bitmask. You can only pass one parameter with each call. + */ + ignore_value (posix_fadvise (fd, 0, 0, POSIX_FADV_RANDOM)); +#endif +} + +/** + * Hint that we will access the data only once. + * + * It's OK to call this on a non-file since we ignore failure as it is + * only a hint. + */ +void +guestfs_int_fadvise_noreuse (int fd) +{ +#if defined(HAVE_POSIX_FADVISE) && defined(POSIX_FADV_NOREUSE) + /* It's not clear from the man page, but the 'advice' parameter is + * NOT a bitmask. You can only pass one parameter with each call. + */ + ignore_value (posix_fadvise (fd, 0, 0, POSIX_FADV_NOREUSE)); +#endif +} + +/** + * Hint that we will not access the data in the near future. + * + * It's OK to call this on a non-file since we ignore failure as it is + * only a hint. + */ +void +guestfs_int_fadvise_dontneed (int fd) +{ +#if defined(HAVE_POSIX_FADVISE) && defined(POSIX_FADV_DONTNEED) + /* It's not clear from the man page, but the 'advice' parameter is + * NOT a bitmask. You can only pass one parameter with each call. + */ + ignore_value (posix_fadvise (fd, 0, 0, POSIX_FADV_DONTNEED)); +#endif +} + +/** + * Hint that we will access the data in the near future. + * + * It's OK to call this on a non-file since we ignore failure as it is + * only a hint. + */ +void +guestfs_int_fadvise_willneed (int fd) +{ +#if defined(HAVE_POSIX_FADVISE) && defined(POSIX_FADV_WILLNEED) + /* It's not clear from the man page, but the 'advice' parameter is + * NOT a bitmask. You can only pass one parameter with each call. + */ + ignore_value (posix_fadvise (fd, 0, 0, POSIX_FADV_WILLNEED)); +#endif +} -- 2.7.4

Setting POSIX_FADV_RANDOM makes no measurable difference, but at least it's the right thing to do. POSIX_FADV_WILLNEED makes no measurable difference either. Changing POSIX_FADV_NOREUSE on the input file descriptor to POSIX_FADV_DONTNEED slows things down by about 10%. --- builder/pxzcat-c.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/builder/pxzcat-c.c b/builder/pxzcat-c.c index 44722bc..0a1c368 100644 --- a/builder/pxzcat-c.c +++ b/builder/pxzcat-c.c @@ -215,6 +215,9 @@ pxzcat (value filenamev, value outputfilev, unsigned nr_threads) } guestfs_int_fadvise_noreuse (fd); + guestfs_int_fadvise_random (fd); + guestfs_int_fadvise_random (ofd); + guestfs_int_fadvise_willneed (ofd); /* Iterate over blocks. */ iter_blocks (idx, nr_threads, filenamev, fd, outputfilev, ofd); -- 2.7.4