Allow 'cache-clean-interval' in Linux only

Hi all,

The cache-clean-interval setting of qcow2 frees the memory of the L2 cache tables that haven't been used after a certain interval of time.

QEMU uses madvise() with MADV_DONTNEED for this. After that call, the data in the specified cache tables is discarded by the kernel. The problem with this behavior is that it is Linux-specific. madvise() itself is not a standard system call and while other implementations (e.g. FreeBSD) also have MADV_DONTNEED, they don't share the same semantics.

POSIX defines posix_madvise(), which has POSIX_MADV_DONTNEED, and that's what QEMU uses in systems that don't implement madvise(). However POSIX_MADV_DONTNEED also has different semantics and cannot be used for our purposes. As a matter of fact, in glibc it is a no-op:

https://github.molgen.mpg.de/git-mirror/glibc/blob/glibc-2.23/sysdeps/unix/sysv/linux/posix_madvise.c

So while this all is mentioned in the QEMU documentation, there's nothing preventing users of other systems from trying to use this feature. In non-Linux systems it is worse than a no-op: it invalidates perfectly valid cache tables for no reason without freeing their memory.

This series makes Linux a hard requirement for cache-clean-interval and prints an error message in other systems.

Regards,

Berto

Alberto Garcia (4):
qcow2: Make qcow2_cache_table_release() work only in Linux
qcow2: Allow 'cache-clean-interval' in Linux only
qcow2: Remove stale comment
docs: Specify that cache-clean-interval is only supported in Linux

 block/qcow2-cache.c  | 6 +++---
 block/qcow2.c        | 8 ++++++++
 docs/qcow2-cache.txt | 5 +++--
 3 files changed, 14 insertions(+), 5 deletions(-)

2.10.2

We are using QEMU_MADV_DONTNEED to discard the memory of individual L2 cache tables. The problem with this is that those semantics are specific to the Linux madvise() system call. Other implementations of madvise() (including the very Linux implementation of posix_madvise()) don't do that, so we cannot use them for the same purpose.

This patch makes the code Linux-specific and uses madvise() directly since there's no point in going through qemu_madvise() for this.

---
 block/qcow2-cache.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

index 6eaefed..ab8ee2d 100644
@@ -66,7 +66,8 @@ static inline int qcow2_cache_get_table_idx(BlockDriverState *bs,
 static void qcow2_cache_table_release(BlockDriverState *bs, Qcow2Cache *c,
                                       int i, int num_tables)
 {
-#if QEMU_MADV_DONTNEED != QEMU_MADV_INVALID
+/* Using MADV_DONTNEED to discard memory is a Linux-specific feature */
+#ifdef CONFIG_LINUX
     BDRVQcow2State *s = bs->opaque;
     void *t = qcow2_cache_get_table_addr(bs, c, i);
     int align = getpagesize();

@@ -74,7 +75,7 @@ static void qcow2_cache_table_release(BlockDriverState *bs, Qcow2Cache *c,
     size_t offset = QEMU_ALIGN_UP((uintptr_t) t, align) - (uintptr_t) t;
     size_t length = QEMU_ALIGN_DOWN(mem_size - offset, align);
     if (length > 0) {
-        qemu_madvise((uint8_t *) t + offset, length, QEMU_MADV_DONTNEED);
+        madvise((uint8_t *) t + offset, length, MADV_DONTNEED);
     }
 #endif
 }

2.10.2

The cache-clean-interval option of qcow2 only works on Linux. However we allow setting it in other systems regardless of whether it works or not.

In those systems this option is not simply a no-op: it actually invalidates perfectly valid cache tables for no good reason without freeing their memory.

This patch forbids using that option in non-Linux systems.

---
 block/qcow2.c | 8 ++++++++
 1 file changed, 8 insertions(+)

index 7cfcd84..ed9e0f3 100644
@@ -668,6 +668,14 @@ static int qcow2_update_options_prepare(BlockDriverState *bs,
     r->cache_clean_interval =
         qemu_opt_get_number(opts, QCOW2_OPT_CACHE_CLEAN_INTERVAL,
                             s->cache_clean_interval);
+#ifndef CONFIG_LINUX
+    if (r->cache_clean_interval != 0) {
+        error_setg(errp, QCOW2_OPT_CACHE_CLEAN_INTERVAL
+                   " not supported on this host");
+        ret = -EINVAL;
+        goto fail;
+    }
+#endif
     if (r->cache_clean_interval > UINT_MAX) {
         error_setg(errp, "Cache clean interval too big");
         ret = -EINVAL;

2.10.2

We haven't been using CONFIG_MADVISE since 02d0e095031b7fda77de8b

---
 block/qcow2-cache.c | 1 -
 1 file changed, 1 deletion(-)

index ab8ee2d..1d25147 100644
@@ -22,7 +22,6 @@
  * THE SOFTWARE.
  */
 
-/* Needed for CONFIG_MADVISE */
 #include "qemu/osdep.h"
 #include "block/block_int.h"
 #include "qemu-common.h"

2.10.2

Make it clear that having Linux is a hard requirement for this feature.

---
 docs/qcow2-cache.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

index 5bb0607..1fdd6f9 100644
@@ -160,5 +160,6 @@ If unset, the default value for this parameter is 0 and it disables
 this feature.
 
 Note that this functionality currently relies on the MADV_DONTNEED
-argument for madvise() to actually free the memory, so it is not
-useful in systems that don't follow that behavior.
+argument for madvise() to actually free the memory. This is a
+Linux-specific feature, so cache-clean-interval is not supported in
+other systems.

2.10.2

Am 25.11.2016 um 12:27 hat Alberto Garcia geschrieben:

Thanks, applied to the block branch.

Kevin