Re: [libvirt] [PATCH 1/2] conf, docs: Add qcow2 cache configuration support

On 09/07/2017 02:09 AM, Liu Qing wrote: > Random write IOPS will drop dramatically if qcow2 l2 cache could not > cover the whole disk. This patch gives libvirt user a chance to adjust > the qcow2 cache configuration. > > Three new qcow2 driver parameters are added. They are l2-cache-size, > refcount-cache-size and cache-clean-interval. > > The following are from qcow2-cache.txt. > The amount of virtual disk that can be mapped by the L2 and refcount > caches (in bytes) is: > disk_size = l2_cache_size * cluster_size / 8 > disk_size = refcount_cache_size * cluster_size * 8 / refcount_bits > > The parameter "cache-clean-interval" defines an interval (in seconds). > All cache entries that haven't been accessed during that interval are > removed from memory. Suggestion, rewrite a bit: Three new qcow2 driver parameters (l2-cache-size, refcount-cache-size and cache-clean-interval) are added as attributes to a new <qcow2> subelement for a <driver name='qemu' type='qcow2'...> of a <disk> element. The QEMU source docs/qcow2-cache.txt provides the guidelines for defining/configuring values for each. ... That is - let's not bother with repeating what's in the document since it can change, but provide enough information that someone with curiosity would know where to look.

Question from me: does "cache='none'" need to be set in the <driver> element for this to work properly? Do any of the cache values make sense? If it must be none, then we need to check that. If something doesn't make ense, we need to check that too.

> if (def->src->auth) { > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index ed8cb6e..391aaba 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -1433,6 +1433,12 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk, > qemuformat = "luks"; > virBufferAsprintf(buf, "format=%s,", qemuformat); > } > + if (disk->src->l2_cache_size > 0) > + virBufferAsprintf(buf, "l2-cache-size=%llu,", disk->src->l2_cache_size); > + if (disk->src->refcount_cache_size > 0) > + virBufferAsprintf(buf, "refcount-cache-size=%llu,", disk->src->refcount_cache_size); > + if (disk->src->cache_clean_interval > 0) > + virBufferAsprintf(buf, "cache-clean-interval=%llu,", disk->src->cache_clean_interval); These don't belong in this patch, they belong in the next patch. We're not adding the .args here. I can make all the above changes for you - not a problem. But please let me know your thoughts on whether we should keep the sizing recommendation in formatdomain.html.in or just point at the qemu source document. It's a fine line - I like the example, but I also see the downside.

John