| Message ID | 20180808071051.30628-3-lbloch@janustech.com |
|---|---|
| State | New |
| Headers | show |
| Series | qcow2: Make the L2 cache cover the whole image by default | expand |
On Wed 08 Aug 2018 09:10:48 AM CEST, Leonid Bloch wrote: > Sufficient L2 cache can noticeably improve the performance when using > large images with frequent I/O. The memory overhead is not significant > in most cases, as the cache size is only 1 MB for each 8 GB of virtual > image size (with the default cluster size of 64 KB). For cases with very > large images and/or small cluster sizes, there is an upper limit on the > default L2 cache: 32 MB. I find this description a bit confusing. First of all, because it's not true that the default will cover the whole image: we're just increasing it, but any image > 256GB is going to need more than 32MB (with 64KB clusters, that is). Second, because it's not clear what happens if you increase that maximum. Do you have to calculate the new value? Can you make it larger than what you actually need? The way I see it: there are two simple changes from the user's point of view (they can even be two separate patches). 1) The default l2-cache-size is now 32MB. DEFAULT_L2_CACHE_CLUSTERS is useless now and disappears. 2) QEMU will only use as much memory from l2-cache-size as it can use to cover the whole image. Increasing the value of l2-cache-size will not use any additional memory, so it's safe to do. Berto
On 08/08/2018 03:39 PM, Alberto Garcia wrote: > On Wed 08 Aug 2018 09:10:48 AM CEST, Leonid Bloch wrote: >> Sufficient L2 cache can noticeably improve the performance when using >> large images with frequent I/O. The memory overhead is not significant >> in most cases, as the cache size is only 1 MB for each 8 GB of virtual >> image size (with the default cluster size of 64 KB). For cases with very >> large images and/or small cluster sizes, there is an upper limit on the >> default L2 cache: 32 MB. > > I find this description a bit confusing. > > First of all, because it's not true that the default will cover the > whole image: we're just increasing it, but any image > 256GB is going to > need more than 32MB (with 64KB clusters, that is). How about the following: qcow2: Make the default L2 cache try to cover the entire image > > Second, because it's not clear what happens if you increase that > maximum. Do you have to calculate the new value? Can you make it larger > than what you actually need? What if I add after the full text of the comment (which already says about the 'l2-cache-size' option) something like: "In any case, the L2 cache will be set to fit the virtual image size, unless it will require more space than the allowed maximum, in which case it will occupy the allowed maximum only." > > The way I see it: there are two simple changes from the user's point of > view (they can even be two separate patches). > > 1) The default l2-cache-size is now 32MB. DEFAULT_L2_CACHE_CLUSTERS is > useless now and disappears. I don't think that it can be a separate patch, because unless the other logic is changed, the cache will occupy 32 MB *always*, regardless of the image size, and that's quite a big and unneeded overhead. > 2) QEMU will only use as much memory from l2-cache-size as it can use to > cover the whole image. Increasing the value of l2-cache-size will not > use any additional memory, so it's safe to do. I'll add the clarification that it's safe to increase the l2-cache-size value to the commit message. It is reasonably safe, except of some corner cases with very large image sizes. > > Berto >
On Wed 08 Aug 2018 03:07:10 PM CEST, Leonid Bloch wrote: > On 08/08/2018 03:39 PM, Alberto Garcia wrote: >> On Wed 08 Aug 2018 09:10:48 AM CEST, Leonid Bloch wrote: >>> Sufficient L2 cache can noticeably improve the performance when using >>> large images with frequent I/O. The memory overhead is not significant >>> in most cases, as the cache size is only 1 MB for each 8 GB of virtual >>> image size (with the default cluster size of 64 KB). For cases with very >>> large images and/or small cluster sizes, there is an upper limit on the >>> default L2 cache: 32 MB. >> >> I find this description a bit confusing. >> >> First of all, because it's not true that the default will cover the >> whole image: we're just increasing it, but any image > 256GB is going to >> need more than 32MB (with 64KB clusters, that is). > > How about the following: > > qcow2: Make the default L2 cache try to cover the entire image That's what I think it's misleading, the patch only increases the default cache size value. The current 1MB cache also tries to cover the entire image, if the entire image is <= 8GB. But there's no new feature that extends the cache size to cover the entire image. There's no "we used to cover 8GB images by default, but now we cover all images". In other words, the message that I think the user should get is: set l2-cache-size as high as the maximum amount of memory you're willing to use for the cache, and QEMU will ensure that it will only use as much as it needs and no extra memory will be wasted. If you're working with a 100GB image it doesn't matter if you set l2-cache-size to 32MB or 1GB. It will use the exact same amount of RAM. >> The way I see it: there are two simple changes from the user's point of >> view (they can even be two separate patches). >> >> 1) The default l2-cache-size is now 32MB. DEFAULT_L2_CACHE_CLUSTERS is >> useless now and disappears. > I don't think that it can be a separate patch, because unless the other > logic is changed, the cache will occupy 32 MB *always*, regardless of > the image size, and that's quite a big and unneeded overhead. Change the order of both patches then :-) 1) If l2-cache-size > l2_metadata_size, then make l2-cache-size = l2_metadata_size. This is already useful on its own, even with the current default of 1MB. 2) Increase the default to 32MB. This won't waste additional memory for small images because of the previous patch, and will cover images up to 256GB. If you have larger images you would need to increase l2-cache-size manually if you want to cache all the L2 metadata. The way I see it is that the important change is (1): it's safe to increase l2-cache-size, it won't waste memory[*]. (2) is nice too because it will cover larger images by default, but if you're running VMs with disk images larger than 256GB (which is not an extreme scenario) you still need to increase the cache. [*] In practice it's not wasting too much memory even now, because even if you allocate a 32MB cache but only need 1MB, those extra 31MB are going to be uninitialized and unused, and therefore not taking up any physical memory. But you still need to have Qcow2CachedTable structures for those entries, and any loop that needs to walk Qcow2Cache.entries is going to be slower. So it's a good idea not to allocate unneeded memory and to make this an explicit promise to the user. Berto
On 08/08/2018 04:58 PM, Alberto Garcia wrote: > On Wed 08 Aug 2018 03:07:10 PM CEST, Leonid Bloch wrote: >> On 08/08/2018 03:39 PM, Alberto Garcia wrote: >>> On Wed 08 Aug 2018 09:10:48 AM CEST, Leonid Bloch wrote: >>>> Sufficient L2 cache can noticeably improve the performance when using >>>> large images with frequent I/O. The memory overhead is not significant >>>> in most cases, as the cache size is only 1 MB for each 8 GB of virtual >>>> image size (with the default cluster size of 64 KB). For cases with very >>>> large images and/or small cluster sizes, there is an upper limit on the >>>> default L2 cache: 32 MB. >>> >>> I find this description a bit confusing. >>> >>> First of all, because it's not true that the default will cover the >>> whole image: we're just increasing it, but any image > 256GB is going to >>> need more than 32MB (with 64KB clusters, that is). >> >> How about the following: >> >> qcow2: Make the default L2 cache try to cover the entire image > > That's what I think it's misleading, the patch only increases the > default cache size value. The current 1MB cache also tries to cover the > entire image, if the entire image is <= 8GB. But there's no new feature > that extends the cache size to cover the entire image. There's no "we > used to cover 8GB images by default, but now we cover all images". > > In other words, the message that I think the user should get is: set > l2-cache-size as high as the maximum amount of memory you're willing to > use for the cache, and QEMU will ensure that it will only use as much as > it needs and no extra memory will be wasted. If you're working with a > 100GB image it doesn't matter if you set l2-cache-size to 32MB or > 1GB. It will use the exact same amount of RAM. Then: qcow2: Make the L2 cache size relative to the size of the image [Something like] Now the L2 cache will be only as large as needed to cover the entire image, until a preset maximum (which is 32 MB by default). ? > >>> The way I see it: there are two simple changes from the user's point of >>> view (they can even be two separate patches). >>> >>> 1) The default l2-cache-size is now 32MB. DEFAULT_L2_CACHE_CLUSTERS is >>> useless now and disappears. >> I don't think that it can be a separate patch, because unless the other >> logic is changed, the cache will occupy 32 MB *always*, regardless of >> the image size, and that's quite a big and unneeded overhead. > > Change the order of both patches then :-) Do you really think it's necessary? The increase of the default max size is directly tied to the functionality change: it will be harmful to increase the maximum before the new functionality is implemented, and there is no need to change the functionality if the default max is not increased. Leonid. > > 1) If l2-cache-size > l2_metadata_size, then make l2-cache-size = > l2_metadata_size. This is already useful on its own, even with the > current default of 1MB. > > 2) Increase the default to 32MB. This won't waste additional memory for > small images because of the previous patch, and will cover images up > to 256GB. If you have larger images you would need to increase > l2-cache-size manually if you want to cache all the L2 metadata. > > The way I see it is that the important change is (1): it's safe to > increase l2-cache-size, it won't waste memory[*]. (2) is nice too > because it will cover larger images by default, but if you're running > VMs with disk images larger than 256GB (which is not an extreme > scenario) you still need to increase the cache. > > [*] In practice it's not wasting too much memory even now, because even > if you allocate a 32MB cache but only need 1MB, those extra 31MB are > going to be uninitialized and unused, and therefore not taking up any > physical memory. But you still need to have Qcow2CachedTable structures > for those entries, and any loop that needs to walk Qcow2Cache.entries is > going to be slower. So it's a good idea not to allocate unneeded memory > and to make this an explicit promise to the user. > > Berto >
On Wed 08 Aug 2018 04:35:19 PM CEST, Leonid Bloch wrote: >>>> The way I see it: there are two simple changes from the user's point of >>>> view (they can even be two separate patches). >>>> >>>> 1) The default l2-cache-size is now 32MB. DEFAULT_L2_CACHE_CLUSTERS is >>>> useless now and disappears. >>> I don't think that it can be a separate patch, because unless the other >>> logic is changed, the cache will occupy 32 MB *always*, regardless of >>> the image size, and that's quite a big and unneeded overhead. >> >> Change the order of both patches then :-) > > Do you really think it's necessary? The increase of the default max > size is directly tied to the functionality change: it will be harmful > to increase the maximum before the new functionality is implemented, > and there is no need to change the functionality if the default max is > not increased. I think that we're looking at this from two different perspectives. a) If I understand you correctly, you see this as a way to make the user forget about the L2 cache: we guarantee that it's going to be big enough for the entire image, so simply forget about it. Exception: if you're using very large images you will have to set its size manually, but for the vast majority of cases you'll be alright with the default (32MB). b) The way I see it: setting the right L2 cache size is not trivial, it depends on the image and cluster sizes, and it involves a trade-off between how much memory you want to use and how much performance you're willing to sacrifice. QEMU has many use cases and there's no good default, you need to make the numbers yourself if you want to fine-tune it. Don't blindly trust the new default size (32MB) because it won't be enough for many cases. But we can promise you this: make l2-cache-size the maximum amount of memory you're willing to spend on this disk image's cache, and we guarantee that we'll only use the amount that we need to give you the best performance. I hope (a) was a fair description of what you're trying to achieve with these patches. But I also hope that you can see why making l2_cache_size = MIN(l2_cache_size, virtual_disk_size / (s->cluster_size / 8)) is a worthwhile change on its own, even if we didn't increase the default cache size to 32MB. Berto
On Wed 08 Aug 2018 03:58:02 PM CEST, Alberto Garcia wrote: > 1) If l2-cache-size > l2_metadata_size, then make l2-cache-size = > l2_metadata_size. This is already useful on its own, even with the > current default of 1MB. > > 2) Increase the default to 32MB. This won't waste additional memory for > small images because of the previous patch, and will cover images up > to 256GB. If you have larger images you would need to increase > l2-cache-size manually if you want to cache all the L2 metadata. Let's try this way: we had many users requesting us to add a new option to set l2-cache-size=100%, but we never agreed on a good API. Patch (1) would do precisely that (l2-cache-size=1T). Patch (2) changes the default, which may be better and probably enough for many users, but it's not what solves the problem. Berto
On 08/08/2018 06:16 PM, Alberto Garcia wrote: > On Wed 08 Aug 2018 04:35:19 PM CEST, Leonid Bloch wrote: >>>>> The way I see it: there are two simple changes from the user's point of >>>>> view (they can even be two separate patches). >>>>> >>>>> 1) The default l2-cache-size is now 32MB. DEFAULT_L2_CACHE_CLUSTERS is >>>>> useless now and disappears. >>>> I don't think that it can be a separate patch, because unless the other >>>> logic is changed, the cache will occupy 32 MB *always*, regardless of >>>> the image size, and that's quite a big and unneeded overhead. >>> >>> Change the order of both patches then :-) >> >> Do you really think it's necessary? The increase of the default max >> size is directly tied to the functionality change: it will be harmful >> to increase the maximum before the new functionality is implemented, >> and there is no need to change the functionality if the default max is >> not increased. > > I think that we're looking at this from two different perspectives. > > a) If I understand you correctly, you see this as a way to make the user > forget about the L2 cache: we guarantee that it's going to be big > enough for the entire image, so simply forget about it. Exception: if > you're using very large images you will have to set its size > manually, but for the vast majority of cases you'll be alright with > the default (32MB). Yes, just with a small fix: my aim is not to make the user forget about the L2 cache, my aim is to make it as large as needed to cover the entire image in order to increase the performance. This implies increasing its size. Because for images smaller than 8 GB the performance will stay the same, and the memory usage will not be that different: less than 1 MB difference, while the overall QEMU memory overhead is about 600 MB. That's why I think that increasing the max size is an integral part of this patch, because just changing the behavior, without changing the max size, will not cause a noticeable improvement. But it will cause some complications, like changing the code for the current maximal value, and then changing to 32 MB in a separate patch. This doesn't look necessary to me. Leonid. > > b) The way I see it: setting the right L2 cache size is not trivial, it > depends on the image and cluster sizes, and it involves a trade-off > between how much memory you want to use and how much performance > you're willing to sacrifice. QEMU has many use cases and there's no > good default, you need to make the numbers yourself if you want to > fine-tune it. Don't blindly trust the new default size (32MB) because > it won't be enough for many cases. But we can promise you this: make > l2-cache-size the maximum amount of memory you're willing to spend on > this disk image's cache, and we guarantee that we'll only use the > amount that we need to give you the best performance. > > I hope (a) was a fair description of what you're trying to achieve with > these patches. But I also hope that you can see why making l2_cache_size > = MIN(l2_cache_size, virtual_disk_size / (s->cluster_size / 8)) is a > worthwhile change on its own, even if we didn't increase the default > cache size to 32MB. > > Berto >
diff --git a/block/qcow2.c b/block/qcow2.c index ec9e6238a0..98cb96aaca 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -777,29 +777,35 @@ static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts, uint64_t *refcount_cache_size, Error **errp) { BDRVQcow2State *s = bs->opaque; - uint64_t combined_cache_size; + uint64_t combined_cache_size, l2_cache_max_setting; bool l2_cache_size_set, refcount_cache_size_set, combined_cache_size_set; - int min_refcount_cache = MIN_REFCOUNT_CACHE_SIZE * s->cluster_size; + uint64_t min_refcount_cache = MIN_REFCOUNT_CACHE_SIZE * s->cluster_size; combined_cache_size_set = qemu_opt_get(opts, QCOW2_OPT_CACHE_SIZE); l2_cache_size_set = qemu_opt_get(opts, QCOW2_OPT_L2_CACHE_SIZE); refcount_cache_size_set = qemu_opt_get(opts, QCOW2_OPT_REFCOUNT_CACHE_SIZE); combined_cache_size = qemu_opt_get_size(opts, QCOW2_OPT_CACHE_SIZE, 0); - *l2_cache_size = qemu_opt_get_size(opts, QCOW2_OPT_L2_CACHE_SIZE, 0); + l2_cache_max_setting = qemu_opt_get_size(opts, QCOW2_OPT_L2_CACHE_SIZE, + DEFAULT_L2_CACHE_MAX_SIZE); *refcount_cache_size = qemu_opt_get_size(opts, QCOW2_OPT_REFCOUNT_CACHE_SIZE, 0); *l2_cache_entry_size = qemu_opt_get_size( opts, QCOW2_OPT_L2_CACHE_ENTRY_SIZE, s->cluster_size); + uint64_t virtual_disk_size = bs->total_sectors * BDRV_SECTOR_SIZE; + uint64_t max_l2_cache = virtual_disk_size / (s->cluster_size / 8); + *l2_cache_size = MIN(max_l2_cache, l2_cache_max_setting); + if (combined_cache_size_set) { if (l2_cache_size_set && refcount_cache_size_set) { error_setg(errp, QCOW2_OPT_CACHE_SIZE ", " QCOW2_OPT_L2_CACHE_SIZE " and " QCOW2_OPT_REFCOUNT_CACHE_SIZE " may not be set " "at the same time"); return; - } else if (*l2_cache_size > combined_cache_size) { + } else if (l2_cache_size_set && + (l2_cache_max_setting > combined_cache_size)) { error_setg(errp, QCOW2_OPT_L2_CACHE_SIZE " may not exceed " QCOW2_OPT_CACHE_SIZE); return; @@ -814,29 +820,16 @@ static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts, } else if (refcount_cache_size_set) { *l2_cache_size = combined_cache_size - *refcount_cache_size; } else { - uint64_t virtual_disk_size = bs->total_sectors * BDRV_SECTOR_SIZE; - uint64_t max_l2_cache = virtual_disk_size / (s->cluster_size / 8); - /* Assign as much memory as possible to the L2 cache, and * use the remainder for the refcount cache */ - if (combined_cache_size >= max_l2_cache + min_refcount_cache) { - *l2_cache_size = max_l2_cache; + if (combined_cache_size >= *l2_cache_size + min_refcount_cache) { *refcount_cache_size = combined_cache_size - *l2_cache_size; } else { - *refcount_cache_size = - MIN(combined_cache_size, min_refcount_cache); + *refcount_cache_size = MIN(combined_cache_size, + min_refcount_cache); *l2_cache_size = combined_cache_size - *refcount_cache_size; } } - } else { - if (!l2_cache_size_set) { - *l2_cache_size = MAX(DEFAULT_L2_CACHE_BYTE_SIZE, - (uint64_t)DEFAULT_L2_CACHE_CLUSTERS - * s->cluster_size); - } - if (!refcount_cache_size_set) { - *refcount_cache_size = min_refcount_cache; - } } if (*l2_cache_entry_size < (1 << MIN_CLUSTER_BITS) || diff --git a/block/qcow2.h b/block/qcow2.h index 81b844e936..d77a31d932 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -73,9 +73,7 @@ /* Must be at least 4 to cover all cases of refcount table growth */ #define MIN_REFCOUNT_CACHE_SIZE 4 /* clusters */ -/* Whichever is more */ -#define DEFAULT_L2_CACHE_CLUSTERS 8 /* clusters */ -#define DEFAULT_L2_CACHE_BYTE_SIZE 1048576 /* bytes */ +#define DEFAULT_L2_CACHE_MAX_SIZE 0x2000000U /* bytes */ #define DEFAULT_CLUSTER_SIZE 65536 diff --git a/docs/qcow2-cache.txt b/docs/qcow2-cache.txt index 5bf2a8ad29..c7625cdeb3 100644 --- a/docs/qcow2-cache.txt +++ b/docs/qcow2-cache.txt @@ -97,12 +97,14 @@ need: l2_cache_size = disk_size_GB * 131072 refcount_cache_size = disk_size_GB * 32768 -QEMU has a default L2 cache of 1MB (1048576 bytes) and a refcount -cache of 256KB (262144 bytes), so using the formulas we've just seen -we have +QEMU will use a default L2 cache sufficient to cover the entire virtual +size of an image, which with the default cluster size will result in 1 MB +of cache for every 8 GB of virtual image size: - 1048576 / 131072 = 8 GB of virtual disk covered by that cache - 262144 / 32768 = 8 GB + 65536 / 8 = 8192 = 8 GB / 1 MB + +A default refcount cache is 4 times the cluster size, which defaults to +256 KB (262144 bytes). How to configure the cache sizes @@ -121,8 +123,11 @@ There are a few things that need to be taken into account: - Both caches must have a size that is a multiple of the cluster size (or the cache entry size: see "Using smaller cache sizes" below). - - The default L2 cache size is 8 clusters or 1MB (whichever is more), - and the minimum is 2 clusters (or 2 cache entries, see below). + - The default L2 cache size will cover the entire virtual size of an + image, but is capped at 32 MB (enough for image sizes of up to 256 GB + with the default cluster size). This maximum value can be reduced or + enlarged using the "l2-cache-size" option. The minimum is 2 clusters + (or 2 cache entries, see below). - The default (and minimum) refcount cache size is 4 clusters. @@ -180,9 +185,8 @@ Some things to take into account: always uses the cluster size as the entry size. - If the L2 cache is big enough to hold all of the image's L2 tables - (as explained in the "Choosing the right cache sizes" section - earlier in this document) then none of this is necessary and you - can omit the "l2-cache-entry-size" parameter altogether. + (the default behavior) then none of this is necessary and you can + omit the "l2-cache-entry-size" parameter altogether. Reducing the memory usage diff --git a/qemu-options.hx b/qemu-options.hx index f6804758d3..d6e15b2f06 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -756,9 +756,9 @@ The maximum total size of the L2 table and refcount block caches in bytes @item l2-cache-size The maximum size of the L2 table cache in bytes -(default: if cache-size is not defined - 1048576 bytes or 8 clusters, whichever -is larger; otherwise, as large as possible or needed within the cache-size, -while permitting the requested or the minimal refcount cache size) +(default: if cache-size is not defined - 32M; otherwise, as large as possible +or needed within the cache-size, while permitting the requested or the minimal +refcount cache size) @item refcount-cache-size The maximum size of the refcount block cache in bytes diff --git a/tests/qemu-iotests/137 b/tests/qemu-iotests/137 index 87965625d8..e3fb078588 100755 --- a/tests/qemu-iotests/137 +++ b/tests/qemu-iotests/137 @@ -109,7 +109,6 @@ $QEMU_IO \ -c "reopen -o cache-size=1M,l2-cache-size=64k,refcount-cache-size=64k" \ -c "reopen -o cache-size=1M,l2-cache-size=2M" \ -c "reopen -o cache-size=1M,refcount-cache-size=2M" \ - -c "reopen -o l2-cache-size=256T" \ -c "reopen -o l2-cache-entry-size=33k" \ -c "reopen -o l2-cache-entry-size=128k" \ -c "reopen -o refcount-cache-size=256T" \ diff --git a/tests/qemu-iotests/137.out b/tests/qemu-iotests/137.out index 6a2ffc71fd..70f245ae7a 100644 --- a/tests/qemu-iotests/137.out +++ b/tests/qemu-iotests/137.out @@ -19,7 +19,6 @@ Parameter 'lazy-refcounts' expects 'on' or 'off' cache-size, l2-cache-size and refcount-cache-size may not be set at the same time l2-cache-size may not exceed cache-size refcount-cache-size may not exceed cache-size -L2 cache size too big L2 cache entry size must be a power of two between 512 and the cluster size (65536) L2 cache entry size must be a power of two between 512 and the cluster size (65536) Refcount cache size too big
Sufficient L2 cache can noticeably improve the performance when using large images with frequent I/O. The memory overhead is not significant in most cases, as the cache size is only 1 MB for each 8 GB of virtual image size (with the default cluster size of 64 KB). For cases with very large images and/or small cluster sizes, there is an upper limit on the default L2 cache: 32 MB. To modify this limit one can use the already existing 'l2-cache-size' option. This option was previously documented as the *maximum* L2 cache size, and this patch makes it behave as such, instead of a constant size. Also, the existing option 'cache-size' can limit the sum of both L2 and refcount caches, as previously. Signed-off-by: Leonid Bloch <lbloch@janustech.com> --- block/qcow2.c | 33 +++++++++++++-------------------- block/qcow2.h | 4 +--- docs/qcow2-cache.txt | 24 ++++++++++++++---------- qemu-options.hx | 6 +++--- tests/qemu-iotests/137 | 1 - tests/qemu-iotests/137.out | 1 - 6 files changed, 31 insertions(+), 38 deletions(-)