The way we used posix_fadvise was wrong, and yet right! Rich.
Richard W.M. Jones
2016-Apr-14 13:54 UTC
[Libguestfs] [PATCH 1/2] utils, builder: Add wrappers for posix_fadvise.
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
Richard W.M. Jones
2016-Apr-14 13:54 UTC
[Libguestfs] [PATCH 2/2] builder: pxzcat: Add further posix_fadvise hints.
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
Pino Toscano
2016-Apr-14 14:14 UTC
Re: [Libguestfs] [PATCH 1/2] utils, builder: Add wrappers for posix_fadvise.
On Thursday 14 April 2016 14:54:18 Richard W.M. Jones wrote:> 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. > ---Mostly ok, just one note below:> +extern void guestfs_int_fadvise_dontneed (int fd);Unless I'm mistaken, _dontneed is not used yet, so IMHO it would be better to not add it for now; it can always be added when needed. Thanks, -- Pino Toscano