Re: [libvirt] [PATCH] Add support L2 table cache for qcow2 disk

On 06/29/2018 05:46 AM, dujiancheng wrote: > The patch add support L2 table cache for qcow2 disk. > L2 table cache can improve IO read and write performance for qcow2 img. > Example: random 4K read requests on a fully populated 100GB image (SSD backend and vm with directsync cacha mode), > IOPS increased by 7 times. > This commit message has some very long lines and does not explain the new element you are introducing. Also, you are missing Signed-off-by line which is required. > --- > src/conf/domain_conf.c | 25 +++++++++++++++++++++++++ > src/conf/domain_conf.h | 4 ++++ > src/qemu/qemu_command.c | 7 +++++++ > src/qemu/qemu_domain.c | 4 ++++ > 4 files changed, 40 insertions(+) Any new element that is introduced must go hand in hand with RNG schema adjustment, documentation and a test case. I believe virsh was complaining when you edited your domain and added <diskCache/>: virsh # edit fedora error: Reconnected to the hypervisor error: XML document failed to validate against schema: Unable to validate doc against //docs/schemas/domain.rng Extra element devices in interleave Element domain failed to validate content Failed. Try again? [y,n,i,f,?]: > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index b8b5345..cb9fb05 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -9502,6 +9502,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, > char *vendor = NULL; > char *product = NULL; > char *domain_name = NULL; > + char *disk_l2_cache_size = NULL; > + char *disk_cache_clean_interval = NULL; > > if (!(def = virDomainDiskDefNew(xmlopt))) > return NULL; > @@ -9701,6 +9703,27 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, > } > } else if (virXMLNodeNameEqual(cur, "boot")) { > /* boot is parsed as part of virDomainDeviceInfoParseXML */ > + } else if (virXMLNodeNameEqual(cur, "diskCache")) { > + disk_l2_cache_size = > + virXMLPropString(cur, "disk_l2_cache_size"); This is rather unfortunate name for attribute. Also, is there a reason you have not put the assignment onto one line? > + if (disk_l2_cache_size && > + virStrToLong_ui(disk_l2_cache_size, NULL, 0, > + &def->disk_cache.disk_l2_cache_size) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("invalid disk L2 cache size '%s'"), > + disk_l2_cache_size); > + goto error; > + } > + disk_cache_clean_interval = > + virXMLPropString(cur, "disk_cache_clean_interval"); > + if (disk_cache_clean_interval && > + virStrToLong_ui(disk_cache_clean_interval, NULL, 0, > + &def->disk_cache.disk_cache_clean_interval) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("invalid disk cache clean interval '%s'"), > + disk_cache_clean_interval); > + goto error; > + } > } > } So what this adds is: <disk type='file' device='disk'> <driver name='qemu' type='qcow2' discard='unmap'/> <source file='/var/lib/libvirt/images/fedora.qcow2'/> <diskCache disk_l2_cache_size='128' disk_cache_clean_interval='8'/> <!-- This is the new element ^^ --> <target dev='sda' bus='scsi'/> <boot order='1'/> <address type='drive' controller='0' bus='0' target='0' unit='0'/> </disk> Well, from my example, what units are "128" and "8" in? Also, in libvirt we give users possibility to specify other units than KiB (even though we report back size in KiB), for instance look at <memory/> numa <cell/>. Also, your proposal is not that future proof. My suggestion is: <cache> <cache level='2'> <size unit='KiB'>128</size> </cache> <clean interval='8'/> </cache> But I'm sure there's even better approach.