Clean unused entries in the qcow2 L2/refcount cache
v4 cover Clean unused entries in the qcow2 L2/refcount cache
v4:
- Revert the 'cache-clean-interval' change. This should probably go into a new BlockDeviceInfoSpecific struct (along with other settings), but is out of the scope for this series.
v3: https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg05473.html
- Add 'cache-clean-interval' field to ImageInfoSpecificQCow2.
v2: https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg05316.html
- Clarify that the block-commit mentioned in the first patch refers to the HMP commit command.
- Check the value of cache_clean_interval and cast it accordingly to prevent it from overflowing.
v1: https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg03510.html
Regards,
Berto
Alberto Garcia (3):
qcow2: mark the memory as no longer needed after qcow2_cache_empty()
qcow2: add option to clean unused cache entries after some time
qcow2: reorder fields in Qcow2CachedTable to reduce padding
block/qcow2-cache.c | 57 +++++++++++++++++++++++++++++++++++++++++++++- block/qcow2.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++ block/qcow2.h | 4 ++++ qapi/block-core.json | 6 ++++- 4 files changed, 129 insertions(+), 2 deletions(-)
Signature · 1 line
2.1.4
PATCH v4 1/3 qcow2: mark the memory as no longer needed after qcow2_cache_empty()
After having emptied the cache, the data in the cache tables is no longer useful, so we can tell the kernel that we are done with it. In Linux this frees the resources associated with it.
The effect of this can be seen in the HMP commit operation: it moves data from the top to the base image (and fills both caches), then it empties the top image. At this point the data in that cache is no longer needed so it's just wasting memory.
--- block/qcow2-cache.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
block/qcow2-cache.c +20 -0
@@ -22,8 +22,10 @@* THE SOFTWARE. */ +#include <sys/mman.h> #include "block/block_int.h" #include "qemu-common.h" +#include "qemu/osdep.h" #include "qcow2.h" #include "trace.h"
@@ -60,6 +62,22 @@ static inline int qcow2_cache_get_table_idx(BlockDriverState *bs,return idx; } +static void qcow2_cache_table_release (BlockDriverState *bs,Qcow2Cache *c, +int i,int num_tables) +{ +#if QEMU_MADV_DONTNEED !=QEMU_MADV_INVALID +BDRVQcowState *s = bs->opaque; +void *t =qcow2_cache_get_table_addr (bs, c, i); +long align =sysconf (_SC_PAGESIZE); +size_t mem_size = (size_t ) s->cluster_size * num_tables; +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 ); + } +#endif +} +Qcow2Cache *qcow2_cache_create (BlockDriverState *bs,int num_tables) {BDRVQcowState *s = bs->opaque;
@@ -237,6 +255,8 @@ int qcow2_cache_empty(BlockDriverState *bs, Qcow2Cache *c) c->entries[i].lru_counter =0 ; } +qcow2_cache_table_release (bs, c,0 , c->size); + c->lru_counter =0 ;return 0 ;
Signature · 1 line
2.1.4
PATCH v4 3/3 qcow2: reorder fields in Qcow2CachedTable to reduce padding
In reply to Alberto GarciaChanging the current ordering saves 8 bytes per cache entry in x86_64.
--- block/qcow2-cache.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
block/qcow2-cache.c +1 -1
@@ -31,9 +31,9 @@typedef struct Qcow2CachedTable {int64_t offset; -bool dirty;uint64_t lru_counter;int ref; +bool dirty; }Qcow2CachedTable ;struct Qcow2Cache {
Signature · 1 line
2.1.4
PATCH v4 2/3 qcow2: add option to clean unused cache entries after some time
In reply to Alberto GarciaThis adds a new 'cache-clean-interval' option that cleans all qcow2 cache entries that haven't been used in a certain interval, given in seconds.
This allows setting a large L2 cache size so it can handle scenarios with lots of I/O and at the same time use little memory during periods of inactivity.
This feature currently relies on MADV_DONTNEED to free that memory, so it is not useful in systems that don't follow that behavior.
--- block/qcow2-cache.c | 35 ++++++++++++++++++++++++++++ block/qcow2.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++ block/qcow2.h | 4 ++++ qapi/block-core.json | 6 ++++- 4 files changed, 108 insertions(+), 1 deletion(-)
block/qcow2-cache.c +35 -0
@@ -43,6 +43,7 @@ struct Qcow2Cache {bool depends_on_flush;void *table_array;uint64_t lru_counter; +uint64_t cache_clean_lru_counter; }; static inline void *qcow2_cache_get_table_addr(BlockDriverState *bs,
@@ -78,6 +79,40 @@ static void qcow2_cache_table_release(BlockDriverState *bs, Qcow2Cache *c,#endif } +static inline bool can_clean_entry (Qcow2Cache *c,int i) +{ +Qcow2CachedTable *t = &c->entries[i]; +return t->ref ==0 && !t->dirty && t->offset !=0 && + t->lru_counter <= c->cache_clean_lru_counter; +} + +void qcow2_cache_clean_unused (BlockDriverState *bs,Qcow2Cache *c) +{ +int i =0 ; +while (i < c->size) { +int to_clean =0 ; + +/* Skip the entries that we don't need to clean */ +while (i < c->size && !can_clean_entry (c, i)) { + i++; + } + +/* And count how many we can clean in a row */ +while (i < c->size &&can_clean_entry (c, i)) { + c->entries[i].offset =0 ; + c->entries[i].lru_counter =0 ; + i++; + to_clean++; + } + +if (to_clean >0 ) { +qcow2_cache_table_release (bs, c, i - to_clean, to_clean); + } + } + + c->cache_clean_lru_counter = c->lru_counter; +} +Qcow2Cache *qcow2_cache_create (BlockDriverState *bs,int num_tables) {BDRVQcowState *s = bs->opaque;
block/qcow2.c +64 -0
106 lines not on this page · raw message
block/qcow2.h +4 -0
@@ -93,6 +93,7 @@#define QCOW2_OPT_CACHE_SIZE "cache-size"#define QCOW2_OPT_L2_CACHE_SIZE "l2-cache-size"#define QCOW2_OPT_REFCOUNT_CACHE_SIZE "refcount-cache-size" +#define QCOW2_OPT_CACHE_CLEAN_INTERVAL "cache-clean-interval" typedef struct QCowHeader { uint32_t magic;
@@ -236,6 +237,8 @@ typedef struct BDRVQcowState {Qcow2Cache * l2_table_cache;Qcow2Cache * refcount_block_cache; +QEMUTimer *cache_clean_timer; +unsigned cache_clean_interval;uint8_t *cluster_cache;uint8_t *cluster_data;
@@ -581,6 +584,7 @@ int qcow2_cache_set_dependency(BlockDriverState *bs, Qcow2Cache *c, Qcow2Cache *dependency);void qcow2_cache_depends_on_flush (Qcow2Cache *c); +void qcow2_cache_clean_unused (BlockDriverState *bs,Qcow2Cache *c);int qcow2_cache_empty (BlockDriverState *bs,Qcow2Cache *c); int qcow2_cache_get(BlockDriverState *bs, Qcow2Cache *c, uint64_t offset,
qapi/block-core.json +5 -1
@@ -1538,6 +1538,9 @@ # @refcount-cache-size: #optional the maximum size of the refcount block cache # in bytes (since 2.2) # +# @cache-clean-interval: #optional clean unused entries in the L2 and refcount +# caches. The interval is in seconds (since 2.4) +# # Since: 1.7 ## { 'struct': 'BlockdevOptionsQcow2',
@@ -1549,7 +1552,8 @@ '*overlap-check': 'Qcow2OverlapChecks', '*cache-size': 'int', '*l2-cache-size': 'int', - '*refcount-cache-size': 'int' } } + '*refcount-cache-size': 'int', + '*cache-clean-interval': 'int' } } ##
Signature · 1 line
2.1.4
Re: [Qemu-devel] [PATCH 1/3] qcow2: mark the memory as no longer needed after qcow2_cache_empty()
In reply to Alberto GarciaAm 29.05.2015 um 11:24 hat Alberto Garcia geschrieben:
Quoted 20 lines
After having emptied the cache, the data in the cache tables is no longer useful, so we can tell the kernel that we are done with it. In Linux this frees the resources associated with it.
The effect of this can be seen in the HMP commit operation: it moves data from the top to the base image (and fills both caches), then it empties the top image. At this point the data in that cache is no longer needed so it's just wasting memory.
Signed-off-by Alberto Garcia <[email protected]>Reviewed-by Max Reitz <[email protected]>--- block/qcow2-cache.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)block/qcow2-cache.c +0
-0
block/qcow2-cache.c line 30
Quoted 9 lines
block/qcow2-cache.c +2
-0@@ -22,8 +22,10 @@* THE SOFTWARE. */ +#include <sys/mman.h> #include "block/block_int.h" #include "qemu-common.h" +#include "qemu/osdep.h" #include "qcow2.h" #include "trace.h"
This breaks the mingw build:
/mnt/qemu/block/qcow2-cache.c:25:22: fatal error: sys/mman.h: No such file or directory
#include <sys/mman.h>
block/qcow2-cache.c static inline int qcow2_cache_get_table_idx(BlockDriverState *bs, · line 71
Quoted 11 lines
block/qcow2-cache.c +7
-0@@ -60,6 +62,22 @@ static inline int qcow2_cache_get_table_idx(BlockDriverState *bs,return idx; } +static void qcow2_cache_table_release (BlockDriverState *bs,Qcow2Cache *c, +int i,int num_tables) +{ +#if QEMU_MADV_DONTNEED !=QEMU_MADV_INVALID +BDRVQcowState *s = bs->opaque; +void *t =qcow2_cache_get_table_addr (bs, c, i); +long align =sysconf (_SC_PAGESIZE);
It seems that getpagesize() is usually used in qemu.
block/qcow2-cache.c static inline int qcow2_cache_get_table_idx(BlockDriverState *bs, · line 74
Quoted 3 lines
block/qcow2-cache.c +3
-0+size_t mem_size = (size_t ) s->cluster_size * num_tables; +size_t offset =QEMU_ALIGN_UP ((uintptr_t ) t, align) - (uintptr_t ) t; +size_t length =QEMU_ALIGN_DOWN (mem_size - offset, align);
Instead of all the aligning here, shouldn't we just make sure that the tables are created with the right alignment?
block/qcow2-cache.c static inline int qcow2_cache_get_table_idx(BlockDriverState *bs, · line 79
Quoted 5 lines
block/qcow2-cache.c +5
-0+if (length >0 ) { +qemu_madvise ((uint8_t *) t + offset, length,QEMU_MADV_DONTNEED ); + } +#endif +}
Kevin
Re: [Qemu-devel] [PATCH 1/3] qcow2: mark the memory as no longer needed after qcow2_cache_empty()
Quoted 11 lines · Kevin Wolf
block/qcow2-cache.c line 30
Quoted 6 lines
block/qcow2-cache.c +2
-0+#include <sys/mman.h> #include "block/block_int.h" #include "qemu-common.h" +#include "qemu/osdep.h" #include "qcow2.h" #include "trace.h" This breaks the mingw build:
/mnt/qemu/block/qcow2-cache.c:25:22: fatal error:sys/mman.h: No such file or directory
#include <sys/mman.h>
Ok, I can use #if defined(CONFIG_MADVISE) || defined(CONFIG_POSIX_MADVISE) as it's done in util/osdep.c for the same header.
Quoted 6 lines
block/qcow2-cache.c static inline int qcow2_cache_get_table_idx(BlockDriverState *bs, · line 71
Quoted 4 lines
block/qcow2-cache.c +4
-0+#if QEMU_MADV_DONTNEED !=QEMU_MADV_INVALID +BDRVQcowState *s = bs->opaque; +void *t =qcow2_cache_get_table_addr (bs, c, i); +long align =sysconf (_SC_PAGESIZE);It seems that
getpagesize()is usually used in qemu.
The getpagesize() manual page actually recommends using sysconf() for portability reasons. But other than that I don't have a problem with getpagesize() if it's the preferred choice in QEMU.
Quoted 6 lines
block/qcow2-cache.c static inline int qcow2_cache_get_table_idx(BlockDriverState *bs, · line 74
Quoted 3 lines
block/qcow2-cache.c +3
-0+size_t mem_size = (size_t ) s->cluster_size * num_tables; +size_t offset =QEMU_ALIGN_UP ((uintptr_t ) t, align) - (uintptr_t ) t; +size_t length =QEMU_ALIGN_DOWN (mem_size - offset, align);Instead of all the aligning here, shouldn't we just make sure that the tables are created with the right alignment?
The tables should have the right alignment and their size should be a multiple of the page size. The latter condition is not necessarily true (a cluster can be smaller than one page) so I'd keep that code in any case.
Berto
Re: [Qemu-devel] [PATCH 1/3] qcow2: mark the memory as no longer needed after qcow2_cache_empty()
Am 02.06.2015 um 12:50 hat Alberto Garcia geschrieben:
Quoted 22 lines · Kevin Wolf
Quoted 10 lines
block/qcow2-cache.c line 30
Quoted 7 lines
block/qcow2-cache.c +2
-0+#include <sys/mman.h> #include "block/block_int.h" #include "qemu-common.h" +#include "qemu/osdep.h" #include "qcow2.h" #include "trace.h" This breaks the mingw build:
Quoted 2 lines
/mnt/qemu/block/qcow2-cache.c:25:22: fatal error:sys/mman.h: No such file or directory
#include <sys/mman.h>Ok, I can use #if
defined(CONFIG_MADVISE)||defined(CONFIG_POSIX_MADVISE)as it's done inutil/osdep.cfor the same header.Quoted 5 lines
Quoted 4 lines
block/qcow2-cache.c +4
-0+#if QEMU_MADV_DONTNEED !=QEMU_MADV_INVALID +BDRVQcowState *s = bs->opaque; +void *t =qcow2_cache_get_table_addr (bs, c, i); +long align =sysconf (_SC_PAGESIZE);
Quoted 5 lines
Quoted 1 line
It seems that
getpagesize()is usually used in qemu.The
getpagesize()manual page actually recommends usingsysconf()for portability reasons. But other than that I don't have a problem withgetpagesize()if it's the preferred choice in QEMU.
There is a Windows implementation for getpagesize() in qemu, so I guess it's actually more portable in our context.
Now, as you'll #ifdef this code out for Windows, that probably doesn't matter, so it's just about consistency.
Quoted 11 lines
Quoted 6 lines
block/qcow2-cache.c static inline int qcow2_cache_get_table_idx(BlockDriverState *bs, · line 74
Quoted 3 lines
block/qcow2-cache.c +3
-0+size_t mem_size = (size_t ) s->cluster_size * num_tables; +size_t offset =QEMU_ALIGN_UP ((uintptr_t ) t, align) - (uintptr_t ) t; +size_t length =QEMU_ALIGN_DOWN (mem_size - offset, align);Instead of all the aligning here, shouldn't we just make sure that the tables are created with the right alignment?
The tables should have the right alignment and their size should be a multiple of the page size. The latter condition is not necessarily true (a cluster can be smaller than one page) so I'd keep that code in any case.
We could adjust the size to MAX(size, pagesize), but perhaps that wastes a bit too much memory indeed.
Kevin
Re: [Qemu-devel] [PATCH 2/3] qcow2: add option to clean unused cache entries after some time
In reply to Alberto GarciaAm 29.05.2015 um 11:24 hat Alberto Garcia geschrieben:
Quoted 19 lines
This adds a new 'cache-clean-interval' option that cleans all qcow2 cache entries that haven't been used in a certain interval, given in seconds.
This allows setting a large L2 cache size so it can handle scenarios with lots of I/O and at the same time use little memory during periods of inactivity.
This feature currently relies on
MADV_DONTNEEDto free that memory, so it is not useful in systems that don't follow that behavior.Signed-off-by Alberto Garcia <[email protected]>Reviewed-by Max Reitz <[email protected]>qapi/block-core.json +0
-0
qapi/block-core.json line 1542
Quoted 5 lines
qapi/block-core.json +2
-0@@ -1538,6 +1538,9 @@ # @refcount-cache-size: #optional the maximum size of the refcount block cache # in bytes (since 2.2) # +# @cache-clean-interval: #optional clean unused entries in the L2 and refcount +# caches. The interval is in seconds (since 2.4)
Should add that 0 means disabling the cleaning, and 0 is the default.
Kevin
Re: [Qemu-devel] [PATCH 3/3] qcow2: reorder fields in Qcow2CachedTable to reduce padding
In reply to Alberto GarciaAm 29.05.2015 um 11:24 hat Alberto Garcia geschrieben:
Quoted 5 lines
Changing the current ordering saves 8 bytes per cache entry in
x86_64.Signed-off-by Alberto Garcia <[email protected]>Reviewed-by Max Reitz <[email protected]>Reviewed-by Eric Blake <[email protected]>
Re: [Qemu-devel] [PATCH 1/3] qcow2: mark the memory as no longer needed after qcow2_cache_empty()
In reply to Kevin WolfQuoted 8 lines · Kevin Wolf
Quoted 5 lines
Quoted 1 line
It seems that
getpagesize()is usually used in qemu.The
getpagesize()manual page actually recommends usingsysconf()for portability reasons. But other than that I don't have a problem withgetpagesize()if it's the preferred choice in QEMU.There is a Windows implementation for
getpagesize()in qemu, so I guess it's actually more portable in our context.
Ok, I'll change that.
Quoted 2 lines
Now, as you'll #ifdef this code out for Windows, that probably doesn't matter, so it's just about consistency.
There's actually no need to #ifdef that code out, I only do it because I know in advance that it's going to be a no-op, but qemu_madvise() itself doesn't break or anything if the operation is not supported.
Berto